Hi Jack, Sorry for the extremely delayed reply, I've been trying to catch up on my patch queue. On Tue, Oct 22, 2013 at 12:33:27PM -0700, Jack Pham wrote: > From: Hemant Kumar <hemantk@xxxxxxxxxxxxxx> > > USB control transfers can contain an optional IN data stage, in which > case the xHCI driver would queue an additional TRB. The interrupt on > short packet (ISP) bit is set so that the host controller driver can > update the URB's actual_length field if packet is received has length > less than the queued buffer. > > A zero-length packet (ZLP) received during the data stage is a special > case of a short packet but is currently not handled properly since during > the subsequent status stage the driver ends up overwriting the > actual_length field with transfer_buffer_length, which was the original > buffer length. A function driver will then incorrectly interpret the > completed URB as being of full length. > > Fix this by setting a flag when a ZLP is received in the data stage so > that the urb->actual_length remains 0 and does not get overwritten in > the status stage. I do finally understand the issue, after staring at the spaghetti code for a couple days. There's also a couple other corner cases that don't look like they're covered: Case 1: - Successful data OUT phase - Short status IN phase - In this case, if the upper layer doesn't set the URB_SHORT_NOT_OK flag, urb->status would be set to 0, urb->actual_length = transfer_buffer_length - End result: upper layer thinks the control OUT completed fine, when it didn't. Alan, I'm actually not sure what error status should be returned in this case. Should it be -EREMOTEIO or -EPROTO? I'm actually not convinced we can even get a short completion code for a status TRB. The xHCI spec says "The xHC shall wait indefinitely for a Success, Stall Error or other error response from device for the Status stage." I'm not sure if a short completion code is included in that "other error response" phrase. Case 2: - Babble on data IN phase - 0.95 xHCI host does not halt endpoint (the 0.96 spec errata clarified that endpoints should be halted on a babble error) - At this point, I'm not sure what completion code the status OUT phase will finish with. If it's successful, the babble status is not passed up in urb->status. > Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx> > Signed-off-by: Jack Pham <jackp@xxxxxxxxxxxxxx> > --- > Hi Sarah, > > Here's an RFC I'm hoping will revive this thread: > > http://marc.info/?l=linux-usb&m=138128415812207&w=2 > > The discussion was left hanging after Alan asked: > > } 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? > > This patch attemps to address this very issue by "catching" the ZLP in the > data stage handler and marking a flag so that actual_length doesn't get > overwritten before giving back the URB. > > Can you please have a look? > > Thanks, > Jack > > drivers/usb/host/xhci-ring.c | 6 ++++++ > drivers/usb/host/xhci.h | 3 +++ > 2 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 1e2f3f4..a896474 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -2162,6 +2162,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; > @@ -2171,6 +2173,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 03c74b7..cbd1497 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1283,6 +1283,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; > }; I would like to avoid adding extra overhead to the xhci_td structure, since it's allocated for every single URB. However, my attempt at a similar patch, by moving the extra data we need to store into the xhci_ring structure and addressing those other corner cases is a bit more complex. Can you test this and see if it solves your issue? The new field could also potentially be used to fix an outstanding issue with short transfers on non-control endpoints. The xHCI 1.0 specification changed short transfer behavior for chained TRBs such that we get a short completion for the TRB, and then we will also get a successful completion for the last TRB in the TD, which has the IOC flag set. We're technically not supposed to over-write the TD or anything they reference until we get that last successful TRB, but right now we hand back the URB and move the ring dequeue pointer on the first short TRB completion. 8<------------------------------------------------------------------>8 diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 49b8bd063fab..70b4aff3883f 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -186,6 +186,7 @@ static void xhci_initialize_ring_info(struct xhci_ring *ring, * accounting purpose */ ring->num_trbs_free = ring->num_segs * (TRBS_PER_SEGMENT - 1) - 1; + ring->td_status = -EINPROGRESS; } /* Allocate segments and link them for a ring */ diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 53c2e296467f..49731eab1357 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -836,10 +836,12 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id, * If we stopped on the TD we need to cancel, then we have to * move the xHC endpoint ring dequeue pointer past this TD. */ - if (cur_td == ep->stopped_td) + if (cur_td == ep->stopped_td) { xhci_find_new_dequeue_state(xhci, slot_id, ep_index, cur_td->urb->stream_id, cur_td, &deq_state); + ep_ring->td_status = -EINPROGRESS; + } else td_to_noop(xhci, ep_ring, cur_td, false); remove_finished_td: @@ -2063,6 +2065,7 @@ td_cleanup: list_del_init(&td->cancelled_td_list); urb_priv->td_cnt++; + ep_ring->td_status = -EINPROGRESS; /* Giveback the urb when all the tds are completed */ if (urb_priv->td_cnt == urb_priv->length) { ret = 1; @@ -2116,18 +2119,23 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td, } break; case COMP_SHORT_TX: - if (td->urb->transfer_flags & URB_SHORT_NOT_OK) - *status = -EREMOTEIO; + /* Setup phase can never be short, since it's always OUT */ + /* If this is a short data phase, store the status for later. */ + if (event_trb != ep_ring->dequeue && event_trb != td->last_trb) + ep_ring->td_status = -EREMOTEIO; + /* Status IN phases could also be short. */ else - *status = 0; + *status = -EREMOTEIO; break; case COMP_STOP_INVAL: case COMP_STOP: return finish_td(xhci, td, event_trb, event, ep, status, false); default: if (!xhci_requires_manual_halt_cleanup(xhci, - ep_ctx, trb_comp_code)) + ep_ctx, trb_comp_code)) { + ep_ring->td_status = *status; break; + } xhci_dbg(xhci, "TRB error code %u, " "halted endpoint index = %u\n", trb_comp_code, ep_index); @@ -2153,19 +2161,24 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td, 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; - } else { + /* + * Setup and data executed successfully, update + * actual_length to reflect transferred data phase. + */ + if (ep_ring->td_status == -EINPROGRESS) td->urb->actual_length = td->urb->transfer_buffer_length; - } + /* + * We had a short packet on the data phase. + * Update urb->status only if the upper layer cares. + */ + else if (ep_ring->td_status == -EREMOTEIO && + (td->urb->transfer_flags + & URB_SHORT_NOT_OK)) + *status = -EREMOTEIO; + /* Some other error status on the data phase */ + else + *status = ep_ring->td_status; } else { /* Maybe the event was for the data stage? */ td->urb->actual_length = diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 03c74b7965f8..03e0a0eb4d94 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1333,6 +1333,7 @@ struct xhci_ring { unsigned int num_trbs_free_temp; enum xhci_ring_type type; bool last_td_was_short; + int td_status; }; struct xhci_erst_entry { -- 1.8.3.3 -- 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
