Hi everyone, Inline commented. On 4.2.2020 9.29, Lauri Jakku wrote: > On 4.2.2020 9.20, Greg KH wrote: >> 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? > for example Manjaro has issues (I got my self randomly working usb mouse) > > > https://forum.manjaro.org/t/usb-devices-mice-stop-working-randomly/113154/18 > > > >>> --------------------------------------------------------------------------------------- >>> >>> >From d4214685de329ebe07dfd2298bce46dfae5f80bf Mon Sep 17 00:00:00 2001 >>> From: Lauri Jakku <lja@xxxxxx> >>> 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@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..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? > The ETIMEDOUT definition is there, so yeah. >>> >>> #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? > I tried with 5 times setting, and my mouse works quite well now, i thing that > > the 20 times (Max sleep time of 400ms) is good. It does report timeout, when > > 20 times tried to communicate with HW, so if device is fine etc. there is no > > sleeping at all done. > > >> thanks, >> >> greg k-h -- Br, Lauri J.
