Re: [9front] CDC ACM nusb/serial driver

[cinap_lenrek@felloff.net]

here goes some annotations for problems:

diff -r 23bc1d0e2dd2 sys/src/cmd/nusb/serial/cdc.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/sys/src/cmd/nusb/serial/cdc.c	Thu Jul 30 21:06:29 2020 -0700
@@ -0,0 +1,301 @@
+#include <u.h>
+#include <libc.h>
+#include <thread.h>
+#include <fcall.h>
+#include <9p.h>
+#include "usb.h"
+#include "serial.h"
+
+enum {
+	Kyocera		= 0x0482,
+	AHK3001V	= 0x0203,
+};
+
+enum {
+	ScACM	= 2,
+	AT		= 1,
+	None	= 0,
+	LineCoding	= 0x20,
+	SetBreak	= 0x23,
+	ControlState = 0x22,
+};
+
+enum {
+	ClassCDCData = 0x0a,
+	SubclassData = 0,
+};
+
+enum {
+	SubCDCCM	= 1,
+	SubCDCACM	= 2,
+	SubCDCUnion	= 6,
+};


these seem quite generic, is it worth dealing
with a struct here for a 3 byte header?

+typedef struct raw_desc {
+	uchar bLength;
+	uchar bDescriptorType;
+	uchar bDescriptorSubtype;
+	uchar bbytes[1];
+} raw_desc;
+

this thing is essentially just, 2 bytes:

+typedef struct cm_desc {
+	uchar bLength;
+	uchar bDescriptorType;
+	uchar bDescriptorSubtype;
+	uchar bmCapabilities;
+	uchar bDataInterface;
+} cm_desc;
+
+enum {
+	CMDoesCM	= 1,
+	CMOverData	= 2,
+};

this thing one one byte:

+typedef struct acm_desc {
+	uchar bLength;
+	uchar bDescriptorType;
+	uchar bDescriptorSubtype;
+	uchar bmCapabilities;
+} acm_desc;
+
+enum {
+	ACMHasFeature	= 1,
+	ACMHasLine		= 2,
+	ACMHasBreak		= 4,
+	ACMHasNetwork	= 8,
+};

this thing is two byte:

+typedef struct union_desc {
+	uchar bLength;
+	uchar bDescriptorType;
+	uchar bDescriptorSubtype;
+	uchar bMasterInterface;
+	uchar bSlaveInterface[1];
+} union_desc;

first interesting stuct, but you can't use it like you do.

+typedef struct cdc_notify {
+	uchar bmRequestType;
+	uchar bNotification;
+	ushort wValue;
+	ushort wIndex;
+	ushort wLength;
+	uchar data[16];
+} cdc_notify;

here we might have a problem, you read this structure directly
using sizeof(). wValue, WIndex and wLength are 2 bytes, so this
only works on little endian machines. also, the compiler might
pad your structure resulting in wrong size and field offsets.
you need to read the data as bytes, and then parse it... or
use only byte arrays and a constant for the binary length
instead of sizeof(). and for acccess the two byte fields,
you shift and or the two bytes.

+enum {
+	Notification = 0xa1,
+	SerialState = 0x20,
+};
+
+enum {
+	DcdStatus	= 0x01,
+	DsrStatus	= 0x02,
+	RingStatus	= 0x08,
+};
+
+static int
+cdcsetparam(Serialport *p)
+{
+	int res;
+	uchar vals[7];
+
+	PUT4(vals, p->baud);
+
+	vals[4] = 0;
+	if(p->stop == 1)
+		vals[4] = 0;
+	if(p->stop == 15)
+		vals[4] = 1;
+	if(p->stop == 2)
+		vals[4] = 2;
+
+	vals[5] = p->parity;
+
+	vals[6] = p->bits;
+
+	res = usbcmd(p->s->dev, Rh2d | Rclass | Riface, LineCoding, 0, 0, vals, 7);
+
+	return res;
+}
+
+void
+getcaps(Serialport *p)
+{
+	int i, j;
+	Desc *desc;
+	Iface *iface;
+	raw_desc ddesc;
+	cm_desc dcm;
+	acm_desc dacm;
+	union_desc dunion;
+	int cur_ifc;
+
+	for (i = 0; i < Niface; i++) {
+		iface = p->s->dev->usb->conf[0]->iface[i];
+		if (iface == nil)
+			continue;
+
+		cur_ifc = iface->id;
+		if (cur_ifc != p->ctl_ifc && Class(iface->csp) == ClassCDCData && Subclass(iface->csp) == SubclassData && p->data_ifc == -1)
+			p->data_ifc = cur_ifc;
+
+		if (cur_ifc == p->ctl_ifc)
+			for (j = 0; j < Nddesc; j++) {
+				desc = p->s->dev->usb->ddesc[j];
+				if (desc == nil)
+					continue;
+
+				ddesc = (raw_desc)desc->data;

wtf, just cast the pointers, man. 

+				if (ddesc.bDescriptorType == Dfunction) {
+					switch (ddesc.bDescriptorSubtype) {
+					case SubCDCCM:
+						dcm = (cm_desc)ddesc;

no! dont copy the whole struct. this is just wrong.

+						p->data_ifc = dcm.bDataInterface;
+						break;
+					case SubCDCACM:
+						dacm = (acm_desc)ddesc;
+						p->hasbreak = dacm.bmCapabilities & ACMHasBreak;
+						break;
+					case SubCDCUnion:
+						dunion = (union_desc)ddesc;
+						p->data_ifc = dunion.bSlaveInterface[0];
+						break;
+					}
+				}
+			}
+	}
+}
+
+void
+statusreader(void *u)
+{
+	Serialport *p = u;
+	uchar buf[sizeof(cdc_notify)];
+	cdc_notify *notify;
+
+	notify = (cdc_notify*)buf;
+	threadsetname("statusreaderproc");
+

see comment above about the struct:

+	while(read(p->epintr->dfd, buf, sizeof(cdc_notify)) > 0) {
+		if (notify->bmRequestType == Notification && notify->bNotification == SerialState && notify->wLength == 2) {
+			p->dcd = notify->data[0] & DcdStatus;
+			p->dsr = notify->data[0] & DsrStatus;
+			p->ring = notify->data[0] & RingStatus;
+		} else {
+			fprint(2, "serial: unknown intr message: reqtype:%02x notify:%02x len:%d state:%02x\n", notify->bmRequestType, notify->bNotification, notify->wLength, notify->data[0]);
+		}
+	}
+
+	fprint(2, "serial: statusreader exiting: %r\n");
+	closedev(p->s->dev);
+}
+
+static int
+cdcinit(Serialport *p)
+{
+	p->baud = 115200;
+	p->bits = 8;
+	p->parity = 0;
+	p->stop = 1;
+	cdcsetparam(p);
+
+	incref(p->s->dev);
+	proccreate(statusreader, p, 8*1024);
+
+	return 0;
+}
+
+static int
+cdcsetbreak(Serialport *p, int val)
+{
+	if(p->hasbreak == 0)
+		return 0;
+
+	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, SetBreak, (val != 0? 0xffff: 0x0000), 0, nil, 0);
+}
+
+static int
+cdcsendlines(Serialport *p)
+{
+	if(p->rts)
+		p->ctlstate |= CtlRTS;
+	else
+		p->ctlstate &= ~CtlRTS;
+	if(p->dtr)
+		p->ctlstate |= CtlDTR;
+	else
+		p->ctlstate &= ~CtlDTR;
+
+	return usbcmd(p->s->dev, Rh2d | Rclass | Riface, ControlState, p->ctlstate, 0, nil, 0);
+}
+
+static int
+cdcclearpipes(Serialport *p)
+{
+	if(unstall(p->s->dev, p->epout, Eout) < 0)
+		dprint(2, "serial: unstall epout: %r\n");
+	if(unstall(p->s->dev, p->epin, Ein) < 0)
+		dprint(2, "serial: unstall epin: %r\n");
+	if(unstall(p->s->dev, p->epintr, Ein) < 0)
+		dprint(2, "serial: unstall epintr: %r\n");
+
+	return 0;
+}
+
+static int
+cdcwait4data(Serialport *p, uchar *data, int count)
+{
+	int n;
+
+	qunlock(p->s);
+	while ((n = read(p->epin->dfd, data, count)) == 0)
+		;
+	qlock(p->s);
+
+	return n;
+}
+
+static Serialops cdcops = {
+	.init		= cdcinit,
+	.setparam	= cdcsetparam,
+	.setbreak	= cdcsetbreak,
+	.sendlines	= cdcsendlines,
+	.clearpipes	= cdcclearpipes,
+	.wait4data	= cdcwait4data,
+};
+
+int
+cdcprobe(Serial *ser)
+{
+	int i, ifcs;
+	Usbdev *ud = ser->dev->usb;
+	Iface *iface;
+	ulong csp;
+
+	ifcs = 0;
+	if (ud->vid == Kyocera || ud->did == AHK3001V)
+		ifcs++;
+	else for (i = 0; i < Niface; i++) {
+		iface = ud->conf[0]->iface[i];
+		if (iface == nil)
+			continue;
+
+		csp = iface->csp;
+		if (Class(csp) == Clcomms && Subclass(csp) == ScACM && (Proto(csp) == AT || Proto(csp) == None)) {
+			ser->p[ifcs].ctl_ifc = i;
+			ser->p[ifcs].s = ser;
+			getcaps(&ser->p[ifcs]);
+			ifcs++;
+		}
+	}
+
+	if (ifcs == 0)
+		return -1;
+
+	ser->nifcs = ifcs;
+	ser->isacm = 1;
+	ser->outhdrsz = 0;
+	ser->hasepintr = 1;
+	ser->Serialops = cdcops;
+	return 0;
+}
diff -r 23bc1d0e2dd2 sys/src/cmd/nusb/serial/mkfile
--- a/sys/src/cmd/nusb/serial/mkfile	Tue Jun 09 12:23:24 2020 -0700
+++ b/sys/src/cmd/nusb/serial/mkfile	Thu Jul 30 21:06:29 2020 -0700
@@ -1,7 +1,7 @@
 </$objtype/mkfile
 
 TARG=serial
-OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O
+OFILES=ftdi.$O serial.$O prolific.$O ucons.$O silabs.$O ch340.$O cdc.$O
 HFILES=\
 	../lib/usb.h\
 	serial.h\
diff -r 23bc1d0e2dd2 sys/src/cmd/nusb/serial/serial.c
--- a/sys/src/cmd/nusb/serial/serial.c	Tue Jun 09 12:23:24 2020 -0700
+++ b/sys/src/cmd/nusb/serial/serial.c	Thu Jul 30 21:06:29 2020 -0700
@@ -624,23 +624,46 @@
 
 	epintr = epin = epout = -1;
 
-	/*
-	 * interfc 0 means start from the start which is equiv to
-	 * iterate through endpoints probably, could be done better
-	 */
-	eps = ser->dev->usb->conf[0]->iface[ifc]->ep;
+	if(ser->isacm){
+		eps = ser->dev->usb->conf[0]->iface[ser->p[ifc].ctl_ifc]->ep;
 
-	for(i = 0; i < Nep; i++){
-		if((ep = eps[i]) == nil)
-			continue;
-		if(ser->hasepintr && ep->type == Eintr &&
-		    ep->dir == Ein && epintr == -1)
-			epintr = ep->id;
-		if(ep->type == Ebulk){
-			if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
-				epin = ep->id;
-			if((ep->dir == Eout || ep->dir == Eboth) && epout == -1)
-				epout = ep->id;

both of these for-loops (ser->isacm cases) appears to be identical
except the search for the intr endpoint, which you could easily
override (ser->hasintr = 0) in the cdc driver, no?

+		for(i = 0; i < Nep; i++){
+			if((ep = eps[i]) == nil)
+				continue;
+			if(ep->type == Eintr && ep->dir == Ein && epintr == -1)
+				epintr = ep->id;
+		}
+
+		eps = ser->dev->usb->conf[0]->iface[ser->p[ifc].data_ifc]->ep;
+		for(i = 0; i < Nep; i++){
+			if((ep = eps[i]) == nil)
+				continue;
+			if(ep->type == Ebulk){
+				if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
+					epin = ep->id;
+				if((ep->dir == Eout || ep->dir == Eboth) && epout == -1)
+					epout = ep->id;
+			}
+		}
+	} else {
+		/*
+		 * interfc 0 means start from the start which is equiv to
+		 * iterate through endpoints probably, could be done better
+		 */
+		eps = ser->dev->usb->conf[0]->iface[ifc]->ep;
+
+		for(i = 0; i < Nep; i++){
+			if((ep = eps[i]) == nil)
+				continue;
+			if(ser->hasepintr && ep->type == Eintr &&
+			    ep->dir == Ein && epintr == -1)
+				epintr = ep->id;
+			if(ep->type == Ebulk){
+				if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
+					epin = ep->id;
+				if((ep->dir == Eout || ep->dir == Eboth) && epout == -1)
+					epout = ep->id;
+			}
 		}
 	}
 	dprint(2, "serial[%d]: ep ids: in %d out %d intr %d\n", ifc, epin, epout, epintr);
@@ -714,6 +737,7 @@
 extern int slprobe(Serial *ser);
 extern int chprobe(Serial *ser);
 extern int uconsprobe(Serial *ser);
+extern int cdcprobe(Serial *ser);
 
 void
 threadmain(int argc, char* argv[])
@@ -750,7 +774,8 @@
 	&& ftprobe(ser)
 	&& slprobe(ser)
 	&& plprobe(ser)
-	&& chprobe(ser))
+	&& chprobe(ser)
+	&& cdcprobe(ser))
 		sysfatal("no serial devices found");
 
 	for(i = 0; i < ser->nifcs; i++){
diff -r 23bc1d0e2dd2 sys/src/cmd/nusb/serial/serial.h
--- a/sys/src/cmd/nusb/serial/serial.h	Tue Jun 09 12:23:24 2020 -0700
+++ b/sys/src/cmd/nusb/serial/serial.h	Thu Jul 30 21:06:29 2020 -0700
@@ -60,6 +60,10 @@
 
 	int	interfc;	/* interfc on the device for ftdi */
 
+	int hasbreak;	/* CDC ACM may not have break */
+	int ctl_ifc;	/* epintr iface for CDC ACM */
+	int data_ifc;	/* epin/out iface for CDC ACM */
+
 	Channel *w4data;
 	Channel *gotdata;
 	int	ndata;
@@ -81,6 +85,7 @@
 
 	int	jtag;		/* index of jtag interface, -1 none */
 	int	nifcs;		/* # of serial interfaces, including JTAG */
+	int isacm; 		/* CDC ACM interfaces handled differently */
 	Serialport p[Maxifc];
 	int	maxrtrans;
 	int	maxwtrans;

--
cinap