[PATCH 0/5] usb: dwc2: fix low power issues found on stm32mp1

[PATCH 0/5] usb: dwc2: fix low power issues found on stm32mp1

[Fabrice Gasnier]

This series addresses several hang, bad_irq or re-initialisation issues
found on stm32mp1 when going to system low power (stop mode):
- First two fixes can be reproduced on stm32mp13 DK board, stm32mp15 DK
  and EVAL boards.
- Subsequent fixes can be reproduced on stm32mp15 EVAL board that uses
  an external Vbus regulator and stm32 dedicated detection logic.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Fabrice Gasnier (5):
      usb: dwc2: fix recovery after PM domain off
      usb: dwc2: fix a possible interrupt storm on stm32mp1
      usb: dwc2: hcd: fix possible successive vbus supply enables
      usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume
      usb: dwc2: platform: fix stm32mp15 detection logic power management

 drivers/usb/dwc2/core.c      |   1 -
 drivers/usb/dwc2/core.h      |   2 -
 drivers/usb/dwc2/core_intr.c |   6 +-
 drivers/usb/dwc2/hcd.c       |  11 +--
 drivers/usb/dwc2/platform.c  | 222 +++++++++++++++++++++----------------------
 5 files changed, 118 insertions(+), 124 deletions(-)
---
base-commit: 1a5dad490a3f8533ae955be59ac5d26a3dfb6166
change-id: 20261007-dwc2-low-power-fixes-stm32-0c1e5e68b252

Best regards,
--  
Fabrice Gasnier <fabrice.gasnier@foss.st.com>

[PATCH 1/5] usb: dwc2: fix recovery after PM domain off

[Fabrice Gasnier]

The commit ba6e518d136b ("usb: dwc2: Implement recovery after PM domain
off") introduces a regression on stm32mp1x, which freeze the platform
when going to low power.

When the controller has been put into clock gating, reading some of
the registers to backup them from the suspend() routine in platform.c
lead to platform hang.
On stm32, the issue is systematic in host mode:
- _dwc2_hcd_suspend() calls dwc2_host_enter_clock_gating() every time.
- dwc2_suspend() calls dwc2_host_backup_critical_registers() that freezes.
This may happen in device mode too, if the host has suspended the bus.
Then clock gating has been entered, before calling dwc2_suspend().

So rather than backing-up registers that can lead to freeze when in
clock gating mode upon suspend (to restore then later):
- keep the canary detection logic (GUSBCFG TOUCAL)
- move it after the host/device mode has been restored, e.g.
  after dwc2_force_dr_mode() or drd resume code
- re-initialize the core as done during probe, when calling
  dwc2_hcd_init()
- re-enable the global interrupts.

Fixes: ba6e518d136b ("usb: dwc2: Implement recovery after PM domain off")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/dwc2/core.c     |  1 -
 drivers/usb/dwc2/core.h     |  2 --
 drivers/usb/dwc2/platform.c | 54 ++++++++++++++-------------------------------
 3 files changed, 16 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index f375c5185bfe..368da20ae14d 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -43,7 +43,6 @@ int dwc2_backup_global_registers(struct dwc2_hsotg *hsotg)
 	/* Backup global regs */
 	gr = &hsotg->gr_backup;
 
-	gr->gintsts = dwc2_readl(hsotg, GINTSTS);
 	gr->gotgctl = dwc2_readl(hsotg, GOTGCTL);
 	gr->gintmsk = dwc2_readl(hsotg, GINTMSK);
 	gr->gahbcfg = dwc2_readl(hsotg, GAHBCFG);
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 767251aa1aa3..1b6441164c6c 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -668,7 +668,6 @@ struct dwc2_hw_params {
 /**
  * struct dwc2_gregs_backup - Holds global registers state before
  * entering partial power down
- * @gintsts:		Backup of GINTSTS register
  * @gotgctl:		Backup of GOTGCTL register
  * @gintmsk:		Backup of GINTMSK register
  * @gahbcfg:		Backup of GAHBCFG register
@@ -685,7 +684,6 @@ struct dwc2_hw_params {
  * @valid:		True if registers values backuped.
  */
 struct dwc2_gregs_backup {
-	u32 gintsts;
 	u32 gotgctl;
 	u32 gintmsk;
 	u32 gahbcfg;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ef0d73077034..643d67ce28ea 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -692,14 +692,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 		regulator_disable(dwc2->usb33d);
 	}
 
-	if (is_device_mode)
-		ret = dwc2_gadget_backup_critical_registers(dwc2);
-	else
-		ret = dwc2_host_backup_critical_registers(dwc2);
-
-	if (ret)
-		return ret;
-
 	if (dwc2->ll_hw_enabled &&
 	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
@@ -709,24 +701,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	return ret;
 }
 
-static int dwc2_restore_critical_registers(struct dwc2_hsotg *hsotg)
-{
-	struct dwc2_gregs_backup *gr;
-
-	gr = &hsotg->gr_backup;
-
-	if (!gr->valid) {
-		dev_err(hsotg->dev, "No valid register backup, failed to restore\n");
-		return -EINVAL;
-	}
-
-	if (gr->gintsts & GINTSTS_CURMODE_HOST)
-		return dwc2_host_restore_critical_registers(hsotg);
-
-	return dwc2_gadget_restore_critical_registers(hsotg, DWC2_RESTORE_DCTL |
-						      DWC2_RESTORE_DCFG);
-}
-
 static int __maybe_unused dwc2_resume(struct device *dev)
 {
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
@@ -742,18 +716,6 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	}
 	dwc2->phy_off_for_suspend = false;
 
-	/*
-	 * During suspend it's possible that the power domain for the
-	 * DWC2 controller is disabled and all register values get lost.
-	 * In case the GUSBCFG register is not initialized, it's clear the
-	 * registers must be restored.
-	 */
-	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
-		ret = dwc2_restore_critical_registers(dwc2);
-		if (ret)
-			return ret;
-	}
-
 	if (dwc2->params.activate_stm_id_vb_detection) {
 		unsigned long flags;
 		u32 ggpio, gotgctl;
@@ -786,6 +748,22 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 		dwc2_drd_resume(dwc2);
 	}
 
+	/*
+	 * During suspend it's possible that the power domain for the
+	 * DWC2 controller is disabled and all register values get lost.
+	 * In case the GUSBCFG register is not initialized, it's clear the
+	 * registers must be restored. So invoke here the necessary init
+	 * as done by dwc2_hcd_init() during probe.
+	 */
+	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
+		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
+			ret = dwc2_core_init(dwc2, true);
+			if (ret)
+				return ret;
+			dwc2_enable_global_interrupts(dwc2);
+		}
+	}
+
 	if (dwc2_is_device_mode(dwc2))
 		ret = dwc2_hsotg_resume(dwc2);
 

