ChangeSet 1.1722.97.58, 2004/06/09 11:09:31-07:00, stern@rowland.harvard.edu [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 Signed-off-by: Greg Kroah-Hartman drivers/usb/core/devices.c | 2 ++ drivers/usb/core/hcd-pci.c | 4 +--- drivers/usb/core/hcd.c | 13 ++++++++++--- drivers/usb/core/hcd.h | 6 +++--- drivers/usb/core/hub.c | 23 ++++++++++++++++------- drivers/usb/core/usb.c | 2 ++ drivers/usb/gadget/dummy_hcd.c | 5 ++--- drivers/usb/host/ehci-hcd.c | 5 ++--- drivers/usb/host/hc_sl811_rh.c | 10 ++++++++-- drivers/usb/host/ohci-hcd.c | 5 ++--- drivers/usb/host/ohci-omap.c | 4 +--- drivers/usb/host/ohci-sa1111.c | 4 +--- drivers/usb/host/uhci-hcd.c | 5 ++--- 13 files changed, 52 insertions(+), 36 deletions(-) diff -Nru a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c --- a/drivers/usb/core/devices.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/devices.c Fri Jun 18 10:57:32 2004 @@ -589,6 +589,8 @@ 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); diff -Nru a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c --- a/drivers/usb/core/hcd-pci.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/hcd-pci.c Fri Jun 18 10:57:32 2004 @@ -229,7 +229,6 @@ 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 @@ 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); diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c --- a/drivers/usb/core/hcd.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/hcd.c Fri Jun 18 10:57:32 2004 @@ -764,8 +764,9 @@ * * 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 @@ 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 @@ 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); diff -Nru a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h --- a/drivers/usb/core/hcd.h Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/hcd.h Fri Jun 18 10:57:32 2004 @@ -339,7 +339,8 @@ 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 @@ 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); } /*-------------------------------------------------------------------------*/ diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c --- a/drivers/usb/core/hub.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/hub.c Fri Jun 18 10:57:32 2004 @@ -892,12 +892,14 @@ } 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 @@ */ 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 @@ goto fail; } - /* now dev is visible to other tasks */ - hdev->children[port] = udev; retval = 0; fail: @@ -1550,7 +1558,7 @@ 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 @@ 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; diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/core/usb.c Fri Jun 18 10:57:32 2004 @@ -883,6 +883,8 @@ 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; diff -Nru a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c --- a/drivers/usb/gadget/dummy_hcd.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/gadget/dummy_hcd.c Fri Jun 18 10:57:32 2004 @@ -1664,7 +1664,7 @@ 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 @@ 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; diff -Nru a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c --- a/drivers/usb/host/ehci-hcd.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/ehci-hcd.c Fri Jun 18 10:57:32 2004 @@ -520,7 +520,7 @@ /* 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 @@ * 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; diff -Nru a/drivers/usb/host/hc_sl811_rh.c b/drivers/usb/host/hc_sl811_rh.c --- a/drivers/usb/host/hc_sl811_rh.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/hc_sl811_rh.c Fri Jun 18 10:57:32 2004 @@ -557,18 +557,24 @@ 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; } diff -Nru a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c --- a/drivers/usb/host/ohci-hcd.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/ohci-hcd.c Fri Jun 18 10:57:32 2004 @@ -561,7 +561,7 @@ } /* 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 @@ } 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); diff -Nru a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c --- a/drivers/usb/host/ohci-omap.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/ohci-omap.c Fri Jun 18 10:57:32 2004 @@ -454,7 +454,6 @@ */ 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 @@ 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); diff -Nru a/drivers/usb/host/ohci-sa1111.c b/drivers/usb/host/ohci-sa1111.c --- a/drivers/usb/host/ohci-sa1111.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/ohci-sa1111.c Fri Jun 18 10:57:32 2004 @@ -237,7 +237,6 @@ */ 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 @@ 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; diff -Nru a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c --- a/drivers/usb/host/uhci-hcd.c Fri Jun 18 10:57:32 2004 +++ b/drivers/usb/host/uhci-hcd.c Fri Jun 18 10:57:32 2004 @@ -2185,7 +2185,7 @@ 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 @@ 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 @@ err_alloc_term_td: usb_put_dev(udev); - hcd->self.root_hub = NULL; err_alloc_root_hub: dma_pool_destroy(uhci->qh_pool);