Re: USB HID random timeout failures fix proposal

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Feb 04, 2020 at 05:44:45AM +0200, Lauri Jakku wrote:
> Hi,
> 
> 
> I made a patch that does to tackle the USB HID device random behavior by
> 
> re-trying to send command 400ms, 20ms sleep per try.

WHat random behavior are you referring to?

> 
> ---------------------------------------------------------------------------------------
> 
> >From d4214685de329ebe07dfd2298bce46dfae5f80bf Mon Sep 17 00:00:00 2001
> From: Lauri Jakku <lja@iki.fi>
> Date: Tue, 4 Feb 2020 04:52:01 +0200
> Subject: [PATCH] USB HID random timeout failures fixed by trying 20 times
>  send, 20ms apart, control messages, if error is timeout.

The subject line of the patch is a bit too big, and you have no
changelog text in the body of the message, making this patch not able to
be applied.

Also, no need to insert it like this, just use git send-email to submit
it.


> 
> Signed-off-by: Lauri Jakku <lja@iki.fi>
> ---
>  drivers/usb/core/message.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 5adf489428aa..5d615b2f92d8 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -20,6 +20,7 @@
>  #include <linux/usb/hcd.h>     /* for usbcore internals */
>  #include <linux/usb/of.h>
>  #include <asm/byteorder.h>
> +#include <linux/errno.h>

Are you sure this is needed?

>  
>  #include "usb.h"
>  
> @@ -137,7 +138,10 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request,
>                     __u16 size, int timeout)
>  {
>         struct usb_ctrlrequest *dr;
> -       int ret;
> +       int ret = -ETIMEDOUT;
> +
> +       /* retry_cnt * 10ms, max retry time set to 400ms */
> +       int retry_cnt = 20;
>  
>         dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>         if (!dr)
> @@ -149,11 +153,27 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request,
>         dr->wIndex = cpu_to_le16(index);
>         dr->wLength = cpu_to_le16(size);
>  
> -       ret = usb_internal_control_msg(dev, pipe, dr, data, size, timeout);
> +       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 (max 200ms) before quitting.
> +                */
> +               if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
> +                       msleep(200);
> +               else if (ret == -ETIMEDOUT)
> +                       msleep(20);
> +
> +               /* Loop while timeout, max retry_cnt times. */
> +       } while ((retry_cnt-- > 0) && (ret == -ETIMEDOUT));

Why are we looping always if something went wrong?

And don't we already have a "timeout"?  Why not rely on that?

thanks,

greg k-h