> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Friday, August 05, 2011 2:45 AM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx > Subject: [RFC 3/3] xhci: Remove TDs from TD lists when URBs are > canceled. > > When a driver tries to cancel an URB, and the host controller is dying, > xhci_urb_dequeue will giveback the URB without removing the xhci_tds > that comprise that URB from the td_list or the cancelled_td_list. This > can cause a race condition between the driver calling URB dequeue and > the stop endpoint command watchdog timer. > > If the timer fires on a dying host, and a driver attempts to resubmit > while the watchdog timer has dropped the xhci->lock to giveback a > cancelled URB, URBs may be given back by the xhci_urb_dequeue() > function. > At that point, the URB's priv pointer will be freed and set to NULL, > but > the TDs will remain on the td_list. This will cause an oops in > xhci_giveback_urb_in_irq() when the watchdog timer attempts to loop > through the endpoints' td_lists, giving back killed URBs. > > Make sure that xhci_urb_dequeue() removes TDs from the TD lists and > canceled TD lists before it gives back the URB. > > This patch should be backported to kernels as old as 2.6.36. > > Signed-off-by: Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> > Cc: Andiry Xu <andiry.xu@xxxxxxx> > --- > drivers/usb/host/xhci-ring.c | 16 ++++++++-------- > drivers/usb/host/xhci.c | 7 +++++++ > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci- > ring.c > index 5b55ba3..fdcfd8f 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -740,7 +740,7 @@ remove_finished_td: > * so remove it from the endpoint ring's TD list. Keep it > in > * the cancelled TD list for URB completion later. > */ > - list_del(&cur_td->td_list); > + list_del_init(&cur_td->td_list); > } > last_unlinked_td = cur_td; > xhci_stop_watchdog_timer_in_irq(xhci, ep); > @@ -768,7 +768,7 @@ remove_finished_td: > do { > cur_td = list_entry(ep->cancelled_td_list.next, > struct xhci_td, cancelled_td_list); > - list_del(&cur_td->cancelled_td_list); > + list_del_init(&cur_td->cancelled_td_list); > > /* Clean up the cancelled URB */ > /* Doesn't matter what we pass for status, since the core > will > @@ -876,9 +876,9 @@ void xhci_stop_endpoint_command_watchdog(unsigned > long arg) > cur_td = list_first_entry(&ring->td_list, > struct xhci_td, > td_list); > - list_del(&cur_td->td_list); > + list_del_init(&cur_td->td_list); > if (!list_empty(&cur_td->cancelled_td_list)) > - list_del(&cur_td->cancelled_td_list); > + list_del_init(&cur_td->cancelled_td_list); > xhci_giveback_urb_in_irq(xhci, cur_td, > -ESHUTDOWN, "killed"); > } > @@ -887,7 +887,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned > long arg) > &temp_ep->cancelled_td_list, > struct xhci_td, > cancelled_td_list); > - list_del(&cur_td->cancelled_td_list); > + list_del_init(&cur_td->cancelled_td_list); > xhci_giveback_urb_in_irq(xhci, cur_td, > -ESHUTDOWN, "killed"); > } > @@ -1579,10 +1579,10 @@ td_cleanup: > else > *status = 0; > } > - list_del(&td->td_list); > + list_del_init(&td->td_list); > /* Was this TD slated to be cancelled but completed anyway? > */ > if (!list_empty(&td->cancelled_td_list)) > - list_del(&td->cancelled_td_list); > + list_del_init(&td->cancelled_td_list); > > urb_priv->td_cnt++; > /* Giveback the urb when all the tds are completed */ > @@ -3361,7 +3361,7 @@ cleanup: > /* Clean up a partially enqueued isoc transfer. */ > > for (i--; i >= 0; i--) > - list_del(&urb_priv->td[i]->td_list); > + list_del_init(&urb_priv->td[i]->td_list); > > /* Use the first TD as a temporary variable to turn the TDs we've > queued > * into No-ops with a software-owned cycle bit. That way the > hardware > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 8e84acf..3a0f695 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1252,6 +1252,13 @@ int xhci_urb_dequeue(struct usb_hcd *hcd, struct > urb *urb, int status) > if (temp == 0xffffffff || (xhci->xhc_state & XHCI_STATE_HALTED)) > { > xhci_dbg(xhci, "HW died, freeing TD.\n"); > urb_priv = urb->hcpriv; > + for (i = urb_priv->td_cnt; i < urb_priv->length; i++) { > + td = urb_priv->td[i]; > + if (!list_empty(&td->td_list)) > + list_del_init(&td->td_list); > + if (!list_empty(&td->cancelled_td_list)) > + list_del_init(&td->cancelled_td_list); > + } > Can we just add the urb's unprocessed tds to cancelled_td_list here, and let the watchdog function give it back and free urb_priv later? I think that complies with the normal path we dequeue a urb. Thanks, Andiry -- 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
