linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 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

* Re: USB HID fix: Retry sending timedout device commands 20 times
  2020-02-04  8:17 ` USB HID fix: Retry sending timedout device commands 20 times Benjamin Tissoires
@ 2020-02-04  9:05   ` Oliver Neukum
  2020-02-04  9:27     ` Lauri Jakku
  0 siblings, 1 reply; 3+ messages in thread
From: Oliver Neukum @ 2020-02-04  9:05 UTC (permalink / raw)
  To: Benjamin Tissoires, Lauri Jakku
  Cc: Jiri Kosina, open list:HID CORE LAYER, Greg Kroah-Hartman,
	Linux USB Mailing List

Am Dienstag, den 04.02.2020, 09:17 +0100 schrieb Benjamin Tissoires:
> 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.

Yes. You need a least to consider teh unplug case.

	Regards
		Oliver


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: USB HID fix: Retry sending timedout device commands 20 times
  2020-02-04  9:05   ` Oliver Neukum
@ 2020-02-04  9:27     ` Lauri Jakku
  0 siblings, 0 replies; 3+ messages in thread
From: Lauri Jakku @ 2020-02-04  9:27 UTC (permalink / raw)
  To: Oliver Neukum, Benjamin Tissoires
  Cc: Jiri Kosina, open list:HID CORE LAYER, Greg Kroah-Hartman,
	Linux USB Mailing List


On 4.2.2020 11.05, Oliver Neukum wrote:
> Am Dienstag, den 04.02.2020, 09:17 +0100 schrieb Benjamin Tissoires:
>> 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.
> Yes. You need a least to consider teh unplug case.
>
> 	Regards
> 		Oliver

The patch does not loop in normal case, only if there are timeouts, and

even then not more than 400ms .. 20ms sleep while retry.


-- 
Br,
Lauri J.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-02-04  9:27 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <9cf20b6f-5bfa-2346-ca9f-5ca81245bfff@pp.inet.fi>
2020-02-04  8:17 ` USB HID fix: Retry sending timedout device commands 20 times Benjamin Tissoires
2020-02-04  9:05   ` Oliver Neukum
2020-02-04  9:27     ` Lauri Jakku

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).