USB: change locking for device-level autosuspend

This patch (as1323) changes the locking requirements for
usb_autosuspend_device(), usb_autoresume_device(), and
usb_try_autosuspend_device().  This isn't a very important change;
mainly it's meant to make the locking more uniform.

The most tricky part of the patch involves changes to usbdev_open().
To avoid an ABBA locking problem, it was necessary to reduce the
region protected by usbfs_mutex.  Since that mutex now protects only
against simultaneous open and remove, this posed no difficulty -- its
scope was larger than necessary.

And it turns out that usbfs_mutex is no longer needed in
usbdev_release() at all.  The list of usbfs "ps" structures is now
protected by the device lock instead of by usbfs_mutex.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 431d17287a86fdad7581b55a4f3be4e860143594..825e0abfed0acdc74553ef58995b4be4a3196608 100644 (file)
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -654,19 +654,21 @@ static int usbdev_open(struct inode *inode, struct file *file)
        int ret;
 
        lock_kernel();
-       /* Protect against simultaneous removal or release */
-       mutex_lock(&usbfs_mutex);
 
        ret = -ENOMEM;
        ps = kmalloc(sizeof(struct dev_state), GFP_KERNEL);
        if (!ps)
-               goto out;
+               goto out_free_ps;
 
        ret = -ENODEV;
 
+       /* Protect against simultaneous removal or release */
+       mutex_lock(&usbfs_mutex);
+
        /* usbdev device-node */
        if (imajor(inode) == USB_DEVICE_MAJOR)
                dev = usbdev_lookup_by_devt(inode->i_rdev);
+
 #ifdef CONFIG_USB_DEVICEFS
        /* procfs file */
        if (!dev) {
@@ -678,13 +680,19 @@ static int usbdev_open(struct inode *inode, struct file *file)
                        dev = NULL;
        }
 #endif
-       if (!dev || dev->state == USB_STATE_NOTATTACHED)
-               goto out;
+       mutex_unlock(&usbfs_mutex);
+
+       if (!dev)
+               goto out_free_ps;
+
+       usb_lock_device(dev);
+       if (dev->state == USB_STATE_NOTATTACHED)
+               goto out_unlock_device;
+
        ret = usb_autoresume_device(dev);
        if (ret)
-               goto out;
+               goto out_unlock_device;
 
-       ret = 0;
        ps->dev = dev;
        ps->file = file;
        spin_lock_init(&ps->lock);
@@ -702,14 +710,17 @@ static int usbdev_open(struct inode *inode, struct file *file)
        smp_wmb();
        list_add_tail(&ps->list, &dev->filelist);
        file->private_data = ps;
+       usb_unlock_device(dev);
        snoop(&dev->dev, "opened by process %d: %s\n", task_pid_nr(current),
                        current->comm);
- out:
-       if (ret) {
-               kfree(ps);
-               usb_put_dev(dev);
-       }
-       mutex_unlock(&usbfs_mutex);
+       unlock_kernel();
+       return ret;
+
+ out_unlock_device:
+       usb_unlock_device(dev);
+       usb_put_dev(dev);
+ out_free_ps:
+       kfree(ps);
        unlock_kernel();
        return ret;
 }
@@ -724,10 +735,7 @@ static int usbdev_release(struct inode *inode, struct file *file)
        usb_lock_device(dev);
        usb_hub_release_all_ports(dev, ps);
 
-       /* Protect against simultaneous open */
-       mutex_lock(&usbfs_mutex);
        list_del_init(&ps->list);
-       mutex_unlock(&usbfs_mutex);
 
        for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
                        ifnum++) {

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index fcafb2dce3aca12736816d23966f17e4613e8a88..2b39583040d0b3eb2b6401f712ba5cc735466527 100644 (file)
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1478,8 +1478,7 @@ void usb_autoresume_work(struct work_struct *work)
  * driver requires remote-wakeup capability during autosuspend but remote
  * wakeup is disabled, the autosuspend will fail.
  *
- * Often the caller will hold @udev's device lock, but this is not
- * necessary.
+ * The caller must hold @udev's device lock.
  *
  * This routine can run only in process context.
  */
@@ -1503,6 +1502,8 @@ void usb_autosuspend_device(struct usb_device *udev)
  * for an active interface is greater than 0, or autosuspend is not allowed
  * for any other reason, no autosuspend request will be queued.
  *
+ * The caller must hold @udev's device lock.
+ *
  * This routine can run only in process context.
  */
 void usb_try_autosuspend_device(struct usb_device *udev)
@@ -1526,8 +1527,7 @@ void usb_try_autosuspend_device(struct usb_device *udev)
  * @udev's usage counter is incremented to prevent subsequent autosuspends.
  * However if the autoresume fails then the usage counter is re-decremented.
  *
- * Often the caller will hold @udev's device lock, but this is not
- * necessary (and attempting it might cause deadlock).
+ * The caller must hold @udev's device lock.
  *
  * This routine can run only in process context.
  */

diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 1b3c00b3ca3f7a66232f64db3619df819e2c48d3..d8f3bfe1559fc18a860beb4d84384ecc819345e3 100644 (file)
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -352,6 +352,7 @@ set_autosuspend(struct device *dev, struct device_attribute *attr,
                return -EINVAL;
        value *= HZ;
 
+       usb_lock_device(udev);
        udev->autosuspend_delay = value;
        if (value >= 0)
                usb_try_autosuspend_device(udev);
@@ -359,6 +360,7 @@ set_autosuspend(struct device *dev, struct device_attribute *attr,
                if (usb_autoresume_device(udev) == 0)
                        usb_autosuspend_device(udev);
        }
+       usb_unlock_device(udev);
        return count;
 }