On Tue, Nov 03, 2020 at 11:02:25AM -0800, Wesley Cheng wrote:
>
>
> On 10/28/2020 6:07 PM, Alan Stern wrote:
> >> --- a/drivers/usb/dwc3/gadget.c
> >> +++ b/drivers/usb/dwc3/gadget.c
> >> @@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
> >> int is_on)
> >> unsigned long flags;
> >> int ret;
> >>
> >> + if (pm_runtime_suspended(dwc->dev)) {
> >> + pm_request_resume(dwc->dev);
> >> + return 0;
> >> + }
> >
> > Isn't this racy? What happens if the controller was active but a
> > runtime suspend occurs right here?
> >
> > Alan Stern
> >
>
> Hi Alan,
>
> Ah, yes you're right. I was hoping that the PM runtime layer would be
> utilizing the spinlock when reading out the runtime status, but even
> then, we wouldn't be able to catch intermediate states with this API
> (i.e. RPM_RESUMING or RPM_SUSPENDING)
>
> Tried a few different approaches, and came up with something like the
> following:
>
> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> {
> ...
> ret = pm_runtime_get_sync(dwc->dev);
> if (!ret) {
> pm_runtime_put(dwc->dev);
> return 0;
> }
> ...
> pm_runtime_put(dwc->dev);
>
> return 0;
> }
>
> I think this should be good to address your concern. The only way we
> would be able to ensure that the runtime PM state doesn't enter
> idle/suspend is if we increment the usage count for the duration we're
> accessing the DWC3 registers. With the synchronous PM runtime resume
> call, we can also ensure that no pending runtime suspends are executing
> in parallel while running this code.
That's correct.
> The check for the zero return value would be for avoiding running the
> DWC3 run stop sequence for the case where we executed the runtime PM
> resume, as the DWC3 runtime PM resume routine will set the run stop bit
> in there.
If you need to add an explanation of this subtle point in your email
message, then you should add a similar explanation as a comment in the
code. And don't forget to check for ret < 0 (i.e., a resume error).
Alan Stern
