[PATCH] Re: bug in handling of highspeed usb HID devices
From: Christian Krause
Date: Fri Oct 14 2005 - 17:07:20 EST
On Thu, 13 Oct 2005 15:48:39 -0700, Greg KH wrote:
> On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
>> Here is a small patch which solves the whole problem:
> The patch is at the wrong level, and has spaces instead of tabs.
> And no "signed-off-by" line :(
> Take a look at Documentation/SubmittingPatches for how to create a patch
> that I can apply and forward on.
Ok, next try with Signed-off-by above the patch. Please apologize the
During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.
Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.
The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).
After some debugging in hid-core.c I've found the reason.
In case of a highspeed device, the endpoint interval is re-calculated in
/* handle potential highspeed HID correctly */
interval = endpoint->bInterval;
if (dev->speed == USB_SPEED_HIGH)
interval = 1 << (interval - 1);
Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
hid_irq_in, hid, interval);
Unfortunately the same calculation as above is done a second time in
usb_fill_int_urb in the file include/linux/usb.h:
if (dev->speed == USB_SPEED_HIGH)
urb->interval = 1 << (interval - 1);
urb->interval = interval;
This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:
hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow
Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
/* too small? */
if (urb->interval <= 0)
The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)
Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.
Signed-off-by: Christian Krause <chkr@xxxxxxxxxxx>
--- linux-126.96.36.199/drivers/usb/input/hid-core.c.old 2005-10-12 21:29:29.000000000 +0200
+++ linux-188.8.131.52/drivers/usb/input/hid-core.c 2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@ static struct hid_device *usb_hid_config
if ((endpoint->bmAttributes & 3) != 3) /* Not an interrupt endpoint */
- /* handle potential highspeed HID correctly */
- interval = endpoint->bInterval;
- if (dev->speed == USB_SPEED_HIGH)
- interval = 1 << (interval - 1);
/* Change the polling interval of mice. */
if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
interval = hid_mousepoll_interval;
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/