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

[Alan Stern <stern@rowland.harvard.edu>]

On Wed, 26 Jun 2019, Oliver Neukum wrote:

> Am Dienstag, den 25.06.2019, 10:08 -0400 schrieb Alan Stern:
> > On Tue, 25 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > > 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
> 
> That is a race in principle. You should reverse steps 1 and 2

I disagree.  First, it's not really a race.  It doesn't matter whether
the URB error occurs during completion or during submission; either way
the program will become aware that the device has suspended.

Second, in all likelihood the program will be submitting URBs more or
less continually anyway.  Think of an HID driver, for example.  It
always keeps an interrupt URB active so that it can monitor what is
happening to the device.

If you really think it would be better, we could add an IS_SUSPENDED
ioctl so that the program can poll for state changes.  But that seems
like a pretty bad approach to me.  We could even add a WAIT_FOR_SUSPEND
ioctl, but programs basically will never want to wait for a suspend to
occur.

> > > 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
> 
> It seems to me that there ought to be one API for that. Either an
> ioctl or poll.

Isn't a WAIT_FOR_RESUME ioctl one API?

In general, an ioctl is more flexible than poll -- poll can report only
a very limited number of types of state change.

> > > 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?
> > 
> > Yes, except for one thing: In step 4, it is _not_ guaranteed that the 
> > device is active when WAIT_FOR_RESUME returns.  The only guarantee is 
> > that a resume did occur sometime after step 1, but the device might 
> > have gone back into suspend after that occurred.
> 
> Now is that a good API? Shouldn't we rather have an API for either
> * resume the device now and bump the counter
> * bump the counter when te device resumes

It makes the code cleaner: The usage counter is incremented by the
FORBID_SUSPEND ioctl and decremented by the ALLOW_SUSPEND ioctl.  
Nothing else.  Now you're suggesting that WAIT_FOR_RESUME should also
increment the usage counter upon return.  Programs may not even want
that behavior (consider the possibility that WAIT_FOR_RESUME was
interrupted by a signal).

In any case, we do need two separate APIs: one to resume the device
immediately, and one to wait for a resume to occur.  Programs will want 
to do both these things.

> I don't see a value in not having a defined power state after resume.

Well, we also don't have a defined power state after ALLOW_SUSPEND.  I
don't see this as a problem.  It's more of a necessity, in fact --
there's no way to keep a userspace program fully up to date on the
device's state, because suspend and resume events are asynchronous.

> > > 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().
> > 
> > 3 sounds reasonable at first, but I'm not sure it would work.  
> > Consider what would happen if the device is suspended very briefly and
> > then wakes up.  The usbdev_poll() call might not return, because by the
> > time it checks udev->state, the state has already changed back to
> > USB_STATE_CONFIGURED.
> 
> Indeed. It seems to me that any power transition should be reported
> back.

Sorry, I don't understand: Reported back where?  Keep in mind that it
is not feasible to report every transition to a userspace program.  
And even if you somehow did, the last reported value would be out of
date as soon as the program received it.

> > In any case, we shouldn't do 4.  It would prevent the device from ever
> > going into suspend, because the program would want to continue making
> > usbfs ioctl calls while waiting for the suspend to occur.
> 
> Exactly.
> 
> > > 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.
> > 
> > What if the device never does go into suspend?  The poll() call
> > wouldn't return and the program would just stop working.
> 
> Well, that is why you can poll for multiple events at the same
> time and the syscall has a timeout.

The events that poll reports on are really targeted toward
communication channels (e.g., data available to read, high-priority
data received, channel closed), not suspend/resume state changes.  
Poll really wasn't meant for this purpose.

> > > 2. Is it safe to wait for udev->state to be of a particular value?
> > 
> > No, not really, because the state can change.
> 
> That is a general issue with poll. You cannot be sure that there
> is still data to be read when you are ready to read.

Why not?  If you're dealing with a tty device and poll says there is 
data available to read, the data won't go away by itself.  It'll remain 
in the buffer until you read it.

Alan Stern