Re: [PATCH v5] USB: HID: random timeout failures tackle try.

[Lauri Jakku <ljakku77@gmail.com>]

Hi,

Inline comments.

On 4.2.2020 20.15, Greg KH wrote:
> On Tue, Feb 04, 2020 at 07:52:39PM +0200, Lauri Jakku wrote:
>> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
>> index 5adf489428aa..2e0bfe70f7c5 100644
>> --- a/drivers/usb/core/message.c
>> +++ b/drivers/usb/core/message.c
> Ok, changelog issues aside:
>
>
>> @@ -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;
> No need to initialize this.
>
>> +
>> +	/* retry_cnt * 20ms, max retry time set to 400ms */
>> +	int retry_cnt = 20;
> Why?  You just now changed how all drivers will deal with timeouts.  And
> you didn't change any drivers :(
>
>
>>  
>>  	dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>>  	if (!dr)
>> @@ -149,11 +153,52 @@ 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. Adapt timeout
>> +		 * to be smaller when we have timeout'd first time.
>> +		 */
>> +		if (dev->quirks & USB_QUIRK_DELAY_CTRL_MSG)
>> +			msleep(200);
>> +		else if (ret == -ETIMEDOUT) {
>> +			static int timeout_happened = 0;
> Woah, what about this function being called multiple times from
> different devices all at the same time?


I did not tought of that, yes, it will break alot of things like that, it can't be

like that.


Could the flag be in some platform-data? hmm platformdata

would be the right place, or do you have suggestions. I have tought

about the move from core -> hid ..would that be better, then in hid

module put the flag so that it is per device.


> Are you _SURE_ you want this to be static?
>
> Hint, no way, not at all, this doesn't do at all what you think it does.
> We support more than one USB device in the system at a time :)
>
>> +
>> +			if ( ! timeout_happened ) {
>> +				timeout_happened = 1;
>> +
>> +				/* 
>> +				 * If timeout is given, divide it
>> +				 * by 100, if not, put 10ms timeout.
>> +				 * 
> Always run scripts/checkpatch.pl on your patches so you do not get
> grumpy maintainers telling you to run scripts/checkpatch.pl on your
> patches.
>
Uh, forgot.. just starting to learn from general developer, to

kernel developer. I'll try to remember that.

>> +				 * Then safeguard: if timeout is under
>> +				 * 10ms, make timeout to be 10ms.
>> +				 */
>> +
>> +				if (timeout > 0)
>> +					timeout /= 100;
>> +				else
>> +					timeout = 10;
>> +
>> +				if (timeout < 10)
>> +					timeout = 10;
> What is with this "backing off"?  Why?
>
> Again, you just broke how all USB drivers treat the timeout value here,
> what happens for devices that do NOT want this retried?

Hmm, maybe the quirk method could be used to ena/disa the retry.

>
> We do not want to retry in the USB core, _UNLESS_ the caller asks us to
> do so.  Otherwise we will break things.

Yeah, i'm turning to like the idea to have the retry moved to hid module and

have quirk define to disable/enable it.


Starting really like the idea, I'm yet no kernel guru, learning while doing now.


I really should buy a book about linux kernel.

>
> I understand this works for your one device, but realize we need to
> support all devices in existance, at the same time :)


Good comments, thanks for review.


This is what i do (soonish, tomorrow perhaps):


1. move patchcode from core -> hid module

2. add variable & quirck to _disable_ retry code done to HID module

  (variable per platformdata, would used by hid module, no need to

   mess with core)


Now is already so late that, if i start coding, i got in the zone and 6h is

like breeze.


> thanks,
>
> greg k-h