diff mbox series

usb: dwc3: core: avoid reading register after bus clk is disabled

Message ID 20241105071426.2411166-1-xu.yang_2@nxp.com
State New
Headers show
Series usb: dwc3: core: avoid reading register after bus clk is disabled | expand

Commit Message

Xu Yang Nov. 5, 2024, 7:14 a.m. UTC 
The driver may go through below sequence when works as device mode:

dwc3_suspend()
  dwc3_suspend_common()
    dwc3_core_exit()
      dwc3_clk_disable()
	clk_disable_unprepare(dwc->bus_clk);
    dwc3_enable_susphy()
      dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));

Then the driver will read dwc3 register after bus clk is disabled. If this
happens, the kernel will hang there. This will move dwc3_enable_susphy()
ahead to avoid such issue.

Fixes: 705e3ce37bcc ("usb: dwc3: core: Fix system suspend on TI AM62 platforms")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/dwc3/core.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Roger Quadros Nov. 5, 2024, 1:56 p.m. UTC | #1 
Hello Xu,

On 05/11/2024 09:14, Xu Yang wrote:
> The driver may go through below sequence when works as device mode:
> 
> dwc3_suspend()
>   dwc3_suspend_common()
>     dwc3_core_exit()
>       dwc3_clk_disable()
> 	clk_disable_unprepare(dwc->bus_clk);
>     dwc3_enable_susphy()
>       dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
> 
> Then the driver will read dwc3 register after bus clk is disabled. If this
> happens, the kernel will hang there. This will move dwc3_enable_susphy()
> ahead to avoid such issue.
> 
> Fixes: 705e3ce37bcc ("usb: dwc3: core: Fix system suspend on TI AM62 platforms")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Thank you for the patch. But this was already addressed yesterday.

https://lore.kernel.org/all/20241104-am62-lpm-usb-fix-v1-1-e93df73a4f0d@kernel.org/

> ---
>  drivers/usb/dwc3/core.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index de434f78c560..b0f1e32d426f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2347,6 +2347,15 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  			    (dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)) &
>  			    DWC3_GUSB3PIPECTL_SUSPHY);
>  
> +	if (!PMSG_IS_AUTO(msg)) {

This alone is not enough as device might have been runtime suspended before
system suspend and we will still try to access the registers below causing a fault.

> +		/*
> +		 * TI AM62 platform requires SUSPHY to be
> +		 * enabled for system suspend to work.
> +		 */
> +		if (!dwc->susphy_state)
> +			dwc3_enable_susphy(dwc, true);
> +	}
> +
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (pm_runtime_suspended(dwc->dev))
> @@ -2398,15 +2407,6 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		break;
>  	}
>  
> -	if (!PMSG_IS_AUTO(msg)) {
> -		/*
> -		 * TI AM62 platform requires SUSPHY to be
> -		 * enabled for system suspend to work.
> -		 */
> -		if (!dwc->susphy_state)
> -			dwc3_enable_susphy(dwc, true);
> -	}
> -
>  	return 0;
>  }
>
Xu Yang Nov. 6, 2024, 1:52 a.m. UTC | #2 
On Tue, Nov 05, 2024 at 03:56:00PM +0200, Roger Quadros wrote:
> Hello Xu,
> 
> On 05/11/2024 09:14, Xu Yang wrote:
> > The driver may go through below sequence when works as device mode:
> > 
> > dwc3_suspend()
> >   dwc3_suspend_common()
> >     dwc3_core_exit()
> >       dwc3_clk_disable()
> > 	clk_disable_unprepare(dwc->bus_clk);
> >     dwc3_enable_susphy()
> >       dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
> > 
> > Then the driver will read dwc3 register after bus clk is disabled. If this
> > happens, the kernel will hang there. This will move dwc3_enable_susphy()
> > ahead to avoid such issue.
> > 
> > Fixes: 705e3ce37bcc ("usb: dwc3: core: Fix system suspend on TI AM62 platforms")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> 
> Thank you for the patch. But this was already addressed yesterday.
> 
> https://lore.kernel.org/all/20241104-am62-lpm-usb-fix-v1-1-e93df73a4f0d@kernel.org/

Got it! Thanks!

> 
> > ---
> >  drivers/usb/dwc3/core.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index de434f78c560..b0f1e32d426f 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -2347,6 +2347,15 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  			    (dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)) &
> >  			    DWC3_GUSB3PIPECTL_SUSPHY);
> >  
> > +	if (!PMSG_IS_AUTO(msg)) {
> 
> This alone is not enough as device might have been runtime suspended before
> system suspend and we will still try to access the registers below causing a fault.

Okay. Good!

Best Regards,
Xu Yang
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index de434f78c560..b0f1e32d426f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2347,6 +2347,15 @@  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 			    (dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)) &
 			    DWC3_GUSB3PIPECTL_SUSPHY);
 
+	if (!PMSG_IS_AUTO(msg)) {
+		/*
+		 * TI AM62 platform requires SUSPHY to be
+		 * enabled for system suspend to work.
+		 */
+		if (!dwc->susphy_state)
+			dwc3_enable_susphy(dwc, true);
+	}
+
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
 		if (pm_runtime_suspended(dwc->dev))
@@ -2398,15 +2407,6 @@  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		break;
 	}
 
-	if (!PMSG_IS_AUTO(msg)) {
-		/*
-		 * TI AM62 platform requires SUSPHY to be
-		 * enabled for system suspend to work.
-		 */
-		if (!dwc->susphy_state)
-			dwc3_enable_susphy(dwc, true);
-	}
-
 	return 0;
 }