On Tue, Feb 04, 2020 at 01:06:59PM +0200, Lauri Jakku wrote: > There is multiple reports of random behaviour of USB HID devices. > > I have mouse that acts sometimes quite randomly, I debugged with > logs others have published: that there is HW timeouts that leave > device in state that it is errorneus. > > To fix this, I introduce retry mechanism in root of USB HID drivers. > > Fix does not slow down operations at all if there is no -ETIMEDOUT > got from control message sending. > > If there is one, then sleep 20ms and try again. Retry count is 20 > witch translates maximium of 400ms before giving up. If the 400ms > boundary is reached the HW is really bad. That's not even true. The caller passes in a timeout, in many cases 5 seconds, which you allow to expire up to 20 times on top of your arbitrary 400 ms delay. So that's 100.4 seconds... > JUST to be clear: > This does not make USB HID devices to sleep anymore than > before, if all is golden. > > Why modify usb-hid-core: No need to modify driver by driver. Because you cannot decide how every use should handle timeouts. Just fix up the driver that needs this. > Signed-off-by: Lauri Jakku <lja@xxxxxx> > --- > 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..b375e376ea22 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> > > #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 * 20ms, 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 400ms) before quitting. > + */ > + if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG) > + msleep(200); > + else if (ret == -ETIMEDOUT) > + msleep(20); > + > + /* Loop while timeout, max loops: retry_cnt times. */ > + } while ((retry_cnt-- > 0) && (ret == -ETIMEDOUT)); > > - /* Linger a bit, prior to the next control message. */ > - if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG) > - msleep(200); > > kfree(dr); Johan