Greg:
This patch plugs a major hole in USB device locking. Whenever a new
driver is loaded and the matching unbound interfaces are probed, no
devices are locked! Similarly, when a driver is unloaded and its
interfaces are unbound, nothing is locked.
The patch fixes the problem by introducing a new rwsem and encapsulating
the locking procedures. To lock a single device, the new routine
usb_lock_device() gets a readlock on the rwsem and a lock on the device's
->serialize. When drivers are loaded or removed, all devices are
effectively locked by getting a writelock on the rwsem. The rwsem is
also writelocked in one other oddball circumstance: when telling the
driver-model core to reprobe all the unbound interfaces on a bus.
Details of the patch:
Implement usb_lock_device(), usb_unlock_device(),
usb_lock_all_devices(), usb_unlock_all_devices(), and
usb_trylock_device().
Call usb_(un)lock_all_devices() when registering and
deregistering USB drivers and when calling bus_rescan_devices().
Convert all existing places that access usbdev->serialize
directly to make them call the appropriate locking routine.
Add comments warning about the need to use the new routines,
and change the terminology in existing comments to talk about
locking a device rather than owning usbdev->serialize.
Add proper locking to usb_find_device() and match_device(),
to protect against topology changes while scanning the
device tree.
With this change, the USB device locking model is almost complete. There
remains only the need for appropriate locking for routines that call
usb_reset_device(). That patch will come next.
Please apply.
Alan Stern
Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
diff -Nru a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
--- a/drivers/usb/core/devices.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/devices.c Thu Jun 24 10:41:12 2004
@@ -452,7 +452,7 @@
* nbytes - the maximum number of bytes to write
* skip_bytes - the number of bytes to skip before writing anything
* file_offset - the offset into the devices file on completion
- * The caller must own the usbdev->serialize semaphore.
+ * The caller must have locked the device.
*/
static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes, loff_t
*skip_bytes, loff_t *file_offset,
struct usb_device *usbdev, struct usb_bus *bus, int
level, int index, int count)
@@ -556,10 +556,10 @@
struct usb_device *childdev = usbdev->children[chix];
if (childdev) {
- down(&childdev->serialize);
+ usb_lock_device(childdev);
ret = usb_device_dump(buffer, nbytes, skip_bytes, file_offset,
childdev,
bus, level + 1, chix, ++cnt);
- up(&childdev->serialize);
+ usb_unlock_device(childdev);
if (ret == -EFAULT)
return total_written;
total_written += ret;
@@ -591,9 +591,9 @@
/* recurse through all children of the root hub */
if (!bus->root_hub)
continue;
- down(&bus->root_hub->serialize);
+ usb_lock_device(bus->root_hub);
ret = usb_device_dump(&buf, &nbytes, &skip_bytes, ppos, bus->root_hub,
bus, 0, 0, 0);
- up(&bus->root_hub->serialize);
+ usb_unlock_device(bus->root_hub);
if (ret < 0) {
up(&usb_bus_list_lock);
return ret;
diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
--- a/drivers/usb/core/devio.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/devio.c Thu Jun 24 10:41:12 2004
@@ -111,7 +111,7 @@
int i;
pos = *ppos;
- down(&dev->serialize);
+ usb_lock_device(dev);
if (!connected(dev)) {
ret = -ENODEV;
goto err;
@@ -173,7 +173,7 @@
}
err:
- up(&dev->serialize);
+ usb_unlock_device(dev);
return ret;
}
@@ -514,7 +514,7 @@
struct usb_device *dev = ps->dev;
unsigned int ifnum;
- down(&dev->serialize);
+ usb_lock_device(dev);
list_del_init(&ps->list);
if (connected(dev)) {
@@ -523,7 +523,7 @@
releaseintf(ps, ifnum);
destroy_all_async(ps);
}
- up(&dev->serialize);
+ usb_unlock_device(dev);
usb_put_dev(dev);
ps->dev = NULL;
kfree(ps);
@@ -1012,9 +1012,9 @@
break;
if (signal_pending(current))
break;
- up(&dev->serialize);
+ usb_unlock_device(dev);
schedule();
- down(&dev->serialize);
+ usb_lock_device(dev);
}
remove_wait_queue(&ps->wait, &wait);
set_current_state(TASK_RUNNING);
@@ -1137,7 +1137,11 @@
/* let kernel drivers try to (re)bind to the interface */
case USBDEVFS_CONNECT:
+ usb_unlock_device(ps->dev);
+ usb_lock_all_devices();
bus_rescan_devices(intf->dev.bus);
+ usb_unlock_all_devices();
+ usb_lock_device(ps->dev);
break;
/* talk directly to the interface's driver */
@@ -1180,9 +1184,9 @@
if (!(file->f_mode & FMODE_WRITE))
return -EPERM;
- down(&dev->serialize);
+ usb_lock_device(dev);
if (!connected(dev)) {
- up(&dev->serialize);
+ usb_unlock_device(dev);
return -ENODEV;
}
@@ -1282,7 +1286,7 @@
ret = proc_ioctl(ps, p);
break;
}
- up(&dev->serialize);
+ usb_unlock_device(dev);
if (ret >= 0)
inode->i_atime = CURRENT_TIME;
return ret;
diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/hcd.c Thu Jun 24 10:41:12 2004
@@ -795,9 +795,9 @@
return (retval < 0) ? retval : -EMSGSIZE;
}
- down (&usb_dev->serialize);
+ usb_lock_device (usb_dev);
retval = usb_new_device (usb_dev);
- up (&usb_dev->serialize);
+ usb_unlock_device (usb_dev);
if (retval) {
usb_dev->bus->root_hub = NULL;
dev_err (parent_dev, "can't register root hub for %s, %d\n",
@@ -1526,13 +1526,13 @@
unsigned i;
/* hc's root hub is removed later removed in hcd->stop() */
- down (&hub->serialize);
usb_set_device_state(hub, USB_STATE_NOTATTACHED);
+ usb_lock_device (hub);
for (i = 0; i < hub->maxchild; i++) {
if (hub->children [i])
usb_disconnect (&hub->children [i]);
}
- up (&hub->serialize);
+ usb_unlock_device (hub);
}
/**
diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
--- a/drivers/usb/core/hub.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/hub.c Thu Jun 24 10:41:12 2004
@@ -839,7 +839,7 @@
* @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
+ * udev->state is _not_ protected by the device lock. 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.
@@ -930,7 +930,7 @@
/* lock the bus list on behalf of HCDs unregistering their root hubs */
if (!udev->parent)
down(&usb_bus_list_lock);
- down(&udev->serialize);
+ usb_lock_device(udev);
dev_info (&udev->dev, "USB disconnect, address %d\n", udev->devnum);
@@ -959,7 +959,7 @@
*pdev = NULL;
spin_unlock_irq(&device_state_lock);
- up(&udev->serialize);
+ usb_unlock_device(udev);
if (!udev->parent)
up(&usb_bus_list_lock);
@@ -1657,7 +1657,7 @@
* udev becomes globally accessible, although presumably
* no one will look at it until hdev is unlocked.
*/
- down (&udev->serialize);
+ usb_lock_device (udev);
status = 0;
/* We mustn't add new devices if the parent hub has
@@ -1681,7 +1681,7 @@
}
}
- up (&udev->serialize);
+ usb_unlock_device (udev);
if (status)
goto loop;
@@ -1747,7 +1747,7 @@
/* Lock the device, then check to see if we were
* disconnected while waiting for the lock to succeed. */
- down(&hdev->serialize);
+ usb_lock_device(hdev);
if (hdev->state != USB_STATE_CONFIGURED ||
!hdev->actconfig ||
hub != usb_get_intfdata(
@@ -1863,7 +1863,7 @@
}
loop:
- up(&hdev->serialize);
+ usb_unlock_device(hdev);
usb_put_dev(hdev);
} /* end while (1) */
diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c
--- a/drivers/usb/core/message.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/message.c Thu Jun 24 10:41:12 2004
@@ -1022,6 +1022,8 @@
* use usb_set_interface() on the interfaces it claims. Resetting the whole
* configuration would affect other drivers' interfaces.
*
+ * The caller must have locked the device.
+ *
* Returns zero on success, else a negative error code.
*/
int usb_reset_configuration(struct usb_device *dev)
@@ -1029,8 +1031,8 @@
int i, retval;
struct usb_host_config *config;
- /* caller must own dev->serialize (config won't change)
- * and the usb bus readlock (so driver bindings are stable);
+ /* caller must have locked the device and must own
+ * the usb bus readlock (so driver bindings are stable);
* so calls during probe() are fine
*/
@@ -1087,7 +1089,7 @@
* usb_set_configuration - Makes a particular device setting be current
* @dev: the device whose configuration is being updated
* @configuration: the configuration being chosen.
- * Context: !in_interrupt(), caller holds dev->serialize
+ * Context: !in_interrupt(), caller has locked the device
*
* This is used to enable non-default device modes. Not all devices
* use this kind of configurability; many devices only have one
@@ -1108,8 +1110,8 @@
* usb_set_interface().
*
* This call is synchronous. The calling context must be able to sleep,
- * and must not hold the driver model lock for USB; usb device driver
- * probe() methods may not use this routine.
+ * must have locked the device, and must not hold the driver model lock
+ * for USB; usb device driver probe() methods cannot use this routine.
*
* Returns zero on success, or else the status code returned by the
* underlying call that failed. On succesful completion, each interface
@@ -1123,8 +1125,6 @@
struct usb_host_config *cp = NULL;
struct usb_interface **new_interfaces = NULL;
int n, nintf;
-
- /* dev->serialize guards all config changes */
for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
if (dev->config[i].desc.bConfigurationValue == configuration) {
diff -Nru a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
--- a/drivers/usb/core/sysfs.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/sysfs.c Thu Jun 24 10:41:12 2004
@@ -55,9 +55,9 @@
if (sscanf (buf, "%u", &config) != 1 || config > 255)
return -EINVAL;
- down(&udev->serialize);
+ usb_lock_device(udev);
value = usb_set_configuration (udev, config);
- up(&udev->serialize);
+ usb_unlock_device(udev);
return (value < 0) ? value : count;
}
diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/usb.c Thu Jun 24 10:41:12 2004
@@ -39,6 +39,7 @@
#include <linux/spinlock.h>
#include <linux/errno.h>
#include <linux/smp_lock.h>
+#include <linux/rwsem.h>
#include <linux/usb.h>
#include <asm/io.h>
@@ -62,6 +63,8 @@
int nousb; /* Disable USB when built into kernel image */
/* Not honored on modular build */
+static DECLARE_RWSEM(usb_all_devices_rwsem);
+
static int generic_probe (struct device *dev)
{
@@ -149,7 +152,9 @@
new_driver->driver.probe = usb_probe_interface;
new_driver->driver.remove = usb_unbind_interface;
+ usb_lock_all_devices();
retval = driver_register(&new_driver->driver);
+ usb_unlock_all_devices();
if (!retval) {
pr_info("%s: registered new driver %s\n",
@@ -178,7 +183,9 @@
{
pr_info("%s: deregistering driver %s\n", usbcore_name, driver->name);
+ usb_lock_all_devices();
driver_unregister (&driver->driver);
+ usb_unlock_all_devices();
usbfs_update_special();
}
@@ -200,7 +207,7 @@
* alternate settings available for this interfaces.
*
* Don't call this function unless you are bound to one of the interfaces
- * on this device or you own the dev->serialize semaphore!
+ * on this device or you have locked the device!
*/
struct usb_interface *usb_ifnum_to_if(struct usb_device *dev, unsigned ifnum)
{
@@ -233,7 +240,7 @@
* drivers avoid such mistakes.
*
* Don't call this function unless you are bound to the intf interface
- * or you own the device's ->serialize semaphore!
+ * or you have locked the device!
*/
struct usb_host_interface *usb_altnum_to_altsetting(struct usb_interface *intf,
unsigned int altnum)
@@ -301,9 +308,9 @@
* way to bind to an interface is to return the private data from
* the driver's probe() method.
*
- * Callers must own the driver model's usb bus writelock. So driver
- * probe() entries don't need extra locking, but other call contexts
- * may need to explicitly claim that lock.
+ * Callers must lock the device and own the driver model's usb bus writelock.
+ * So driver probe() entries don't need extra locking, but other call contexts
+ * may need to explicitly claim those locks.
*/
int usb_driver_claim_interface(struct usb_driver *driver, struct usb_interface
*iface, void* priv)
{
@@ -334,8 +341,8 @@
* also causes the driver disconnect() method to be called.
*
* This call is synchronous, and may not be used in an interrupt context.
- * Callers must own the usb_device serialize semaphore and the driver model's
- * usb bus writelock. So driver disconnect() entries don't need extra locking,
+ * Callers must lock the device and own the driver model's usb bus writelock.
+ * So driver disconnect() entries don't need extra locking,
* but other call contexts may need to explicitly claim those locks.
*/
void usb_driver_release_interface(struct usb_driver *driver,
@@ -828,6 +835,72 @@
put_device(&intf->dev);
}
+/**
+ * usb_lock_device - acquire the lock for a usb device structure
+ * @udev: device that's being locked
+ *
+ * Use this routine rather than manipulating udev->serialize directly.
+ * This is necessary for proper interaction with usb_lock_all_devices().
+ */
+void usb_lock_device(struct usb_device *udev)
+{
+ down_read(&usb_all_devices_rwsem);
+ down(&udev->serialize);
+}
+
+/**
+ * usb_trylock_device - attempt to acquire the lock for a usb device structure
+ * @udev: device that's being locked
+ *
+ * Use this routine rather than manipulating udev->serialize directly.
+ * This is necessary for proper interaction with usb_lock_all_devices().
+ *
+ * Returns 1 if successful, 0 if contention.
+ */
+int usb_trylock_device(struct usb_device *udev)
+{
+ if (!down_read_trylock(&usb_all_devices_rwsem))
+ return 0;
+ if (!down_trylock(&udev->serialize)) {
+ up_read(&usb_all_devices_rwsem);
+ return 0;
+ }
+ return 1;
+}
+
+/**
+ * usb_unlock_device - release the lock for a usb device structure
+ * @udev: device that's being unlocked
+ *
+ * Use this routine rather than manipulating udev->serialize directly.
+ * This is necessary for proper interaction with usb_lock_all_devices().
+ */
+void usb_unlock_device(struct usb_device *udev)
+{
+ up(&udev->serialize);
+ up_read(&usb_all_devices_rwsem);
+}
+
+/**
+ * usb_lock_all_devices - acquire the lock for all usb device structures
+ *
+ * This is necessary when registering a new driver or probing a bus,
+ * since the driver-model core may try to use any usb_device.
+ */
+void usb_lock_all_devices(void)
+{
+ down_write(&usb_all_devices_rwsem);
+}
+
+/**
+ * usb_unlock_all_devices - release the lock for all usb device structures
+ */
+void usb_unlock_all_devices(void)
+{
+ up_write(&usb_all_devices_rwsem);
+}
+
+
static struct usb_device *match_device(struct usb_device *dev,
u16 vendor_id, u16 product_id)
{
@@ -849,8 +922,10 @@
/* look through all of the children of this device */
for (child = 0; child < dev->maxchild; ++child) {
if (dev->children[child]) {
+ usb_lock_device(dev->children[child]);
ret_dev = match_device(dev->children[child],
vendor_id, product_id);
+ usb_unlock_device(dev->children[child]);
if (ret_dev)
goto exit;
}
@@ -885,7 +960,9 @@
bus = container_of(buslist, struct usb_bus, bus_list);
if (!bus->root_hub)
continue;
+ usb_lock_device(bus->root_hub);
dev = match_device(bus->root_hub, vendor_id, product_id);
+ usb_unlock_device(bus->root_hub);
if (dev)
goto exit;
}
@@ -1362,6 +1439,10 @@
EXPORT_SYMBOL(usb_put_dev);
EXPORT_SYMBOL(usb_get_dev);
EXPORT_SYMBOL(usb_hub_tt_clear_buffer);
+
+EXPORT_SYMBOL(usb_lock_device);
+EXPORT_SYMBOL(usb_trylock_device);
+EXPORT_SYMBOL(usb_unlock_device);
EXPORT_SYMBOL(usb_driver_claim_interface);
EXPORT_SYMBOL(usb_driver_release_interface);
diff -Nru a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
--- a/drivers/usb/core/usb.h Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/core/usb.h Thu Jun 24 10:41:12 2004
@@ -24,5 +24,8 @@
extern void usb_set_device_state(struct usb_device *udev,
enum usb_device_state new_state);
+extern void usb_lock_all_devices(void);
+extern void usb_unlock_all_devices(void);
+
/* for labeling diagnostics */
extern const char *usbcore_name;
diff -Nru a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
--- a/drivers/usb/host/ehci-hub.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/host/ehci-hub.c Thu Jun 24 10:41:12 2004
@@ -81,7 +81,7 @@
}
-/* caller owns root->serialize, and should reset/reinit on error */
+/* caller has locked the root hub, and should reset/reinit on error */
static int ehci_hub_resume (struct usb_hcd *hcd)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
diff -Nru a/drivers/usb/host/ohci-hub.c b/drivers/usb/host/ohci-hub.c
--- a/drivers/usb/host/ohci-hub.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/host/ohci-hub.c Thu Jun 24 10:41:12 2004
@@ -165,7 +165,7 @@
static int hc_restart (struct ohci_hcd *ohci);
-/* caller owns root->serialize */
+/* caller has locked the root hub */
static int ohci_hub_resume (struct usb_hcd *hcd)
{
struct ohci_hcd *ohci = hcd_to_ohci (hcd);
@@ -301,9 +301,9 @@
{
struct usb_hcd *hcd = _hcd;
- down (&hcd->self.root_hub->serialize);
+ usb_lock_device (hcd->self.root_hub);
(void) ohci_hub_resume (hcd);
- up (&hcd->self.root_hub->serialize);
+ usb_unlock_device (hcd->self.root_hub);
}
#else
@@ -381,12 +381,12 @@
&& ((OHCI_CTRL_HCFS | OHCI_SCHED_ENABLES)
& ohci->hc_control)
== OHCI_USB_OPER
- && down_trylock (&hcd->self.root_hub->serialize) == 0
+ && usb_trylock_device (hcd->self.root_hub) == 0
) {
ohci_vdbg (ohci, "autosuspend\n");
(void) ohci_hub_suspend (&ohci->hcd);
ohci->hcd.state = USB_STATE_RUNNING;
- up (&hcd->self.root_hub->serialize);
+ usb_unlock_device (hcd->self.root_hub);
}
#endif
diff -Nru a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
--- a/drivers/usb/host/ohci-pci.c Thu Jun 24 10:41:12 2004
+++ b/drivers/usb/host/ohci-pci.c Thu Jun 24 10:41:12 2004
@@ -127,9 +127,9 @@
#ifdef CONFIG_USB_SUSPEND
(void) usb_suspend_device (hcd->self.root_hub);
#else
- down (&hcd->self.root_hub->serialize);
+ usb_lock_device (hcd->self.root_hub);
(void) ohci_hub_suspend (hcd);
- up (&hcd->self.root_hub->serialize);
+ usb_unlock_device (hcd->self.root_hub);
#endif
/* let things settle down a bit */
@@ -175,9 +175,9 @@
/* get extra cleanup even if remote wakeup isn't in use */
retval = usb_resume_device (hcd->self.root_hub);
#else
- down (&hcd->self.root_hub->serialize);
+ usb_lock_device (hcd->self.root_hub);
retval = ohci_hub_resume (hcd);
- up (&hcd->self.root_hub->serialize);
+ usb_unlock_device (hcd->self.root_hub);
#endif
if (retval == 0) {
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h Thu Jun 24 10:41:12 2004
+++ b/include/linux/usb.h Thu Jun 24 10:41:12 2004
@@ -279,6 +279,17 @@
struct usb_tt;
+/**
+ * struct usb_device - kernel's representation of a USB device
+ *
+ * FIXME: Write the kerneldoc!
+ *
+ * WARNING: Drivers should not attempt to manipulate usbdev->serialize
+ * directly. Instead use usb_lock_device() and usb_unlock_device().
+ *
+ * Usbcore drivers should not set usbdev->state directly. Instead use
+ * usb_set_device_state().
+ */
struct usb_device {
int devnum; /* Address on USB bus */
char devpath [16]; /* Use in messages: /port/port/... */
@@ -331,6 +342,11 @@
extern struct usb_device *usb_get_dev(struct usb_device *dev);
extern void usb_put_dev(struct usb_device *dev);
+
+/* for device locking -- don't manipulate usbdev->serialize directly! */
+extern void usb_lock_device(struct usb_device *udev);
+extern int usb_trylock_device(struct usb_device *udev);
+extern void usb_unlock_device(struct usb_device *udev);
/* mostly for devices emulating SCSI over USB */
extern int usb_reset_device(struct usb_device *dev);
-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 -
digital self defense, top technical experts, no vendor pitches,
unmatched networking opportunities. Visit www.blackhat.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
