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
>
>
> 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.
>
Patch looks correct to me from a purely logical perspective. (IOW I read that file first time :)
But since interval is passed as a parameter to the usb_fill_int_urb() function, I think it is more natural to remove the recalculation from usb_fill_int_urb() - If caller passes a parameter and has enough info to determine its value, it makes sense for the caller to pass in the right value and the callee to just take it as it is.
Not a big deal anyway though.
Parag
Hi,
On Wed, 12 Oct 2005 21:10:53 +0000, kernel-stuff wrote:
>> 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.
>>
> Patch looks correct to me from a purely logical perspective. (IOW I
> read that file first time :)
> But since interval is passed as a parameter to the usb_fill_int_urb()
> function, I think it is more natural to remove the recalculation from
> usb_fill_int_urb() - If caller passes a parameter and has enough info
> to determine its value, it makes sense for the caller to pass in the
> right value and the callee to just take it as it is.
Yes, but in this case we have to check all callers of usb_fill_int_urb
to do the recalculation. E.g. in input/usbmouse.c
endpoint->bInterval is used directly as parameter to usb_fill_int_urb.
To avoid breaking things (my suggested patch has no impact on any other
usb driver) and to solve the problem shortly, I suggest to
use my patch and do some kind of refactoring later (You are right,
for a clean interface the interval parameter should have the same
meaning independend of the speed).
Best regards,
Christian
>
> Yes, but in this case we have to check all callers of usb_fill_int_urb
> to do the recalculation. E.g. in input/usbmouse.c
> endpoint->bInterval is used directly as parameter to usb_fill_int_urb.
Absolutely correct - There are 41 callers per LXR in 2.6.11 - may be even more in the current kernel.
>
> To avoid breaking things (my suggested patch has no impact on any other
> usb driver) and to solve the problem shortly, I suggest to
> use my patch and do some kind of refactoring later (You are right,
> for a clean interface the interval parameter should have the same
> meaning independend of the speed).
>
>
Agreed. Looking at some of the callers, it will take some time and refactoring to fix all of them. For now, it makes sense to put your patch in if no one has an objection.
Thanks
Parag
Hi,
On Wed, 12 Oct 2005 23:32:50 +0000, Parag Warudkar wrote:
>> To avoid breaking things (my suggested patch has no impact on any other
>> usb driver) and to solve the problem shortly, I suggest to
>> use my patch and do some kind of refactoring later (You are right,
>> for a clean interface the interval parameter should have the same
>> meaning independend of the speed).
> Agreed. Looking at some of the callers, it will take some time and
> refactoring to fix all of them. For now, it makes sense to put your
> patch in if no one has an objection.
Ok, then only one question remains: How do I get this patch applied to
the official kernel tree?
Thanks & best regards,
Christian
> 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 message and patch to [email protected].
If no-one picks it up from there, Andrew - would you be able to pick this one
either for -mm and then mainline or directly for mainline?
Parag
[email protected] (Parag Warudkar) wrote:
>
> > 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 message and patch to [email protected].
>
> If no-one picks it up from there, Andrew - would you be able to pick this one
> either for -mm and then mainline or directly for mainline?
>
Yup, it's in my (huge) backlog queue.
On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
>
> 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
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.
Also, what device needs this patch? Is it a device that I can buy
today?
thanks,
greg k-h
>
> Also, what device needs this patch? Is it a device that I can buy
> today?
>
> thanks,
>
> greg k-h
The patch is for hid-core.c - I don't think it is device specific - original problem
should affect all High Speed USB HID devices.
To summarize -
Current code just looks plain wrong since the same logic is repeated twice - endpoint->bInterval is operated upon twice if the device is HIGH SPEED one.
if (dev->speed == USB_SPEED_HIGH)
interval = 1 << (interval - 1);
This is first done in hid-code.c:usb_hid_configure() which then passes interval to usb.h:usb_fill_int_urb() which again repeats the same logic as above!
Parag
On Fri, Oct 14, 2005 at 02:10:17AM +0000, Parag Warudkar wrote:
> >
> > Also, what device needs this patch? Is it a device that I can buy
> > today?
> >
> > thanks,
> >
> > greg k-h
>
> The patch is for hid-core.c - I don't think it is device specific -
> original problem should affect all High Speed USB HID devices.
I realize that, my point is are there any such devices out in the wild
that people can buy and use? I do not know of any.
> To summarize -
>
> Current code just looks plain wrong since the same logic is repeated
> twice - endpoint->bInterval is operated upon twice if the device is
> HIGH SPEED one.
I agree. That's wrong and should be fixed. A patch would be nice...
thanks,
greg k-h
> I agree. That's wrong and should be fixed. A patch would be nice...
>
> thanks,
>
> greg k-h
Christian,
Mind reposting the patch after making it compliant with Documentation/SubmittingPatches or -
AKPM's famous - http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt ?
Send it to at least [email protected], [email protected] and to
Greg KH.
If you can't for some reason, let me know - I will post one later today.
Thanks
Parag