[9fans] usb serial bug

[erik quanstrom]

lyons# 6.out
ehci 0xfffffe00dfa23000: port 1 didn't reset within 500 ms; sts 0x1101
usb/hub... usb/serial... epout 1 epin 1
serial: open i/o ep data: '/dev/usb/ep4.1/data' permission denied
6.out: serial: serial: no endpoints found for ifc 0
139: 6.out: bootes: fault addr=0xfffffffe kpc=0x223be7
6.out 139: suicide: sys: trap: fault read addr=0xfffffffe pc=0x223be7

i think this is caused by devusb.c:/^usbopen insisting that
the mode be OREAD or OWRITE. the little patch below
disables the new code that tries to open the endpoint data
ORDWR. (unless i've misread the code.)

this is a WORKAROUND. a proper fix would be something
like allowing ORDWR in usbopen if that is indeed the problem.

; diffy -c /sys/src/cmd/usb/serial/serial.c
/n/dump/2013/0213/sys/src/cmd/usb/serial/serial.c:716,724 - /sys/src/cmd/usb/serial/serial.c:716,724
ep->dir == Ein && epintr == -1)
epintr = ep->id;
if(ep->type == Ebulk){
- if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
+ if((ep->dir == Ein /* || ep->dir == Eboth*/) && epin == -1)
epin = ep->id;
- if((ep->dir == Ein || ep->dir == Eboth) && epout == -1)
+ if((ep->dir == Ein /* || ep->dir == Eboth*/) && epout == -1)
epout = ep->id;
}
}


- erik

[Richard Miller]

Post by erik quanstrom
this is a WORKAROUND. a proper fix would be something
like allowing ORDWR in usbopen if that is indeed the problem.

No, endpoints are unidirectional by design; in the usb spec there are
no read/write endpoints. The confusing thing in the spec is that two
different endpoints can have the same endpoint number - they are
distinguished by direction. So what looks like a read/write endpoint
number 1 is really two separate endpoints, input ep 1 and output ep 1.
The current Plan 9 usb architecture perpetuates the confusion by
referring to them both with one name epN.1, but you still have to open
them both independently.

[erik quanstrom]

Post by Richard Miller

Post by erik quanstrom
this is a WORKAROUND. a proper fix would be something
like allowing ORDWR in usbopen if that is indeed the problem.

No, endpoints are unidirectional by design; in the usb spec there are
no read/write endpoints. The confusing thing in the spec is that two
different endpoints can have the same endpoint number - they are
distinguished by direction. So what looks like a read/write endpoint
number 1 is really two separate endpoints, input ep 1 and output ep 1.
The current Plan 9 usb architecture perpetuates the confusion by
referring to them both with one name epN.1, but you still have to open
them both independently.

in that case, shouldn't these three blocks be reverted?

- erik

/n/dump/2012/1201/sys/src/cmd/usb/serial/serial.c:648,654 - /n/dump/2013/0212/sys/src/cmd/usb/serial/serial.c:647,657
fprint(2, "serial: openep %d: %r\n", epin);
return -1;
}
- p->epout = openep(ser->dev, epout);
+ if(epout == epin){
+ incref(p->epin);
+ p->epout = p->epin;
+ }else
+ p->epout = openep(ser->dev, epout);
if(p->epout == nil){
fprint(2, "serial: openep %d: %r\n", epout);
closedev(p->epin);
/n/dump/2012/1201/sys/src/cmd/usb/serial/serial.c:674,681 - /n/dump/2013/0212/sys/src/cmd/usb/serial/serial.c:677,688

if(ser->seteps!= nil)
ser->seteps(p);
- opendevdata(p->epin, OREAD);
- opendevdata(p->epout, OWRITE);
+ if(p->epin == p->epout)
+ opendevdata(p->epin, ORDWR);
+ else{
+ opendevdata(p->epin, OREAD);
+ opendevdata(p->epout, OWRITE);
+ }
if(p->epin->dfd < 0 ||p->epout->dfd < 0 ||
(ser->hasepintr && p->epintr->dfd < 0)){
fprint(2, "serial: open i/o ep data: %r\n");
/n/dump/2012/1201/sys/src/cmd/usb/serial/serial.c:709,717 - /n/dump/2013/0212/sys/src/cmd/usb/serial/serial.c:716,724
ep->dir == Ein && epintr == -1)
epintr = ep->id;
if(ep->type == Ebulk){
- if(ep->dir == Ein && epin == -1)
+ if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
epin = ep->id;
- if(ep->dir == Eout && epout == -1)
+ if((ep->dir == Ein || ep->dir == Eboth) && epout == -1)
epout = ep->id;
}
}

