[PATCH] USB: Fix bus-list root-hub race

There are a few places where the code enumerates through all the USB
devices on all the buses, starting with each bus's root hub and working
down.  However a bus does not always have a root hub, and the code does
not check that the root_hub pointer is non-NULL.  This patch fixes the
problem, using the usb_bus_list_lock semaphore to synchronize access when
root hubs are added or removed.

In addition it seemed like a good idea to minimize the time that a
non-fully-configured root hub is accessible through the bus's pointer.  So
this patch delays setting the pointer and holds usb_bus_list_lock while
configuring a root hub.

It turned out that a bunch of things needed to be changed for all this to
work:

	Check for NULL root_hub pointer in usb_device_read() and
	usb_find_device().

	Pass the root-hub device as a separate argument to
	hcd_register_root().

	Make usb_register_root_hub() acquire the usb_bus_list_lock and
	set the bus->root_hub pointer.

	For consistency's sake, move the place where the children[]
	pointer to a non-root-hub device gets stored as close as possible
	to where usb_new_device() is called.

	Make usb_disconnect() acquire the usb_bus_list_lock when removing
	a root hub.

	Change usb_hcd_pci_remove() and the non-PCI host drivers so that
	they call usb_disconnect() with a pointer to the bus's root_hub
	pointer, not a pointer to a temporary variable.

	Change all the host controller drivers not to store the root_hub
	pointer in the bus structure but instead to pass it as a new
	argument to hcd_register_root().

I made some attempt to update the hc_sl811 driver along with the rest, but
it's pretty clear that driver won't work in the current framework.  Among
other things, it never reads the root hub's device descriptor.  To what
extent is the driver really supported?
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

drivers/usb/core/devices.c

