Mayuresh:
Whatever happened to this discussion? Did you reach a decision on
whether the proposed API addition would suit your needs?
Alan Stern
On Tue, 20 Nov 2018, Mayuresh Kulkarni wrote:
> On Fri, 2018-11-16 at 11:08 -0500, Alan Stern wrote:
> > On Fri, 16 Nov 2018, Mayuresh Kulkarni wrote:
> >
> > >
> > > Thanks for the comments. Based on the info so far, attempting to summarize
> > > the
> > > probable solution, to ensure that I understand it clearly -
> > >
> > > Facts -
> > > 1. USBFS driver grabs a PM ref-count in .open and drops it in .close which
> > > means
> > > USB device cannot suspend untill user-space closes it (even if all
> > > interface
> > > drivers report "idle" to usb-core).
> > > 2. Since the .ioctl "knows" that .open has ensured to keep device active,
> > > it
> > > does not call PM runtime APIs.
> > >
> > > Proposal -
> > > 1. Add new ioctl: suspend & wait-for-resume
> > > 2. suspend ioctl: decrements PM ref count and return
> > > 3. wait-for-resume ioctl: wait for resume or timeout or signal
> > Do you really want to have a timeout for this ioctl? Maybe it isn't
> > important -- I don't know.
> >
>
> Agreed, the timeout probably is not needed in this proposal.
>
> > >
> > > 4. Modify .ioctl implementation to do PM runtime calls except for above
> > > "new"
> > > ioctl calls (so pm_runtime_get_sync -> ioctl -> response ->
> > > pm_runtime_put_sync). This also means, pm runtime get/put will be in both
> > > .open/.close.
> > That's not exactly what I had in mind. Open will do:
> >
> > ps->runtime_active = true;
> >
> > The new suspend ioctl will do this:
> >
> > if (ps->runtime_active) {
> > usb_autosuspend_device(ps->dev);
> > ps->runtime_active = false;
> > }
> >
> > and the old ioctls (and close) will do this at the start:
> >
> > if (!ps->runtime_active) {
> > if (usb_autoresume_device(ps->dev))
> > return -EIO; /* Could not resume */
> > ps->runtime_active = true;
> > }
> >
> > This means that after any interaction with the device, you will have to
> > call the suspend ioctl again if you want the device to go back to
> > sleep.
> >
>
> Thanks, looks good.
>
> > >
> > > Use-case analysis -
> > > 1. Remote-wake: Due to device's remote wake, wait-for-resume will return
> > > successfully. The user space caller then need to queue a request to "know"
> > > the
> > > reason of remote-wake.
> > > 2. Host-wake: The user-space caller issues any ioctl supported by .ioctl
> > > method.
> > > Due to (4) above, the device will be resumed and the ioctl will be
> > > performed.
> > Correct.
> >
> > >
> > > For (2) in use-case analysis, the user-space caller's wait-for-resume will
> > > also
> > > return, but since it knows that it has initiated the ioctl, it may or may
> > > not
> > > decide to queue a request. Instead, when ioctl returns it can call
> > > wait-for-
> > > resume again.
> > Yes. Of course, your app will have some way to check for user
> > interaction with the device. Doing these checks while the device is
> > suspended would be counter-productive, since the check itself would
> > wake up the device. So you will probably want to do a check as soon as
> > you know the device has woken up, regardless of the cause. If you
> > don't, you run the risk of not noticing a user interaction.
> >
> > >
> > > Am I getting in sync with your comments?
> > >
> > > What issue(s) you anticipate in above proposal due to inherent race
> > > condition
> > > between host and remote-wake?
> > Only what I mentioned above, that your program should check for user
> > interaction whenever it knows the device has woken up.
> >
>
> Thanks, looks good.
>
> > >
> > > Based on my meagre understanding of usb-core, it feels
> > > like usb_lock_device/usb_unlock_device calls around remote-wake and usbfs
> > > ioctl
> > > should help with race condition, right?
> > No, they will not help. This is not a race between two different parts
> > of the kernel both trying to communicate with the device; it is a race
> > between the kernel and the user. usb_lock_device doesn't prevent the
> > user from interacting with the device. :-)
> >
> > Alan Stern
>
> I will go back and review this proposal internally. Possibly also attempt to
> implement a quick version of it and see how it behaves. Will keep this email
> thread posted with relevant updates.
>
> Thanks Alan and Oliver for the all inputs and comments so far.
