Patch mceusb: fix invalid urb interval

Patch mceusb: fix invalid urb interval

[Martin Kittel]

Hi,

I had trouble getting my MCE remote control to work on my new Intel
mainboard. It was working fine with older boards but with the new board
there would be no reply from the remote just after the setup package was
received during the init phase.
I traced the problem down to the mceusb_dev_recv function where the received
urb is resubmitted again. The problem is that my new board is so blazing
fast that the initial urb was processed in less than a single 125
microsecond interval, so the urb as it was received had urb->interval set to 0.
As the urb is just resubmitted as it came in it now had an invalid interval
set and was rejected.
The patch just resets urb->interval to its initial value and adds some error
diagnostics (which would have saved me a lot of time during my analysis).

Any comment is welcome.

Best wishes,

Martin.


diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index 3c76101..c5313cb 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -1030,7 +1030,7 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir
 static void mceusb_dev_recv(struct urb *urb)
 {
        struct mceusb_dev *ir;
-       int buf_len;
+       int buf_len, res;
 
        if (!urb)
                return;
@@ -1067,7 +1067,11 @@ static void mceusb_dev_recv(struct urb *urb)
                break;
        }
 
-       usb_submit_urb(urb, GFP_ATOMIC);
+       urb->interval = ir->usb_ep_out->bInterval; /* reset urb interval */
+       res = usb_submit_urb(urb, GFP_ATOMIC);
+       if (res) {
+               mce_dbg(ir->dev, "restart request FAILED! (res=%d)\n", res);
+       }
 }
 
 static void mceusb_get_emulator_version(struct mceusb_dev *ir)

Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab]

Em Sun, 10 Nov 2013 10:50:45 +0000 (UTC)
Martin Kittel <linux@martin-kittel.de> escreveu:

> Hi,
> 
> I had trouble getting my MCE remote control to work on my new Intel
> mainboard. It was working fine with older boards but with the new board
> there would be no reply from the remote just after the setup package was
> received during the init phase.
> I traced the problem down to the mceusb_dev_recv function where the received
> urb is resubmitted again. The problem is that my new board is so blazing
> fast that the initial urb was processed in less than a single 125
> microsecond interval, so the urb as it was received had urb->interval set to 0.
> As the urb is just resubmitted as it came in it now had an invalid interval
> set and was rejected.
> The patch just resets urb->interval to its initial value and adds some error
> diagnostics (which would have saved me a lot of time during my analysis).
> 
> Any comment is welcome.
> 
> Best wishes,

You forgot to send your signed-off-by:
> 
> Martin.
> 
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index 3c76101..c5313cb 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -1030,7 +1030,7 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir
>  static void mceusb_dev_recv(struct urb *urb)
>  {
>         struct mceusb_dev *ir;
> -       int buf_len;
> +       int buf_len, res;
>  

Please use tabs and not spaces. Note: This could be something wrong with your
emailer that could be mangling whitespaces.


>         if (!urb)
>                 return;
> @@ -1067,7 +1067,11 @@ static void mceusb_dev_recv(struct urb *urb)
>                 break;
>         }
>  
> -       usb_submit_urb(urb, GFP_ATOMIC);
> +       urb->interval = ir->usb_ep_out->bInterval; /* reset urb interval */
> +       res = usb_submit_urb(urb, GFP_ATOMIC);
> +       if (res) {
> +               mce_dbg(ir->dev, "restart request FAILED! (res=%d)\n", res);
> +       }

No need for braces here. Just do:
+	if (res)
+		mce_dbg(ir->dev, "restart request FAILED! (res=%d)\n", res);

>  }

>  
>  static void mceusb_get_emulator_version(struct mceusb_dev *ir)
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

Re: Patch mceusb: fix invalid urb interval

[Sean Young]

On Sun, Nov 10, 2013 at 10:50:45AM +0000, Martin Kittel wrote:
> Hi,
> 
> I had trouble getting my MCE remote control to work on my new Intel
> mainboard. It was working fine with older boards but with the new board
> there would be no reply from the remote just after the setup package was
> received during the init phase.
> I traced the problem down to the mceusb_dev_recv function where the received
> urb is resubmitted again. The problem is that my new board is so blazing
> fast that the initial urb was processed in less than a single 125
> microsecond interval, so the urb as it was received had urb->interval set to 0.
> As the urb is just resubmitted as it came in it now had an invalid interval
> set and was rejected.
> The patch just resets urb->interval to its initial value and adds some error
> diagnostics (which would have saved me a lot of time during my analysis).

