From 62e299e61a6ffe8131fa85a984c3058b68586f5d Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Fri, 8 Jan 2010 12:56:19 -0500 Subject: [PATCH] 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 Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/devio.c | 40 +++++++++++++++++++++++---------------- drivers/usb/core/driver.c | 8 ++++---- drivers/usb/core/sysfs.c | 2 ++ 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 431d17287a86..825e0abfed0a 100644 --- 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 fcafb2dce3ac..2b39583040d0 100644 --- 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 1b3c00b3ca3f..d8f3bfe1559f 100644 --- 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; }