diff mbox series

[v1] usb: dwc3: re-enable runtime PM after failed resume

Message ID 20240906005803.1824339-1-royluo@google.com (mailing list archive)
State Superseded
Headers show
Series [v1] usb: dwc3: re-enable runtime PM after failed resume | expand

Commit Message

Roy Luo Sept. 6, 2024, 12:58 a.m. UTC
When dwc3_resume_common() returns an error, runtime pm is left in
disabled state in dwc3_resume(). The next dwc3_suspend_common()
should be skipped as the device is already in suspended state but
it's not because power.disable_depth is non-zero.
Ensures runtime PM is always re-enabled even after failed resume
attempts.

Fixes: 68c26fe58182 ("usb: dwc3: set pm runtime active before resume common")
Cc: stable@vger.kernel.org
Signed-off-by: Roy Luo <royluo@google.com>
---
 drivers/usb/dwc3/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


base-commit: ad618736883b8970f66af799e34007475fe33a68

Comments

Thinh Nguyen Sept. 7, 2024, 12:58 a.m. UTC | #1
On Fri, Sep 06, 2024, Roy Luo wrote:
> When dwc3_resume_common() returns an error, runtime pm is left in
> disabled state in dwc3_resume(). The next dwc3_suspend_common()

What issue did you see when dwc3_suspend_common is not skipped?

BR,
Thinh

> should be skipped as the device is already in suspended state but
> it's not because power.disable_depth is non-zero.
> Ensures runtime PM is always re-enabled even after failed resume
> attempts.
> 
> Fixes: 68c26fe58182 ("usb: dwc3: set pm runtime active before resume common")
> Cc: stable@vger.kernel.org
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
>  drivers/usb/dwc3/core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index ccc3895dbd7f..1928b074b2df 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2537,7 +2537,7 @@ static int dwc3_suspend(struct device *dev)
>  static int dwc3_resume(struct device *dev)
>  {
>  	struct dwc3	*dwc = dev_get_drvdata(dev);
> -	int		ret;
> +	int		ret = 0;
>  
>  	pinctrl_pm_select_default_state(dev);
>  
> @@ -2547,12 +2547,11 @@ static int dwc3_resume(struct device *dev)
>  	ret = dwc3_resume_common(dwc, PMSG_RESUME);
>  	if (ret) {
>  		pm_runtime_set_suspended(dev);
> -		return ret;
>  	}
>  
>  	pm_runtime_enable(dev);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void dwc3_complete(struct device *dev)
> 
> base-commit: ad618736883b8970f66af799e34007475fe33a68
> -- 
> 2.46.0.469.g59c65b2a67-goog
>
Roy Luo Sept. 13, 2024, 6:02 p.m. UTC | #2
On Fri, Sep 6, 2024 at 5:58 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> On Fri, Sep 06, 2024, Roy Luo wrote:
> > When dwc3_resume_common() returns an error, runtime pm is left in
> > disabled state in dwc3_resume(). The next dwc3_suspend_common()
>
> What issue did you see when dwc3_suspend_common is not skipped?

Apologies for the delayed response.

To answer your question, if dwc3_suspend_common() isn't skipped, it
can lead to issues because dwc->dev is already in a suspended state.
This could mean its parent devices (like the power domain or glue
driver) are also suspended and may have released resources that dwc
requires.
Consequently, calling dwc3_suspend_common() in this situation could
result in attempts to access unclocked or unpowered registers.

Regards,
Roy Luo
Thinh Nguyen Sept. 13, 2024, 6:12 p.m. UTC | #3
On Fri, Sep 13, 2024, Roy Luo wrote:
> On Fri, Sep 6, 2024 at 5:58 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >
> > On Fri, Sep 06, 2024, Roy Luo wrote:
> > > When dwc3_resume_common() returns an error, runtime pm is left in
> > > disabled state in dwc3_resume(). The next dwc3_suspend_common()
> >
> > What issue did you see when dwc3_suspend_common is not skipped?
> 
> Apologies for the delayed response.
> 
> To answer your question, if dwc3_suspend_common() isn't skipped, it
> can lead to issues because dwc->dev is already in a suspended state.
> This could mean its parent devices (like the power domain or glue
> driver) are also suspended and may have released resources that dwc
> requires.
> Consequently, calling dwc3_suspend_common() in this situation could
> result in attempts to access unclocked or unpowered registers.
> 

Can you include this info in the commit message?

And while at it, can you also update minor style change to remove the
brackets for single line if statement to this:

 	ret = dwc3_resume_common(dwc, PMSG_RESUME);
 	if (ret)
 		pm_runtime_set_suspended(dev);

Thanks,
Thinh
Roy Luo Sept. 13, 2024, 11:24 p.m. UTC | #4
On Fri, Sep 13, 2024 at 11:12 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Can you include this info in the commit message?
>
> And while at it, can you also update minor style change to remove the
> brackets for single line if statement to this:
>
>         ret = dwc3_resume_common(dwc, PMSG_RESUME);
>         if (ret)
>                 pm_runtime_set_suspended(dev);
>

Sure, sent out v2 for review.

Regards,
Roy Luo
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index ccc3895dbd7f..1928b074b2df 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2537,7 +2537,7 @@  static int dwc3_suspend(struct device *dev)
 static int dwc3_resume(struct device *dev)
 {
 	struct dwc3	*dwc = dev_get_drvdata(dev);
-	int		ret;
+	int		ret = 0;
 
 	pinctrl_pm_select_default_state(dev);
 
@@ -2547,12 +2547,11 @@  static int dwc3_resume(struct device *dev)
 	ret = dwc3_resume_common(dwc, PMSG_RESUME);
 	if (ret) {
 		pm_runtime_set_suspended(dev);
-		return ret;
 	}
 
 	pm_runtime_enable(dev);
 
-	return 0;
+	return ret;
 }
 
 static void dwc3_complete(struct device *dev)