Re: USB HID fix: Retry sending timedout device commands 20 times

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

Hi Lauri,

On Tue, Feb 4, 2020 at 3:21 AM Lauri Jakku <lauri.jakku@pp.inet.fi> wrote:
>
> Hi,
> I made possible fix for USB HID devices randomly fail to operate correctly.

Can you tell us a little bit more of which devices are failing, and on
which platform? I never had such failure, so I guess it must be device
specific.

>
> Fix: If device reports timedout operation, try re-send command 20 times, 10ms apart,
> before giving up. I got 5.5-series kernel running with 5 times resending and it seems
> to be quite good, i made the patch of 20 times resending. That should be enough for
> most cases.

Ouch, very much ouch. Resending 20 times on a generic path when the
timeout can be several seconds is pretty much a bad thing. Again, this
should be limited to the device you are talking to, and not be
generic. Or maybe you are encountering a usb controller issue.

>
> Code for drivers/usb/core/message.c include error definitions with adding include:
>
> #include <linux/errno.h>
>
>
> Code for drivers/usb/core/message.c function usb_control_msg:

This code / idea should be sent to linux-usb@vger.kernel.org (Cc-ed),
not just linux-input. This code touches the USB part, and we have no
control of it in the HID or input tree.

Cheers,
Benjamin

>
> int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request,
>    __u8 requesttype, __u16 value, __u16 index, void *data,
>    __u16 size, int timeout)
> {
>     struct usb_ctrlrequest *dr;
>     int ret = -ETIMEDOUT;
>     int retry_cnt = 20;
>
> dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
> if (!dr)
> return -ENOMEM;
>
> dr->bRequestType = requesttype;
> dr->bRequest = request;
> dr->wValue = cpu_to_le16(value);
> dr->wIndex = cpu_to_le16(index);
> dr->wLength = cpu_to_le16(size);
>
>     do
>     {
>         ret = usb_internal_control_msg(dev, pipe, dr, data, size, timeout);
>
>         /*
>          * Linger a bit, prior to the next control message or if return
>          * value is timeout, but do try few times before quitting.
>          */
>         if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
>             msleep(200);
>         else if (ret == -ETIMEDOUT) {
>             dev_dbg(dev,
>                     "timed out, trying %d times more.\n",
>                     retry_cnt);
>             msleep(10);
>         }
>
>     } while ( (retry_cnt-- > 0)
>                 &&
>               (ret == -ETIMEDOUT));
>
>
> kfree(dr);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(usb_control_msg);
>
>
> --
> Br,
> Lauri J.