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
