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

Stephen Warren
2015-12-16T03:07:26Z

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?