Re: [PATCH] usb: core: devio: add ioctls for suspend and resume

[Mayuresh Kulkarni <mkulkarni@opensource.cirrus.com>]

On Mon, 2019-06-24 at 13:48 -0400, Alan Stern wrote:
> On Mon, 24 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > 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().
> Here's the subroutine as it appears in the patch:
> 
> 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);
> 	}
> }
> 
> Please point out exactly where in this code you think
> usbfs_notify_resume() tries to acquire the device lock.
> 

Oops, sorry. Please accept my apology on this one.
You are right, the original patch you send doesn't have the lock
in usbfs_notify_suspend/resume.

> (Although, to be fair, I think udev->filelist needs to be protected by
> usbfs_mutex.  I will add this in the next version of the patch.)

OK.

> 
> > 
> > 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.
> No, you're not allowed to do that.  See the documentation at the top 
> of the source code for usb_autosuspend_device() and 
> usb_autoresume_device() in driver.c.
> 
> > 
> > > 
> > > > 
> > > > 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?
> That's right.  We can't notify userspace about every state transition 
> the device makes, so userspace may very well get out-of-sync with the 
> actual device state.
> 
> > 
> > > 
> > > 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?
> There are two possible ways a userspace program can monitor the 
> device's state:
> 
>     1.	The program can leave an URB (typically on an interrupt 
> 	endpoint) running constantly, and the device can send its 
> 	response to the URB whenever something happens.
> 
>     2.	The program can poll the device by submitting an URB 
> 	periodically to see if anything has happened since the last 
> 	poll.
> 
> In case 1, the program would leave the URB running even after doing
> the 
> ALLOW_SUSPEND ioctl.  That way the program will be aware of anything 
> that happens to the device before it suspends.  When the device does
> go 
> into suspend, the URB will be terminated.
> 
> In case 2, the program would continue polling the device even after 
> doing the ALLOW_SUSPEND ioctl.  When the device does go into suspend, 
> the polling URB will fail.
> 

Right, so user space should do the following when it determines the
device is idle from its point of view -

1. Call ALLOW_SUSPEND ioctl
2. Queue an URB and wait for its REAP. When the wait returns -EFAIL (or
something similar), that is the indication that the device is no longer
active (or suspended)
3. Call WAIT_FOR_RESUME ioctl
4. When WAIT_FOR_RESUME ioctl returns, it is guaranteed that device is
active.
5. Call FORBID_SUSPEND ioctl and read the cause of resume.
6. Go to (1) when appropriate

Have I summarized this approach correctly from user-space point of view?

> If the program doesn't do either of these (that is, if it leaves the 
> device completely idle) then it won't know if the user presses a 
> button or does anything else to the device before the device is put 
> into suspend.
> 
> > 
> > > 
> > > 	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?
> Correct.
> 
> > 
> > > 
> > > 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?
> Basically there is no way to notify userspace when the device is 
> suspended.  Instead, we have to assume that the userspace program
> will 
> figure out what has happened all by itself when its URBs begin
> failing.
> 

Based on discussion so far and my understanding, how about below
approach -

1. Have ALLOW_SUSPEND and WAIT_FOR_RESUME ioctls. As before,
ALLOW_SUSPEND calls usb_autosuspend_device() while WAIT_FOR_RESUME waits
for resume.
2. Have usbfs_notify_suspend() and usbfs_notify_resume() as per your
previous patch (i.e.: system/resume callbacks at device level).
3. Extend usbdev_poll() to wait for udev->state == USB_STATE_SUSPENDED
when events == POLLPRI. Return POLLPRI when state = USB_STATE_SUSPENDED.
4. As before, any ioctl != (ALLOW_SUSPEND or WAIT_FOR_RESUME)
calls usb_autoresume_device().

The corresponding user-space calls would be -
A. When determined device is idle, call ALLOW_SUSPEND ioctl.
B. Call poll(device_fd, POLLPRI). When poll returns check revents
== POLLPRI.
C. Call WAIT_FOR_RESUME ioctl.
D. When WAIT_FOR_RESUME ioctl returns, read resume reason.
E. Go to (A).

The constraint on (1) above is - ALLOW_SUSPEND should be called when
user-space decides device is idle. I think this is not a hard constraint
since poll() for suspend will return POLLPRI when device is suspended
which is different from what it returns when ASYNC URB is completed.

Few points I am unsure of are -
1. Is it OK for a driver to re-purpose POLLPRI for its own use
or POLLPRI has a unique meaning system wide?
2. Is it safe to wait for udev->state to be of a particular value?

If this approach could work, I can spend time on this one as well.

Thanks,

> If necessary, we could add an extra ioctl (IS_SUSPENDED) which would 
> return 0 or 1 depending on whether the device is active or
> suspended.  
> But I don't think we will need this.
> 
> Alan Stern
>