What mceusb devices is this? Could you provide lsusb -vvv output please?

Also what usb hub are you using? Another user is reporting problems with an
xhci hub.


Sean

Re: Patch mceusb: fix invalid urb interval

[Martin Kittel]

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
      Endpoint Descriptor:
	bLength			7
	bDescriptorType		5
	bEndpointAddress     0x02  EP 2 OUT
	bmAttributes		2
	  Transfer Type		   Bulk
	  Synch Type		   None
	  Usage Type		   Data
	wMaxPacketSize	   0x0040  1x 64 bytes
	bInterval	       10
Device Status:	   0x0000
  (Bus Powered)

-----------

>From 67589c156e4b205821dda67f7e96804224c24cb8 Mon Sep 17 00:00:00 2001
From: Martin Kittel <linux@martin-kittel.de>
Date: Wed, 11 Dec 2013 21:08:49 +0100
Subject: [PATCH] mceusb: fix invalid urb interval

With very fast usb hubs it can happen that urbs are processed
in less than a single 126 microsecond interval. Such an urb has
urb->interval set to 0 on receive and s rejected on resubmit.
Make sure urb->interval is reset to its initial value before
resubmitting it.

Signed-off-by: Martin Kittel <linux@martin-kittel.de>
---
 drivers/media/rc/mceusb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index 3c76101..6652f6a 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -1030,7 +1030,7 @@ static void mceusb_process_ir_data(struct
mceusb_dev *ir, int buf_len)
 static void mceusb_dev_recv(struct urb *urb)
 {
 	struct mceusb_dev *ir;
-	int buf_len;
+	int buf_len, res;

 	if (!urb)
 		return;
@@ -1067,7 +1067,10 @@ static void mceusb_dev_recv(struct urb *urb)
 		break;
 	}

-	usb_submit_urb(urb, GFP_ATOMIC);
+	urb->interval = ir->usb_ep_out->bInterval; /* reset urb interval */
+	res = usb_submit_urb(urb, GFP_ATOMIC);
+	if (res)
+		mce_dbg(ir->dev, "restart request FAILED! (res=%d)\n", res);
 }

 static void mceusb_get_emulator_version(struct mceusb_dev *ir)
-- 
1.8.4.rc3

Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab]

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

Re: Patch mceusb: fix invalid urb interval

[Sean Young]

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.

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

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)


Sean

Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab]

Em Wed, 15 Jan 2014 16:52:45 +0000
Sean Young <sean@mess.org> escreveu:

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

Yes, this could be a possible fix.

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

Maybe then changing the interval to 3 could also fix it, if the device
is using high speed (480 kHz), and 1 otherwise.

E. g. changing the logic to something like:

if (dev->speed == USB_SPEED_HIGH || dev->speed == USB_SPEED_SUPER) {
	if (ep_in)
		ep_in->bInterval = 3;
	if (ep_out)
	 	ep_out->bInterval = 3;
} else {
	if (ep_in)
		ep_in->bInterval = 1;
	if (ep_out)
	 	ep_out->bInterval = 1;
}

At the device probing logic.

I think that using a bulk transfer, if it works, is a better solution,
though.

Regards,
Mauro

Re: Patch mceusb: fix invalid urb interval

[Martin Kittel]

Hi Mauro, hi Sean,

On 01/15/2014 06:59 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 15 Jan 2014 16:52:45 +0000
> Sean Young <sean@mess.org> escreveu:
> 
>> 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.
> 
> Yes, this could be a possible fix.
> 

I have tried this and the driver is working fine without my initial fix.
I haven't been running with the bulk setting for long, but so far I
haven't seen the spurious 'xhci_queue_intr_tx: <number> callbacks
suppressed' messages like I have before.

The current version of the patch against 3.13-rc8 is below.

Please let me know if there is anything else I should check or further
rework is needed.

Thanks for your help and best wishes,

Martin.

-----------

>From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
From: Martin Kittel <linux@martin-kittel.de>
Date: Sun, 19 Jan 2014 21:24:55 +0100
Subject: [PATCH] mceusb: use endpoint xfer mode as advertised

mceusb always sets endpoints to interrupt transfer mode no matter
what the device itself is advertising. This causes trouble on xhci
hubs. This patch changes the behavior to honor the device endpoint
settings.

Signed-off-by: Martin Kittel <linux@martin-kittel.de>
---
 drivers/media/rc/mceusb.c | 51
++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
index a25bb15..67df9a6 100644
--- a/drivers/media/rc/mceusb.c
+++ b/drivers/media/rc/mceusb.c
@@ -1277,32 +1277,37 @@ static int mceusb_dev_probe(struct usb_interface
*intf,

 		if ((ep_in == NULL)
 			&& ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
-			    == USB_DIR_IN)
-			&& (((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
-			    == USB_ENDPOINT_XFER_BULK)
-			|| ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
-			    == USB_ENDPOINT_XFER_INT))) {
-
-			ep_in = ep;
-			ep_in->bmAttributes = USB_ENDPOINT_XFER_INT;
-			ep_in->bInterval = 1;
-			mce_dbg(&intf->dev, "acceptable inbound endpoint "
-				"found\n");
+			    == USB_DIR_IN)) {
+			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+			    == USB_ENDPOINT_XFER_BULK) {
+				ep_in = ep;
+				mce_dbg(&intf->dev, "acceptable bulk inbound endpoint "
+					"found\n");
+			}
+			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+			    == USB_ENDPOINT_XFER_INT) {
+				ep_in = ep;
+				ep_in->bInterval = 1;
+				mce_dbg(&intf->dev, "acceptable interrupt inbound endpoint "
+					"found\n");
+			}
 		}
