* Re: USB HID fix: Retry sending timedout device commands 20 times
[not found] <9cf20b6f-5bfa-2346-ca9f-5ca81245bfff@pp.inet.fi>
@ 2020-02-04 8:17 ` Benjamin Tissoires
2020-02-04 9:05 ` Oliver Neukum
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Tissoires @ 2020-02-04 8:17 UTC (permalink / raw)
To: Lauri Jakku
Cc: Jiri Kosina, open list:HID CORE LAYER, Greg Kroah-Hartman,
Linux USB Mailing List
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.
^ permalink raw reply [flat|nested] 3+ messages in thread