Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

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.

On my eyes, though, 64ms seems to be a good enough interval to get
those events.

Jarod/Sean,

Are there any good reason for the mceusb driver to do this?
	ep_in->bInterval = 1;
	ep_out->bInterval = 1;

At least on my tests here with audio with xHCI and EHCI with audio on
em28xx, it seems that EHCI just uses the USB endpoint interval, when
urb->interval == 1, while xHCI uses whatever value stored there.

So, IMHO, the right fix would be to do:

diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index a25bb1581e46..9a0c2ca53f3a 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -1285,7 +1285,6 @@ static int mceusb_dev_probe(struct usb_interface *intf,
 
 			ep_in = ep;
 			ep_in->bmAttributes = USB_ENDPOINT_XFER_INT;
-			ep_in->bInterval = 1;
 			mce_dbg(&intf->dev, "acceptable inbound endpoint "
 				"found\n");
 		}
@@ -1300,7 +1299,6 @@ static int mceusb_dev_probe(struct usb_interface *intf,
 
 			ep_out = ep;
 			ep_out->bmAttributes = USB_ENDPOINT_XFER_INT;
-			ep_out->bInterval = 1;
 			mce_dbg(&intf->dev, "acceptable outbound endpoint "
 				"found\n");
 		}


Martin,

Could you please see if the above patch is enough to fix it?

Thanks!
Mauro