summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlan Stern <stern@rowland.harvard.edu>2006-08-11 16:01:45 -0400
committerGreg Kroah-Hartman <gregkh@suse.de>2006-09-27 11:58:54 -0700
commit455b25fb209c8241e2163b491228b28667d82c1c (patch)
treec7155eb33fa8b2916d52472fb517165b3b32813c
parentde06a3b842b31b31220637c869f112cfbc1a5ef6 (diff)
usbcore: make hcd_endpoint_disable wait for queue to drain
The inconsistent lock state problem in usbcore (the one that shows up when an HCD is unloaded) comes down to two inter-related problems: usb_rh_urb_dequeue() isn't set up to be called with interrupts disabled. hcd_endpoint_disable() doesn't wait for all URBs on the endpoint's queue to complete. The two problems are related because the one type of URB that isn't likely to be complete when hcd_endpoint_disable() returns is a root-hub URB. Right now usb_rh_urb_dequeue() waits for them to complete, and it assumes interrupts are enabled so it can wait. But hcd_endpoint_disable() calls it with interrupts disabled. Now, it should be legal to unlink root-hub URBs with interrupts disabled. The solution is to move the waiting into hcd_endpoint_disable(), where it belongs. This patch (as754) does that. It turns out to be completely safe to replace the del_timer_sync() with a simple del_timer(). It doesn't matter if the timer routine is running; hcd_root_hub_lock will synchronize the two threads and the status URB will complete with an unlink error, as it should. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
-rw-r--r--drivers/usb/core/hcd.c65
1 files changed, 35 insertions, 30 deletions
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index dc9628c5893..ea20a3a5a9b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -633,31 +633,20 @@ static int rh_urb_enqueue (struct usb_hcd *hcd, struct urb *urb)
/*-------------------------------------------------------------------------*/
-/* Asynchronous unlinks of root-hub control URBs are legal, but they
- * don't do anything. Status URB unlinks must be made in process context
- * with interrupts enabled.
+/* Unlinks of root-hub control URBs are legal, but they don't do anything
+ * since these URBs always execute synchronously.
*/
static int usb_rh_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
{
- if (usb_pipeendpoint(urb->pipe) == 0) { /* Control URB */
- if (in_interrupt())
- return 0; /* nothing to do */
-
- spin_lock_irq(&urb->lock); /* from usb_kill_urb */
- ++urb->reject;
- spin_unlock_irq(&urb->lock);
-
- wait_event(usb_kill_urb_queue,
- atomic_read(&urb->use_count) == 0);
+ unsigned long flags;
- spin_lock_irq(&urb->lock);
- --urb->reject;
- spin_unlock_irq(&urb->lock);
+ if (usb_pipeendpoint(urb->pipe) == 0) { /* Control URB */
+ ; /* Do nothing */
} else { /* Status URB */
if (!hcd->uses_new_polling)
- del_timer_sync (&hcd->rh_timer);
- local_irq_disable ();
+ del_timer (&hcd->rh_timer);
+ local_irq_save (flags);
spin_lock (&hcd_root_hub_lock);
if (urb == hcd->status_urb) {
hcd->status_urb = NULL;
@@ -667,7 +656,7 @@ static int usb_rh_urb_dequeue (struct usb_hcd *hcd, struct urb *urb)
spin_unlock (&hcd_root_hub_lock);
if (urb)
usb_hcd_giveback_urb (hcd, urb, NULL);
- local_irq_enable ();
+ local_irq_restore (flags);
}
return 0;
@@ -1355,7 +1344,8 @@ done:
/*-------------------------------------------------------------------------*/
/* disables the endpoint: cancels any pending urbs, then synchronizes with
- * the hcd to make sure all endpoint state is gone from hardware. use for
+ * the hcd to make sure all endpoint state is gone from hardware, and then
+ * waits until the endpoint's queue is completely drained. use for
* set_configuration, set_interface, driver removal, physical disconnect.
*
* example: a qh stored in ep->hcpriv, holding state related to endpoint
@@ -1374,22 +1364,13 @@ hcd_endpoint_disable (struct usb_device *udev, struct usb_host_endpoint *ep)
local_irq_disable ();
- /* FIXME move most of this into message.c as part of its
- * endpoint disable logic
- */
-
/* ep is already gone from udev->ep_{in,out}[]; no more submits */
rescan:
spin_lock (&hcd_data_lock);
list_for_each_entry (urb, &ep->urb_list, urb_list) {
int tmp;
- /* another cpu may be in hcd, spinning on hcd_data_lock
- * to giveback() this urb. the races here should be
- * small, but a full fix needs a new "can't submit"
- * urb state.
- * FIXME urb->reject should allow that...
- */
+ /* the urb may already have been unlinked */
if (urb->status != -EINPROGRESS)
continue;
usb_get_urb (urb);
@@ -1431,6 +1412,30 @@ rescan:
might_sleep ();
if (hcd->driver->endpoint_disable)
hcd->driver->endpoint_disable (hcd, ep);
+
+ /* Wait until the endpoint queue is completely empty. Most HCDs
+ * will have done this already in their endpoint_disable method,
+ * but some might not. And there could be root-hub control URBs
+ * still pending since they aren't affected by the HCDs'
+ * endpoint_disable methods.
+ */
+ while (!list_empty (&ep->urb_list)) {
+ spin_lock_irq (&hcd_data_lock);
+
+ /* The list may have changed while we acquired the spinlock */
+ urb = NULL;
+ if (!list_empty (&ep->urb_list)) {
+ urb = list_entry (ep->urb_list.prev, struct urb,
+ urb_list);
+ usb_get_urb (urb);
+ }
+ spin_unlock_irq (&hcd_data_lock);
+
+ if (urb) {
+ usb_kill_urb (urb);
+ usb_put_urb (urb);
+ }
+ }
}
/*-------------------------------------------------------------------------*/