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

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

On Wed, 2019-06-26 at 11:07 -0400, Alan Stern wrote:
> On Wed, 26 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > On Tue, 2019-06-25 at 10:08 -0400, Alan Stern wrote:
> > 
> > > 
> > > > 
> > > > 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().
> > > 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.
> > > 
> > I see what you mean here, could be problematic.
> > 
> > > 
> > > 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.
> > > 
> > In poll approach, due to the contraint I mentioned, it is not
> > expected
> > from user-space to interact with device *after* it calls
> > ALLOW_SUSPEND
> > ioctl. This is because, it has determined that device is idle from
> > its
> > point of view.
> What if the user interacts with the device at this point?  Wouldn't 
> the program want to know about it?
> 
> Even if your program doesn't care about user interaction with an idle
> device, there undoubtedly will be other programs that _do_
> care.  This 
> mechanism we are designing needs to work with all programs.
> 

OK.

> > 
> > But after a while, there could be a situation where the user-space
> > wants
> > to send some message to device (due to end user interaction) then,
> > having (4) would be handy to cancel the undone suspend or cause
> > host-
> > wake.
> That's what the FORBID_SUSPEND ioctl does.  We don't need (4) for
> this.
> 

Right, OK.

> > 
> > > 
> > > > 
> > > > 2. Is it safe to wait for udev->state to be of a particular
> > > > value?
> > > No, not really, because the state can change.
> > > m[c]++;
> > If you remember, I had also suggested to use root-hub suspend in
> > generic_suspend() to call usbfs_notify_suspend/_resume APIs. It
> > might be
> > possible to use that in usbdev_poll() and send POLLPRI when root-hub
> > suspends.
> I don't see how that would help.  It would just make things more 
> confusing.  Forget about root-hub events for now.
> 

Yes sure. Thanks.

> > 
> > > 
> > > > 
> > > > If this approach could work, I can spend time on this one as
> > > > well.
> > > What advantage do you think your proposal has over my proposal?
> > > 
> > Not necessarily advantage(s), but few things that I could think of
> > are -
> > 
> > 1. In poll based approach, user-space notion of device's state is in
> > sync with actual device state.
> NO!  That simply is not true.  You don't seem to be able to grasp this
> point.
> 
> Consider this: Suppose the computer is busy running many other 
> programs.  Your program gets swapped out because a bunch of other 
> higher-priority tasks need to run.  By the time your program gets a 
> chance to run again, the device could have gone through several 
> suspend/resume transitions.  There's no way the program can keep
> track 
> of all that.
> 

Yeah I see what you mean.

> If you want a more likely example, consider this: Your program calls 
> ALLOW_SUSPEND and then calls poll().  The device suspends and the 
> poll() returns.  But then, before your program can do anything else, 
> the device resumes.  Now the device is active but your program thinks 
> it is suspended -- the program is out of sync.
> 
> Face it: It is _impossible_ for a program to truly know the device's 
> current state at all times (unless the device is not allowed to
> suspend 
> at all).
> 

Yep, thanks.

> > 
> > This is partially true with the 3 ioctl
> > approach suggested by you (partially because resume might not be
> > current
> > device state when wait-for-resume returns).
> Agreed.  But so what?  What abilities does your program lose as a 
> result of the fact that the device might be suspended when 
> WAIT_FOR_RESUME returns?
> 
> > 
> > 2. In 3 ioctl approach, user-space can still communicate with device
> > after calling ALLOW_SUSPEND. With poll based approach, we put a
> > constraint on user-space that, call ALLOW_SUSPEND only when you
> > determine you are not going to interact with device.
> That sounds like a disadvantage for your poll-based approach: It 
> constrains the behavior of userspace programs.
> 
> > 
> > I know for (2) above, you have commented before that -
> > A. It is desirable to be able to communicate with the device till it
> > is
> > actually suspended.
> > B. The possibility of device not going into suspend for long time,
> > so
> > user-space will not be able to proceed.
> > 
> > The counter comments to them are:
> > For (A), *usually*, the driver by some means determine device is
> > idle
> > and then schedules a suspend. After that, it is not expecting to
> > communicate with the device till resume happens. If it has to
> > communicate, it has to call resume first and then proceed.
> _Some_ programs will behave that way but other programs will not;
> they will want to continue communicating with the device right up
> until
> the time it suspends.  The kernel has to work with _all_ programs.
> 

Right, OK.

> > 
> > For (B), that is why we need ability to cancel current undone
> > suspend
> > operation, to allow the user-space to initiate the communication.
> And that is what FORBID_SUSPEND does.  You don't need to give up on
> (A) 
> in order to handle (B).
> 
> > 
> > Hope some of these points makes sense.
> None of them seem convincing, at least, not to me.
> 

Thanks for all the comments and clarifications, Alan.

I will check the 3-ioctl approach on the platform I have using the test
program I had used to send my original patch.

Just for my understanding, the below sequence should also work -
1. Call ALLOW_SUSPEND
2. Previously queued URB fails ==> device no longer active
3. Call WAIT_FOR_RESUME
4. After a while (say > 10 sec), assume no remote-wake by device. But
user-space wants to communicate with the device (due to some end-user
activity).
In this case, the user space needs to call FORBID_SUSPEND ioctl. When
that returns, it is safe to assume device is active.
5. Once done, go-to (1).

Could you please cross-confirm? Thanks,

> Alan Stern
>