-- 
2.43.0

Re: [PATCH 1/5] usb: dwc2: fix recovery after PM domain off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc2: gadget state lost on system resume with clock gating
- [Medium] usb: dwc2: missing resource cleanup on dwc2_core_init failure
--

commit 0a7c02abb23c7f52b48654e1d1781cd4041fb820
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Subject: usb: dwc2: fix recovery after PM domain off

This commit modifies the PM suspend and resume recovery mechanism for dwc2
to prevent freezes when reading registers during clock gating. It removes
the manual backup of critical registers during suspend and moves the core
re-initialization logic to dwc2_resume().

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..643d67ce28ea2 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -692,14 +692,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
>  		regulator_disable(dwc2->usb33d);
>  	}
>  
> -	if (is_device_mode)
> -		ret = dwc2_gadget_backup_critical_registers(dwc2);
> -	else
> -		ret = dwc2_host_backup_critical_registers(dwc2);
> -
> -	if (ret)
> -		return ret;
> -
>  	if (dwc2->ll_hw_enabled &&
>  	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
>  		ret = __dwc2_lowlevel_hw_disable(dwc2);

[ ... ]

> @@ -786,6 +748,22 @@ static int __maybe_unused dwc2_resume(struct device *dev)
>  		dwc2_drd_resume(dwc2);
>  	}
>  
> +	/*
> +	 * During suspend it's possible that the power domain for the
> +	 * DWC2 controller is disabled and all register values get lost.
> +	 * In case the GUSBCFG register is not initialized, it's clear the
> +	 * registers must be restored. So invoke here the necessary init
> +	 * as done by dwc2_hcd_init() during probe.
> +	 */
> +	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
> +		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {

[Severity: High]
Does this leave the USB gadget permanently unresponsive if the USB bus was
suspended by the host prior to system suspend?

Since the manual backup of critical gadget registers was removed from
dwc2_suspend(), the platform cuts power to the controller and wipes all
hardware registers during suspend. On system resume, this check explicitly
skips dwc2_core_init() for pure peripheral mode (USB_DR_MODE_PERIPHERAL).

Subsequently, dwc2_hsotg_resume() is called, which can return early:

drivers/usb/dwc2/gadget.c:dwc2_hsotg_resume() {
	...
	if (hsotg->lx_state == DWC2_L2)
		return 0;
	...
}

Will this cause the device core and endpoints to remain completely
uninitialized since they are never restored or re-initialized?

> +			ret = dwc2_core_init(dwc2, true);
> +			if (ret)
> +				return ret;

[Severity: Medium]
Does this error path leak hardware resources?

Earlier in dwc2_resume(), __dwc2_lowlevel_hw_enable() is called to enable
clocks and regulators. If dwc2_core_init() fails and returns directly here,
does it bypass the required __dwc2_lowlevel_hw_disable() cleanup?

> +			dwc2_enable_global_interrupts(dwc2);
> +		}
> +	}
> +
>  	if (dwc2_is_device_mode(dwc2))
>  		ret = dwc2_hsotg_resume(dwc2);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com?part=1

