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
