Re: Query on usb/core/devio.c

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

On Tue, 2019-04-23 at 17:12 +0100, Mayuresh Kulkarni wrote:
> On Mon, 2019-04-01 at 16:22 -0400, Alan Stern wrote:
> > 
> > Mayuresh:
> > 
> > Whatever happened to this discussion?  Did you reach a decision on 
> > whether the proposed API addition would suit your needs?
> > 
> > Alan Stern
> Hi Alan,
> 
> Apologies for not being able to respond to this email thread before.
> Around mid-Dec of 2018, I got allocated to some other completely different
> project for couple of months.
> 
> Just at the of start of Apr 2019, I am back to the USB-audio project and this
> discussion.
> So almost perfect timing for your nudge.
> 
> I am in process of setting up my environment and should have some result at-
> most 
> by early next week. I am attempting to verify the use-case of suspend/resume
> with: host wake and remote wake.
> 
> Thanks again for your nudge.
> 

Hi Alan et al,

I added the proposed IOCTLs of suspend/resume to the platform I am using
internally. With that, I am able to verify below cases -
1. suspend -> wait-for-resume: resume caused by remote-wake from our USB device
2. suspend -> wait-for-resume: resume caused by host-wake (i.e. my test
application sends a message to our USB device).

In both the instances, after wait-for-resume, I can see host scheduling L2 and
actual L2 happens after the auto-suspend time-out expires (I am using default
value for it).

Below are the URB snoops for each case -

Remote-wake -
Here I cause the remote-wake activity on our USB-device approx. 20 sec after
calling wait-for-resume.

[  218.299803] usb 1-1: ioctl-suspend
[  218.299978] usb 1-1: wait-for-resume

[  222.022157] msm-dwc3 a800000.ssusb: DWC3 in low power mode

[  239.065016] msm-dwc3 a800000.ssusb: DWC3 exited from low power mode

[  239.145063] usb 1-1: driver-resume: runtime-active = 1
[  239.145444] usb 1-1: wait-for-resume...done

Host-wake -
Here I send the new command approx. 8 seconds after calling wait-for-resume.

[  152.760438] usb 1-1: ioctl-suspend
[  152.760717] usb 1-1: wait-for-resume

[  156.068823] msm-dwc3 a800000.ssusb: DWC3 in low power mode

[  160.765638] usb 1-1: suspended..resume now

[  160.768442] msm-dwc3 a800000.ssusb: DWC3 exited from low power mode

[  160.823889] usb 1-1: driver-resume: runtime-active = 1
[  160.823998] usb 1-1: resume done..active now

With that said, shall I send a patch of above changes for review, rebased to
usb-next branch - https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
/log/?h=usb-next?

Thanks,

> > 
> > 
> > 
> > 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.