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
