Hi Sarah, Have you had a chance to look at Alan's response, and my patch below? Jack On Tue, Oct 08, 2013 at 07:02:31PM -0700, Jack Pham wrote: > Hi Sarah, Alan, > > I'm taking over reporting of this issue for Hemant while he is out on > vacation. > > On Mon, Oct 07, 2013 at 05:44:56PM -0400, Alan Stern wrote: > > On Mon, 7 Oct 2013, Sarah Sharp wrote: > > > What should urb->actual_length be set to instead? The device sent > > > zero bytes in the data stage and urb->transfer_buffer_length *is* > > > zero, so it > > > > No, urb->transfer_buffer_length > 0. That's the point. > > > > If I understand the OP correctly, he is saying that a 0-length > > response to (for example) an 8-byte control-in transfer will end up > > with urb->actual_length set to 8 rather than 0. > > That's correct. > > In our case we are not setting URB_SHORT_NOT_OK, and the URB's > transfer_buffer_length is non-zero, so the data stage TRB does get > enqueued. The device responds with a zero-length data packet but that > is resulting in the URB incorrectly getting its actual_length set to > transfer_buffer_length, instead of 0 as we would expect. > > This problem is not seen when connected to an EHCI controller. > > On Tue, Oct 08, 2013 at 10:19:11AM -0400, Alan Stern wrote: > > On Mon, 7 Oct 2013, Sarah Sharp wrote: > > > > > > i am using 3.10 kernel. Also i looked at tip i see same implementation for > > > > process_ctrl_td() > > > > > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/host/xhci-ring.c > > > > > > Yeah, the implementation has always been this way. I've known since the > > > beginning of writing the xHCI driver that it needs to handle zero-length > > > data phases, but hadn't had ever had a report that it was required by a > > > device under Linux. > > > > This code, near the end of process_ctrl_td(), looks a little strange: > > > > /* > > * Did we transfer any data, despite the errors that might have > > * happened? I.e. did we get past the setup stage? > > */ > > if (event_trb != ep_ring->dequeue) { > > /* The event was for the status stage */ > > if (event_trb == td->last_trb) { > > if (td->urb->actual_length != 0) { > > /* Don't overwrite a previously set error code > > */ > > if ((*status == -EINPROGRESS || *status == 0) && > > (td->urb->transfer_flags > > & URB_SHORT_NOT_OK)) > > /* Did we already see a short data > > * stage? */ > > *status = -EREMOTEIO; > > > > If you already saw a short data stage, why isn't the status already set > > to -EREMOTEIO? > > > > Also what's the reason for the test "td->urb->actual_length != 0"? > > What does this have to do with getting a short data stage? Are you > > assuming that at this point, actual_length will be nonzero if and only > > if there was a short data stage? > > > > } else { > > td->urb->actual_length = > > td->urb->transfer_buffer_length; > > } > > > > What's the purpose of this clause? > > > > It looks like the driver is confusing "actual_length == 0" with > > "actual_length was never set". What if actual_length _was_ previously > > set, but it was set to 0? > > Alan, your analysis is consistent with our findings as well. We are > seeing that if actual_length got set to 0 in the data stage, then when > the status stage is processed the above else clause ends up overwriting > this value before handing back the URB. Thus the zero-length packet is > incorrectly reported as a successful transfer of transfer_buffer_length. > > Here's a patch we came up with that seems to fix the immediate issue. > However, I'm not sure if this has other side effects or not, e.g. if > URB_SHORT_NOT_OK is set. > > Jack > > ----8<---- > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 204110c..507a72d 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -2056,6 +2056,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td, > /* Did we already see a short data > * stage? */ > *status = -EREMOTEIO; > + } else if (td->zlp_data) { > + td->zlp_data = false; > } else { > td->urb->actual_length = > td->urb->transfer_buffer_length; > @@ -2065,6 +2067,10 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td, > td->urb->actual_length = > td->urb->transfer_buffer_length - > EVENT_TRB_LEN(le32_to_cpu(event->transfer_len)); > + > + if (td->urb->actual_length == 0) > + td->zlp_data = true; > + > xhci_dbg(xhci, "Waiting for status " > "stage event\n"); > return 0; > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 773583a..25e751a 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1258,6 +1258,9 @@ struct xhci_td { > struct xhci_segment *start_seg; > union xhci_trb *first_trb; > union xhci_trb *last_trb; > + > + /* ZLP received in data stage of a control transfer */ > + bool zlp_data; > }; > > /* xHCI command default timeout value */ -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
