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

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

On Thu, 2019-06-20 at 11:49 -0400, Alan Stern wrote:
> On Thu, 20 Jun 2019, Mayuresh Kulkarni wrote:
> 
> > 
> > On Wed, 2019-06-19 at 10:40 -0400, Alan Stern wrote:
> > 
> > > 
> > > 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.
> That's up to you.  There are lots of different ways to set the 
> autosuspend delay.  For example, you could create a udev rule that 
> would do it only for the devices your program handles.
> 
> > 
> > > 
> > > 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.
> Obviously the code would need to be more careful.  It would call 
> usb_autoresume_device() only if ps->suspend_requested was true.
> 
> Alan Stern
> 
> > 
> > So, looks like the original proposed change seems better here. On
> > the
> > side note, I am still in process of verifying the code changes.

Hi Alan,

With the suggested modification (of having suspend/resume of usbfs at
device level instead of interface level), looks like I am seeing a
deadlock described as below -

Pre-condition: USB device is connected but suspended before running test
program.

1. The test program calls open(/dev/bus/usb/...).
2. This ends up calling usbdev_open().
3. usbdev_open() takes the lock and calls usb_autoresume_device().
4. usb_autoresume_device() calls pm_runtime_get_sync(). Due to _sync
version, this call will return after calling the resume call-back of
driver (please correct me if wrong).
5. This ends up calling generic_resume() which
calls usbfs_notify_resume().
6. Now usbfs_notify_resume() also wants the same lock that usbdev_open()
in (3) has already taken.

My observation so far is - when the USB device is connected for first
time, Android's USB manager server is able to open the device and reads
its descriptors using usbfs. But the test is not able to. The change is
auto-suspend in between device connect and run of test program.

I am still analysing the situation here to see if pre-condition above
really makes difference or not. So please take this update with pinch of
salt. However, still I wanted send this update and get a quick review to
ensure I am not wandering in weeds.

Thanks,