@@ -589,6 +589,8 @@ static ssize_t usb_device_read(struct file *file, char __user *buf, size_t nbyte
 		bus = list_entry(buslist, struct usb_bus, bus_list);
 
 		/* recurse through all children of the root hub */
+		if (!bus->root_hub)
+			continue;
 		down(&bus->root_hub->serialize);
 		ret = usb_device_dump(&buf, &nbytes, &skip_bytes, ppos, bus->root_hub, bus, 0, 0, 0);
 		up(&bus->root_hub->serialize);

drivers/usb/core/hcd-pci.c

@@ -229,7 +229,6 @@ EXPORT_SYMBOL (usb_hcd_pci_probe);
 void usb_hcd_pci_remove (struct pci_dev *dev)
 {
 	struct usb_hcd		*hcd;
-	struct usb_device	*hub;
 
 	hcd = pci_get_drvdata(dev);
 	if (!hcd)
@@ -239,12 +238,11 @@ void usb_hcd_pci_remove (struct pci_dev *dev)
 	if (in_interrupt ())
 		BUG ();
 
-	hub = hcd->self.root_hub;
 	if (HCD_IS_RUNNING (hcd->state))
 		hcd->state = USB_STATE_QUIESCING;
 
 	dev_dbg (hcd->self.controller, "roothub graceful disconnect\n");
-	usb_disconnect (&hub);
+	usb_disconnect (&hcd->self.root_hub);
 
 	hcd->driver->stop (hcd);
 	hcd_buffer_destroy (hcd);

drivers/usb/core/hcd.c

@@ -764,8 +764,9 @@ EXPORT_SYMBOL (usb_deregister_bus);
  *
  * The USB host controller calls this function to register the root hub
  * properly with the USB subsystem.  It sets up the device properly in
- * the device model tree, and then calls usb_new_device() to register the
- * usb device.  It also assigns the root hub's USB address (always 1).
+ * the device tree and stores the root_hub pointer in the bus structure,
+ * then calls usb_new_device() to register the usb device.  It also
+ * assigns the root hub's USB address (always 1).
  */
 int usb_register_root_hub (struct usb_device *usb_dev, struct device *parent_dev)
 {
@@ -779,6 +780,9 @@ int usb_register_root_hub (struct usb_device *usb_dev, struct device *parent_dev
 	set_bit (devnum, usb_dev->bus->devmap.devicemap);
 	usb_dev->state = USB_STATE_ADDRESS;
 
+	down (&usb_bus_list_lock);
+	usb_dev->bus->root_hub = usb_dev;
+
 	usb_dev->epmaxpacketin[0] = usb_dev->epmaxpacketout[0] = 64;
 	retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE);
 	if (retval != sizeof usb_dev->descriptor) {
@@ -790,9 +794,12 @@ int usb_register_root_hub (struct usb_device *usb_dev, struct device *parent_dev
 	down (&usb_dev->serialize);
 	retval = usb_new_device (usb_dev);
 	up (&usb_dev->serialize);
-	if (retval)
+	if (retval) {
+		usb_dev->bus->root_hub = NULL;
 		dev_err (parent_dev, "can't register root hub for %s, %d\n",
 				usb_dev->dev.bus_id, retval);
+	}
+	up (&usb_bus_list_lock);
 	return retval;
 }
 EXPORT_SYMBOL (usb_register_root_hub);

drivers/usb/core/hcd.h

@@ -339,7 +339,8 @@ extern void usb_deregister_bus (struct usb_bus *);
 extern int usb_register_root_hub (struct usb_device *usb_dev,
 		struct device *parent_dev);
 
-static inline int hcd_register_root (struct usb_hcd *hcd)
+static inline int hcd_register_root (struct usb_device *usb_dev,
+		struct usb_hcd *hcd)
 {
 	/* hcd->driver->start() reported can_wakeup, probably with
 	 * assistance from board's boot firmware.
@@ -349,8 +350,7 @@ static inline int hcd_register_root (struct usb_hcd *hcd)
 		dev_dbg (hcd->self.controller, "supports USB remote wakeup\n");
 	hcd->remote_wakeup = hcd->can_wakeup;
 
-	return usb_register_root_hub (
-		hcd_to_bus (hcd)->root_hub, hcd->self.controller);
+	return usb_register_root_hub (usb_dev, hcd->self.controller);
 }
 
 /*-------------------------------------------------------------------------*/

drivers/usb/core/hub.c

@@ -892,12 +892,14 @@ void usb_disconnect(struct usb_device **pdev)
 	}
 	ops = bus->op;
 
-	*pdev = NULL;
-
 	/* mark the device as inactive, so any further urb submissions for
 	 * this device will fail.
 	 */
 	udev->state = USB_STATE_NOTATTACHED;
+
+	/* lock the bus list on behalf of HCDs unregistering their root hubs */
+	if (!udev->parent)
+		down(&usb_bus_list_lock);
 	down(&udev->serialize);
 
 	dev_info (&udev->dev, "USB disconnect, address %d\n", udev->devnum);
@@ -914,12 +916,20 @@ void usb_disconnect(struct usb_device **pdev)
 	 */
 	usb_disable_device(udev, 0);
 
-	/* Free the device number and remove the /proc/bus/usb entry */
+	/* Free the device number, remove the /proc/bus/usb entry and
+	 * the sysfs attributes, and delete the parent's children[]
+	 * (or root_hub) pointer.
+	 */
 	dev_dbg (&udev->dev, "unregistering device\n");
 	release_address(udev);
 	usbfs_remove_device(udev);
-	up(&udev->serialize);
 	usb_remove_sysfs_dev_files(udev);
+	*pdev = NULL;
+
+	up(&udev->serialize);
+	if (!udev->parent)
+		up(&usb_bus_list_lock);
+
 	device_unregister(&udev->dev);
 }
 
@@ -1403,8 +1413,6 @@ hub_port_init (struct usb_device *hdev, struct usb_device *udev, int port)
 		goto fail;
 	}
 
-	/* now dev is visible to other tasks */
-	hdev->children[port] = udev;
 	retval = 0;
 
 fail:
@@ -1550,7 +1558,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port,
 			goto loop;
 		}
 
-		/* reset, get descriptor, add to hub's children */
+		/* reset and get descriptor */
 		status = hub_port_init(hdev, udev, port);
 		if (status < 0)
 			goto loop;
@@ -1603,6 +1611,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port,
 			check_highspeed (hub, udev, port);
 
 		/* Run it through the hoops (find a driver, etc) */
+		hdev->children[port] = udev;
 		status = usb_new_device(udev);
 		if (status)
 			goto loop;

drivers/usb/core/usb.c

@@ -883,6 +883,8 @@ struct usb_device *usb_find_device(u16 vendor_id, u16 product_id)
 	     buslist != &usb_bus_list; 
 	     buslist = buslist->next) {
 		bus = container_of(buslist, struct usb_bus, bus_list);
+		if (!bus->root_hub)
+			continue;
 		dev = match_device(bus->root_hub, vendor_id, product_id);
 		if (dev)
 			goto exit;

drivers/usb/gadget/dummy_hcd.c

@@ -1664,7 +1664,7 @@ static int dummy_start (struct usb_hcd *hcd)
 	INIT_LIST_HEAD (&hcd->dev_list);
 	usb_register_bus (bus);
 
-	bus->root_hub = root = usb_alloc_dev (0, bus, 0);
+	root = usb_alloc_dev (0, bus, 0);
 	if (!root) {
 		retval = -ENOMEM;
 clean1:
@@ -1678,8 +1678,7 @@ static int dummy_start (struct usb_hcd *hcd)
 	root->speed = USB_SPEED_HIGH;
 
 	/* ...then configured, so khubd sees us. */
-	if ((retval = hcd_register_root (&dum->hcd)) != 0) {
-		bus->root_hub = 0;
+	if ((retval = hcd_register_root (root, &dum->hcd)) != 0) {
 		usb_put_dev (root);
 clean2:
 		dum->hcd.state = USB_STATE_QUIESCING;

drivers/usb/host/ehci-hcd.c

@@ -520,7 +520,7 @@ static int ehci_start (struct usb_hcd *hcd)
 
 	/* wire up the root hub */
 	bus = hcd_to_bus (hcd);
-	bus->root_hub = udev = usb_alloc_dev (NULL, bus, 0);
+	udev = usb_alloc_dev (NULL, bus, 0);
 	if (!udev) {
 done2:
 		ehci_mem_cleanup (ehci);
@@ -553,11 +553,10 @@ static int ehci_start (struct usb_hcd *hcd)
 	 * and device drivers may start it running.
 	 */
 	udev->speed = USB_SPEED_HIGH;
-	if (hcd_register_root (hcd) != 0) {
+	if (hcd_register_root (udev, hcd) != 0) {
 		if (hcd->state == USB_STATE_RUNNING)
 			ehci_ready (ehci);
 		ehci_reset (ehci);
-		bus->root_hub = 0;
 		usb_put_dev (udev); 
 		retval = -ENODEV;
 		goto done2;

drivers/usb/host/hc_sl811_rh.c

@@ -557,18 +557,24 @@ static int rh_unlink_urb (struct urb * urb)
 static int rh_connect_rh (hci_t * hci)
 {
 	struct usb_device *usb_dev;
+	int retval;
 
 	hci->rh.devnum = 0;
 	usb_dev = usb_alloc_dev (NULL, hci->bus, 0);
 	if (!usb_dev)
 		return -ENOMEM;
 
-	hci->bus->root_hub = usb_dev;
 	usb_dev->devnum = 1;
 	usb_dev->bus->devnum_next = usb_dev->devnum + 1;
 	set_bit (usb_dev->devnum, usb_dev->bus->devmap.devicemap);
 
-	if (usb_new_device (usb_dev) != 0) {
+	down (&usb_bus_list_lock);
+	hci->bus->root_hub = usb_dev;
+	retval = usb_new_device (usb_dev);
+	if (retval != 0)
+		hci->bus->root_hub = NULL;
+	up (&usb_bus_list_lock);
+	if (retval != 0) {
 		usb_put_dev (usb_dev);
 		return -ENODEV;
 	}

drivers/usb/host/ohci-hcd.c

@@ -561,7 +561,7 @@ static int hc_start (struct ohci_hcd *ohci)
 	}
  
 	/* connect the virtual root hub */
-	bus->root_hub = udev = usb_alloc_dev (NULL, bus, 0);
+	udev = usb_alloc_dev (NULL, bus, 0);
 	ohci->hcd.state = USB_STATE_RUNNING;
 	if (!udev) {
 		disable (ohci);
@@ -571,9 +571,8 @@ static int hc_start (struct ohci_hcd *ohci)
 	}
 
 	udev->speed = USB_SPEED_FULL;
-	if (hcd_register_root (&ohci->hcd) != 0) {
+	if (hcd_register_root (udev, &ohci->hcd) != 0) {
 		usb_put_dev (udev);
-		bus->root_hub = NULL;
 		disable (ohci);
 		ohci->hc_control &= ~OHCI_CTRL_HCFS;
 		writel (ohci->hc_control, &ohci->regs->control);

drivers/usb/host/ohci-omap.c

@@ -454,7 +454,6 @@ int usb_hcd_omap_probe (const struct hc_driver *driver,
  */
 void usb_hcd_omap_remove (struct usb_hcd *hcd, struct omap_dev *dev)
 {
-	struct usb_device	*hub;
 	void *base;
 
 	info ("remove: %s, state %x", hcd->self.bus_name, hcd->state);
@@ -462,11 +461,10 @@ void usb_hcd_omap_remove (struct usb_hcd *hcd, struct omap_dev *dev)
 	if (in_interrupt ())
 		BUG ();
 
-	hub = hcd->self.root_hub;
 	hcd->state = USB_STATE_QUIESCING;
 
 	dbg ("%s: roothub graceful disconnect", hcd->self.bus_name);
-	usb_disconnect (&hub);
+	usb_disconnect (&hcd->self.root_hub);
 
 	hcd->driver->stop (hcd);
 	hcd_buffer_destroy (hcd);

drivers/usb/host/ohci-sa1111.c

@@ -237,7 +237,6 @@ int usb_hcd_sa1111_probe (const struct hc_driver *driver,
  */
 void usb_hcd_sa1111_remove (struct usb_hcd *hcd, struct sa1111_dev *dev)
 {
-	struct usb_device	*hub;
 	void *base;
 
 	info ("remove: %s, state %x", hcd->self.bus_name, hcd->state);
@@ -245,11 +244,10 @@ void usb_hcd_sa1111_remove (struct usb_hcd *hcd, struct sa1111_dev *dev)
 	if (in_interrupt ())
 		BUG ();
 
-	hub = hcd->self.root_hub;
 	hcd->state = USB_STATE_QUIESCING;
 
 	dbg ("%s: roothub graceful disconnect", hcd->self.bus_name);
-	usb_disconnect (&hub);
+	usb_disconnect (&hcd->self.root_hub);
 
 	hcd->driver->stop (hcd);
 	hcd->state = USB_STATE_HALT;

drivers/usb/host/uhci-hcd.c

@@ -2185,7 +2185,7 @@ static int uhci_start(struct usb_hcd *hcd)
 
 	uhci->rh_numports = port;
 
-	hcd->self.root_hub = udev = usb_alloc_dev(NULL, &hcd->self, 0);
+	udev = usb_alloc_dev(NULL, &hcd->self, 0);
 	if (!udev) {
 		dev_err(uhci_dev(uhci), "unable to allocate root hub\n");
 		goto err_alloc_root_hub;
@@ -2267,7 +2267,7 @@ static int uhci_start(struct usb_hcd *hcd)
 
 	udev->speed = USB_SPEED_FULL;
 
-	if (usb_register_root_hub(udev, uhci_dev(uhci)) != 0) {
+	if (hcd_register_root(udev, &uhci->hcd) != 0) {
 		dev_err(uhci_dev(uhci), "unable to start root hub\n");
 		retval = -ENOMEM;
 		goto err_start_root_hub;
@@ -2295,7 +2295,6 @@ static int uhci_start(struct usb_hcd *hcd)
 
 err_alloc_term_td:
 	usb_put_dev(udev);
-	hcd->self.root_hub = NULL;
 
 err_alloc_root_hub:
 	dma_pool_destroy(uhci->qh_pool);