ChangeSet 1.1186, 2003/05/20 11:48:24-07:00, david-b@pacbell.net

[PATCH] USB: Fix machine lockup when unloading HC driver (part 2)

Alan Stern wrote:
> I suggest you just forget about acquiring the lock in status_dequeue() and
> simply leave it as
>
> 	del_timer_sync (&hcd->rh_timer);
> 	hcd->rh_timer.data = 0;

Hmm, so if some other URB gets queued in that window,
it'll get trashed?  Unlikely .. the clean fix would be
making the status endpoint have a real URB queue.

I combined your suggested change with two others:
(a) protect the status-unlink and control completion
    handlers against IRQs [ the cases Duncan noted]
(b) use mod_timer to retrigger the timer, instead of the
    heavy weight path.


diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c	Tue May 20 17:25:02 2003
+++ b/drivers/usb/core/hcd.c	Tue May 20 17:25:02 2003
@@ -324,6 +324,7 @@
 	const u8	*bufp = 0;
 	u8		*ubuf = urb->transfer_buffer;
 	int		len = 0;
+	unsigned long	flags;
 
 	typeReq  = (cmd->bRequestType << 8) | cmd->bRequest;
 	wValue   = le16_to_cpu (cmd->wValue);
@@ -436,7 +437,9 @@
 	}
 
 	/* any errors get returned through the urb completion */
+	local_irq_save (flags);
 	usb_hcd_giveback_urb (hcd, urb, NULL);
+	local_irq_restore (flags);
 	return 0;
 }
 
@@ -500,13 +503,13 @@
 
 	/* complete the status urb, or retrigger the timer */
 	spin_lock (&hcd_data_lock);
-	hcd->rh_timer.data = 0;
 	if (length > 0) {
+		hcd->rh_timer.data = 0;
 		urb->actual_length = length;
 		urb->status = 0;
 		urb->hcpriv = 0;
 	} else
-		rh_status_urb (hcd, urb);
+		mod_timer (&hcd->rh_timer, jiffies + HZ/4);
 	spin_unlock (&hcd_data_lock);
 	spin_unlock (&urb->lock);
 
@@ -541,15 +544,14 @@
 {
 	unsigned long	flags;
 
-	spin_lock_irqsave (&hcd_data_lock, flags);
-	hcd->rh_timer.data = 0;
-	spin_unlock_irqrestore (&hcd_data_lock, flags);
-
 	/* note:  always a synchronous unlink */
 	del_timer_sync (&hcd->rh_timer);
+	hcd->rh_timer.data = 0;
 
+	local_irq_save (flags);
 	urb->hcpriv = 0;
 	usb_hcd_giveback_urb (hcd, urb, NULL);
+	local_irq_restore (flags);
 }
 
 /*-------------------------------------------------------------------------*/