-
 		if ((ep_out == NULL)
 			&& ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
-			    == USB_DIR_OUT)
-			&& (((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
-			    == USB_ENDPOINT_XFER_BULK)
-			|| ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
-			    == USB_ENDPOINT_XFER_INT))) {
-
-			ep_out = ep;
-			ep_out->bmAttributes = USB_ENDPOINT_XFER_INT;
-			ep_out->bInterval = 1;
-			mce_dbg(&intf->dev, "acceptable outbound endpoint "
-				"found\n");
+			    == USB_DIR_OUT)) {
+			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+			    == USB_ENDPOINT_XFER_BULK) {
+				ep_out = ep;
+				mce_dbg(&intf->dev, "acceptable bulk outbound endpoint "
+					"found\n");
+			}
+			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
+			    == USB_ENDPOINT_XFER_INT) {
+				ep_out = ep;
+				ep_out->bInterval = 1;
+				mce_dbg(&intf->dev, "acceptable interrupt outbound endpoint "
+					"found\n");
+			}
 		}
 	}
 	if (ep_in == NULL) {
-- 
1.8.4.rc3

Re: Patch mceusb: fix invalid urb interval

[Sean Young]

On Sun, Jan 19, 2014 at 10:05:15PM +0100, Martin Kittel wrote:
> Hi Mauro, hi Sean,
> 
> On 01/15/2014 06:59 PM, Mauro Carvalho Chehab wrote:
> > Em Wed, 15 Jan 2014 16:52:45 +0000
> > Sean Young <sean@mess.org> escreveu:
> > 
> >> 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.
> > 
> > Yes, this could be a possible fix.
> > 
> 
> I have tried this and the driver is working fine without my initial fix.
> I haven't been running with the bulk setting for long, but so far I
> haven't seen the spurious 'xhci_queue_intr_tx: <number> callbacks
> suppressed' messages like I have before.
> 
> The current version of the patch against 3.13-rc8 is below.
> 
> Please let me know if there is anything else I should check or further
> rework is needed.
> 
> Thanks for your help and best wishes,
> 
> Martin.
> 
> -----------
> 
> >From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
> From: Martin Kittel <linux@martin-kittel.de>
> Date: Sun, 19 Jan 2014 21:24:55 +0100
> Subject: [PATCH] mceusb: use endpoint xfer mode as advertised
> 
> mceusb always sets endpoints to interrupt transfer mode no matter
> what the device itself is advertising. This causes trouble on xhci
> hubs. This patch changes the behavior to honor the device endpoint
> settings.

This patch is wrong. I get:

[   60.962727] ------------[ cut here ]------------
[   60.962729] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:452 usb_submit_u
rb+0x1fd/0x5b0()
[   60.962730] usb 3-2: BOGUS urb xfer, pipe 1 != type 3

This is because the patch no longer sets the endpoints to interrupt
endpoints, but still uses the interrupt functions like
usb_fill_int_urb().

> 
> Signed-off-by: Martin Kittel <linux@martin-kittel.de>
> ---
>  drivers/media/rc/mceusb.c | 51
> ++++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c
> index a25bb15..67df9a6 100644
> --- a/drivers/media/rc/mceusb.c
> +++ b/drivers/media/rc/mceusb.c
> @@ -1277,32 +1277,37 @@ static int mceusb_dev_probe(struct usb_interface
> *intf,
> 
>  		if ((ep_in == NULL)
>  			&& ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
> -			    == USB_DIR_IN)
> -			&& (((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> -			    == USB_ENDPOINT_XFER_BULK)
> -			|| ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> -			    == USB_ENDPOINT_XFER_INT))) {
> -
> -			ep_in = ep;
> -			ep_in->bmAttributes = USB_ENDPOINT_XFER_INT;
> -			ep_in->bInterval = 1;
> -			mce_dbg(&intf->dev, "acceptable inbound endpoint "
> -				"found\n");
> +			    == USB_DIR_IN)) {
> +			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> +			    == USB_ENDPOINT_XFER_BULK) {
> +				ep_in = ep;
> +				mce_dbg(&intf->dev, "acceptable bulk inbound endpoint "
> +					"found\n");
> +			}
> +			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> +			    == USB_ENDPOINT_XFER_INT) {
> +				ep_in = ep;
> +				ep_in->bInterval = 1;
> +				mce_dbg(&intf->dev, "acceptable interrupt inbound endpoint "
> +					"found\n");
> +			}
>  		}
> -
>  		if ((ep_out == NULL)
>  			&& ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
> -			    == USB_DIR_OUT)
> -			&& (((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> -			    == USB_ENDPOINT_XFER_BULK)
> -			|| ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> -			    == USB_ENDPOINT_XFER_INT))) {
> -
> -			ep_out = ep;
> -			ep_out->bmAttributes = USB_ENDPOINT_XFER_INT;
> -			ep_out->bInterval = 1;
> -			mce_dbg(&intf->dev, "acceptable outbound endpoint "
> -				"found\n");
> +			    == USB_DIR_OUT)) {
> +			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> +			    == USB_ENDPOINT_XFER_BULK) {
> +				ep_out = ep;
> +				mce_dbg(&intf->dev, "acceptable bulk outbound endpoint "
> +					"found\n");
> +			}
> +			if ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> +			    == USB_ENDPOINT_XFER_INT) {
> +				ep_out = ep;
> +				ep_out->bInterval = 1;
> +				mce_dbg(&intf->dev, "acceptable interrupt outbound endpoint "
> +					"found\n");
> +			}
>  		}
>  	}
>  	if (ep_in == NULL) {
> -- 
> 1.8.4.rc3
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Patch mceusb: fix invalid urb interval

[Jarod Wilson]

On Sun, Jan 19, 2014 at 09:56:48PM +0000, Sean Young wrote:
> On Sun, Jan 19, 2014 at 10:05:15PM +0100, Martin Kittel wrote:
> > Hi Mauro, hi Sean,
...
> > >From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
> > From: Martin Kittel <linux@martin-kittel.de>
> > Date: Sun, 19 Jan 2014 21:24:55 +0100
> > Subject: [PATCH] mceusb: use endpoint xfer mode as advertised
> > 
> > mceusb always sets endpoints to interrupt transfer mode no matter
> > what the device itself is advertising. This causes trouble on xhci
> > hubs. This patch changes the behavior to honor the device endpoint
> > settings.
> 
> This patch is wrong. I get:
> 
> [   60.962727] ------------[ cut here ]------------
> [   60.962729] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:452 usb_submit_u
> rb+0x1fd/0x5b0()
> [   60.962730] usb 3-2: BOGUS urb xfer, pipe 1 != type 3
> 
> This is because the patch no longer sets the endpoints to interrupt
> endpoints, but still uses the interrupt functions like
> usb_fill_int_urb().

Crap, I sent a working patch to everyone a few days ago, but from a new
host that didn't have relay stuff set up yet, so I don't think anyone got
the message. Oops... I'll try to dig it back up. Its a quick fix, but its
tested as fully functional on multiple devices here, including a mix of
ones that claim bulk and interrupt, ones with no bInterval, ones with
different non-0 bIntervals, etc.

-- 
Jarod Wilson
jarod@redhat.com

Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab]

