Commit 678539cf authored by Sarah Sharp's avatar Sarah Sharp Committed by Greg Kroah-Hartman

USB: xhci: Handle URB cancel, complete and resubmit race.

In the old code, there was a race condition between the stop endpoint
command and the URB submission process.  When the stop endpoint command is
handled by the event handler, the endpoint ring is assumed to be stopped.
When a stop endpoint command is queued, URB submissions are to not ring
the doorbell.  The old code would check the number of pending URBs to be
canceled, and would not ring the doorbell if it was non-zero.

However, the following race condition could occur with the old code:

1. Cancel an URB, add it to the list of URBs to be canceled, queue the stop
   endpoint command, and increment ep->cancels_pending to 1.
2. The URB finishes on the HW, and an event is enqueued to the event ring
   (at the same time as 1).
3. The stop endpoint command finishes, and the endpoint is halted.  An
   event is queued to the event ring.
4. The event handler sees the finished URB, notices it was to be
   canceled, decrements ep->cancels_pending to 0, and removes it from the to
   be canceled list.
5. The event handler drops the lock and gives back the URB.  The
   completion handler requeues the URB (or a different driver enqueues a new
   URB).  This causes the endpoint's doorbell to be rung, since
   ep->cancels_pending == 0.  The endpoint is now running.
6. A second URB is canceled, and it's added to the canceled list.
   Since ep->cancels_pending == 0, a new stop endpoint command is queued, and
   ep->cancels_pending is incremented to 1.
7. The event handler then sees the completed stop endpoint command.  The
   handler assumes the endpoint is stopped, but it isn't.  It attempts to
   move the dequeue pointer or change TDs to cancel the second URB, while the
   hardware is actively accessing the endpoint ring.

To eliminate this race condition, a new endpoint state bit is introduced,
EP_HALT_PENDING.  When this bit is set, a stop endpoint command has been
queued, and the command handler has not begun to process the URB
cancellation list yet.  The endpoint doorbell should not be rung when this
is set.  Set this when a stop endpoint command is queued, clear it when
the handler for that command runs, and check if it's set before ringing a
doorbell.  ep->cancels_pending is eliminated, because it is no longer
used.

