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