ChangeSet 1.1722.97.65, 2004/06/11 16:51:56-07:00, [EMAIL PROTECTED]

[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 <[EMAIL PROTECTED]>
Signed-off-by: Greg Kroah-Hartman <[EMAIL PROTECTED]>


 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;



-------------------------------------------------------
This SF.Net email is sponsored by The 2004 JavaOne(SM) Conference
Learn from the experts at JavaOne(SM), Sun's Worldwide Java Developer
Conference, June 28 - July 1 at the Moscone Center in San Francisco, CA
REGISTER AND SAVE! http://java.sun.com/javaone/sf Priority Code NWMGYKND
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to