[9fans] usb serial bug

[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

Re: [9fans] usb serial bug

[Richard Miller]

> 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.

Re: [9fans] usb serial bug

[erik quanstrom]

On Thu Feb 14 05:24:27 EST 2013, 9fans@hamnavoe.com wrote:
> > 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;
  		}
  	}

Re: [9fans] usb serial bug

[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

Re: [9fans] usb serial bug

[Richard Miller]

I said:

>> 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 replied:
> 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.

Re: [9fans] usb serial bug

[Gorka Guardiola]

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

On Feb 14, 2013, at 8:24 PM, Richard Miller <9fans@hamnavoe.com> wrote:

> I said:
>
>>> 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 replied:
>> 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.
>
>

Re: [9fans] usb serial bug

[Gorka Guardiola]

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

Re: [9fans] usb serial bug

[Richard Miller]

> With the Ein fix, it works again with the
> Trendnet TU-S9

Thanks, I've sent it as a patch.