On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
>
> >
> > Hi Alan,
> >
> > With the suggested modification (of having suspend/resume of usbfs
> > at
> > device level instead of interface level), looks like I am seeing a
> > deadlock described as below -
> >
> > Pre-condition: USB device is connected but suspended before running
> > test
> > program.
> >
> > 1. The test program calls open(/dev/bus/usb/...).
> > 2. This ends up calling usbdev_open().
> > 3. usbdev_open() takes the lock and calls usb_autoresume_device().
> > 4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
> > version, this call will return after calling the resume call-back of
> > driver (please correct me if wrong).
> > 5. This ends up calling generic_resume() which
> > calls usbfs_notify_resume().
> > 6. Now usbfs_notify_resume() also wants the same lock that
> > usbdev_open()
> > in (3) has already taken.
> What lock are you talking about? usbfs_notify_resume() doesn't take
> any locks.
The lock I am talking about is used
by usb_lock_device/usb_unlock_device. It is shared between usbdev_open()
and usbfs_notify_resume().
I think for this approach, we would also need to
call usb_autosuspend_device/usb_autoresume_device out of the
usb_lock/unlock_device pair.
>
> >
> > My observation so far is - when the USB device is connected for
> > first
> > time, Android's USB manager server is able to open the device and
> > reads
> > its descriptors using usbfs. But the test is not able to. The change
> > is
> > auto-suspend in between device connect and run of test program.
> >
> > I am still analysing the situation here to see if pre-condition
> > above
> > really makes difference or not. So please take this update with
> > pinch of
> > salt. However, still I wanted send this update and get a quick
> > review to
> > ensure I am not wandering in weeds.
> This doesn't sound like a real problem.
>
> However, I have been thinking about how to do all this in light of
> Oliver's comments, and it seems like we should make some changes.
>
> First, let's change the ioctls' names. Instead of RESUME and SUSPEND,
> I'll call them FORBID_SUSPEND and ALLOW_SUSPEND. The way they work
> should be clear: ALLOW_SUSPEND will permit the device to be suspended
> but might not cause a suspend to happen immediately. FORBID_SUSPEND
> will cause an immediate resume if the device is currently suspended
> and
> will prevent the device from being suspended in the future. The new
> names more accurately reflect what the ioctls actually do.
>
> Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has
> occurred more recently than the most recent ALLOW_SUSPEND ioctl. So
> for example, if the program calls ALLOW_SUSPEND, and the device
> suspends, and then the device resumes, and then the device suspends
> again, and then the program calls WAIT_FOR_RESUME, the ioctl will
> return immediately even though the device is currently suspended.
> Thus you don't have to worry about missing a remote resume. This also
> means that when WAIT_FOR_RESUME returns, the program should call
> FORBID_SUSPEND to ensure that the device is active and doesn't go
> back
> into suspend.
>
1. For the above example, since resume can happen anytime, the user-
space's suspend and resume notion would get out-of-sync with actual
device state, right?
> In practice, your program would use the ioctls as follows:
>
> When the device file is opened, the kernel will automatically
> do the equivalent of FORBID_SUSPEND (to remain compatible
> with the current behavior).
>
> When the program is ready for the device to suspend, it will
> call the ALLOW_SUSPEND ioctl. But it won't cancel the
> outstanding URBs; instead it will continue to interact
> normally with the device, because the device might not be
> suspended for some time.
>
I think it is reasonable to expect that user-space will call
ALLOW_SUSPEND when there is no URB pending from its side. If that is not
the case, how can it expect the device to suspend when it has some
outstanding work pending?
> When the device does go into suspend, URBs will start failing
> with an appropriate error code (EHOSTUNREACH, ESHUTDOWN,
> EPROTO, or something similar). In this way the program will
> realize the device is suspended. At that point the program
> should call the WAIT_FOR_RESUME ioctl and stop trying to
> communicate with the device.
>
> When WAIT_FOR_RESUME returns, the program should call the
> FORBID_SUSPEND ioctl and resume normal communication with the
> device.
>
Due to this condition, the example in (1) above will not cause a wait on
current resume operation, since after WAIT_FOR_RESUME returns, the user-
space will call FORBID_SUSPEND, which will bring device out of current
suspend state. So, the wait effectively didn't happen, right?
> How does that sound?
My concerns are mentioned above, hope they make sense.
I will give this approach a shot with my test program and see how that
goes.
Just to re-iterate, the main issue we have now is - how to notify user-
space of suspend, so that it can safely call wait-for-resume, right?
Thanks,
>
> The proposed patch is below.
>
> Alan Stern
>
>
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -60,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 {
> @@ -699,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;
> @@ -712,12 +713,29 @@ 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;
> +
> + list_for_each_entry(ps, &udev->filelist, list) {
> + WRITE_ONCE(ps->not_yet_resumed, 0);
> + wake_up_all(&ps->wait_for_resume);
> + }
> +}
> +
> 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)
> @@ -1008,6 +1026,7 @@ 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();
> @@ -1032,8 +1051,11 @@ static int usbdev_release(struct inode *
> struct usb_device *dev = ps->dev;
> unsigned int ifnum;
> struct async *as;
> + int rc = 0;
>
> usb_lock_device(dev);
> + if (ps->suspend_allowed)
> + rc = usb_autoresume_device(dev);
> usb_hub_release_all_ports(dev, ps);
>
> list_del_init(&ps->list);
> @@ -1044,7 +1066,8 @@ static int usbdev_release(struct inode *
> releaseintf(ps, ifnum);
> }
> destroy_all_async(ps);
> - usb_autosuspend_device(dev);
> + if (rc == 0)
> + usb_autosuspend_device(dev);
> usb_unlock_device(dev);
> usb_put_dev(dev);
> put_pid(ps->disc_pid);
> @@ -2355,6 +2378,45 @@ 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);
> +
> + return ret;
> +}
> +
> /*
> * NOTE: All requests here that have interface numbers as parameters
> * are assuming that somehow the configuration has been prevented
> from
> @@ -2549,6 +2611,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:
> @@ -2620,6 +2691,8 @@ static void usbdev_remove(struct usb_dev
> 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);
> 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 */
>
