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.
