[PATCH 5b/6] USB: make hub driver's release more robust

This revised patch (as893b) improves the method used by the hub driver to
release its private data structure.  The current code is non-robust,
relying on a memory region not getting reused by another driver after
it has been freed.

Signed-off-by: Alan Stern <[email protected]>

---

It turned out that making khubd responsible doing the final usb_put_intf() 
wasn't such a good idea.  It caused a problem with dummy_hcd.  Doing 
things this way should be better; now all device references held by the 
hub driver will be dropped before hub_disconnect() returns.


Index: usb-2.6/drivers/usb/core/hub.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/hub.c
+++ usb-2.6/drivers/usb/core/hub.c
@@ -49,6 +49,9 @@ struct usb_hub {
 	int			error;		/* last reported error */
 	int			nerrors;	/* track consecutive errors */
 
+	struct completion	*finish;	/* signal that khubd is done
+							using this hub */
+
 	struct list_head	event_list;	/* hubs w/data or errs ready */
 	unsigned long		event_bits[1];	/* status change bitmask */
 	unsigned long		change_bits[1];	/* ports with logical connect
@@ -67,12 +70,15 @@ struct usb_hub {
 	unsigned		limited_power:1;
 	unsigned		quiescing:1;
 	unsigned		activating:1;
+	unsigned		disconnected:1;
 
 	unsigned		has_indicators:1;
 	u8			indicator[USB_MAXCHILDREN];
 	struct delayed_work	leds;
 };
 
+/* Sentinel value for hub->finish to indicate khubd is using the hub */
+#define	HUB_IN_USE(hub)		((struct completion *) (hub))
 
 /* Protect struct usb_device->state and ->children members
  * Note: Both are also protected by ->dev.sem, except that ->state can
@@ -324,7 +330,7 @@ static void kick_khubd(struct usb_hub *h
 	to_usb_interface(hub->intfdev)->pm_usage_cnt = 1;
 
 	spin_lock_irqsave(&hub_event_lock, flags);
-	if (list_empty(&hub->event_list)) {
+	if (!hub->disconnected & list_empty(&hub->event_list)) {
 		list_add_tail(&hub->event_list, &hub_event_list);
 		wake_up(&khubd_wait);
 	}
@@ -333,6 +339,7 @@ static void kick_khubd(struct usb_hub *h
 
 void usb_kick_khubd(struct usb_device *hdev)
 {
+	/* FIXME: What if hdev isn't bound to the hub driver? */
 	kick_khubd(hdev_to_hub(hdev));
 }
 
@@ -931,6 +938,21 @@ static void hub_disconnect(struct usb_in
 {
 	struct usb_hub *hub = usb_get_intfdata (intf);
 	struct usb_device *hdev;
+	struct completion wait_for_khubd;
+
+	/* Take the hub off the event list */
+	spin_lock_irq(&hub_event_lock);
+	hub->disconnected = 1;
+	list_del_init(&hub->event_list);
+
+	/* Wait for khubd to finish using the hub */
+	if (hub->finish == HUB_IN_USE(hub)) {
+		hub->finish = &wait_for_khubd;
+		init_completion(&wait_for_khubd);
+	}
+	spin_unlock_irq(&hub_event_lock);
+	if (hub->finish)
+		wait_for_completion(&wait_for_khubd);
 
 	/* Disconnect all children and quiesce the hub */
 	hub->error = 0;
@@ -945,10 +967,6 @@ static void hub_disconnect(struct usb_in
 	usb_free_urb(hub->urb);
 	hub->urb = NULL;
 
-	spin_lock_irq(&hub_event_lock);
-	list_del_init(&hub->event_list);
-	spin_unlock_irq(&hub_event_lock);
-
 	kfree(hub->descriptor);
 	hub->descriptor = NULL;
 
@@ -2600,19 +2618,19 @@ static void hub_events(void)
 	 * safe since we delete the hub from the event list.
 	 * Not the most efficient, but avoids deadlocks.
 	 */
+	spin_lock_irq(&hub_event_lock);
 	while (1) {
 
 		/* Grab the first entry at the beginning of the list */
-		spin_lock_irq(&hub_event_lock);
-		if (list_empty(&hub_event_list)) {
-			spin_unlock_irq(&hub_event_lock);
+		if (list_empty(&hub_event_list))
 			break;
-		}
-
 		tmp = hub_event_list.next;
 		list_del_init(tmp);
 
 		hub = list_entry(tmp, struct usb_hub, event_list);
+		hub->finish = HUB_IN_USE(hub);
+		spin_unlock_irq(&hub_event_lock);
+
 		hdev = hub->hdev;
 		intf = to_usb_interface(hub->intfdev);
 		hub_dev = &intf->dev;
@@ -2625,13 +2643,10 @@ static void hub_events(void)
 				(u16) hub->change_bits[0],
 				(u16) hub->event_bits[0]);
 
-		usb_get_intf(intf);
-		spin_unlock_irq(&hub_event_lock);
-
 		/* Lock the device, then check to see if we were
 		 * disconnected while waiting for the lock to succeed. */
 		usb_lock_device(hdev);
-		if (hub != usb_get_intfdata(intf))
+		if (unlikely(hub->disconnected))
 			goto loop;
 
 		/* If the hub has died, clean up after it */
@@ -2794,9 +2809,17 @@ loop_autopm:
 			usb_autopm_enable(intf);
 loop:
 		usb_unlock_device(hdev);
-		usb_put_intf(intf);
 
+		/* If hub_disconnect() is waiting for us to finish using
+		 * the hub, tell it we are now done.
+		 */
+		spin_lock_irq(&hub_event_lock);
+		if (hub->finish == HUB_IN_USE(hub))
+			hub->finish = NULL;
+		else
+			complete(hub->finish);
         } /* end while (1) */
+	spin_unlock_irq(&hub_event_lock);
 }
 
 static int hub_thread(void *__unused)


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel