ChangeSet 1.1722.97.59, 2004/06/09 11:10:09-07:00, stern@rowland.harvard.edu [PATCH] USB: Minor tidying up of hub driver After my last few changesets there were a few small items that needed to be tidied up. Update kerneldoc to reflect the actual operation of usb_disconnect() and usb_new_device(). The new locking requirements are listed too, though they aren't all implemented yet. Fulfill the new locking requirement in hcd_panic(). Remove unneeded local variables to conserve stack space in usb_disconnect(), which calls itself recursively. In hub_port_connect_change(), store the parent's children[] pointer as late as possible and don't lock the new device until then (that's when it becomes globally accessible). This will minimize the time that the not-fully-configured device structure is visible to other parts of the kernel. Signed-off-by: Alan Stern Signed-off-by: Greg Kroah-Hartman drivers/usb/core/hcd.c | 2 ++ drivers/usb/core/hub.c | 48 +++++++++++++++++++++++++----------------------- 2 files changed, 27 insertions(+), 23 deletions(-) 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:17 2004 +++ b/drivers/usb/core/hcd.c Fri Jun 18 10:57:17 2004 @@ -1579,11 +1579,13 @@ unsigned i; /* hc's root hub is removed later removed in hcd->stop() */ + down (&hub->serialize); hub->state = USB_STATE_NOTATTACHED; for (i = 0; i < hub->maxchild; i++) { if (hub->children [i]) usb_disconnect (&hub->children [i]); } + up (&hub->serialize); } /** 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:17 2004 +++ b/drivers/usb/core/hub.c Fri Jun 18 10:57:17 2004 @@ -868,6 +868,9 @@ * Context: !in_interrupt () * * Something got disconnected. Get rid of it, and all of its children. + * If *pdev is a normal device then the parent hub should be locked. + * If *pdev is a root hub then this routine will acquire the + * usb_bus_list_lock on behalf of the caller. * * Only hub drivers (including virtual root hub drivers for host * controllers) should ever call this. @@ -877,20 +880,12 @@ void usb_disconnect(struct usb_device **pdev) { struct usb_device *udev = *pdev; - struct usb_bus *bus; - struct usb_operations *ops; int i; if (!udev) { pr_debug ("%s nodev\n", __FUNCTION__); return; } - bus = udev->bus; - if (!bus) { - pr_debug ("%s nobus\n", __FUNCTION__); - return; - } - ops = bus->op; /* mark the device as inactive, so any further urb submissions for * this device will fail. @@ -906,9 +901,8 @@ /* Free up all the children before we remove this device */ for (i = 0; i < USB_MAXCHILDREN; i++) { - struct usb_device **child = udev->children + i; - if (*child) - usb_disconnect(child); + if (udev->children[i]) + usb_disconnect(&udev->children[i]); } /* deallocate hcd/hardware state ... nuking all pending urbs and @@ -987,21 +981,23 @@ /* * usb_new_device - perform initial device setup (usbcore-internal) - * @dev: newly addressed device (in ADDRESS state) + * @udev: newly addressed device (in ADDRESS state) * * This is called with devices which have been enumerated, but not yet * configured. The device descriptor is available, but not descriptors - * for any device configuration. The caller owns dev->serialize, and - * the device is not visible through sysfs or other filesystem code. + * for any device configuration. The caller must have locked udev and + * either the parent hub (if udev is a normal device) or else the + * usb_bus_list_lock (if udev is a root hub). The parent's pointer to + * udev has already been installed, but udev is not yet visible through + * sysfs or other filesystem code. * * Returns 0 for success (device is configured and listed, with its - * interfaces, in sysfs); else a negative errno value. On error, one - * reference count to the device has been dropped. + * interfaces, in sysfs); else a negative errno value. * * This call is synchronous, and may not be used in an interrupt context. * * Only the hub driver should ever call this; root hub registration - * uses it only indirectly. + * uses it indirectly. */ int usb_new_device(struct usb_device *udev) { @@ -1052,6 +1048,7 @@ if (err) { dev_err(&udev->dev, "can't set config #%d, error %d\n", c, err); + usb_remove_sysfs_dev_files(udev); device_del(&udev->dev); goto fail; } @@ -1548,8 +1545,6 @@ udev->speed = USB_SPEED_LOW; else udev->speed = USB_SPEED_UNKNOWN; - - down (&udev->serialize); /* set the address */ choose_address(udev); @@ -1610,10 +1605,20 @@ && highspeed_hubs != 0) check_highspeed (hub, udev, port); - /* Run it through the hoops (find a driver, etc) */ + /* Store the parent's children[] pointer. At this point + * udev becomes globally accessible, although presumably + * no one will look at it until hdev is unlocked. + */ + down (&udev->serialize); hdev->children[port] = udev; + + /* Run it through the hoops (find a driver, etc) */ status = usb_new_device(udev); if (status) + hdev->children[port] = NULL; + + up (&udev->serialize); + if (status) goto loop; status = hub_power_remaining(hub, hdev); @@ -1622,16 +1627,13 @@ "%dmA power budget left\n", 2 * status); - up (&udev->serialize); return; loop: - hdev->children[port] = NULL; hub_port_disable(hdev, port); usb_disable_endpoint(udev, 0 + USB_DIR_IN); usb_disable_endpoint(udev, 0 + USB_DIR_OUT); release_address(udev); - up (&udev->serialize); usb_put_dev(udev); if (status == -ENOTCONN) break;