Re: Patch mceusb: fix invalid urb interval

[Jarod Wilson <jarod@redhat.com>]

On Wed, Jan 15, 2014 at 04:52:45PM +0000, Sean Young wrote:
> On Wed, Jan 15, 2014 at 01:49:17PM -0200, Mauro Carvalho Chehab wrote:
> > Hi Martin,
> > 
> > Em Wed, 11 Dec 2013 21:34:55 +0100
> > Martin Kittel <linux@martin-kittel.de> escreveu:
> > 
> > > Hi Mauro, hi Sean,
> > > 
> > > thanks for considering the patch. I have added an updated version at the
> > > end of this mail.
> > > 
> > > Regarding the info Sean was requesting, it is indeed an xhci hub. I also
> > > added the details of the remote itself.
> > > 
> > > Please let me know if there is anything missing.
> > > 
> > > Best wishes,
> > > 
> > > Martin.
> > > 
> > > 
> > > lsusb -vvv
> > > ------
> > > Bus 001 Device 002: ID 2304:0225 Pinnacle Systems, Inc. Remote Kit
> > > Infrared Transceiver
> > > Device Descriptor:
> > >   bLength		 18
> > >   bDescriptorType	  1
> > >   bcdUSB	       2.00
> > >   bDeviceClass		  0 (Defined at Interface level)
> > >   bDeviceSubClass	  0
> > >   bDeviceProtocol	  0
> > >   bMaxPacketSize0	  8
> > >   idVendor	     0x2304 Pinnacle Systems, Inc.
> > >   idProduct	     0x0225 Remote Kit Infrared Transceiver
> > >   bcdDevice	       0.01
> > >   iManufacturer		  1 Pinnacle Systems
> > >   iProduct		  2 PCTV Remote USB
> > >   iSerial		  5 7FFFFFFFFFFFFFFF
> > >   bNumConfigurations	  1
> > >   Configuration Descriptor:
> > >     bLength		    9
> > >     bDescriptorType	    2
> > >     wTotalLength	   32
> > >     bNumInterfaces	    1
> > >     bConfigurationValue	    1
> > >     iConfiguration	    3 StandardConfiguration
> > >     bmAttributes	 0xa0
> > >       (Bus Powered)
> > >       Remote Wakeup
> > >     MaxPower		  100mA
> > >     Interface Descriptor:
> > >       bLength		      9
> > >       bDescriptorType	      4
> > >       bInterfaceNumber	      0
> > >       bAlternateSetting	      0
> > >       bNumEndpoints	      2
> > >       bInterfaceClass	    255 Vendor Specific Class
> > >       bInterfaceSubClass      0
> > >       bInterfaceProtocol      0
> > >       iInterface	      4 StandardInterface
> > >       Endpoint Descriptor:
> > > 	bLength			7
> > > 	bDescriptorType		5
> > > 	bEndpointAddress     0x81  EP 1 IN
> > > 	bmAttributes		2
> > > 	  Transfer Type		   Bulk
> > > 	  Synch Type		   None
> > > 	  Usage Type		   Data
> > > 	wMaxPacketSize	   0x0040  1x 64 bytes
> > > 	bInterval	       10
> > 
> > Hmm... interval is equal to 10, e. g. 125us * 2^(10 - 1) = 64 ms.
> > 
> > I'm wandering why mceusb is just forcing the interval to 1 (125ms). That
> > sounds wrong, except, of course, if the endpoint descriptor is wrong.
> 
> Note that the endpoint descriptor describes it as a bulk endpoint, but
> it is used as a interrupt endpoint by the driver. For bulk endpoints,
> the interval should not be used (?).
> 
> Maybe the correct solution would be to use the endpoints as bulk endpoints
> if that is what the endpoint says? mceusb devices come in interrupt and 
> bulk flavours.

Yeah, I just went through a number of my devices here. I have the same
pinnacle one with bulk and 10, a topseed with bulk and 1, a topseed with
interrupt and 0, a philips with bulk and 0... There's a pretty nasty mix
of them. The interrupt and 0 one actually winds up with a default
bInterval of 32 from the usb subsystem, but the bulk/0 one sticks with a
default of 0.

Something to properly handle bulk vs. interrupt might be useful, but at
least at first glance here, simply saying

if (ep_{in,out}->bInterval == 0)
	ep_{in,out}->bInterval = 8;

seems to work just fine here with the stack of mceusb devices I've tried
so far (all hooked to usb 1.1 and/or usb 2.0 ports).

> > On my eyes, though, 64ms seems to be a good enough interval to get
> > those events.
> 
> Each packet will be 64 bytes, and at 64 ms you should be able to 960 
> bytes per second. That's more than enough.
> 
> > Jarod/Sean,
> > 
> > Are there any good reason for the mceusb driver to do this?
> > 	ep_in->bInterval = 1;
> > 	ep_out->bInterval = 1;
> 
> I don't know.

It was basically a cover for the bulk/bInterval=0 case.

> The xhci driver is not happy about the interval being changed. With
> CONFIG_USB_DEBUG you get:
> 
> usb 3-12: Driver uses different interval (8 microframes) than xHCI (1 microframe)

I suppose I need to get a machine with usb3 up and running to poke at...

-- 
Jarod Wilson
jarod@redhat.com