If class driver wants to SetFeature(ENDPOINT_HALT) and later tries to talk to the stalled endpoint, xhci will move endpoint to EP_STATE_STALL and subsequent usb_submit_urb() will not cause a USB token to be shifted into the data lines.
Because of that, peripheral will never have any means of STALLing a follow up token. This is a known error at least with g_zero + testusb -t 13 and can be easily reproduced. Cc: <sta...@vger.kernel.org> # v3.14+ Signed-off-by: Felipe Balbi <ba...@ti.com> --- this is a second version of patch sent originally in January this year [1]. I suppose this is still not the best fix (as noted on code comment below), but it'll serve to restart the thread on what's the best way to fix this. I thought about clearing endpoint halt if !control from prepare_ring(), but I'm not entirely of the implications there either. I'm adding stable to Cc for v3.14+ (which is the earlier kernel I could easily run to reproduce this) but it's likely a much older bug. Still, if nobody other than me complained until now, perhaps we don't care as much ? [1] http://marc.info/?l=linux-usb&m=139025060301432&w=2 drivers/usb/host/xhci-ring.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index bc6fcbc..4e8c3cf 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -1826,13 +1826,45 @@ static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td, if (trb_comp_code == COMP_STALL) { /* The transfer is completed from the driver's * perspective, but we need to issue a set dequeue - * command for this stalled endpoint to move the dequeue - * pointer past the TD. We can't do that here because - * the halt condition must be cleared first. Let the - * USB class driver clear the stall later. + * command for this stalled endpoint to move the + * dequeue pointer past the TD. + * + * Before we can move the dequeue pointer past the TD, + * we must first clear the halt condition, however, we + * can't clear such condition for control endpoints + * since that can only be cleared through a USB Reset. + * + * Note that this isn't 100% safe because as soon as a + * stalled TD is completed, we will clear the halt + * condition on the endpoint from the host side, which + * might not always be what we want. + * + * Perhaps a better approach would be to differentiate + * SetFeature(ENDPOINT_HALT) from a STALL due to error + * or Babble and only clear halt in that case. + * + * Another difficulty with this case is that if class + * driver wants to talk to a halted endpoint to verify + * SetFeature(ENDPOINT_HALT) was implemented correctly + * by firmware (one such case is g_zero + testusb -t + * 13), without clearing HALT on the host side, EP + * doorbell will be rung but endpoint will not exit + * EP_STATE_HALTED, because we *must* first issue a + * Reset Endpoint command to move the EP to the + * EP_STATE_STOPPED before EP doorbell works again. + * + * So, we will issue 'Reset Endpoint' here to make sure a + * subsequent usb_submit_urb() will actually cause EP + * doorbell to ring so a USB token is shifted into the + * wire and peripheral has a chance of replying with + * STALL again. */ ep->stopped_td = td; ep->stopped_stream = ep_ring->stream_id; + if (!usb_endpoint_xfer_control(&td->urb->ep->desc)) + xhci_cleanup_halted_endpoint(xhci, + slot_id, ep_index, ep_ring->stream_id, + td, event_trb); } else if (xhci_requires_manual_halt_cleanup(xhci, ep_ctx, trb_comp_code)) { /* Other types of errors halt the endpoint, but the -- 2.1.0.GIT -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
