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

[Dan Carpenter <dan.carpenter@oracle.com>]

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


> +/* Wrapper function to send control messages */

Don't include obvious comments like this.

> +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).


> +	int call_timeout = USBHID_CONTROL_COMMAND_TIMEOUT_CALC(timeout);

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

> +	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.

> +	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

> +
> +		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?

regards,
dan carpenter