Make sure to ring the doorbell for an endpoint when the stop endpoint
command handler runs, even if the canceled URB list is empty.  All
canceled URBs could have completed and new URBs could have been enqueued
without the doorbell being rung before the command was handled.
Signed-off-by: default avatarSarah Sharp <sarah.a.sharp@linux.intel.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
parent 0c487206
...@@ -817,12 +817,12 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) ...@@ -817,12 +817,12 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
xhci_debug_ring(xhci, ep_ring); xhci_debug_ring(xhci, ep_ring);
td = (struct xhci_td *) urb->hcpriv; td = (struct xhci_td *) urb->hcpriv;
ep->cancels_pending++;
list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list); list_add_tail(&td->cancelled_td_list, &ep->cancelled_td_list);
/* Queue a stop endpoint command, but only if this is /* Queue a stop endpoint command, but only if this is
* the first cancellation to be handled. * the first cancellation to be handled.
*/ */
if (ep->cancels_pending == 1) { if (!(ep->ep_state & EP_HALT_PENDING)) {
ep->ep_state |= EP_HALT_PENDING;
xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index); xhci_queue_stop_endpoint(xhci, urb->dev->slot_id, ep_index);
xhci_ring_cmd_db(xhci); xhci_ring_cmd_db(xhci);
} }
......
...@@ -306,7 +306,7 @@ static void ring_ep_doorbell(struct xhci_hcd *xhci, ...@@ -306,7 +306,7 @@ static void ring_ep_doorbell(struct xhci_hcd *xhci,
/* Don't ring the doorbell for this endpoint if there are pending /* Don't ring the doorbell for this endpoint if there are pending
* cancellations because the we don't want to interrupt processing. * cancellations because the we don't want to interrupt processing.
*/ */
if (!ep->cancels_pending && !(ep_state & SET_DEQ_PENDING) if (!(ep_state & EP_HALT_PENDING) && !(ep_state & SET_DEQ_PENDING)
&& !(ep_state & EP_HALTED)) { && !(ep_state & EP_HALTED)) {
field = xhci_readl(xhci, db_addr) & DB_MASK; field = xhci_readl(xhci, db_addr) & DB_MASK;
xhci_writel(xhci, field | EPI_TO_DB(ep_index), db_addr); xhci_writel(xhci, field | EPI_TO_DB(ep_index), db_addr);
...@@ -507,8 +507,11 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci, ...@@ -507,8 +507,11 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
ep = &xhci->devs[slot_id]->eps[ep_index]; ep = &xhci->devs[slot_id]->eps[ep_index];
ep_ring = ep->ring; ep_ring = ep->ring;
if (list_empty(&ep->cancelled_td_list)) if (list_empty(&ep->cancelled_td_list)) {
ep->ep_state &= ~EP_HALT_PENDING;
ring_ep_doorbell(xhci, slot_id, ep_index);
return; return;
}
/* Fix up the ep ring first, so HW stops executing cancelled TDs. /* Fix up the ep ring first, so HW stops executing cancelled TDs.
* We have the xHCI lock, so nothing can modify this list until we drop * We have the xHCI lock, so nothing can modify this list until we drop
...@@ -535,9 +538,9 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci, ...@@ -535,9 +538,9 @@ static void handle_stopped_endpoint(struct xhci_hcd *xhci,
* the cancelled TD list for URB completion later. * the cancelled TD list for URB completion later.
*/ */
list_del(&cur_td->td_list); list_del(&cur_td->td_list);
ep->cancels_pending--;
} }
last_unlinked_td = cur_td; last_unlinked_td = cur_td;
ep->ep_state &= ~EP_HALT_PENDING;
/* If necessary, queue a Set Transfer Ring Dequeue Pointer command */ /* If necessary, queue a Set Transfer Ring Dequeue Pointer command */
if (deq_state.new_deq_ptr && deq_state.new_deq_seg) { if (deq_state.new_deq_ptr && deq_state.new_deq_seg) {
...@@ -1249,10 +1252,9 @@ static int handle_tx_event(struct xhci_hcd *xhci, ...@@ -1249,10 +1252,9 @@ static int handle_tx_event(struct xhci_hcd *xhci,
} }
list_del(&td->td_list); list_del(&td->td_list);
/* Was this TD slated to be cancelled but completed anyway? */ /* Was this TD slated to be cancelled but completed anyway? */
if (!list_empty(&td->cancelled_td_list)) { if (!list_empty(&td->cancelled_td_list))
list_del(&td->cancelled_td_list); list_del(&td->cancelled_td_list);
ep->cancels_pending--;
}
/* Leave the TD around for the reset endpoint function to use /* Leave the TD around for the reset endpoint function to use
* (but only if it's not a control endpoint, since we already * (but only if it's not a control endpoint, since we already
* queued the Set TR dequeue pointer command for stalled * queued the Set TR dequeue pointer command for stalled
......
...@@ -652,10 +652,10 @@ struct xhci_virt_ep { ...@@ -652,10 +652,10 @@ struct xhci_virt_ep {
struct xhci_ring *new_ring; struct xhci_ring *new_ring;
unsigned int ep_state; unsigned int ep_state;
#define SET_DEQ_PENDING (1 << 0) #define SET_DEQ_PENDING (1 << 0)
#define EP_HALTED (1 << 1) #define EP_HALTED (1 << 1) /* For stall handling */
#define EP_HALT_PENDING (1 << 2) /* For URB cancellation */
/* ---- Related to URB cancellation ---- */ /* ---- Related to URB cancellation ---- */
struct list_head cancelled_td_list; struct list_head cancelled_td_list;
unsigned int cancels_pending;
/* The TRB that was last reported in a stopped endpoint ring */ /* The TRB that was last reported in a stopped endpoint ring */
union xhci_trb *stopped_trb; union xhci_trb *stopped_trb;
struct xhci_td *stopped_td; struct xhci_td *stopped_td;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment