[RFC/PATCH v2] usb: host: xhci: issue 'Reset Endpoint' for !CONTROL EPs from finish_td()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: <stable@xxxxxxxxxxxxxxx> # v3.14+
Signed-off-by: Felipe Balbi <balbi@xxxxxx>
---

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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]