[PATCH 2/5] usb: dwc2: fix a possible interrupt storm on stm32mp1

[Fabrice Gasnier]

On stm32mp1, upon resume from low power (stop) mode, in host mode, a
port interrupt storm may be seen before the hardware is accessible.

The core is able to generate interrupts during system resume in
platform.c when calling dwc2_resume()->__dwc2_lowlevel_hw_enable(). As
the PHY and clocks become available, a resume IRQ can be seen with a
Host Port Interrupt (PrtInt).

The wakeup interrupt handler make the controller to exit power down mode,
but the HCD handler returns IRQ_NONE, due to the HW accessible flag has
been cleared in _dwc2_hcd_suspend() earlier. It ends-up with the bad_irq
handler.

So set the HCD_FLAG_HW_ACCESSIBLE flag when the resume IRQ is handled to
exit from power down mode.

Fixes: ef5e0eec4764 ("usb: dwc2: Add clock gating exiting flow by system resume")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/dwc2/core_intr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 9565e6a52725..8f83c36d002d 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -453,6 +453,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		}
 	} else {
 		if (hsotg->lx_state == DWC2_L2) {
+			struct usb_hcd *hcd = hsotg->priv;
+
 			if (hsotg->in_ppd) {
 				ret = dwc2_exit_partial_power_down(hsotg, 1,
 								   true);
@@ -463,8 +465,10 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 
 			if (hsotg->params.power_down ==
 			    DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended &&
-			    !hsotg->params.no_clock_gating)
+			    !hsotg->params.no_clock_gating) {
 				dwc2_host_exit_clock_gating(hsotg, 1);
+				set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+			}
 
 			/*
 			 * If we've got this quirk then the PHY is stuck upon

-- 
2.43.0

Re: [PATCH 2/5] usb: dwc2: fix a possible interrupt storm on stm32mp1

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc2: NULL dereference of hcd in peripheral-only mode
- [High] usb: dwc2: incomplete fix for interrupt storm during resume from PPD/Hibernation
--

commit b421af8a6c891894bff06680a711320a770d78db
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: fix a possible interrupt storm on stm32mp1

This commit addresses an interrupt storm on stm32mp1 seen during system resume
from low power mode when in host mode. It sets the HCD_FLAG_HW_ACCESSIBLE flag
in dwc2_handle_wakeup_detected_intr() after exiting clock gating, allowing
the controller to successfully process interrupts.

Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 9565e6a52725a..8f83c36d002da 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -453,6 +453,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  		}
>  	} else {
>  		if (hsotg->lx_state == DWC2_L2) {
> +			struct usb_hcd *hcd = hsotg->priv;
> +
>  			if (hsotg->in_ppd) {
>  				ret = dwc2_exit_partial_power_down(hsotg, 1,
>  								   true);
> @@ -463,8 +465,10 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  
>  			if (hsotg->params.power_down ==
>  			    DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended &&
> -			    !hsotg->params.no_clock_gating)
> +			    !hsotg->params.no_clock_gating) {
>  				dwc2_host_exit_clock_gating(hsotg, 1);
> +				set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +			}

[Severity: High]
Is the interrupt storm fix incomplete because the HCD_FLAG_HW_ACCESSIBLE
restoration is restricted to the clock gating case?

If the platform uses Partial Power Down (DWC2_POWER_DOWN_PARAM_PARTIAL) or
Hibernation (DWC2_POWER_DOWN_PARAM_HIBERNATION), _dwc2_hcd_suspend() also
clears the HCD_FLAG_HW_ACCESSIBLE flag.

When the system wakes up in those modes, dwc2_handle_wakeup_detected_intr()
executes the partial power down exit paths (such as the call to
dwc2_exit_partial_power_down() just above this block) but never restores the
HCD_FLAG_HW_ACCESSIBLE flag since this restoration is placed inside the
DWC2_POWER_DOWN_PARAM_NONE condition.

Will the same interrupt storm bug persist for platforms using PPD or
Hibernation?

[Severity: High]
Can this unconditionally dereference hcd and cause a kernel panic in a
peripheral-only configuration?

If the system is built with CONFIG_USB_DWC2_PERIPHERAL=y or constrained to
peripheral mode (dr_mode == USB_DR_MODE_PERIPHERAL), dwc2_hcd_init() is
skipped, meaning hsotg->priv (and thus hcd) remains NULL.

If the hardware is suspended (hsotg->lx_state == DWC2_L2), and an OTG cable
insertion or hardware transient autonomously switches the controller's CURMODE
bit to Host mode, dwc2_is_device_mode() will return false.

Execution will then enter this host branch in
dwc2_handle_wakeup_detected_intr(), leading to a NULL pointer dereference
when it tries to update hcd->flags.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com?part=2

[PATCH 3/5] usb: dwc2: hcd: fix possible successive vbus supply enables

[Fabrice Gasnier]

External VBUS supply may have already been enabled, before entering
dwc2_hcd_reinit (e.g. the controller is in host mode, without any device
plugged).
This results in vbus enable count to increase every time this occurs.
As consequence, dwc2_vbus_supply_exit won't disable vbus upon suspend
request.
Fix this by reading PWR bit before and after dwc2_hcd_reinit() call.

Fixes: cd7cd0e6cedf ("usb: dwc2: fix unbalanced use of external vbus-supply")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/dwc2/hcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index cd0dc876b421..4ab058e6ef9a 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4264,9 +4264,11 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 		return 0;	/* why 0 ?? */
 	}
 
+	hprt0 = dwc2_read_hprt0(hsotg);
+
 	dwc2_hcd_reinit(hsotg);
 
-	hprt0 = dwc2_read_hprt0(hsotg);
+	hprt0 ^= dwc2_read_hprt0(hsotg);
 	/* Has vbus power been turned on in dwc2_core_host_init ? */
 	if (hprt0 & HPRT0_PWR) {
 		/* Enable external vbus supply before resuming root hub */

-- 
2.43.0

Re: [PATCH 3/5] usb: dwc2: hcd: fix possible successive vbus supply enables

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: dwc2: regulator leak due to symmetric XOR logic
--

commit f865d013106075de51e4e0c898c2ab0fe2bc50f1
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: hcd: fix possible successive vbus supply enables

External VBUS supply may have already been enabled, before entering
dwc2_hcd_reinit. This results in vbus enable count to increase every
time this occurs, and dwc2_vbus_supply_exit won't disable vbus upon
suspend request. Fix this by reading PWR bit before and after
dwc2_hcd_reinit() call.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index d5275cd658d4c..b7b4ff466d2b5 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4264,9 +4264,11 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>  		return 0;	/* why 0 ?? */
>  	}
>  
> +	hprt0 = dwc2_read_hprt0(hsotg);
> +
>  	dwc2_hcd_reinit(hsotg);
>  
> -	hprt0 = dwc2_read_hprt0(hsotg);
> +	hprt0 ^= dwc2_read_hprt0(hsotg);
>  	/* Has vbus power been turned on in dwc2_core_host_init ? */
>  	if (hprt0 & HPRT0_PWR) {
>  		/* Enable external vbus supply before resuming root hub */
>  		spin_unlock_irqrestore(&hsotg->lock, flags);
>  		ret = dwc2_vbus_supply_init(hsotg);

[Severity: Medium]
Since this XOR operation is symmetric, does it also evaluate to true if
HPRT0_PWR transitions from 1 to 0?

A 1 to 0 transition seems structurally possible if the hardware
asynchronously clears the power bit (e.g. due to an overcurrent fault
HPRT0_OVRCURRCHG) during the execution window.

If this occurs, could the driver erroneously call dwc2_vbus_supply_init()
when power was lost, improperly incrementing the regulator enable count?

Later cleanup paths like dwc2_hcd_stop() or ClearPortFeature
USB_PORT_FEAT_POWER will observe HPRT0_PWR == 0 and skip calling
dwc2_vbus_supply_exit(), which appears to lead to a permanent regulator
use count leak. The issue is not fixed in subsequent patches in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com?part=3

[PATCH 4/5] usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

[Fabrice Gasnier]

It's not required to remove port power upon system suspend, as explained
in commit 113f86d0c302 ("usb: dwc2: Update partial power down entering by
system suspend"). It can be addressed with hub ctrl (USB_PORT_FEAT_POWER).

Moreover, with current implementation runtime PM calls bus suspend/resume
routines, when no device is present on the port. In this case,
"b.port_connect_status" variable is 0, this makes:
- the _dwc2_hcd_suspend routine to skip power savings, vbus disable.
- the _dwc2_hcd_resume() is fully executed, including vbus enable
So, the vbus regulator enable count constantly increase. As a result, vbus
won't be turned off in case the role changes later to device mode.

So remove the disable/enable of external vbus in bus suspend/resume
routines to avoid this.

Fixes: 113f86d0c302 ("usb: dwc2: Update partial power down entering by system suspend")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/dwc2/hcd.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 4ab058e6ef9a..b5374a4b904f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4387,10 +4387,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		goto skip_power_saving;
 	}
 
-	spin_unlock_irqrestore(&hsotg->lock, flags);
-	dwc2_vbus_supply_exit(hsotg);
-	spin_lock_irqsave(&hsotg->lock, flags);
-
 	/* Ask phy to be suspended */
 	if (!IS_ERR_OR_NULL(hsotg->uphy)) {
 		spin_unlock_irqrestore(&hsotg->lock, flags);
@@ -4499,10 +4495,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* Enable external vbus supply after resuming the port. */
 	spin_unlock_irqrestore(&hsotg->lock, flags);
-	dwc2_vbus_supply_init(hsotg);
-
 	/* Wait for controller to correctly update D+/D- level */
 	usleep_range(3000, 5000);
 	spin_lock_irqsave(&hsotg->lock, flags);

-- 
2.43.0

Re: [PATCH 4/5] usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: dwc2: port power state desync causing VBUS loss
--

commit 54bc47ce99a8e233efd17f79eba5998d68323c1d
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

This commit removes the dwc2_vbus_supply_exit and dwc2_vbus_supply_init
calls during system suspend and resume. It aims to prevent incorrectly
increasing the regulator enable count when no device is connected to the
port.

Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c

[ ... ]

> @@ -4499,10 +4495,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>  		spin_lock_irqsave(&hsotg->lock, flags);
>  	}
>  
> -	/* Enable external vbus supply after resuming the port. */
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> -	dwc2_vbus_supply_init(hsotg);
> -
>  	/* Wait for controller to correctly update D+/D- level */
>  	usleep_range(3000, 5000);
>  	spin_lock_irqsave(&hsotg->lock, flags);

[Severity: High]
Does removing the physical regulator initialization here cause a state
desynchronization with the HPRT0_PWR bit?

Later in _dwc2_hcd_resume(), the HPRT0_PWR bit is unconditionally set to 1:

	dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
			HPRT0_ENACHG, HPRT0);

If the port is unpowered prior to this (e.g. at boot before the hub driver
powers it on), HPRT0_PWR becomes 1 without the physical regulator being
turned on.

When the hub driver subsequently tries to power on the port in
dwc2_hcd_hub_control(), it checks the prior state of HPRT0_PWR:

		pwr = hprt0 & HPRT0_PWR;
		hprt0 |= HPRT0_PWR;
		dwc2_writel(hsotg, hprt0, HPRT0);
		if (!pwr)
			dwc2_vbus_supply_init(hsotg);

Since _dwc2_hcd_resume() already forced HPRT0_PWR to 1, the condition (!pwr)
is false and dwc2_vbus_supply_init() is incorrectly skipped.

Could this leave the physical VBUS permanently off and prevent USB device
detection from functioning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com?part=4

[PATCH 5/5] usb: dwc2: platform: fix stm32mp15 detection logic power management

[Fabrice Gasnier]

On stm32mp15, the stm detection logic can be left enabled even though the
other hw resources gets disabled from the probe (peripheral mode only):
	/* Gadget code manages lowlevel hw on its own */
	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
		dwc2_lowlevel_hw_disable(hsotg);

Then when going to system low power, stm detection logic resources now
remains enabled since dwc2_suspend() checks:
	if (!dwc2->ll_hw_enabled)
		return 0;

In case the stm32mp15 goes standby, the registers contents gets lost
and may not be restored so dwc2 becomes unusable. This is the case when
no gadget driver has been bound until that point.

So move the stm detection logic enable/disable procedures to dedicated
dwc2_stm32_hw_[enable/disable] routines. Call these routines
from the __dwc2_lowlevel_hw_[enable/disable] routines.

Factorize some of the probing code. Replace regulator_enable() and
GGPIO register poking by direct call to dwc2_stm32_hw_enable().
The dwc2_lowlevel_hw_enable() call done earlier during probing isn't
able to do it: the activate_stm_id_vb_detection is only known after
the dwc2_init_params() has run (and probe the usb33d-supply).

Fixes: 2b94b054ac49 ("usb: dwc2: fix hang during suspend if set as peripheral")
Fixes: a415083a11cc ("usb: dwc2: add support for STM32MP15 SoCs USB OTG HS and FS")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/usb/dwc2/platform.c | 170 +++++++++++++++++++++++++-------------------
 1 file changed, 95 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 643d67ce28ea..ddab95b443a9 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -91,6 +91,80 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
+static int dwc2_stm32_hw_enable(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+	int ret;
+
+	/* params are only known, once dwc2_init_params() has been probed */
+	if (!hsotg->params.activate_stm_id_vb_detection)
+		return 0;
+
+	ret = regulator_enable(hsotg->usb33d);
+	if (ret)
+		return ret;
+
+	ggpio = dwc2_readl(hsotg, GGPIO);
+	ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
+	ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
+	dwc2_writel(hsotg, ggpio, GGPIO);
+
+	/* ID/VBUS detection startup time */
+	usleep_range(5000, 7000);
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	/* Remove overrides when resuming from the disable */
+	gotgctl = dwc2_readl(hsotg, GOTGCTL);
+	gotgctl &= ~GOTGCTL_DBNCE_FLTR_BYPASS;
+	gotgctl &= ~(GOTGCTL_BVALOEN | GOTGCTL_AVALOEN |
+		     GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL);
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	return 0;
+}
+
+static int dwc2_stm32_hw_disable(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+	int ret;
+
+	if (!hsotg->params.activate_stm_id_vb_detection)
+		return 0;
+
+	/*
+	 * Need to force the mode to the current mode to avoid Mode
+	 * Mismatch Interrupt when ID detection will be disabled.
+	 */
+	dwc2_force_mode(hsotg, !dwc2_is_device_mode(hsotg));
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	gotgctl = dwc2_readl(hsotg, GOTGCTL);
+	/* bypass debounce filter, enable overrides */
+	gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
+	gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN;
+	/* Force A / B session if needed */
+	if (gotgctl & GOTGCTL_ASESVLD)
+		gotgctl |= GOTGCTL_AVALOVAL;
+	if (gotgctl & GOTGCTL_BSESVLD)
+		gotgctl |= GOTGCTL_BVALOVAL;
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	ggpio = dwc2_readl(hsotg, GGPIO);
+	ggpio &= ~GGPIO_STM32_OTG_GCCFG_IDEN;
+	ggpio &= ~GGPIO_STM32_OTG_GCCFG_VBDEN;
+	dwc2_writel(hsotg, ggpio, GGPIO);
+
+	ret = regulator_disable(hsotg->usb33d);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 {
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
@@ -129,8 +203,22 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 	if (ret)
 		goto err_dis_clk;
 
+	ret = dwc2_stm32_hw_enable(hsotg);
+	if (ret)
+		goto err_dis_phy;
+
 	return 0;
 
+err_dis_phy:
+	if (hsotg->uphy) {
+		usb_phy_shutdown(hsotg->uphy);
+	} else if (hsotg->plat && hsotg->plat->phy_exit) {
+		hsotg->plat->phy_exit(pdev, hsotg->plat->phy_type);
+	} else {
+		phy_power_off(hsotg->phy);
+		phy_exit(hsotg->phy);
+	}
+
 err_dis_clk:
 	if (hsotg->clk)
 		clk_disable_unprepare(hsotg->clk);
@@ -166,6 +254,10 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
 	int ret = 0;
 
+	ret = dwc2_stm32_hw_disable(hsotg);
+	if (ret)
+		return ret;
+
 	if (hsotg->uphy) {
 		usb_phy_shutdown(hsotg->uphy);
 	} else if (hsotg->plat && hsotg->plat->phy_exit) {
@@ -346,9 +438,6 @@ static void dwc2_driver_remove(struct platform_device *dev)
 
 	dwc2_drd_exit(hsotg);
 
-	if (hsotg->params.activate_stm_id_vb_detection)
-		regulator_disable(hsotg->usb33d);
-
 	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
 }
@@ -541,33 +630,23 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		goto error;
 
 	if (hsotg->params.activate_stm_id_vb_detection) {
-		u32 ggpio;
-
 		hsotg->usb33d = devm_regulator_get(hsotg->dev, "usb33d");
 		if (IS_ERR(hsotg->usb33d)) {
 			retval = PTR_ERR(hsotg->usb33d);
 			dev_err_probe(hsotg->dev, retval, "failed to request usb33d supply\n");
 			goto error;
 		}
-		retval = regulator_enable(hsotg->usb33d);
+		retval = dwc2_stm32_hw_enable(hsotg);
 		if (retval) {
-			dev_err_probe(hsotg->dev, retval, "failed to enable usb33d supply\n");
+			dev_err_probe(hsotg->dev, retval, "dwc2_stm32_hw_enable failed\n");
 			goto error;
 		}
-
-		ggpio = dwc2_readl(hsotg, GGPIO);
-		ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(hsotg, ggpio, GGPIO);
-
-		/* ID/VBUS detection startup time */
-		usleep_range(5000, 7000);
 	}
 
 	retval = dwc2_drd_init(hsotg);
 	if (retval) {
 		dev_err_probe(hsotg->dev, retval, "failed to initialize dual-role\n");
-		goto error_init;
+		goto error;
 	}
 
 	if (hsotg->dr_mode != USB_DR_MODE_HOST) {
@@ -637,9 +716,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
 error_drd:
 	dwc2_drd_exit(hsotg);
 
-error_init:
-	if (hsotg->params.activate_stm_id_vb_detection)
-		regulator_disable(hsotg->usb33d);
 error:
 	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
@@ -661,37 +737,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 
 	dwc2_drd_suspend(dwc2);
 
-	if (dwc2->params.activate_stm_id_vb_detection) {
-		unsigned long flags;
-		u32 ggpio, gotgctl;
-
-		/*
-		 * Need to force the mode to the current mode to avoid Mode
-		 * Mismatch Interrupt when ID detection will be disabled.
-		 */
-		dwc2_force_mode(dwc2, !is_device_mode);
-
-		spin_lock_irqsave(&dwc2->lock, flags);
-		gotgctl = dwc2_readl(dwc2, GOTGCTL);
-		/* bypass debounce filter, enable overrides */
-		gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
-		gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN;
-		/* Force A / B session if needed */
-		if (gotgctl & GOTGCTL_ASESVLD)
-			gotgctl |= GOTGCTL_AVALOVAL;
-		if (gotgctl & GOTGCTL_BSESVLD)
-			gotgctl |= GOTGCTL_BVALOVAL;
-		dwc2_writel(dwc2, gotgctl, GOTGCTL);
-		spin_unlock_irqrestore(&dwc2->lock, flags);
-
-		ggpio = dwc2_readl(dwc2, GGPIO);
-		ggpio &= ~GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio &= ~GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(dwc2, ggpio, GGPIO);
-
-		regulator_disable(dwc2->usb33d);
-	}
-
 	if (dwc2->ll_hw_enabled &&
 	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
@@ -716,31 +761,6 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	}
 	dwc2->phy_off_for_suspend = false;
 
-	if (dwc2->params.activate_stm_id_vb_detection) {
-		unsigned long flags;
-		u32 ggpio, gotgctl;
-
-		ret = regulator_enable(dwc2->usb33d);
-		if (ret)
-			return ret;
-
-		ggpio = dwc2_readl(dwc2, GGPIO);
-		ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(dwc2, ggpio, GGPIO);
-
-		/* ID/VBUS detection startup time */
-		usleep_range(5000, 7000);
-
-		spin_lock_irqsave(&dwc2->lock, flags);
-		gotgctl = dwc2_readl(dwc2, GOTGCTL);
-		gotgctl &= ~GOTGCTL_DBNCE_FLTR_BYPASS;
-		gotgctl &= ~(GOTGCTL_BVALOEN | GOTGCTL_AVALOEN |
-			     GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL);
-		dwc2_writel(dwc2, gotgctl, GOTGCTL);
-		spin_unlock_irqrestore(&dwc2->lock, flags);
-	}
-
 	if (!dwc2->role_sw) {
 		/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
 		dwc2_force_dr_mode(dwc2);

-- 
2.43.0

Re: [PATCH 5/5] usb: dwc2: platform: fix stm32mp15 detection logic power management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc2: ERR_PTR dereference and unbalanced regulator_disable on probe error
- [High] usb: dwc2: resource leak of PHY and clocks via early return in __dwc2_lowlevel_hw_disable
--

commit 7d8da634deddde8e939ffe9d7757702801ebe78b
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

    usb: dwc2: platform: fix stm32mp15 detection logic power management
    
    On stm32mp15, the stm detection logic can be left enabled even though the
    other hw resources gets disabled from the probe. This moves the stm
    detection logic enable/disable procedures to dedicated routines.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 643d67ce28ea2..ddab95b443a99 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
[ ... ]
> @@ -91,6 +91,80 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
[ ... ]
> +static int dwc2_stm32_hw_disable(struct dwc2_hsotg *hsotg)
> +{
[ ... ]
> +	ret = regulator_disable(hsotg->usb33d);

[Severity: High]
If devm_regulator_get() fails during dwc2_driver_probe(), can this
dereference an ERR_PTR?

In dwc2_driver_probe():

	hsotg->usb33d = devm_regulator_get(hsotg->dev, "usb33d");
	if (IS_ERR(hsotg->usb33d)) {
		retval = PTR_ERR(hsotg->usb33d);
		...
		goto error;
	}

At the error label, if hsotg->ll_hw_enabled is true, the code calls
dwc2_lowlevel_hw_disable() which invokes __dwc2_lowlevel_hw_disable().
That in turn calls dwc2_stm32_hw_disable().

Will dwc2_stm32_hw_disable() then blindly call regulator_disable() on the
ERR_PTR in hsotg->usb33d?

Also, if devm_regulator_get() succeeded but dwc2_stm32_hw_enable() failed,
will this trigger an unbalanced regulator_disable() warning?

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
[ ... ]
> @@ -166,6 +254,10 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>  	struct platform_device *pdev = to_platform_device(hsotg->dev);
>  	int ret = 0;
>  
> +	ret = dwc2_stm32_hw_disable(hsotg);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Does this early return leak PHY power states and clocks when
dwc2_stm32_hw_disable() fails?

If dwc2_stm32_hw_disable() encounters an error (for example, due to an
unbalanced regulator_disable() returning an error, or a PMIC communication
error), it returns a non-zero value.

By returning early here, the function completely bypasses the remaining
hardware teardown sequence, including usb_phy_shutdown(), phy_power_off(),
clk_disable_unprepare(), and regulator_bulk_disable().

Can this leave the hardware active during module unload, suspend, or probe
failure?

>  	if (hsotg->uphy) {
>  		usb_phy_shutdown(hsotg->uphy);
>  	} else if (hsotg->plat && hsotg->plat->phy_exit) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com?part=5

Re: [PATCH 0/5] usb: dwc2: fix low power issues found on stm32mp1

[Stefan Wahren]

Hi Fabrice,

[add Thinh to CC]

Am 08.10.26 um 15:33 schrieb Fabrice Gasnier:
> This series addresses several hang, bad_irq or re-initialisation issues
> found on stm32mp1 when going to system low power (stop mode):
> - First two fixes can be reproduced on stm32mp13 DK board, stm32mp15 DK
>    and EVAL boards.
> - Subsequent fixes can be reproduced on stm32mp15 EVAL board that uses
>    an external Vbus regulator and stm32 dedicated detection logic.
>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Minas isn't DWC2 maintainer anymore:
https://lore.kernel.org/linux-usb/c2a97d0eafcbaa9509edce5b7b2648d6f9d80863.1790993820.git.Thinh.Nguyen@synopsys.com/ 

> ---
> Fabrice Gasnier (5):
>        usb: dwc2: fix recovery after PM domain off
>        usb: dwc2: fix a possible interrupt storm on stm32mp1
>        usb: dwc2: hcd: fix possible successive vbus supply enables
>        usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume
>        usb: dwc2: platform: fix stm32mp15 detection logic power management
>
>   drivers/usb/dwc2/core.c      |   1 -
>   drivers/usb/dwc2/core.h      |   2 -
>   drivers/usb/dwc2/core_intr.c |   6 +-
>   drivers/usb/dwc2/hcd.c       |  11 +--
>   drivers/usb/dwc2/platform.c  | 222 +++++++++++++++++++++----------------------
>   5 files changed, 118 insertions(+), 124 deletions(-)
> ---
> base-commit: 1a5dad490a3f8533ae955be59ac5d26a3dfb6166
> change-id: 20261007-dwc2-low-power-fixes-stm32-0c1e5e68b252
>
> Best regards,
> --
> Fabrice Gasnier <fabrice.gasnier@foss.st.com>