[U-Boot] [PATCH 2/6] usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage

[Stephen Warren <swarren@wwwdotorg.org>]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> A function is allowed to return NAKs during the DATA stage to control
> data flow control. NAKs during the STATUS stage signal the function
> is still processing the request.

For my own education, do you have a link to the part of the spec that
states that? I'd naively expect the control stage to give a NAK, but
once a control transaction was accepted, the function would have to deal
with it without NAKs? Still, I don't think this change would cause any
issue either way.

> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c

> @@ -907,26 +907,37 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
>  	if (buffer) {
> +		/* DATA stage */

I'd suggest putting that new comment right before the "timeout = ..."
line, since that's the start of DATA stage processing.

If you're adding comments for the stages, perhaps add one at the start
of the CONTROL stage too?

>  		pid = DWC2_HC_PID_DATA1;
> -		ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer,
> -				len, false);
> +		act_len = 0;

I don't think you need that assignment because...

> +		do {
> +			if (get_timer(0) > timeout) {
> +				printf("Timeout during CONTROL DATA stage\n");
> +				return -ETIMEDOUT;
> +			}
> +			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
> +					buffer, len, false);
> +			act_len += dev->act_len;
> +			buffer += dev->act_len;
> +			len -= dev->act_len;

Shouldn't those all be = not += or -=-, just like in the original code?
There's no chunking loop here, so the entire length either happens in
one go or not at all.

>  	pid = DWC2_HC_PID_DATA1;
> -	ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> -			priv->status_buffer, 0, false);
> +	do {
> +		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> +				priv->status_buffer, 0, false);
> +	} while (ret == -EAGAIN);
>  	if (ret)
>  		return ret;

Shouldn't that last loop have a timeout too?