| Message ID | Pine.LNX.4.44L0.1607121150390.1900-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I never received any replies to this message. Should the patch I suggested be merged? Alan Stern On Tue, 12 Jul 2016, Alan Stern wrote: > On Sat, 9 Jul 2016, Mauro Carvalho Chehab wrote: > > > C/C linux-usb Mailing list: > > > > > > Em Wed, 18 May 2016 08:52:28 -0600 > > Wade Berrier <wberrier@gmail.com> escreveu: > > ... > > > > > That message above links to some other threads describing the issue. > > > > Here's a post with a patch that supposedly works: > > > > > > > > http://www.gossamer-threads.com/lists/mythtv/users/587930 > > > > > > > > No idea if that's the "correct" way to fix this. > > > > > > > > I'll be trying that out and then report back... > > > > > > Indeed, this patch does fix the issue: > > > > > > ---------------------- > > > > > > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c > > > index 31ccdcc..03321d4 100644 > > > --- a/drivers/usb/core/config.c > > > +++ b/drivers/usb/core/config.c > > > @@ -247,7 +247,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum, > > > /* For low-speed, 10 ms is the official minimum. > > > * But some "overclocked" devices might want faster > > > * polling so we'll allow it. */ > > > - n = 32; > > > + n = 10; > > > break; > > > } > > > } else if (usb_endpoint_xfer_isoc(d)) { > > > > > > > > > ---------------------- > > > > > > Is this change appropriate to be pushed upstream? Where to go from > > > here? > > > > This issue is at the USB core. So, it should be reported to the > > linux-usb mailing list. > > > > The people there should help about how to proceed to get this > > fixed upstream. > > Here's a proper version of that patch. If this is okay, it can be > merged. > > Alan Stern > > > > Index: usb-4.x/drivers/usb/core/config.c > =================================================================== > --- usb-4.x.orig/drivers/usb/core/config.c > +++ usb-4.x/drivers/usb/core/config.c > @@ -213,8 +213,10 @@ static int usb_parse_endpoint(struct dev > memcpy(&endpoint->desc, d, n); > INIT_LIST_HEAD(&endpoint->urb_list); > > - /* Fix up bInterval values outside the legal range. Use 32 ms if no > - * proper value can be guessed. */ > + /* > + * Fix up bInterval values outside the legal range. > + * Use 10 or 8 ms if no proper value can be guessed. > + */ > i = 0; /* i = min, j = max, n = default */ > j = 255; > if (usb_endpoint_xfer_int(d)) { > @@ -223,13 +225,15 @@ static int usb_parse_endpoint(struct dev > case USB_SPEED_SUPER_PLUS: > case USB_SPEED_SUPER: > case USB_SPEED_HIGH: > - /* Many device manufacturers are using full-speed > + /* > + * Many device manufacturers are using full-speed > * bInterval values in high-speed interrupt endpoint > * descriptors. Try to fix those and fall back to a > - * 32 ms default value otherwise. */ > + * 8 ms default value otherwise. > + */ > n = fls(d->bInterval*8); > if (n == 0) > - n = 9; /* 32 ms = 2^(9-1) uframes */ > + n = 7; /* 8 ms = 2^(7-1) uframes */ > j = 16; > > /* > @@ -247,7 +251,7 @@ static int usb_parse_endpoint(struct dev > /* For low-speed, 10 ms is the official minimum. > * But some "overclocked" devices might want faster > * polling so we'll allow it. */ > - n = 32; > + n = 10; > break; > } > } else if (usb_endpoint_xfer_isoc(d)) { > @@ -255,10 +259,10 @@ static int usb_parse_endpoint(struct dev > j = 16; > switch (to_usb_device(ddev)->speed) { > case USB_SPEED_HIGH: > - n = 9; /* 32 ms = 2^(9-1) uframes */ > + n = 7; /* 8 ms = 2^(7-1) uframes */ > break; > default: /* USB_SPEED_FULL */ > - n = 6; /* 32 ms = 2^(6-1) frames */ > + n = 4; /* 8 ms = 2^(4-1) frames */ > break; > } > } -- 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 Thu Aug 11 16:18, Alan Stern wrote: > I never received any replies to this message. Should the patch I > suggested be merged? > Hello, I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb transceiver works as expected. Thanks, Wade > > > On Tue, 12 Jul 2016, Alan Stern wrote: > > > On Sat, 9 Jul 2016, Mauro Carvalho Chehab wrote: > > > > > C/C linux-usb Mailing list: > > > > > > > > > Em Wed, 18 May 2016 08:52:28 -0600 > > > Wade Berrier <wberrier@gmail.com> escreveu: > > > > ... > > > > > > > That message above links to some other threads describing the issue. > > > > > Here's a post with a patch that supposedly works: > > > > > > > > > > http://www.gossamer-threads.com/lists/mythtv/users/587930 > > > > > > > > > > No idea if that's the "correct" way to fix this. > > > > > > > > > > I'll be trying that out and then report back... > > > > > > > > Indeed, this patch does fix the issue: > > > > > > > > ---------------------- > > > > > > > > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c > > > > index 31ccdcc..03321d4 100644 > > > > --- a/drivers/usb/core/config.c > > > > +++ b/drivers/usb/core/config.c > > > > @@ -247,7 +247,7 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum, > > > > /* For low-speed, 10 ms is the official minimum. > > > > * But some "overclocked" devices might want faster > > > > * polling so we'll allow it. */ > > > > - n = 32; > > > > + n = 10; > > > > break; > > > > } > > > > } else if (usb_endpoint_xfer_isoc(d)) { > > > > > > > > > > > > ---------------------- > > > > > > > > Is this change appropriate to be pushed upstream? Where to go from > > > > here? > > > > > > This issue is at the USB core. So, it should be reported to the > > > linux-usb mailing list. > > > > > > The people there should help about how to proceed to get this > > > fixed upstream. > > > > Here's a proper version of that patch. If this is okay, it can be > > merged. > > > > Alan Stern > > > > > > > > Index: usb-4.x/drivers/usb/core/config.c > > =================================================================== > > --- usb-4.x.orig/drivers/usb/core/config.c > > +++ usb-4.x/drivers/usb/core/config.c > > @@ -213,8 +213,10 @@ static int usb_parse_endpoint(struct dev > > memcpy(&endpoint->desc, d, n); > > INIT_LIST_HEAD(&endpoint->urb_list); > > > > - /* Fix up bInterval values outside the legal range. Use 32 ms if no > > - * proper value can be guessed. */ > > + /* > > + * Fix up bInterval values outside the legal range. > > + * Use 10 or 8 ms if no proper value can be guessed. > > + */ > > i = 0; /* i = min, j = max, n = default */ > > j = 255; > > if (usb_endpoint_xfer_int(d)) { > > @@ -223,13 +225,15 @@ static int usb_parse_endpoint(struct dev > > case USB_SPEED_SUPER_PLUS: > > case USB_SPEED_SUPER: > > case USB_SPEED_HIGH: > > - /* Many device manufacturers are using full-speed > > + /* > > + * Many device manufacturers are using full-speed > > * bInterval values in high-speed interrupt endpoint > > * descriptors. Try to fix those and fall back to a > > - * 32 ms default value otherwise. */ > > + * 8 ms default value otherwise. > > + */ > > n = fls(d->bInterval*8); > > if (n == 0) > > - n = 9; /* 32 ms = 2^(9-1) uframes */ > > + n = 7; /* 8 ms = 2^(7-1) uframes */ > > j = 16; > > > > /* > > @@ -247,7 +251,7 @@ static int usb_parse_endpoint(struct dev > > /* For low-speed, 10 ms is the official minimum. > > * But some "overclocked" devices might want faster > > * polling so we'll allow it. */ > > - n = 32; > > + n = 10; > > break; > > } > > } else if (usb_endpoint_xfer_isoc(d)) { > > @@ -255,10 +259,10 @@ static int usb_parse_endpoint(struct dev > > j = 16; > > switch (to_usb_device(ddev)->speed) { > > case USB_SPEED_HIGH: > > - n = 9; /* 32 ms = 2^(9-1) uframes */ > > + n = 7; /* 8 ms = 2^(7-1) uframes */ > > break; > > default: /* USB_SPEED_FULL */ > > - n = 6; /* 32 ms = 2^(6-1) frames */ > > + n = 4; /* 8 ms = 2^(4-1) frames */ > > break; > > } > > } > > -- 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 Sat, 10 Sep 2016, Wade Berrier wrote: > On Thu Aug 11 16:18, Alan Stern wrote: > > I never received any replies to this message. Should the patch I > > suggested be merged? > > > > Hello, > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > transceiver works as expected. Thank you for testing. Can you provide the "lsusb -v" output for the troublesome IR transceiver? Alan Stern -- 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
Mauro: I just took a look at the mceusb.c source file under drivers/media/rc/. The probe routine checks that ep_in != NULL, but it doesn't check ep_out. This can lead to a NULL-pointer dereference later on, crashing the driver. Such a crash was reported here: http://marc.info/?l=mythtv-users&m=144131333703197&w=2 You should a check for ep_out to the probe routine. Alan Stern -- 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 Thu Sep 15 15:13, Alan Stern wrote: > On Sat, 10 Sep 2016, Wade Berrier wrote: > > > On Thu Aug 11 16:18, Alan Stern wrote: > > > I never received any replies to this message. Should the patch I > > > suggested be merged? > > > > > > > Hello, > > > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > > transceiver works as expected. > > Thank you for testing. Can you provide the "lsusb -v" output for the > troublesome IR transceiver? > Here's the output: Bus 001 Device 006: ID 1784:0006 TopSeed Technology Corp. eHome Infrared Transceiver Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 2.00 bDeviceClass 0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 8 idVendor 0x1784 TopSeed Technology Corp. idProduct 0x0006 eHome Infrared Transceiver bcdDevice 1.02 iManufacturer 1 TopSeed Technology Corp. iProduct 2 eHome Infrared Transceiver iSerial 3 TS004RrP bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 32 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 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 255 Vendor Specific Subclass bInterfaceProtocol 255 Vendor Specific Protocol iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x01 EP 1 OUT bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 0 Device Status: 0x0001 Self Powered Wade -- 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 Thu, 15 Sep 2016, Wade Berrier wrote: > On Thu Sep 15 15:13, Alan Stern wrote: > > On Sat, 10 Sep 2016, Wade Berrier wrote: > > > > > On Thu Aug 11 16:18, Alan Stern wrote: > > > > I never received any replies to this message. Should the patch I > > > > suggested be merged? > > > > > > > > > > Hello, > > > > > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > > > transceiver works as expected. > > > > Thank you for testing. Can you provide the "lsusb -v" output for the > > troublesome IR transceiver? > > > > Here's the output: > > Bus 001 Device 006: ID 1784:0006 TopSeed Technology Corp. eHome Infrared Transceiver > Device Descriptor: > bLength 18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 0 > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize0 8 > idVendor 0x1784 TopSeed Technology Corp. > idProduct 0x0006 eHome Infrared Transceiver > bcdDevice 1.02 > iManufacturer 1 TopSeed Technology Corp. > iProduct 2 eHome Infrared Transceiver > iSerial 3 TS004RrP > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 32 > bNumInterfaces 1 > bConfigurationValue 1 > iConfiguration 0 > 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 255 Vendor Specific Subclass > bInterfaceProtocol 255 Vendor Specific Protocol > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x01 EP 1 OUT > bmAttributes 3 > Transfer Type Interrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0020 1x 32 bytes > bInterval 0 And there's the problem. 0 is an invalid bInterval value for an Interrupt endpoint. > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes 3 > Transfer Type Interrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0020 1x 32 bytes > bInterval 0 Here too. > Device Status: 0x0001 > Self Powered > > Wade Thank you. The patch has been submitted. Alan Stern -- 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 Fri, 16 Sep 2016 10:25:31 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> escreveu: > On Thu, 15 Sep 2016, Wade Berrier wrote: > > > On Thu Sep 15 15:13, Alan Stern wrote: > > > On Sat, 10 Sep 2016, Wade Berrier wrote: > > > > > > > On Thu Aug 11 16:18, Alan Stern wrote: > > > > > I never received any replies to this message. Should the patch I > > > > > suggested be merged? > > > > > > > > > > > > > Hello, > > > > > > > > I applied this updated patch to the fedora23 4.7.2 kernel and the mceusb > > > > transceiver works as expected. > > > > > > Thank you for testing. Can you provide the "lsusb -v" output for the > > > troublesome IR transceiver? > > > > > > > Here's the output: > > > > Bus 001 Device 006: ID 1784:0006 TopSeed Technology Corp. eHome Infrared Transceiver > > Device Descriptor: > > bLength 18 > > bDescriptorType 1 > > bcdUSB 2.00 > > bDeviceClass 0 > > bDeviceSubClass 0 > > bDeviceProtocol 0 > > bMaxPacketSize0 8 > > idVendor 0x1784 TopSeed Technology Corp. > > idProduct 0x0006 eHome Infrared Transceiver > > bcdDevice 1.02 > > iManufacturer 1 TopSeed Technology Corp. > > iProduct 2 eHome Infrared Transceiver > > iSerial 3 TS004RrP > > bNumConfigurations 1 > > Configuration Descriptor: > > bLength 9 > > bDescriptorType 2 > > wTotalLength 32 > > bNumInterfaces 1 > > bConfigurationValue 1 > > iConfiguration 0 > > 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 255 Vendor Specific Subclass > > bInterfaceProtocol 255 Vendor Specific Protocol > > iInterface 0 > > Endpoint Descriptor: > > bLength 7 > > bDescriptorType 5 > > bEndpointAddress 0x01 EP 1 OUT > > bmAttributes 3 > > Transfer Type Interrupt > > Synch Type None > > Usage Type Data > > wMaxPacketSize 0x0020 1x 32 bytes > > bInterval 0 > > And there's the problem. 0 is an invalid bInterval value for an > Interrupt endpoint. Unfortunately, it is a know issue that some mceusb drivers have the bInterval set to zero. > > Device Status: 0x0001 > > Self Powered > > > > Wade > > Thank you. The patch has been submitted. 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
Index: usb-4.x/drivers/usb/core/config.c =================================================================== --- usb-4.x.orig/drivers/usb/core/config.c +++ usb-4.x/drivers/usb/core/config.c @@ -213,8 +213,10 @@ static int usb_parse_endpoint(struct dev memcpy(&endpoint->desc, d, n); INIT_LIST_HEAD(&endpoint->urb_list); - /* Fix up bInterval values outside the legal range. Use 32 ms if no - * proper value can be guessed. */ + /* + * Fix up bInterval values outside the legal range. + * Use 10 or 8 ms if no proper value can be guessed. + */ i = 0; /* i = min, j = max, n = default */ j = 255; if (usb_endpoint_xfer_int(d)) { @@ -223,13 +225,15 @@ static int usb_parse_endpoint(struct dev case USB_SPEED_SUPER_PLUS: case USB_SPEED_SUPER: case USB_SPEED_HIGH: - /* Many device manufacturers are using full-speed + /* + * Many device manufacturers are using full-speed * bInterval values in high-speed interrupt endpoint * descriptors. Try to fix those and fall back to a - * 32 ms default value otherwise. */ + * 8 ms default value otherwise. + */ n = fls(d->bInterval*8); if (n == 0) - n = 9; /* 32 ms = 2^(9-1) uframes */ + n = 7; /* 8 ms = 2^(7-1) uframes */ j = 16; /* @@ -247,7 +251,7 @@ static int usb_parse_endpoint(struct dev /* For low-speed, 10 ms is the official minimum. * But some "overclocked" devices might want faster * polling so we'll allow it. */ - n = 32; + n = 10; break; } } else if (usb_endpoint_xfer_isoc(d)) { @@ -255,10 +259,10 @@ static int usb_parse_endpoint(struct dev j = 16; switch (to_usb_device(ddev)->speed) { case USB_SPEED_HIGH: - n = 9; /* 32 ms = 2^(9-1) uframes */ + n = 7; /* 8 ms = 2^(7-1) uframes */ break; default: /* USB_SPEED_FULL */ - n = 6; /* 32 ms = 2^(6-1) frames */ + n = 4; /* 8 ms = 2^(4-1) frames */ break; } }