| Message ID | 1449980278-19881-3-git-send-email-stefan.bruens@rwth-aachen.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On Sunday, December 13, 2015 at 05:17:54 AM, 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. > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > --- > drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 5ef6deb..db3acd4 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -892,8 +892,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, > struct usb_device *dev, { > int devnum = usb_pipedevice(pipe); > int pid, ret, act_len; > - /* For CONTROL endpoint pid should start with DATA1 */ > int status_direction; > + unsigned long timeout; > > if (devnum == priv->root_hub_devnum) { > dev->status = 0; > @@ -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 */ > pid = DWC2_HC_PID_DATA1; > - ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer, > - len, false); > + act_len = 0; > + 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; > + } while (len && (ret == -EAGAIN)); I can't say I'm a big fan of these conditions ^ . Can you rework a bit so it's more obvious what the code does ? The condition in the while (cond) is really inobvious at a first glance. > if (ret) > return ret; > - act_len = dev->act_len; > - } /* End of DATA stage */ > - else > + status_direction = usb_pipeout(pipe); > + } else { > + /* No-data CONTROL always ends with an IN transaction */ > + status_direction = 1; > act_len = 0; > + } Otherwise: Acked-by: Marek Vasut <marex@denx.de>
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?
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
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 5ef6deb..db3acd4 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -892,8 +892,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, { int devnum = usb_pipedevice(pipe); int pid, ret, act_len; - /* For CONTROL endpoint pid should start with DATA1 */ int status_direction; + unsigned long timeout; if (devnum == priv->root_hub_devnum) { dev->status = 0; @@ -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 */ pid = DWC2_HC_PID_DATA1; - ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer, - len, false); + act_len = 0; + 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; + } while (len && (ret == -EAGAIN)); if (ret) return ret; - act_len = dev->act_len; - } /* End of DATA stage */ - else + status_direction = usb_pipeout(pipe); + } else { + /* No-data CONTROL always ends with an IN transaction */ + status_direction = 1; act_len = 0; + } /* STATUS stage */ - if ((len == 0) || usb_pipeout(pipe)) - status_direction = 1; - else - status_direction = 0; - 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;
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. Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)