> Ok, then only one question remains: How do I get this patch applied to
> the official kernel tree?
>
> Thanks & best regards,
> Christian
I already forwarded your patch to [email protected].
No one seems to have picked it up till now.
This seems to be -stable material since it's a clear cut bug with bad
consequences.
Chris Wright - is the below patch acceptable for -stable?
If no one else cares, there is always Andrew Morton and -mm ! :)
Parag
---------------------- Forwarded Message: ---------------------
From: Christian Krause <[email protected]>
To: [email protected]
Subject: bug in handling of highspeed usb HID devices
Date: Wed, 12 Oct 2005 19:56:16 +0000
Hi folks,
During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.
What happened?
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
driver/usb/input/hid-core.c:
line 1669:
/* 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:
line 1685:
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:
line 933:
if (dev->speed == USB_SPEED_HIGH)
urb->interval = 1 << (interval - 1);
else
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:
line 353:
/* too small? */
if (urb->interval <= 0)
return -EINVAL;
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
hid-core.c.
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.
Here is a small patch which solves the whole problem:
--------------------------
--- hid-core.c.old 2005-10-12 21:29:29.000000000 +0200
+++ hid-core.c 2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@
if ((endpoint->bmAttributes & 3) != 3) /* Not an
interrupt endpoint */
continue;
- /* 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;
----------------------------
Please review my investigation and if you come to the same conclusion
please apply this patch in the next kernel versions. If not, please tell
me why. ;-)
Best regards,
Christian
* [email protected] ([email protected]) wrote:
> This seems to be -stable material since it's a clear cut bug with bad
> consequences.
>
> Chris Wright - is the below patch acceptable for -stable?
This all looks fine. But two things, please send -stable patches to
[email protected], and I've not seen an ACK from any usb developers.
thanks,
-chris
On Thu, Oct 13, 2005 at 02:25:18PM -0700, Chris Wright wrote:
> * [email protected] ([email protected]) wrote:
> > This seems to be -stable material since it's a clear cut bug with bad
> > consequences.
> >
> > Chris Wright - is the below patch acceptable for -stable?
>
> This all looks fine. But two things, please send -stable patches to
> [email protected], and I've not seen an ACK from any usb developers.
I haven't seen a patch even _sent_ in a format that it could be applied
in (please read Documentation/SubmittingPatches.)
thanks,
greg k-h
On Thu, Oct 13, 2005 at 02:25:18PM -0700, Chris Wright wrote:
> * [email protected] ([email protected]) wrote:
> > This seems to be -stable material since it's a clear cut bug with bad
> > consequences.
> >
> > Chris Wright - is the below patch acceptable for -stable?
Also, I don't think this should go into -stable, as there are no
high-speed HID devices available right now, so it really isn't affecting
anyone :)
thanks,
greg k-h
On Fri, 14 Oct 2005 10:13:26 -0700, Greg KH wrote:
> On Thu, Oct 13, 2005 at 02:25:18PM -0700, Chris Wright wrote:
>> * [email protected] ([email protected]) wrote:
>> > This seems to be -stable material since it's a clear cut bug with bad
>> > consequences.
>> >
>> > Chris Wright - is the below patch acceptable for -stable?
> Also, I don't think this should go into -stable, as there are no
> high-speed HID devices available right now, so it really isn't affecting
> anyone :)
No, that's no correct. There are such devices:
Newer KVM-over-IP appliances, like the Avocent's DSR2030 emulates
keyboard, mouse and a mass storage device over USB. I've tested this
specific device with linux and the USB transfers are Highspeed and the
USB device has one interface which is HID. So there are Highspeed USB
devices out there.
With all respect, but I don't really understand why this is important.
The linux kernel clearly violates the USB specification. The calculation of
the real polling interval for interrupt endpoints is
i = 2^(bInterval-1) (according to USB 2.0 spec, chapter 9.6.6) and _not_
i = 2^((2^(bInterval-1))-1) as it is calculated in linux.
This is clearly a bug in linux and should be fixed.
Changing this has _no_ impact on other devices than Highspeed HID devices
and only this patch allows them to work. And there are such devices. At
least Avocent's DSR2030.
Best regards,
Christian
On Fri, Oct 14, 2005 at 07:46:02PM +0200, Christian Krause wrote:
> On Fri, 14 Oct 2005 10:13:26 -0700, Greg KH wrote:
>
> > On Thu, Oct 13, 2005 at 02:25:18PM -0700, Chris Wright wrote:
> >> * [email protected] ([email protected]) wrote:
> >> > This seems to be -stable material since it's a clear cut bug with bad
> >> > consequences.
> >> >
> >> > Chris Wright - is the below patch acceptable for -stable?
>
> > Also, I don't think this should go into -stable, as there are no
> > high-speed HID devices available right now, so it really isn't affecting
> > anyone :)
>
> No, that's no correct. There are such devices:
>
> Newer KVM-over-IP appliances, like the Avocent's DSR2030 emulates
> keyboard, mouse and a mass storage device over USB. I've tested this
> specific device with linux and the USB transfers are Highspeed and the
> USB device has one interface which is HID. So there are Highspeed USB
> devices out there.
Ah, ok, thanks, I wasn't aware of these devices.
> With all respect, but I don't really understand why this is important.
> The linux kernel clearly violates the USB specification. The calculation of
> the real polling interval for interrupt endpoints is
> i = 2^(bInterval-1) (according to USB 2.0 spec, chapter 9.6.6) and _not_
> i = 2^((2^(bInterval-1))-1) as it is calculated in linux.
> This is clearly a bug in linux and should be fixed.
No one is arguing that it shouldn't be.
thanks,
greg k-h