Hans, can you please test this patch?
>8---------------------------------------------------------------------8<
Hans reported an issue with the USBDEVFS_URB_BULK_CONTINUATION flag that
is used by libusb and devio (usbfs). This causes his usb redirection
code, and large bulk transfers, to not work under an xHCI host
controller.
It's explained in the kerneldoc for for usb_unlink_urb() that the host
controller driver is expected to allow URB completion handlers to run on
an error before restarting the endpoint ring:
* Host Controller Drivers (HCDs) place all the URBs for a particular
* endpoint in a queue. Normally the queue advances as the controller
* hardware processes each request. But when an URB terminates with an
* error its queue generally stops (see below), at least until that URB's
* completion routine returns. It is guaranteed that a stopped queue
* will not restart until all its unlinked URBs have been fully retired,
* with their completion routines run, even if that's not until some time
* after the original completion handler returns. The same behavior and
* guarantee apply when an URB terminates because it was unlinked.
*
* Bulk and interrupt endpoint queues are guaranteed to stop whenever an
* URB terminates with any sort of error, including -ECONNRESET, -ENOENT,
* and -EREMOTEIO. Control endpoint queues behave the same way except
* that they are not guaranteed to stop for -EREMOTEIO errors. Queues
* for isochronous endpoints are treated differently, because they must
* advance at fixed rates. Such queues do not stop when an URB
* encounters an error or is unlinked. An unlinked isochronous URB may
* leave a gap in the stream of packets; it is undefined whether such
* gaps can be filled in.
*
* Note that early termination of an URB because a short packet was
* received will generate a -EREMOTEIO error if and only if the
* URB_SHORT_NOT_OK flag is set. By setting this flag, USB device
* drivers can build deep queues for large or complex bulk transfers
* and clean them up reliably after any sort of aborted transfer by
* unlinking all pending URBs at the first fault.
*
* When a control URB terminates with an error other than -EREMOTEIO, it
* is quite likely that the status stage of the transfer will not take
* place.
This patch changes the xHCI ring handling code to allow the URB
completion handler to run before ringing the doorbell on an endpoint
ring. This means adding a new flag to prevent the doorbell ring
(EP_STAY_HALTED), to avoid a race condition with the URB completion
handler and a completing Set TR Dequeue Pointer command (which will
restart the ring). The new flag temporarily stops the ring until the
URB completion function has a chance to cancel any pending URBs. That
will set a different cancellation pending flag, which will also halt the
endpoint ring.
Make sure to set the new EP_STAY_HALTED flag before calling
xhci_cleanup_halted_endpoint(). That function will queue a Set TR
Dequeue command, which will ring the doorbell and restart the ring after
it completes. We need to make sure that the URB completion handlers
have a chance to run before the ring is restarted.
This patch should be backported to kernels as old as 2.6.36, because
that was the first kernel where the xHCI ring code refactoring was
introduced. It will be too hard to port this to the unrefactored code.
Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx>
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
drivers/usb/host/xhci-ring.c | 31 +++++++++++++++++++++++++++++--
drivers/usb/host/xhci.h | 5 +++++
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 23b4aef..890aadf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -295,7 +295,7 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci,
* FIXME - check all the stream rings for pending cancellations.
*/
if ((ep_state & EP_HALT_PENDING) || (ep_state & SET_DEQ_PENDING) ||
- (ep_state & EP_HALTED))
+ (ep_state & EP_HALTED) || (ep_state & EP_STAY_HALTED))
return;
xhci_writel(xhci, DB_VALUE(ep_index, stream_id), db_addr);
/* The CPU has better things to do at this point than wait for a
@@ -1594,12 +1594,20 @@ static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
* 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.
+ *
+ * The endpoint will remain halted until the stall is
+ * cleared, so completion handlers will run before the
+ * ring is restarted.
*/
ep->stopped_td = td;
ep->stopped_trb = event_trb;
ep->stopped_stream = ep_ring->stream_id;
} else if (xhci_requires_manual_halt_cleanup(xhci,
ep_ctx, trb_comp_code)) {
+ /* Let the URB completion handlers run before restarting
+ * the endpoint ring.
+ */
+ ep->ep_state |= EP_STAY_HALTED;
/* Other types of errors halt the endpoint, but the
* class driver doesn't call usb_reset_endpoint() unless
* the error is -EPIPE. Clear the halted status in the
@@ -1658,6 +1666,14 @@ td_cleanup:
}
}
+ /* If we've only kept the ring halted in order for the completion
+ * handlers to run (and not because the ring is halted by a STALL), then
+ * restart the ring.
+ */
+ if (ep->ep_state & EP_STAY_HALTED) {
+ ep->ep_state &= ~EP_STAY_HALTED;
+ xhci_ring_ep_doorbell(xhci, slot_id, ep_index, 0);
+ }
return ret;
}
@@ -1674,6 +1690,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
int ep_index;
struct xhci_ep_ctx *ep_ctx;
u32 trb_comp_code;
+ int ret;
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
xdev = xhci->devs[slot_id];
@@ -1723,9 +1740,19 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
else
td->urb->actual_length = 0;
+ /* Let the URB completion handlers run before restarting the
+ * endpoint ring. finish_td() won't set the EP_STAY_HALTED flag
+ * for COMP_STALL. We have to set it ourselves before we call
+ * xhci_cleanup_halted_endpoint(), since that will queue a Set
+ * TR Dequeue command, which may restart the ring on completion.
+ */
+ ep->ep_state |= EP_STAY_HALTED;
xhci_cleanup_halted_endpoint(xhci,
slot_id, ep_index, 0, td, event_trb);
- return finish_td(xhci, td, event_trb, event, ep, status, true);
+ ret = finish_td(xhci, td, event_trb, event, ep, status, true);
+ ep->ep_state &= ~EP_STAY_HALTED;
+ xhci_ring_ep_doorbell(xhci, slot_id, ep_index, 0);
+ return ret;
}
/*
* Did we transfer any data, despite the errors that might have
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 55c0785..95907ee 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -835,6 +835,11 @@ struct xhci_virt_ep {
#define EP_HAS_STREAMS (1 << 4)
/* Transitioning the endpoint to not using streams, don't enqueue URBs */
#define EP_GETTING_NO_STREAMS (1 << 5)
+/* Stop endpoint ring temporarily until completion handlers have a chance to
+ * cancel any outstanding URBs. URB cancellation will set the EP_HALT_PENDING
+ * flag, which will stop the endpoint ring until all cancellations are complete.
+ */
+#define EP_STAY_HALTED (1 << 6)
/* ---- Related to URB cancellation ---- */
struct list_head cancelled_td_list;
/* The TRB that was last reported in a stopped endpoint ring */
--
1.7.9
--
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
