| Message ID | Pine.LNX.4.44L0.1907051433420.1606-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usbfs: Add ioctls for runtime suspend and resume | expand |
On Fri, 2019-07-05 at 14:51 -0400, Alan Stern wrote: > 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). > Hi Alan, This patch looks good. Let me know the next step(s) and if anything else is needed from me. Thanks. > 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 > A request - I will highly appreciate if someone from Google/Android's USB team comment on this patch. The reason being, when this is merged, I imagine there will be suitable APIs in some future version of Google/Android's USB manager to enable suspend/resume for apps. Thanks. > > 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 */ >
On Thu, 11 Jul 2019, Mayuresh Kulkarni wrote: > On Fri, 2019-07-05 at 14:51 -0400, Alan Stern wrote: > > 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). > > > > Hi Alan, > > This patch looks good. > Let me know the next step(s) and if anything else is needed from me. The next step is to see if there are any comments. If there aren't, I will submit the patch officially. > Thanks. > > > 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 > > > > A request - > > I will highly appreciate if someone from Google/Android's USB team > comment on this patch. The reason being, when this is merged, I imagine > there will be suitable APIs in some future version of Google/Android's > USB manager to enable suspend/resume for apps. Nobody on Google/Android's USB team is likely to comment unless you bring this directly to their attention. Don't assume they will just happen to see it on the mailing list. Also, the last I looked, Android was using version 4.9 of the kernel. Since this patch won't get into the kernel until version 5.4 at the earliest, it may be quite a while before it shows up in Android. Alan Stern
On Thu, Jul 11, 2019 at 10:20:10AM -0400, Alan Stern wrote: > Also, the last I looked, Android was using version 4.9 of the kernel. > Since this patch won't get into the kernel until version 5.4 at the > earliest, it may be quite a while before it shows up in Android. Android does not "use" a specific version of the kernel, they only specify the "minimum kernel version" for a specific Android release. As an example, for Android Q, they require the 4.9 kernel as a minimum, and they have a specific LTS version as well, with updates required over time as well. That being said, you can always use a newer kernel version for Android, and they publish 4.14 and 4.19 kernel trees if you wish to use them, as well as they have a "mainline" branch that is now at 5.2 for companies that want to use those kernels. But for new features, like this one, you will need Android userspace changes to start relying on the kernel change, so that will take a bit of work, and by then the new kernel feature could have been backported as well if it is something they want to rely on. So yes, it would be at least a year before it showed up in Android given they do a release on a yearly cadence. thanks, greg k-h
On Fri, 2019-07-05 at 14:51 -0400, Alan Stern wrote: > 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? Hi Greg, Did you got a chance to look into the above documentation query by Alan? How should we go about documenting these new IOCTLs? > > 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 */ >
On Thu, Jul 25, 2019 at 10:10:27AM +0100, Mayuresh Kulkarni wrote: > On Fri, 2019-07-05 at 14:51 -0400, Alan Stern wrote: > > 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? > > Hi Greg, > > Did you got a chance to look into the above documentation query by Alan? > How should we go about documenting these new IOCTLs? Not yet, sorry, dealing with the backlog of patches after the merge window closed. Give me a week or so... But if you want to try your hand at it first, it's always easier to review a patch than it is to come up with a new one. thanks, greg k-h
On Thu, Jul 25, 2019 at 11:18:09AM -0400, Alan Stern wrote: > On Thu, 25 Jul 2019, Greg KH wrote: > > > > > 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? > > > > > > Hi Greg, > > > > > > Did you got a chance to look into the above documentation query by Alan? > > > How should we go about documenting these new IOCTLs? > > > > Not yet, sorry, dealing with the backlog of patches after the merge > > window closed. > > > > Give me a week or so... > > > > But if you want to try your hand at it first, it's always easier to > > review a patch than it is to come up with a new one. > > Would Documentation/userspace-api/ be the right place to put this > information? It looks like we could take a large chunk of > driver-api/usb/usb.rst (most of it, in fact) and move it over there. Sounds reasonable. > By the way, do you know anything about how the information in > Documentation/userspace-api gets presented to users in general? Is > there anything comparable to the Linux man-pages project? Or are > people just supposed to get hold of the kernel source from somewhere > and read the files there? No idea, but we do build it on every kernel release and put it up on kernel.org to be easily indexed by search engines: https://www.kernel.org/doc/html/latest/userspace-api/index.html thanks, greg k-h
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 */