| Message ID | 20250312223434.3071598-1-royluo@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: dwc3: core: Avoid redundant system suspend/resume callbacks | expand |
On Wed, Mar 12, 2025, Roy Luo wrote: > dwc3 device suspend/resume callbacks were being triggered during system > suspend and resume even if the device was already runtime-suspended. > This is redundant for device mode because the suspend and resume routines > are essentially identical for system PM and runtime PM. > > To prevent these unnecessary callbacks, indicate to the PM core that it > can safely leave the device in runtime suspend if it's already > runtime-suspended in device mode by returning a positive value in > prepare() callback. This optimization only applies to devices without > pinctrl, as pinctrl has distinct logic tied to system suspend/resume. > > Signed-off-by: Roy Luo <royluo@google.com> > --- > Changes in v2: > - leave the pinctrl logic untouched, apply the optimization only when > pinctrl isn't being used. > --- > drivers/usb/dwc3/core.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 66a08b527165..02cdd0727a5e 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -26,6 +26,7 @@ > #include <linux/of_graph.h> > #include <linux/acpi.h> > #include <linux/pinctrl/consumer.h> > +#include <linux/pinctrl/devinfo.h> > #include <linux/reset.h> > #include <linux/bitfield.h> > > @@ -2658,14 +2659,31 @@ static void dwc3_complete(struct device *dev) > dwc3_writel(dwc->regs, DWC3_GUCTL3, reg); > } > } > + > +static int dwc3_prepare(struct device *dev) > +{ > + struct dwc3 *dwc = dev_get_drvdata(dev); > + > + /* > + * Indicate to the PM core that it may safely leave the device in > + * runtime suspend if runtime-suspended already in device mode. > + */ > + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE && > + pm_runtime_suspended(dev) && > + !dev_pinctrl(dev)) > + return 1; > + > + return 0; > +} > #else > #define dwc3_complete NULL > +#define dwc3_prepare NULL > #endif /* CONFIG_PM_SLEEP */ > > static const struct dev_pm_ops dwc3_dev_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume) > .complete = dwc3_complete, > - > + .prepare = dwc3_prepare, > /* > * Runtime suspend halts the controller on disconnection. It relies on > * platforms with custom connection notification to start the controller > > base-commit: 0fed89a961ea851945d23cc35beb59d6e56c0964 > -- > 2.49.0.rc1.451.g8f38331e32-goog > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Wed, Mar 12, 2025 at 10:34:34PM +0000, Roy Luo wrote: > dwc3 device suspend/resume callbacks were being triggered during system > suspend and resume even if the device was already runtime-suspended. > This is redundant for device mode because the suspend and resume routines > are essentially identical for system PM and runtime PM. > > To prevent these unnecessary callbacks, indicate to the PM core that it > can safely leave the device in runtime suspend if it's already > runtime-suspended in device mode by returning a positive value in > prepare() callback. This optimization only applies to devices without > pinctrl, as pinctrl has distinct logic tied to system suspend/resume. > > Signed-off-by: Roy Luo <royluo@google.com> Out of curiosity: What happens if the USB controller is already in runtime suspend (with wakeup interrupts enabled) when a system suspend occurs? Does the fact that the interrupts are enabled mean the controller will remain able to wake up the system even if device_may_wakeup() is false? Alan Stern
On Thu, Mar 13, 2025 at 7:27 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Mar 12, 2025 at 10:34:34PM +0000, Roy Luo wrote: > > dwc3 device suspend/resume callbacks were being triggered during system > > suspend and resume even if the device was already runtime-suspended. > > This is redundant for device mode because the suspend and resume routines > > are essentially identical for system PM and runtime PM. > > > > To prevent these unnecessary callbacks, indicate to the PM core that it > > can safely leave the device in runtime suspend if it's already > > runtime-suspended in device mode by returning a positive value in > > prepare() callback. This optimization only applies to devices without > > pinctrl, as pinctrl has distinct logic tied to system suspend/resume. > > > > Signed-off-by: Roy Luo <royluo@google.com> > > Out of curiosity: What happens if the USB controller is already in > runtime suspend (with wakeup interrupts enabled) when a system suspend > occurs? Does the fact that the interrupts are enabled mean the > controller will remain able to wake up the system even if > device_may_wakeup() is false? > > Alan Stern For this specific device-mode scenario, "runtime suspend (with wakeup interrupts enabled)" seems unlikely to happen, all irqs are masked as a part of dwc3_gadget_suspend() and then the dwc3 core is torn down. If this really happens in a hypothetical scenario, I would expect device_may_wakeup() set to true when wakeup interrupt is enabled. In device_suspend(), it does explicitly check "(device_may_wakeup(dev) || device_wakeup_path(dev))" and won't enable direct_complete if the device could wake up. Best, Roy
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 66a08b527165..02cdd0727a5e 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -26,6 +26,7 @@ #include <linux/of_graph.h> #include <linux/acpi.h> #include <linux/pinctrl/consumer.h> +#include <linux/pinctrl/devinfo.h> #include <linux/reset.h> #include <linux/bitfield.h> @@ -2658,14 +2659,31 @@ static void dwc3_complete(struct device *dev) dwc3_writel(dwc->regs, DWC3_GUCTL3, reg); } } + +static int dwc3_prepare(struct device *dev) +{ + struct dwc3 *dwc = dev_get_drvdata(dev); + + /* + * Indicate to the PM core that it may safely leave the device in + * runtime suspend if runtime-suspended already in device mode. + */ + if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE && + pm_runtime_suspended(dev) && + !dev_pinctrl(dev)) + return 1; + + return 0; +} #else #define dwc3_complete NULL +#define dwc3_prepare NULL #endif /* CONFIG_PM_SLEEP */ static const struct dev_pm_ops dwc3_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume) .complete = dwc3_complete, - + .prepare = dwc3_prepare, /* * Runtime suspend halts the controller on disconnection. It relies on * platforms with custom connection notification to start the controller
dwc3 device suspend/resume callbacks were being triggered during system suspend and resume even if the device was already runtime-suspended. This is redundant for device mode because the suspend and resume routines are essentially identical for system PM and runtime PM. To prevent these unnecessary callbacks, indicate to the PM core that it can safely leave the device in runtime suspend if it's already runtime-suspended in device mode by returning a positive value in prepare() callback. This optimization only applies to devices without pinctrl, as pinctrl has distinct logic tied to system suspend/resume. Signed-off-by: Roy Luo <royluo@google.com> --- Changes in v2: - leave the pinctrl logic untouched, apply the optimization only when pinctrl isn't being used. --- drivers/usb/dwc3/core.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) base-commit: 0fed89a961ea851945d23cc35beb59d6e56c0964