On Wed, 3 Jul 2019, Mayuresh Kulkarni wrote:
> As you had mentioned in one of the comment before, the only addition to
> the patch I have locally is -
> usbfs_notify_resume() has usbfs_mutex lock around list traversal.
>
> Could you please send the patch for review? Please note, I think I am
> not a part of linux-usb mailing-list, so probably need to be in cc to
> get the patch email. Do let me know if something else is needed from me.
Here it is. There are two changes from the previous version:
1. This is rebased on top of a separate patch which Greg has
already accepted:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit?id=ffed60971f3d95923b99ea970862c6ab6a22c20f
2. I implemented Oliver's recommendation that the WAIT_FOR_RESUME
ioctl should automatically do FORBID_SUSPEND before it returns,
if the return code is 0 (that is, it wasn't interrupted by a
signal).
Still to do: Write up the documentation. In fact, the existing
description of usbfs in Documentation/driver-api/usb/usb.rst is sadly
out of date. And it deserves to be split out into a separate file of
its own -- but I'm not sure where it really belongs, considering that
it is an API for userspace, not an internal kernel API.
Greg, suggestions?
Alan Stern
drivers/usb/core/devio.c | 96 ++++++++++++++++++++++++++++++++++++--
drivers/usb/core/generic.c | 5 +
drivers/usb/core/usb.h | 3 +
include/uapi/linux/usbdevice_fs.h | 3 +
4 files changed, 102 insertions(+), 5 deletions(-)
Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -48,6 +48,9 @@
#define USB_DEVICE_MAX (USB_MAXBUS * 128)
#define USB_SG_SIZE 16384 /* split-size for large txs */
+/* Mutual exclusion for ps->list in resume vs. release and remove */
+static DEFINE_MUTEX(usbfs_mutex);
+
struct usb_dev_state {
struct list_head list; /* state list */
struct usb_device *dev;
@@ -57,14 +60,17 @@ struct usb_dev_state {
struct list_head async_completed;
struct list_head memory_list;
wait_queue_head_t wait; /* wake up if a request completed */
+ wait_queue_head_t wait_for_resume; /* wake up upon runtime resume */
unsigned int discsignr;
struct pid *disc_pid;
const struct cred *cred;
void __user *disccontext;
unsigned long ifclaimed;
u32 disabled_bulk_eps;
- bool privileges_dropped;
unsigned long interface_allowed_mask;
+ int not_yet_resumed;
+ bool suspend_allowed;
+ bool privileges_dropped;
};
struct usb_memory {
@@ -696,9 +702,7 @@ static void driver_disconnect(struct usb
destroy_async_on_interface(ps, ifnum);
}
-/* The following routines are merely placeholders. There is no way
- * to inform a user task about suspend or resumes.
- */
+/* We don't care about suspend/resume of claimed interfaces */
static int driver_suspend(struct usb_interface *intf, pm_message_t msg)
{
return 0;
@@ -709,12 +713,32 @@ static int driver_resume(struct usb_inte
return 0;
}
+/* The following routines apply to the entire device, not interfaces */
+void usbfs_notify_suspend(struct usb_device *udev)
+{
+ /* We don't need to handle this */
+}
+
+void usbfs_notify_resume(struct usb_device *udev)
+{
+ struct usb_dev_state *ps;
+
+ /* Protect against simultaneous remove or release */
+ mutex_lock(&usbfs_mutex);
+ list_for_each_entry(ps, &udev->filelist, list) {
+ WRITE_ONCE(ps->not_yet_resumed, 0);
+ wake_up_all(&ps->wait_for_resume);
+ }
+ mutex_unlock(&usbfs_mutex);
+}
+
struct usb_driver usbfs_driver = {
.name = "usbfs",
.probe = driver_probe,
.disconnect = driver_disconnect,
.suspend = driver_suspend,
.resume = driver_resume,
+ .supports_autosuspend = 1,
};
static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
@@ -999,9 +1023,12 @@ static int usbdev_open(struct inode *ino
INIT_LIST_HEAD(&ps->async_completed);
INIT_LIST_HEAD(&ps->memory_list);
init_waitqueue_head(&ps->wait);
+ init_waitqueue_head(&ps->wait_for_resume);
ps->disc_pid = get_pid(task_pid(current));
ps->cred = get_current_cred();
smp_wmb();
+
+ /* Can't race with resume; the device is already active */
list_add_tail(&ps->list, &dev->filelist);
file->private_data = ps;
usb_unlock_device(dev);
@@ -1027,7 +1054,10 @@ static int usbdev_release(struct inode *
usb_lock_device(dev);
usb_hub_release_all_ports(dev, ps);
+ /* Protect against simultaneous resume */
+ mutex_lock(&usbfs_mutex);
list_del_init(&ps->list);
+ mutex_unlock(&usbfs_mutex);
for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
ifnum++) {
@@ -1035,7 +1065,8 @@ static int usbdev_release(struct inode *
releaseintf(ps, ifnum);
}
destroy_all_async(ps);
- usb_autosuspend_device(dev);
+ if (!ps->suspend_allowed)
+ usb_autosuspend_device(dev);
usb_unlock_device(dev);
usb_put_dev(dev);
put_pid(ps->disc_pid);
@@ -2346,6 +2377,47 @@ static int proc_drop_privileges(struct u
return 0;
}
+static int proc_forbid_suspend(struct usb_dev_state *ps)
+{
+ int ret = 0;
+
+ if (ps->suspend_allowed) {
+ ret = usb_autoresume_device(ps->dev);
+ if (ret == 0)
+ ps->suspend_allowed = false;
+ else if (ret != -ENODEV)
+ ret = -EIO;
+ }
+ return ret;
+}
+
+static int proc_allow_suspend(struct usb_dev_state *ps)
+{
+ if (!connected(ps))
+ return -ENODEV;
+
+ WRITE_ONCE(ps->not_yet_resumed, 1);
+ if (!ps->suspend_allowed) {
+ usb_autosuspend_device(ps->dev);
+ ps->suspend_allowed = true;
+ }
+ return 0;
+}
+
+static int proc_wait_for_resume(struct usb_dev_state *ps)
+{
+ int ret;
+
+ usb_unlock_device(ps->dev);
+ ret = wait_event_interruptible(ps->wait_for_resume,
+ READ_ONCE(ps->not_yet_resumed) == 0);
+ usb_lock_device(ps->dev);
+
+ if (ret != 0)
+ return ret;
+ return proc_forbid_suspend(ps);
+}
+
/*
* NOTE: All requests here that have interface numbers as parameters
* are assuming that somehow the configuration has been prevented from
@@ -2540,6 +2612,15 @@ static long usbdev_do_ioctl(struct file
case USBDEVFS_GET_SPEED:
ret = ps->dev->speed;
break;
+ case USBDEVFS_FORBID_SUSPEND:
+ ret = proc_forbid_suspend(ps);
+ break;
+ case USBDEVFS_ALLOW_SUSPEND:
+ ret = proc_allow_suspend(ps);
+ break;
+ case USBDEVFS_WAIT_FOR_RESUME:
+ ret = proc_wait_for_resume(ps);
+ break;
}
done:
@@ -2607,10 +2688,14 @@ static void usbdev_remove(struct usb_dev
struct usb_dev_state *ps;
struct kernel_siginfo sinfo;
+ /* Protect against simultaneous resume */
+ mutex_lock(&usbfs_mutex);
while (!list_empty(&udev->filelist)) {
ps = list_entry(udev->filelist.next, struct usb_dev_state,
list);
destroy_all_async(ps);
wake_up_all(&ps->wait);
+ WRITE_ONCE(ps->not_yet_resumed, 0);
+ wake_up_all(&ps->wait_for_resume);
list_del_init(&ps->list);
if (ps->discsignr) {
clear_siginfo(&sinfo);
@@ -2622,6 +2707,7 @@ static void usbdev_remove(struct usb_dev
ps->disc_pid, ps->cred);
}
}
+ mutex_unlock(&usbfs_mutex);
}
static int usbdev_notify(struct notifier_block *self,
Index: usb-devel/drivers/usb/core/generic.c
===================================================================
--- usb-devel.orig/drivers/usb/core/generic.c
+++ usb-devel/drivers/usb/core/generic.c
@@ -257,6 +257,8 @@ static int generic_suspend(struct usb_de
else
rc = usb_port_suspend(udev, msg);
+ if (rc == 0)
+ usbfs_notify_suspend(udev);
return rc;
}
@@ -273,6 +275,9 @@ static int generic_resume(struct usb_dev
rc = hcd_bus_resume(udev, msg);
else
rc = usb_port_resume(udev, msg);
+
+ if (rc == 0)
+ usbfs_notify_resume(udev);
return rc;
}
Index: usb-devel/drivers/usb/core/usb.h
===================================================================
--- usb-devel.orig/drivers/usb/core/usb.h
+++ usb-devel/drivers/usb/core/usb.h
@@ -95,6 +95,9 @@ extern int usb_runtime_idle(struct devic
extern int usb_enable_usb2_hardware_lpm(struct usb_device *udev);
extern int usb_disable_usb2_hardware_lpm(struct usb_device *udev);
+extern void usbfs_notify_suspend(struct usb_device *udev);
+extern void usbfs_notify_resume(struct usb_device *udev);
+
#else
static inline int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
Index: usb-devel/include/uapi/linux/usbdevice_fs.h
===================================================================
--- usb-devel.orig/include/uapi/linux/usbdevice_fs.h
+++ usb-devel/include/uapi/linux/usbdevice_fs.h
@@ -197,5 +197,8 @@ struct usbdevfs_streams {
#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
#define USBDEVFS_DROP_PRIVILEGES _IOW('U', 30, __u32)
#define USBDEVFS_GET_SPEED _IO('U', 31)
+#define USBDEVFS_FORBID_SUSPEND _IO('U', 32)
+#define USBDEVFS_ALLOW_SUSPEND _IO('U', 33)
+#define USBDEVFS_WAIT_FOR_RESUME _IO('U', 34)
#endif /* _UAPI_LINUX_USBDEVICE_FS_H */
