[PATCH] usb: dwc3: Skip resume if pm_runtime_set_active() fails

[PATCH] usb: dwc3: Skip resume if pm_runtime_set_active() fails

[Ray Chi]

When the system begins to enter suspend mode, dwc3_suspend() is called
by PM suspend. There is a problem that if someone interrupt the system
suspend process between dwc3_suspend() and pm_suspend() of its parent
device, PM suspend will be canceled and attempt to resume suspended
devices so that dwc3_resume() will be called. However, dwc3 and its
parent device (like the power domain or glue driver) may already be
suspended by runtime PM in fact. If this sutiation happened, the
pm_runtime_set_active() in dwc3_resume() will return an error since
parent device was suspended. This can lead to unexpected behavior if
DWC3 proceeds to execute dwc3_resume_common().

EX.
RPM suspend: ... -> dwc3_runtime_suspend()
                      -> rpm_suspend() of parent device
...
PM suspend: ... -> dwc3_suspend() -> pm_suspend of parent device
                                 ^ interrupt, so resume suspended device
          ...  <-  dwc3_resume()  <-/
                      ^ pm_runtime_set_active() returns error

To prevent the problem, this commit will skip dwc3_resume_common() and
return the error if pm_runtime_set_active() fails.

Fixes: 68c26fe58182 ("usb: dwc3: set pm runtime active before resume common")
Cc: stable@vger.kernel.org
Signed-off-by: Ray Chi <raychi@google.com>
---
 drivers/usb/dwc3/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c22b8678e02e..7578c5133568 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2609,12 +2609,15 @@ static int dwc3_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 
 	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		goto out;
 
 	ret = dwc3_resume_common(dwc, PMSG_RESUME);
 	if (ret)
 		pm_runtime_set_suspended(dev);
 
+out:
 	pm_runtime_enable(dev);
 
 	return ret;
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH] usb: dwc3: Skip resume if pm_runtime_set_active() fails

[Thinh Nguyen]

On Mon, Jan 06, 2025, Ray Chi wrote:
> When the system begins to enter suspend mode, dwc3_suspend() is called
> by PM suspend. There is a problem that if someone interrupt the system
> suspend process between dwc3_suspend() and pm_suspend() of its parent
> device, PM suspend will be canceled and attempt to resume suspended
> devices so that dwc3_resume() will be called. However, dwc3 and its
> parent device (like the power domain or glue driver) may already be
> suspended by runtime PM in fact. If this sutiation happened, the
> pm_runtime_set_active() in dwc3_resume() will return an error since
> parent device was suspended. This can lead to unexpected behavior if
> DWC3 proceeds to execute dwc3_resume_common().
> 
> EX.
> RPM suspend: ... -> dwc3_runtime_suspend()
>                       -> rpm_suspend() of parent device
> ...
> PM suspend: ... -> dwc3_suspend() -> pm_suspend of parent device
>                                  ^ interrupt, so resume suspended device
>           ...  <-  dwc3_resume()  <-/
>                       ^ pm_runtime_set_active() returns error
> 
> To prevent the problem, this commit will skip dwc3_resume_common() and
> return the error if pm_runtime_set_active() fails.


So, if the device is runtime suspended, we prevent any interrupt during
system suspend? Any way we can keep the same behavior and allow PM
interruption as when there's no runtime suspend.

BR,
Thinh

Re: [PATCH] usb: dwc3: Skip resume if pm_runtime_set_active() fails

[Thinh Nguyen]

On Wed, Jan 08, 2025, Thinh Nguyen wrote:
> On Mon, Jan 06, 2025, Ray Chi wrote:
> > When the system begins to enter suspend mode, dwc3_suspend() is called
> > by PM suspend. There is a problem that if someone interrupt the system
> > suspend process between dwc3_suspend() and pm_suspend() of its parent
> > device, PM suspend will be canceled and attempt to resume suspended
> > devices so that dwc3_resume() will be called. However, dwc3 and its
> > parent device (like the power domain or glue driver) may already be
> > suspended by runtime PM in fact. If this sutiation happened, the
> > pm_runtime_set_active() in dwc3_resume() will return an error since
> > parent device was suspended. This can lead to unexpected behavior if
> > DWC3 proceeds to execute dwc3_resume_common().
> > 
> > EX.
> > RPM suspend: ... -> dwc3_runtime_suspend()
> >                       -> rpm_suspend() of parent device
> > ...
> > PM suspend: ... -> dwc3_suspend() -> pm_suspend of parent device
> >                                  ^ interrupt, so resume suspended device
> >           ...  <-  dwc3_resume()  <-/
> >                       ^ pm_runtime_set_active() returns error
> > 
> > To prevent the problem, this commit will skip dwc3_resume_common() and
> > return the error if pm_runtime_set_active() fails.
> 
> 
> So, if the device is runtime suspended, we prevent any interrupt during
> system suspend? Any way we can keep the same behavior and allow PM
> interruption as when there's no runtime suspend.
> 

Actually, I misread, the interruption is still there. Just the resume is
prevented.

Looks good to me.

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh