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

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

On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote:
> On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote:
> 
> 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.
> 
> 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.
> 
> 	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.
> 
> How does that sound?
> 
> The proposed patch is below.
> 
> Alan Stern
> 

Hi Alan,

With the 3-ioctl patch you had send, I checked the behaviour using my
test program on our reference platform.

AFAIU, the patch seems to work fine for our use-cases of: remote-wake
and host-wake.
For our typical use-cases, the user-space does not need to communicate
with the device even after calling ALLOW_SUSPEND, so I am not in
position to verify this point.

The test does the below steps -
----------------
A. REMOTE-WAKE -
----------------
1. Open the device file.
2. Find our interface and bind to it.
3. Send multiple commands to our interface.
4. Call ALLOW_SUSPEND.
5. Call WAIT_FOR_RESUME.
6. Wait for sometime (10-20 sec).
7. Do the activity on device which I know causes remote-wake to host.
8. Waiter in (5) above, calls FORBID_SUSPEND and reads the cause of
resume.
9. Release the interface and close the device file.

After (5) + auto-suspend time-out expires, I can see device, root-hub
and host-controller (DWC-3) going into suspend.
After (7), host-controller, root-hub and device resume are seen.

--------------
B. HOST-WAKE -
--------------
1. Open the device file.
2. Find our interface and bind to it.
3. Send multiple commands to our interface.
4. Spawn a thread to cause host-wake.
5. Call ALLOW_SUSPEND.
6. Call WAIT_FOR_RESUME.
7. Signal thread after a delay (10-20 sec).
8. Thread calls FORBID_SUSPEND and sends a command to device.
9. Release the interface, wait for thread-join and close the device
file.

After (6) + auto-suspend time-out expires, device, root-hub and host
controller goes into suspend.
After (8), host-controller, root-hub and device resume are seen. Also
the response to command is correct.

With that said, at this point, the above observations are in-sync with
what I had verified when I had send-out the old patch (with 2-ioctls and
interface based suspend-resume). I am checking if I can verify a bit
more complex use-cases. But not sure, if I can do that with current
setup.

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.

Thanks to You/Oliver/Greg KH for the all the comments and reviews.