Em Mon, 20 Jan 2014 12:36:26 -0500
Jarod Wilson <jarod@redhat.com> escreveu:

> On Sun, Jan 19, 2014 at 09:56:48PM +0000, Sean Young wrote:
> > On Sun, Jan 19, 2014 at 10:05:15PM +0100, Martin Kittel wrote:
> > > Hi Mauro, hi Sean,
> ...
> > > >From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
> > > From: Martin Kittel <linux@martin-kittel.de>
> > > Date: Sun, 19 Jan 2014 21:24:55 +0100
> > > Subject: [PATCH] mceusb: use endpoint xfer mode as advertised
> > > 
> > > mceusb always sets endpoints to interrupt transfer mode no matter
> > > what the device itself is advertising. This causes trouble on xhci
> > > hubs. This patch changes the behavior to honor the device endpoint
> > > settings.
> > 
> > This patch is wrong. I get:
> > 
> > [   60.962727] ------------[ cut here ]------------
> > [   60.962729] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:452 usb_submit_u
> > rb+0x1fd/0x5b0()
> > [   60.962730] usb 3-2: BOGUS urb xfer, pipe 1 != type 3
> > 
> > This is because the patch no longer sets the endpoints to interrupt
> > endpoints, but still uses the interrupt functions like
> > usb_fill_int_urb().
> 
> Crap, I sent a working patch to everyone a few days ago, but from a new
> host that didn't have relay stuff set up yet, so I don't think anyone got
> the message. Oops... I'll try to dig it back up. Its a quick fix, but its
> tested as fully functional on multiple devices here, including a mix of
> ones that claim bulk and interrupt, ones with no bInterval, ones with
> different non-0 bIntervals, etc.

Hi All,

This is still pending on my queue. Any news?

Regards,
Mauro

Re: Patch mceusb: fix invalid urb interval

[Sean Young]

On Mon, Nov 03, 2014 at 02:49:45PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 20 Jan 2014 12:36:26 -0500
> Jarod Wilson <jarod@redhat.com> escreveu:
> 
> > On Sun, Jan 19, 2014 at 09:56:48PM +0000, Sean Young wrote:
> > > On Sun, Jan 19, 2014 at 10:05:15PM +0100, Martin Kittel wrote:
> > > > Hi Mauro, hi Sean,
> > ...
> > > > >From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
> > > > From: Martin Kittel <linux@martin-kittel.de>
> > > > Date: Sun, 19 Jan 2014 21:24:55 +0100
> > > > Subject: [PATCH] mceusb: use endpoint xfer mode as advertised
> > > > 
> > > > mceusb always sets endpoints to interrupt transfer mode no matter
> > > > what the device itself is advertising. This causes trouble on xhci
> > > > hubs. This patch changes the behavior to honor the device endpoint
> > > > settings.
> > > 
> > > This patch is wrong. I get:
> > > 
> > > [   60.962727] ------------[ cut here ]------------
> > > [   60.962729] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:452 usb_submit_u
> > > rb+0x1fd/0x5b0()
> > > [   60.962730] usb 3-2: BOGUS urb xfer, pipe 1 != type 3
> > > 
> > > This is because the patch no longer sets the endpoints to interrupt
> > > endpoints, but still uses the interrupt functions like
> > > usb_fill_int_urb().
> > 
> > Crap, I sent a working patch to everyone a few days ago, but from a new
> > host that didn't have relay stuff set up yet, so I don't think anyone got
> > the message. Oops... I'll try to dig it back up. Its a quick fix, but its
> > tested as fully functional on multiple devices here, including a mix of
> > ones that claim bulk and interrupt, ones with no bInterval, ones with
> > different non-0 bIntervals, etc.
> 
> Hi All,
> 
> This is still pending on my queue. Any news?

