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

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

On Wed, 2019-06-19 at 10:40 -0400, Alan Stern wrote:
> On Wed, 19 Jun 2019, Oliver Neukum wrote:
> 
> > 
> > Am Dienstag, den 18.06.2019, 11:50 -0400 schrieb Alan Stern:
> > > 
> > > On Tue, 18 Jun 2019, Mayuresh Kulkarni wrote:
> > > 
> > > > 
> > > > > 
> > > > > You're right that the program needs to know when the device is
> > > > > about
> > > > > to 
> > > > > be suspended.  But waiting for an ioctl to return isn't a good
> > > > > way 
> > > > > to do it; this needs to be a callback of some sort.  That is,
> > > > > the 
> > > > > kernel also needs to know when the program is ready for the
> > > > > suspend.
> > > > > 
> > > > > I don't know what is the best approach.
> > > > This is becoming tricky now.
> > > Yes.  There probably are mechanisms already in use in other parts
> > > of 
> > > the kernel that would be suitable here, but I don't know what they
> > > are.  
> > > We could try asking some other people for advice.
> > Waiting for an ioctl() is horrible. If you really want to do this
> > poll() would be the obvious API. It is made for waiting for changes
> > of states.
> > 
> > [..]
> > > 
> > > The suspend callback is _not_ responsible for actually suspending
> > > the
> > > device; that is handled by the USB subsystem core.
> > > 
> > > These ideas are indeed applicable to programs using usbfs.  The
> > > kernel
> > Not fully. Usbfs has the same issue as FUSE here. Drivers are per
> > interface but power management is per device. Hence every driver
> > is in the block IO path for these issues. You cannot do block IO
> > in user space. The best you can do is notify of state changes,
> > but you cannot wait for them.
> usbfs access is per-device, not per-interface, but your point remains 
> valid.
> 
> > 
> > > 
> > > needs to have a way to inform the program that the device is about
> > > enter (or has just left) a low-power state, so that the program
> > > can
> > > stop (or start) trying to communicate with the device.  And the
> > > kernel 
> > > needs to know when the program is ready for the state change.
> > That has difficulties based in fundamental issues. We can let user
> > space block power transitions. We can notify it. But we cannot
> > block on it.
> > 
> > It would be easiest to export the usb_autopm_* API to user space.
> But ->suspend and ->resume callbacks are part of that API, and as you 
> say, it is not feasible to have these callbacks run in userspace.
> 
> The only solution I can think of is for the userspace program to first
> set the device's autosuspend delay to 0.  Then whenever the
> WAIT_FOR_RESUME ioctl returns -- even if it returns immediately -- the
> program should assume the suspend is over or is not going to happen.  
> Then the program can run normally for a short while (10 seconds,
> perhaps) before making another attempt to suspend.
> 

Looks like usb_autosuspend_delay parameter is applicable to all USB
devices. Also, since it is a module parameter, it might be tuned on a
particular platform (e.g.: from init.<vendor>.rc on Android).
So, I am not sure if it is good idea to rely on user-space to change it
and restore it to original value when the USB device of interest is
detached.

> The only change I would make to the patch posted earlier is to have 
> proc_wait_for_resume() call usb_autoresume_device() and set 
> ps->suspend_requested = false before returning.
> 
> Mayuresh, how does that sound?

With the code-changes you send me (in response to the
patch), usb_autoresume_device() will be called when the waiter
in proc_wait_for_resume() will return and send some message to "know"
why resume happened.

With above proposed change, proc_wait_for_resume() will return
with usb_autoresume_device() and suspend_requested = false. So when the
user-space will send some message to "know" resume reason, the checks in
ioctl() handler will be skipped.

(Apologies if above is obvious, but still wanted to comment so that we
are on same page).

With that said, I think there would be an issue with "host-wake" case as
below - the sequence of operations are:
- suspend ioctl called: assume actual bus suspend happens
- wait-for-resume ioctl called
- after a while user-space wants to send a message (due to end user
interaction for example)

Here the ioctl() will do usb_autoresume_device() since suspend_requested
= true. This will end up calling usbfs_notify_resume() which will
release waiter in proc_wait_for_resume(). And due to above proposed
change, it will end up calling usb_autoresume_device() again.

As a result, suspend will not happen for next suspend ioctl call.

So, looks like the original proposed change seems better here. On the
side note, I am still in process of verifying the code changes.

> 
> Alan Stern
>