| Message ID | 52DC3E0B.6010202@martin-kittel.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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"); + } } }