I'm pretty sure the proper fix for this problem has been merged already:

commit 0cacb46ace1f433f0ab02af10686f6dc50b5d268
Author: Matt DeVillier <matt.devillier@gmail.com>
Date:   Thu Apr 24 11:16:31 2014 -0300

    [media] fix mceusb endpoint type identification/handling


Sean

Re: Patch mceusb: fix invalid urb interval

[Mauro Carvalho Chehab]

Em Tue, 04 Nov 2014 21:25:49 +0000
Sean Young <sean@mess.org> escreveu:

> On Mon, Nov 03, 2014 at 02:49:45PM -0200, Mauro Carvalho Chehab wrote:
> > Em Mon, 20 Jan 2014 12:36:26 -0500
> > Jarod Wilson <jarod@redhat.com> escreveu:
> > 
> > > On Sun, Jan 19, 2014 at 09:56:48PM +0000, Sean Young wrote:
> > > > On Sun, Jan 19, 2014 at 10:05:15PM +0100, Martin Kittel wrote:
> > > > > Hi Mauro, hi Sean,
> > > ...
> > > > > >From a71676dad29adef9cafb08598e693ec308ba2e95 Mon Sep 17 00:00:00 2001
> > > > > From: Martin Kittel <linux@martin-kittel.de>
> > > > > Date: Sun, 19 Jan 2014 21:24:55 +0100
> > > > > Subject: [PATCH] mceusb: use endpoint xfer mode as advertised
> > > > > 
> > > > > mceusb always sets endpoints to interrupt transfer mode no matter
> > > > > what the device itself is advertising. This causes trouble on xhci
> > > > > hubs. This patch changes the behavior to honor the device endpoint
> > > > > settings.
> > > > 
> > > > This patch is wrong. I get:
> > > > 
> > > > [   60.962727] ------------[ cut here ]------------
> > > > [   60.962729] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:452 usb_submit_u
> > > > rb+0x1fd/0x5b0()
> > > > [   60.962730] usb 3-2: BOGUS urb xfer, pipe 1 != type 3
> > > > 
> > > > This is because the patch no longer sets the endpoints to interrupt
> > > > endpoints, but still uses the interrupt functions like
> > > > usb_fill_int_urb().
> > > 
> > > Crap, I sent a working patch to everyone a few days ago, but from a new
> > > host that didn't have relay stuff set up yet, so I don't think anyone got
> > > the message. Oops... I'll try to dig it back up. Its a quick fix, but its
> > > tested as fully functional on multiple devices here, including a mix of
> > > ones that claim bulk and interrupt, ones with no bInterval, ones with
> > > different non-0 bIntervals, etc.
> > 
> > Hi All,
> > 
> > This is still pending on my queue. Any news?
> 
> I'm pretty sure the proper fix for this problem has been merged already:
> 
> commit 0cacb46ace1f433f0ab02af10686f6dc50b5d268
> Author: Matt DeVillier <matt.devillier@gmail.com>
> Date:   Thu Apr 24 11:16:31 2014 -0300
> 
>     [media] fix mceusb endpoint type identification/handling

Ah, OK. It seems I forgot to remove this from my queue.

Marked as superseed.

Thanks!
Mauro

Re: Patch mceusb: fix invalid urb interval

[Jarod Wilson]

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

Re: Patch mceusb: fix invalid urb interval

[Sean Young]

On Wed, Jan 15, 2014 at 09:55:59PM -0500, Jarod Wilson wrote:
> 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).

I have one device with bulk endpoints, an acer device. I cannot get it to
work on an xhci hub at all. The xhci host just returns completion code
4 (tx error), indicating that the client device did not respond correctly.

I'm trying the logic analyzer but so far no luck getting it to decode usb.

At least for this bug the bInterval is just a red herring.


Sean