ChangeSet 1.1722.97.65, 2004/06/11 16:51:56-07:00, stern@rowland.harvard.edu [PATCH] USB: Mark devices as NOTATTACHED as soon as possible This patch implements something we've been lacking for a long time: a way to mark devices as USB_STATE_NOTATTACHED as soon as we know that they're gone. The usb_device->state member is no longer protected by the ->serialize semaphore; instead there's a new private spinlock. Usbcore routines should no longer set ->state directly; instead they should use the new utility routine usb_set_device_state(). There are protections against changing states while devices are being added or removed. Change assignments to udev->state into calls of usb_set_device_state(). Add new private device_state_lock to the hub driver, along with usb_set_device_state() and recursively_mark_NOTATTACHED(). Acquire the new spinlock while adding or removing children[] pointers. When disabling a port that has a child device, mark the child as NOTATTACHED. You mentioned once having tried to do something like this and running into trouble. Take a good look and let me know if you see any difficulties here. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Greg Kroah-Hartman <greg@kroah.com> drivers/usb/core/hcd.c | 4 - drivers/usb/core/hub.c | 97 +++++++++++++++++++++++++++++++++++++++------ drivers/usb/core/message.c | 8 +-- drivers/usb/core/usb.h | 3 + 4 files changed, 94 insertions(+), 18 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:55:49 2004 +++ b/drivers/usb/core/hcd.c Fri Jun 18 10:55:49 2004 @@ -778,7 +778,7 @@ memset (&usb_dev->bus->devmap.devicemap, 0, sizeof usb_dev->bus->devmap.devicemap); set_bit (devnum, usb_dev->bus->devmap.devicemap); - usb_dev->state = USB_STATE_ADDRESS; + usb_set_device_state(usb_dev, USB_STATE_ADDRESS); down (&usb_bus_list_lock); usb_dev->bus->root_hub = usb_dev; @@ -1580,7 +1580,7 @@ /* hc's root hub is removed later removed in hcd->stop() */ down (&hub->serialize); - hub->state = USB_STATE_NOTATTACHED; + usb_set_device_state(hub, USB_STATE_NOTATTACHED); for (i = 0; i < hub->maxchild; i++) { if (hub->children [i]) usb_disconnect (&hub->children [i]); diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c --- a/drivers/usb/core/hub.c Fri Jun 18 10:55:49 2004 +++ b/drivers/usb/core/hub.c Fri Jun 18 10:55:49 2004 @@ -36,6 +36,9 @@ #include "hcd.h" #include "hub.h" +/* Protect all struct usb_device state members */ +static spinlock_t device_state_lock = SPIN_LOCK_UNLOCKED; + /* Wakes up khubd */ static spinlock_t hub_event_lock = SPIN_LOCK_UNLOCKED; @@ -814,6 +817,7 @@ return 0; } +/* FIXME! This routine should be subsumed into hub_reset */ static void hub_start_disconnect(struct usb_device *hdev) { struct usb_device *parent = hdev->parent; @@ -833,6 +837,51 @@ } +static void recursively_mark_NOTATTACHED(struct usb_device *udev) +{ + int i; + + for (i = 0; i < udev->maxchild; ++i) { + if (udev->children[i]) + recursively_mark_NOTATTACHED(udev->children[i]); + } + udev->state = USB_STATE_NOTATTACHED; +} + +/** + * usb_set_device_state - change a device's current state (usbcore-internal) + * @udev: pointer to device whose state should be changed + * @new_state: new state value to be stored + * + * udev->state is _not_ protected by the udev->serialize semaphore. This + * is so that devices can be marked as disconnected as soon as possible, + * without having to wait for the semaphore to be released. Instead, + * changes to the state must be protected by the device_state_lock spinlock. + * + * Once a device has been added to the device tree, all changes to its state + * should be made using this routine. The state should _not_ be set directly. + * + * If udev->state is already USB_STATE_NOTATTACHED then no change is made. + * Otherwise udev->state is set to new_state, and if new_state is + * USB_STATE_NOTATTACHED then all of udev's descendant's states are also set + * to USB_STATE_NOTATTACHED. + */ +void usb_set_device_state(struct usb_device *udev, + enum usb_device_state new_state) +{ + unsigned long flags; + + spin_lock_irqsave(&device_state_lock, flags); + if (udev->state == USB_STATE_NOTATTACHED) + ; /* do nothing */ + else if (new_state != USB_STATE_NOTATTACHED) + udev->state = new_state; + else + recursively_mark_NOTATTACHED(udev); + spin_unlock_irqrestore(&device_state_lock, flags); +} + + static void choose_address(struct usb_device *udev) { int devnum; @@ -890,7 +939,7 @@ /* mark the device as inactive, so any further urb submissions for * this device will fail. */ - udev->state = USB_STATE_NOTATTACHED; + usb_set_device_state(udev, USB_STATE_NOTATTACHED); /* lock the bus list on behalf of HCDs unregistering their root hubs */ if (!udev->parent) @@ -918,7 +967,11 @@ release_address(udev); usbfs_remove_device(udev); usb_remove_sysfs_dev_files(udev); + + /* Avoid races with recursively_mark_NOTATTACHED() */ + spin_lock_irq(&device_state_lock); *pdev = NULL; + spin_unlock_irq(&device_state_lock); up(&udev->serialize); if (!udev->parent) @@ -1061,7 +1114,7 @@ return 0; fail: - udev->state = USB_STATE_NOTATTACHED; + usb_set_device_state(udev, USB_STATE_NOTATTACHED); return err; } @@ -1166,9 +1219,9 @@ if (status == -ENOTCONN || status == 0) { clear_port_feature(hdev, port + 1, USB_PORT_FEAT_C_RESET); - udev->state = status + usb_set_device_state(udev, status ? USB_STATE_NOTATTACHED - : USB_STATE_DEFAULT; + : USB_STATE_DEFAULT); return status; } @@ -1189,6 +1242,9 @@ { int ret; + if (hdev->children[port]) + usb_set_device_state(hdev->children[port], + USB_STATE_NOTATTACHED); ret = clear_port_feature(hdev, port + 1, USB_PORT_FEAT_ENABLE); if (ret) dev_err(hubdev(hdev), "cannot disable port %d (err = %d)\n", @@ -1271,7 +1327,7 @@ USB_REQ_SET_ADDRESS, 0, udev->devnum, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT); if (retval == 0) - udev->state = USB_STATE_ADDRESS; + usb_set_device_state(udev, USB_STATE_ADDRESS); return retval; } @@ -1538,7 +1594,8 @@ "couldn't allocate port %d usb_device\n", port+1); goto done; } - udev->state = USB_STATE_POWERED; + + usb_set_device_state(udev, USB_STATE_POWERED); /* hub can tell if it's lowspeed already: D- pullup (not D+) */ if (portstatus & USB_PORT_STAT_LOW_SPEED) @@ -1610,12 +1667,28 @@ * no one will look at it until hdev is unlocked. */ down (&udev->serialize); - hdev->children[port] = udev; + status = 0; + + /* We mustn't add new devices if the parent hub has + * been disconnected; we would race with the + * recursively_mark_NOTATTACHED() routine. + */ + spin_lock_irq(&device_state_lock); + if (hdev->state == USB_STATE_NOTATTACHED) + status = -ENOTCONN; + else + hdev->children[port] = udev; + spin_unlock_irq(&device_state_lock); /* Run it through the hoops (find a driver, etc) */ - status = usb_new_device(udev); - if (status) - hdev->children[port] = NULL; + if (!status) { + status = usb_new_device(udev); + if (status) { + spin_lock_irq(&device_state_lock); + hdev->children[port] = NULL; + spin_unlock_irq(&device_state_lock); + } + } up (&udev->serialize); if (status) @@ -1972,7 +2045,7 @@ udev->actconfig->desc.bConfigurationValue, ret); goto re_enumerate; } - udev->state = USB_STATE_CONFIGURED; + usb_set_device_state(udev, USB_STATE_CONFIGURED); for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) { struct usb_interface *intf = udev->actconfig->interface[i]; @@ -1998,7 +2071,7 @@ re_enumerate: /* FIXME make some task re-enumerate; don't just mark unusable */ - udev->state = USB_STATE_NOTATTACHED; + hub_port_disable(parent, port); return -ENODEV; } EXPORT_SYMBOL(__usb_reset_device); diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Fri Jun 18 10:55:49 2004 +++ b/drivers/usb/core/message.c Fri Jun 18 10:55:49 2004 @@ -840,7 +840,7 @@ } dev->actconfig = 0; if (dev->state == USB_STATE_CONFIGURED) - dev->state = USB_STATE_ADDRESS; + usb_set_device_state(dev, USB_STATE_ADDRESS); } } @@ -1045,7 +1045,7 @@ config->desc.bConfigurationValue, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT); if (retval < 0) { - dev->state = USB_STATE_ADDRESS; + usb_set_device_state(dev, USB_STATE_ADDRESS); return retval; } @@ -1183,9 +1183,9 @@ dev->actconfig = cp; if (!cp) - dev->state = USB_STATE_ADDRESS; + usb_set_device_state(dev, USB_STATE_ADDRESS); else { - dev->state = USB_STATE_CONFIGURED; + usb_set_device_state(dev, USB_STATE_CONFIGURED); /* Initialize the new interface structures and the * hc/hcd/usbcore interface/endpoint state. diff -Nru a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h --- a/drivers/usb/core/usb.h Fri Jun 18 10:55:49 2004 +++ b/drivers/usb/core/usb.h Fri Jun 18 10:55:49 2004 @@ -21,5 +21,8 @@ unsigned int size); extern int usb_set_configuration(struct usb_device *dev, int configuration); +extern void usb_set_device_state(struct usb_device *udev, + enum usb_device_state new_state); + /* for labeling diagnostics */ extern const char *usbcore_name;