[Jeff Sickel]

from the dump:

Jan 25 15:54:19 CST 2013 serial 213453 [jmk]
Jan 25 15:54:19 CST 2013 /n/sourcesdump/2013/0214/plan9/386/bin/usb/serial 213453 [jmk]
Dec 10 21:27:37 CST 2012 /n/sourcesdump/2013/0125/plan9/386/bin/usb/serial 211141 [sys]
…

the Dec 10 version works with Prolific USB-to-Serial devices.
The more recent one does not and ends up breaking on treeinsert:

cpu% acid 348184
/proc/348184/text:386 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/386
acid: stk()
treeinsert(node=0xd160d15e,tree=0x4d160)+0x15 /sys/src/libc/port/pool.c:264
pooldel(p=0x24ff8,node=0x4d160)+0xdd /sys/src/libc/port/pool.c:414
blockmerge(b=0x4d160,a=0x49140,pool=0x24ff8)+0x54 /sys/src/libc/port/pool.c:465
poolfreel(v=0x49148,p=0x24ff8)+0xd1 /sys/src/libc/port/pool.c:1213
poolfree(p=0x24ff8,v=0x49148)+0x41 /sys/src/libc/port/pool.c:1327
free(v=0x49150)+0x23 /sys/src/libc/port/malloc.c:250
_schedinit(arg=0x487b0)+0x105 /sys/src/libthread/sched.c:60
main(argc=0x1,argv=0xdfffefb4)+0x38 /sys/src/libthread/main.c:38
_main+0x31 /sys/src/libc/386/main9.s:16

[Richard Miller]

Post by erik quanstrom

Post by Richard Miller
The current Plan 9 usb architecture perpetuates the confusion by
referring to them both with one name epN.1, but you still have to open
them both independently.

in that case, shouldn't these three blocks be reverted?

Erik is right, I was talking through my hat. It's OK to open bulk
endpoints read/write, and the kernel will do the right thing. The
actual problem, which neither of us had spotted although it was
staring us in the face, is this:

if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
epin = ep->id;
if((ep->dir == Ein || ep->dir == Eboth) && epout == -1)
epout = ep->id;

Notice the two occurrences of Ein? The second one obviously should
be Eout. It was a typo (mine, I blush to admit).

My usb serial adapter uses the same ep number for input and output,
so my testing didn't reveal this error. I think the same typo will
account for the double-free bug which Jeff (and Lucio on 4 Feb) reported.

Erik, Jeff, Lucio - please try changing the offending Ein to Eout in
/sys/src/cmd/usb/serial/serial.c:721 and see if your troubles are resolved.

[Gorka Guardiola]

Yes, I am looking into it and just saw this.
G.

Post by Richard Miller

Post by erik quanstrom

Post by Richard Miller
The current Plan 9 usb architecture perpetuates the confusion by
referring to them both with one name epN.1, but you still have to open
them both independently.

in that case, shouldn't these three blocks be reverted?

Erik is right, I was talking through my hat. It's OK to open bulk
endpoints read/write, and the kernel will do the right thing. The
actual problem, which neither of us had spotted although it was
if((ep->dir == Ein || ep->dir == Eboth) && epin == -1)
epin = ep->id;
if((ep->dir == Ein || ep->dir == Eboth) && epout == -1)
epout = ep->id;
Notice the two occurrences of Ein? The second one obviously should
be Eout. It was a typo (mine, I blush to admit).
My usb serial adapter uses the same ep number for input and output,
so my testing didn't reveal this error. I think the same typo will
account for the double-free bug which Jeff (and Lucio on 4 Feb) reported.
Erik, Jeff, Lucio - please try changing the offending Ein to Eout in
/sys/src/cmd/usb/serial/serial.c:721 and see if your troubles are resolved.

[Gorka Guardiola]

With the Ein fix, it works again with the
Trendnet TU-S9 which reports:
vid 0x067b
did 0x2303
which is prolific.
G.

[Richard Miller]

Post by Gorka Guardiola
With the Ein fix, it works again with the
Trendnet TU-S9

Thanks, I've sent it as a patch.