| Message ID | 20140115134917.1450f87c@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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 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 -- 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 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...
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 -- 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 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"); }