* USB HID random timeout failures fix proposal
@ 2020-02-04 3:44 Lauri Jakku
2020-02-04 7:20 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Lauri Jakku @ 2020-02-04 3:44 UTC (permalink / raw)
To: linux-usb, gregkh
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.
---------------------------------------------------------------------------------------
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.
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>
#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));
- /* Linger a bit, prior to the next control message. */
- if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
- msleep(200);
kfree(dr);
--
2.25.0
--
Br,
Lauri J.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: USB HID random timeout failures fix proposal
2020-02-04 3:44 USB HID random timeout failures fix proposal Lauri Jakku
@ 2020-02-04 7:20 ` Greg KH
[not found] ` <3de3f2e8-5ab0-92f7-bdeb-09351042e630@pp.inet.fi>
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2020-02-04 7:20 UTC (permalink / raw)
To: Lauri Jakku; +Cc: linux-usb
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: USB: HID: random timeout failures fix proposal
[not found] ` <3de3f2e8-5ab0-92f7-bdeb-09351042e630@pp.inet.fi>
@ 2020-02-04 7:39 ` Lauri Jakku
0 siblings, 0 replies; 3+ messages in thread
From: Lauri Jakku @ 2020-02-04 7:39 UTC (permalink / raw)
To: Greg KH, linux-usb
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@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?
> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-04 7:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-04 3:44 USB HID random timeout failures fix proposal Lauri Jakku
2020-02-04 7:20 ` Greg KH
[not found] ` <3de3f2e8-5ab0-92f7-bdeb-09351042e630@pp.inet.fi>
2020-02-04 7:39 ` USB: HID: " 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).