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

Hi, inline comments.

On 6.2.2020 12.37, Lauri Jakku wrote:

Hi,

I make small descriptions as you suggested and remove dumb comments

.. this is my first time patching kernel publicly so, I'm not used to

the Proper way, but i'm learning all the time :)


--Lauri Jakku

On 5.2.2020 19.12, Dan Carpenter wrote:

quoted

I never recieved v2-v5 so I can't really comment on those.  If you
didn't send those to linux-usb then let's just pretend we're still on
v1 otherwise it's just confusing.

This is not how we send v2 patches.  Put [PATCH v6], that's good.
Then the normal changelog and the Signed-off-by: then under the ---
cut off put a small comment.

Signed-off-by: you
---
v6: changed commit message
v5: fixed error reported by kbuild
v4: blah blah

quoted

+/* Wrapper function to send control messages */

Don't include obvious comments like this.

Done.

quoted

quoted

+static int usbhid_control_msg(struct usb_device *dev, unsigned int pipe,
+				_u8 request, __u8 requesttype, __u16 value,
+				__u16 index, void *data, __u16 size,
+				int timeout)
+{
+	/* calculate timeout per call, to archieve total timeout requested */

This should be obvious if the variables and functions are named well.

It's weird that we're passing the total timeout and then dividing it
into littler chunks...  Normally we would know the timeout from the
spec and then the total would be something like "a timeout of 400ms
won't annoy users."  (This seems wrong).

Yeah, I tought it also, and this happened: i broke my mouse

when trying to clean up, and just now got new mouse and

keyboard (Microsoft Keyb, Locitech M100 mouse) same

vendors, and identical to old ones) and there seems not to

be any random behavior. so i'm thinking to put to rest this

patch (what I now thing bit bad thing, if some one is

depending on timeout).

quoted

quoted

+	int call_timeout = USBHID_CONTROL_COMMAND_TIMEOUT_CALC(timeout);

CALC is a bad name because obviously it's going to calculate something.

quoted

+	int call_count   = USBHID_CONTROL_COMMAND_RETRY_COUNT;

Just call it "int retry = USBHID_CONTROL_COMMAND_RETRY_COUNT;".  Don't
do anything fancy with the white space.

quoted

+	int ret;
+	int timeout_looping;
+
+	do {
+		ret = usb_control_msg(dev, pipe, request, requesttype,
+				value, index, data, size, call_timeout);

The indenting is wrong.  Use checkpatch.pl --strict

quoted

+
+		timeout_looping =	(call_count-- > 0) &&
+					(ret == -ETIMEDOUT);
+	} while (timeout_looping);

Delete the "timeout_looping" variable.

	} while (ret == -ETIMEDOUT && retry-- > 0);

Have you tested with just one retry?

Not yet, and i think more that this whole thing was due HW error.

Maybe some component was giving up, from mouse (that i broke), but

I make v7 version, one can integrate it to mainstream, or just archive it.


I'll fix the things brought up.

quoted

regards,
dan carpenter

-- 
Br,
Lauri J.