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

[Stefan Bruens]

On Tuesday 15 December 2015 20:07:26 Stephen Warren wrote:
> 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.

"8.5.3 Control Transfers"
"The Data stage, if present, of a control transfer consists of one or more IN 
or OUT transactions and follows the same protocol rules as bulk transfers."
This can also be inferred from the flow charts/state diagrams which state NAKs 
and stalls are not allowed for for "control *setup* transaction" (emphasize 
mine), e.g. Figure 8-31.

"8.5.3.1 Reporting Status Results"
"NAK indicates that the function is still processing the command and that the 
host should continue the Status stage."

I have at least one USB LS mouse which responds with NAKs during control 
transfers, Sigrok LA traces available here:
http://sigrok.org/gitweb/?p=sigrok-dumps.git;a=tree;f=usb/setup

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

Good idea, will do.
 
> >  		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.

No, as each NAK will cause a return from chunk_msg. I see e.g. the following 
interrupt flags in combination with CHHLTD (for GET_DEVICE_DESCRIPTOR):
- SETUP (SSPLIT) -> ACK
  SETUP (CSPLIT) -> NYET NYET ACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET NACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET ACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET NACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET ACK
- STATUS (SSPLIT) -> ACK
  STATUS (CSPLIT) -> NYET NYET ACK

On the first DATA-IN ACK, 8 bytes are returned, the second returns the final 
9th byte.

The NAK handling could be moved into the loop in chunk_msg, but this would 
break INTERRUPT transactions.

A different possibility is to move the timeout check into the chunk_msg loop, 
i.e.
---
xfer_timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
do {
  ...
  ret = wait_for_bit(CHHLTD, timeout=1ms)
  if (ret == -EINTR)
     break;
  if (get_timer(0) > xfer_timeout) {
     printf("Timeout\n");
     ret = -ETIMEDOUT;
     break;
  }
  ...
} while ((done < len) && !stop_transfer);
---

This would also simplify both the INTERRUPT and CONTROL submit functions, and 
BULK submit would finally honour the specified timeout.


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

Correct, but see above.

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424