[PATCH] USB: Fix machine lockup when unloading HC driver

Alan Stern wrote:
> I finally got tired of my computer locking up when I tried to rmmod the
> low-level host controller driver.  It turns out the problem lies in the
> root-hub status urb code in core/hcd.c -- primarily a result of
> rh_report_status() not calling hcd_giveback_urb()...

Or in short:  your patch removes some old logic for the "automagic
interrupt transfer" special casing ... which recently started to
break that rmmod path.

With automagic, the only time an interrupt urb (like the root hub
status urb) could legitimately be given back was for unlink.  But
that unlink doesn't seem to be issued in the same way lately during
the rmmod paths.  (If they're less bizarre lately, that's good!)


> If this patch seems all right, will you please let Greg know it's okay to
> apply it?

I changed a couple minor things below ... basically (a) fixing the
issue Duncan Sands pointed out (always call completions with irqs
disabled, even if hub driver currently doesn't care), (b) better
logic to avoid retriggering the timer during shutdown, (c) not
doing del_timer_sync() while holding that lock, plus (d) a minor
linewrap fix.

drivers/usb/core/hcd.c

@@ -454,20 +454,22 @@ static int rh_status_urb (struct usb_hcd *hcd, struct urb *urb)
 	int	len = 1 + (urb->dev->maxchild / 8);
 
 	/* rh_timer protected by hcd_data_lock */
-	if (timer_pending (&hcd->rh_timer)
+	if (hcd->rh_timer.data
 			|| urb->status != -EINPROGRESS
 			|| urb->transfer_buffer_length < len) {
-		dev_dbg (hcd->controller, "not queuing status urb, stat %d\n", urb->status);
+		dev_dbg (hcd->controller,
+				"not queuing rh status urb, stat %d\n",
+				urb->status);
 		return -EINVAL;
 	}
 
-	urb->hcpriv = hcd;	/* nonzero to indicate it's queued */
 	init_timer (&hcd->rh_timer);
 	hcd->rh_timer.function = rh_report_status;
 	hcd->rh_timer.data = (unsigned long) urb;
 	/* USB 2.0 spec says 256msec; this is close enough */
 	hcd->rh_timer.expires = jiffies + HZ/4;
 	add_timer (&hcd->rh_timer);
+	urb->hcpriv = hcd;	/* nonzero to indicate it's queued */
 	return 0;
 }
 
@@ -481,39 +483,37 @@ static void rh_report_status (unsigned long ptr)
 	unsigned long	flags;
 
 	urb = (struct urb *) ptr;
-	spin_lock_irqsave (&urb->lock, flags);
-	if (!urb->dev) {
-		spin_unlock_irqrestore (&urb->lock, flags);
+	local_irq_save (flags);
+	spin_lock (&urb->lock);
+
+	/* do nothing if the hc is gone or the urb's been unlinked */
+	if (!urb->dev
+			|| urb->status != -EINPROGRESS
+			|| (hcd = urb->dev->bus->hcpriv) == 0
+			|| !HCD_IS_RUNNING (hcd->state)) {
+		spin_unlock (&urb->lock);
+		local_irq_restore (flags);
 		return;
 	}
 
-	hcd = urb->dev->bus->hcpriv;
-	if (urb->status == -EINPROGRESS) {
-		if (HCD_IS_RUNNING (hcd->state)) {
-			length = hcd->driver->hub_status_data (hcd,
-					urb->transfer_buffer);
-			spin_unlock_irqrestore (&urb->lock, flags);
+	length = hcd->driver->hub_status_data (hcd, urb->transfer_buffer);
+
+	/* complete the status urb, or retrigger the timer */
+	spin_lock (&hcd_data_lock);
+	hcd->rh_timer.data = 0;
 	if (length > 0) {
 		urb->actual_length = length;
 		urb->status = 0;
 		urb->hcpriv = 0;
-				urb->complete (urb, NULL);
-				return;
-			}
 	} else
-			spin_unlock_irqrestore (&urb->lock, flags);
-
-		/* retrigger timer until completion:  success or unlink */
-		spin_lock_irqsave (&hcd_data_lock, flags);
 		rh_status_urb (hcd, urb);
-		spin_unlock_irqrestore (&hcd_data_lock, flags);
-	} else {
-		/* this urb's been unlinked */
-		urb->hcpriv = 0;
-		spin_unlock_irqrestore (&urb->lock, flags);
+	spin_unlock (&hcd_data_lock);
+	spin_unlock (&urb->lock);
 
+	/* local irqs are always blocked in completions */
+	if (length > 0)
 		usb_hcd_giveback_urb (hcd, urb, NULL);
-	}
+	local_irq_restore (flags);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -542,11 +542,13 @@ void usb_rh_status_dequeue (struct usb_hcd *hcd, struct urb *urb)
 	unsigned long	flags;
 
 	spin_lock_irqsave (&hcd_data_lock, flags);
-	del_timer_sync (&hcd->rh_timer);
 	hcd->rh_timer.data = 0;
 	spin_unlock_irqrestore (&hcd_data_lock, flags);
 
-	/* we rely on RH callback code not unlinking its URB! */
+	/* note:  always a synchronous unlink */
+	del_timer_sync (&hcd->rh_timer);
+
+	urb->hcpriv = 0;
 	usb_hcd_giveback_urb (hcd, urb, NULL);
 }