| Message ID | 20201028234311.6464-2-wcheng@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow DWC3 runtime suspend if UDC is unbinded | expand |
On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote: > The DWC3 runtime suspend routine checks for the USB connected parameter to > determine if the controller can enter into a low power state. The > connected state is only set to false after receiving a disconnect event. > However, in the case of a device initiated disconnect (i.e. UDC unbind), > the controller is halted and a disconnect event is never generated. Set > the connected flag to false if issuing a device initiated disconnect to > allow the controller to be suspended. > > Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> > --- > drivers/usb/dwc3/gadget.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 5d879b7606d5..6364429b2122 100644 > --- 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 > + > is_on = !!is_on; > > /* > @@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) > dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) % > dwc->ev_buf->length; > } > + dwc->connected = false; > } > > ret = dwc3_gadget_run_stop(dwc, is_on, false); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
On 10/28/2020 6:07 PM, Alan Stern wrote: > On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote: >> The DWC3 runtime suspend routine checks for the USB connected parameter to >> determine if the controller can enter into a low power state. The >> connected state is only set to false after receiving a disconnect event. >> However, in the case of a device initiated disconnect (i.e. UDC unbind), >> the controller is halted and a disconnect event is never generated. Set >> the connected flag to false if issuing a device initiated disconnect to >> allow the controller to be suspended. >> >> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> >> --- >> drivers/usb/dwc3/gadget.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 5d879b7606d5..6364429b2122 100644 >> --- 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. 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. Thanks Regards, Wesley Cheng >> + >> is_on = !!is_on; >> >> /* >> @@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) >> dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) % >> dwc->ev_buf->length; >> } >> + dwc->connected = false; >> } >> >> ret = dwc3_gadget_run_stop(dwc, is_on, false); >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project
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
On 11/3/2020 12:07 PM, Alan Stern wrote: > 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). > Hi Alan, Got it, will do. Yes, I'll include the error conditions as well in the actual change. Thanks again! Thanks Regards, Wesley Cheng
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 5d879b7606d5..6364429b2122 100644 --- 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; + } + is_on = !!is_on; /* @@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) % dwc->ev_buf->length; } + dwc->connected = false; } ret = dwc3_gadget_run_stop(dwc, is_on, false);
The DWC3 runtime suspend routine checks for the USB connected parameter to determine if the controller can enter into a low power state. The connected state is only set to false after receiving a disconnect event. However, in the case of a device initiated disconnect (i.e. UDC unbind), the controller is halted and a disconnect event is never generated. Set the connected flag to false if issuing a device initiated disconnect to allow the controller to be suspended. Signed-off-by: Wesley Cheng <wcheng@codeaurora.org> --- drivers/usb/dwc3/gadget.c | 6 ++++++ 1 file changed, 6 insertions(+)