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

[PATCH v3 0/6] 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 three 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>
---
Changes in v3:
- Update since Sashiko comments
- Add a paramter (need_restore) to gadget resume function based on
  canary detection to avoir possible race on lx_state as raised by
  Sashiko
- Add a dwc2_vbus_supply_restore() to check regulator enable state
  during hcd resume, to enforce it gets restored along with PWR bit
  if it has been turned off earlier.
- Introduced a new patch for a possible race in hcd resume, with
  HCD_FLAG_HW_ACCESSIBLE not being set, but host interrupts are active.
- Refactor stm32 vb/id detection, to rool back in case of regulator
  disable fails.
- Link to v2: https://patch.msgid.link/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com

Changes in v2:
- Several fixes or commit messages updates since Sashiko review (see
  changelog in each patch).
- Add Thinh instead of Minas to the recipients
- Link to v1: https://patch.msgid.link/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com

---
Fabrice Gasnier (6):
      usb: dwc2: fix recovery after PM domain off
      usb: dwc2: fix a possible interrupt storm in hcd resume
      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      |   6 +-
 drivers/usb/dwc2/core_intr.c |   9 +-
 drivers/usb/dwc2/gadget.c    |   4 +-
 drivers/usb/dwc2/hcd.c       |  36 +++++--
 drivers/usb/dwc2/platform.c  | 240 +++++++++++++++++++++++--------------------
 6 files changed, 165 insertions(+), 131 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261007-dwc2-low-power-fixes-stm32-0c1e5e68b252

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

[PATCH v3 1/6] 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.
- add a need_restore parameter to dwc2_hsotg_resume(), to re-initialize
  the gadget in case the host has suspended the bus before going to
  low power (in such case dwc2_hsotg_resume was a no-op).

Note that when in host mode, and the registers have been lost, the
HPRT0_CONNSTS bit remains cleared, until the controller gets resumed
in _dwc2_hcd_resume(). So the resume code isn't skipped and the
registers will be restored.

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>
---
Changes in v3:
- Add a note in the commit message since Sashiko comment on
  _dwc2_hcd_resume() check on HPRT0_CONNSTS bit.
- Add need_restore flag and pass it to dwc2_hsotg_resume() to avoid
  possible interrupt race with lx_state update as raised by Sashiko,
  instead of updating lx_state to DWC2_L0.

Changes in v2:
- In case registers have been lost, ensure the device mode resume takes
  place by updating the lx_state to DWC2_L0, as reported by Sashiko. Note
  in case of error in dwc2_core_init() choice is made to leave the low
  level HW init, as it is the expected state out of the resume routine.
---
 drivers/usb/dwc2/core.c     |  1 -
 drivers/usb/dwc2/core.h     |  6 ++---
 drivers/usb/dwc2/gadget.c   |  4 ++--
 drivers/usb/dwc2/platform.c | 58 +++++++++++++++------------------------------
 4 files changed, 23 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 6a0135e06b00..50627c2baf1d 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 015a5fb7842d..1fa872817aa3 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;
@@ -1415,7 +1413,7 @@ void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg);
 	IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 int dwc2_hsotg_remove(struct dwc2_hsotg *hsotg);
 int dwc2_hsotg_suspend(struct dwc2_hsotg *dwc2);
-int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2);
+int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2, bool need_restore);
 int dwc2_gadget_init(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *dwc2,
 				       bool reset);
@@ -1451,7 +1449,7 @@ static inline int dwc2_hsotg_remove(struct dwc2_hsotg *dwc2)
 { return 0; }
 static inline int dwc2_hsotg_suspend(struct dwc2_hsotg *dwc2)
 { return 0; }
-static inline int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2)
+static inline int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2, bool need_restore)
 { return 0; }
 static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
 { return 0; }
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6c8a9f05fff0..99463e65d5b4 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -5200,11 +5200,11 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
-int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
+int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg, bool need_restore)
 {
 	unsigned long flags;
 
-	if (hsotg->lx_state == DWC2_L2)
+	if (!need_restore && hsotg->lx_state == DWC2_L2)
 		return 0;
 
 	if (hsotg->driver) {
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ef0d73077034..b65b9898bac0 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,27 +701,10 @@ 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);
+	bool need_restore;
 	int ret = 0;
 
 	if (!dwc2->ll_hw_enabled)
@@ -742,18 +717,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,8 +749,25 @@ 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.
+	 */
+	need_restore = !(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK);
+	if (need_restore) {
+		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);
+		ret = dwc2_hsotg_resume(dwc2, need_restore);
 
 	return ret;
 }

-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: dwc2: incomplete gadget state cleanup in
  dwc2_hsotg_resume()
- [High] usb: dwc2: incomplete host mode recovery in dwc2_resume() after
  PM domain off
--

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

usb: dwc2: fix recovery after PM domain off

This commit removes the critical register backup/restore calls in
dwc2_suspend() and dwc2_resume(), and instead re-initializes the core on
resume when GUSBCFG_TOUTCAL indicates that the power domain was turned off.

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6c8a9f05fff00..99463e65d5b4b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5200,11 +5200,11 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> -int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
> +int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg, bool need_restore)
>  {
>  	unsigned long flags;
>  
> -	if (hsotg->lx_state == DWC2_L2)
> +	if (!need_restore && hsotg->lx_state == DWC2_L2)
>  		return 0;
>  
>  	if (hsotg->driver) {

[Severity: High]
When the host has already suspended the bus before system suspend
(hsotg->lx_state == DWC2_L2), does resuming with need_restore == true leave
the gadget's software state out of sync with the reset hardware?

Because hsotg->lx_state != DWC2_L0 during dwc2_hsotg_suspend(), the suspend
path returns early without calling dwc2_hsotg_disconnect() or disabling
non-EP0 endpoints, and the L2 entry path has set hsotg->bus_suspended,
hsotg->in_ppd, or hsotg->hibernated.  On resume, dwc2_hsotg_resume() calls
dwc2_hsotg_core_init_disconnected(hsotg, false), which resets the core and
sets hsotg->lx_state = DWC2_L0, but only disables non-EP0 endpoints when
is_usb_reset is true:

drivers/usb/dwc2/gadget.c:dwc2_hsotg_core_init_disconnected() {
    ...
	if (!is_usb_reset) {
		if (dwc2_core_reset(hsotg, true))
			return;
	} else {
		/* all endpoints should be shutdown */
		for (ep = 1; ep < hsotg->num_of_eps; ep++) {
			if (hsotg->eps_in[ep])
				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
			if (hsotg->eps_out[ep])
				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
		}
	}
    ...
}

This wipes the hardware endpoint configuration and hsotg->fifo_map while
leaving non-EP0 endpoints enabled with queued requests and leaving the
low-power flags set (for example, stale hsotg->hibernated == 1 causes
dwc2_handle_common_intr() to ignore interrupts, and stale hsotg->in_ppd == 1
causes dwc2_hsotg_pullup() to restore pre-suspend register backups).

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..b65b9898bac0d 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c

[ ... ]

> @@ -786,8 +749,25 @@ 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.
> +	 */
> +	need_restore = !(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK);
> +	if (need_restore) {
> +		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
> +			ret = dwc2_core_init(dwc2, true);
> +			if (ret)
> +				return ret;
> +			dwc2_enable_global_interrupts(dwc2);
> +		}
> +	}

[Severity: High]
Does calling only dwc2_core_init() and dwc2_enable_global_interrupts() here
break host-mode recovery when hsotg->params.power_down is
DWC2_POWER_DOWN_PARAM_PARTIAL or DWC2_POWER_DOWN_PARAM_HIBERNATION?

dwc2_core_init() only initializes the common core registers and relies on
_dwc2_hcd_resume() to reinitialize the host registers (HCFG, FIFOs,
HPRT0_PWR, host GINTMSK, and GUSBCFG_TOUTCAL).  However, _dwc2_hcd_resume()
only calls dwc2_hcd_reinit() for DWC2_POWER_DOWN_PARAM_NONE:

drivers/usb/dwc2/hcd.c:_dwc2_hcd_resume() {
    ...
	switch (hsotg->params.power_down) {
	case DWC2_POWER_DOWN_PARAM_PARTIAL:
		ret = dwc2_exit_partial_power_down(hsotg, 0, true);
    ...
		break;
	case DWC2_POWER_DOWN_PARAM_HIBERNATION:
		ret = dwc2_exit_hibernation(hsotg, 0, 0, 1);
    ...
}

When no device was connected during _dwc2_hcd_suspend() (!(dwc2_read_hprt0()
& HPRT0_CONNSTS)), _dwc2_hcd_suspend() jumps to skip_power_saving without
backing up registers, so dwc2_exit_partial_power_down() or
dwc2_exit_hibernation() fails with -EINVAL on gr->valid and hcd_bus_resume()
marks the HCD dead via usb_hc_died().  Even when a device was connected in
DWC2_POWER_DOWN_PARAM_PARTIAL mode, on platforms where GOTGCTL_CURMODE_HOST
is not set in GOTGCTL, dwc2_exit_partial_power_down() takes the device
branch and fails with -EINVAL on dr_backup.valid.  Finally, on an OTG
controller that was in device mode during _dwc2_hcd_suspend() (leaving
lx_state != DWC2_L2) and resumes in host mode, _dwc2_hcd_resume() returns
early at hsotg->lx_state != DWC2_L2 without initializing the host registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=1

[PATCH v3 2/6] usb: dwc2: fix a possible interrupt storm in hcd resume

[Fabrice Gasnier]

The host interrupts are re-enabled in dwc2_hcd_reinit(). So there is a
race during resume between a possible hcd IRQ, and the HW accessible bit
to be set. This can result in a bad_irq to be reported (nobody cared).
So set the HW accessible bit before calling dwc2_hcd_reinit().

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>
---
Note it's been found while debugging with additional mdelay after
dwc2_hcd_reinit() call.
---
Changes in v2:
- Introduce this patch since debug session (since sashiko comment on
  patch 1)
---
 drivers/usb/dwc2/hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index d5275cd658d4..28cde15e0cca 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4469,14 +4469,14 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		 */
 		dwc2_core_init(hsotg, false);
 		dwc2_enable_global_interrupts(hsotg);
-		dwc2_hcd_reinit(hsotg);
-		spin_lock_irqsave(&hsotg->lock, flags);
 
 		/*
 		 * Set HW accessible bit before powering on the controller
 		 * since an interrupt may rise.
 		 */
 		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+		dwc2_hcd_reinit(hsotg);
+		spin_lock_irqsave(&hsotg->lock, flags);
 		break;
 	default:
 		hsotg->lx_state = DWC2_L0;

-- 
2.43.0

Re: [PATCH v3 2/6] usb: dwc2: fix a possible interrupt storm in hcd resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc2: unlocked dwc2_hcd_reinit() call in
  _dwc2_hcd_resume()
--

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

usb: dwc2: fix a possible interrupt storm in hcd resume

This commit moves set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags) before
dwc2_hcd_reinit() in _dwc2_hcd_resume() so that host interrupts enabled
during reinitialization are not rejected as unhandled.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index d5275cd658d4c..28cde15e0cca4 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4469,14 +4469,14 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>  		 */
>  		dwc2_core_init(hsotg, false);
>  		dwc2_enable_global_interrupts(hsotg);
> -		dwc2_hcd_reinit(hsotg);
> -		spin_lock_irqsave(&hsotg->lock, flags);
>  
>  		/*
>  		 * Set HW accessible bit before powering on the controller
>  		 * since an interrupt may rise.
>  		 */
>  		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +		dwc2_hcd_reinit(hsotg);
> +		spin_lock_irqsave(&hsotg->lock, flags);
>  		break;

[Severity: Medium]
Should spin_lock_irqsave(&hsotg->lock, flags) be acquired before setting
HCD_FLAG_HW_ACCESSIBLE and calling dwc2_hcd_reinit(hsotg)?

dwc2_hcd_reinit() requires interrupts disabled and hsotg->lock held, as
its only other caller _dwc2_hcd_start() does:

drivers/usb/dwc2/hcd.c:dwc2_hcd_reinit() {
/*
 * Initializes dynamic portions of the DWC_otg HCD state
 *
 * Must be called with interrupt disabled and spinlock held
 */
static void dwc2_hcd_reinit(struct dwc2_hsotg *hsotg)
}

With HCD_FLAG_HW_ACCESSIBLE now set before dwc2_hcd_reinit(hsotg) while
interrupts are enabled and hsotg->lock is unlocked, a host interrupt can
enter usb_hcd_irq() -> _dwc2_hcd_irq() -> dwc2_handle_hcd_intr() (or a
concurrent caller can take hsotg->lock) while dwc2_hcd_reinit() is
modifying hsotg->flags.d32, hsotg->free_hc_list, channel states, and host
registers:

_dwc2_hcd_resume()
  dwc2_enable_global_interrupts(hsotg)
  set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)
  dwc2_hcd_reinit(hsotg)
    list_del_init(&chan->hc_list_entry)
    <hardirq>
      usb_hcd_irq()
        _dwc2_hcd_irq()
          dwc2_handle_hcd_intr()  <-- races on hsotg state and lists
  spin_lock_irqsave(&hsotg->lock, flags)

Neither set_bit() nor dwc2_hcd_reinit() sleeps, so acquiring hsotg->lock
before enabling global interrupts, setting HCD_FLAG_HW_ACCESSIBLE, and
calling dwc2_hcd_reinit() would avoid this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=2

[PATCH v3 3/6] 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>
---
Changes in v3:
- Set HCD_FLAG_HW_ACCESSIBLE only when dwc2_exit_partial_power_down()
  succeeds.

Changes in v2:
- Add check on hcd before setting HCD_FLAG_HW_ACCESSIBLE flag, to avoid a
  NULL pointer dereference on hcd in peripheral-only mode as raised by
  Sashiko.
- Add same handling in PPD mode as raised by Sashiko. Only handle clock
  gating and partial power down mode (PPD) as pre-existing wakeup handler
  manages these two modes. I don't have Hibernation capable hardware to
  cross-check this mode.
---
 drivers/usb/dwc2/core_intr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 9565e6a52725..df9f6b963fb8 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -453,18 +453,25 @@ 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);
 				if (ret)
 					dev_err(hsotg->dev,
 						"exit partial_power_down failed\n");
+				else if (hcd)
+					set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 			}
 
 			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);
+				if (hcd)
+					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 v3 3/6] usb: dwc2: fix a possible interrupt storm on stm32mp1

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=3

[PATCH v3 4/6] 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>
---
Changes in v2:
- Add a vbus_was_off boolean to check explicitly an off-on transition
  on port power, to enable vbus supply, instead of relying an XOR check
  that would be symmetric, e.g. wrongly enable it in case it gets switched
  off (1 to 0) as reported by Sashiko.
---
 drivers/usb/dwc2/hcd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 28cde15e0cca..cae9d7e13d47 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4249,6 +4249,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	struct usb_bus *bus = hcd_to_bus(hcd);
 	unsigned long flags;
+	bool vbus_was_off;
 	u32 hprt0;
 	int ret;
 
@@ -4264,11 +4265,13 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 		return 0;	/* why 0 ?? */
 	}
 
+	vbus_was_off = !(dwc2_read_hprt0(hsotg) & HPRT0_PWR);
+
 	dwc2_hcd_reinit(hsotg);
 
 	hprt0 = dwc2_read_hprt0(hsotg);
 	/* Has vbus power been turned on in dwc2_core_host_init ? */
-	if (hprt0 & HPRT0_PWR) {
+	if (vbus_was_off && (hprt0 & HPRT0_PWR)) {
 		/* Enable external vbus supply before resuming root hub */
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		ret = dwc2_vbus_supply_init(hsotg);

-- 
2.43.0

Re: [PATCH v3 4/6] usb: dwc2: hcd: fix possible successive vbus supply enables

[sashiko-bot]

> 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>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=4

[PATCH v3 5/6] 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).

PM suspend flow doesn't disable vbus while in host mode (neither by
clearing HPRT0_PWR nor by calling dwc2_vbus_supply_exit()). In sequence:
- dwc2_port_suspend() - enters clock gating (set lx_state to DWC2_L2)
- _dwc2_hcd_suspend() - nothing more to do as already in L2.

PM resume flow after wakeup (two cases as registers may have been lost)
- _dwc2_hcd_resume() - checks HPRT0_CONNSTS
1 - if set (registers not lost) - nothing more to do.
  - followed by dwc2_port_resume() - exits clock gating (sets lx_state to
    DWC2_L0)
2 - if NOT set - exits clock gating, re-initializes the controller,
    restores HPRT0_PWR and calls dwc2_vbus_supply_init() unbalanced.

For the 2nd case, it's unbalanced / enabled each time it happens: 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. Moreover, setting/restoring HPRT0_PWR for the
2nd case will simply make the vbus regulator enable count to be aligned
with it.

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>
---
Changes in v3:
- Sashiko comment (state desync leaves VBUS regulator permanently off)
  Add a dwc2_vbus_supply_restore() to check regulator enable state
  during hcd resume, if it has been turned off earlier, enable it,
  so it is kept sync'ed with PWR bit that gets set.

Changes in v2:
- rephrase commit message since Sashiko review. Hopefully it better
  explains the issue, and why HPRT0_PWR is kept synchronized with
  external vbus supply.
---
 drivers/usb/dwc2/hcd.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index cae9d7e13d47..54e3b5698642 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -147,6 +147,20 @@ static int dwc2_vbus_supply_exit(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
+static int dwc2_vbus_supply_restore(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	if (!hsotg->vbus_supply)
+		return 0;
+
+	ret = regulator_is_enabled(hsotg->vbus_supply);
+	if (!ret)
+		return regulator_enable(hsotg->vbus_supply);
+
+	return ret < 0 ? ret : 0;
+}
+
 /**
  * dwc2_enable_host_interrupts() - Enables the Host mode interrupts
  *
@@ -4388,10 +4402,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);
@@ -4500,9 +4510,14 @@ 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);
+	/*
+	 * Restore external vbus supply after resuming the port.
+	 * Keeps vbus_supply in sync with HPRT0_PWR set here after.
+	 */
+	ret = dwc2_vbus_supply_restore(hsotg);
+	if (ret)
+		return ret;
 
 	/* Wait for controller to correctly update D+/D- level */
 	usleep_range(3000, 5000);

-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] usb: dwc2: _dwc2_hcd_resume() overwrites ret and drops
  power-down exit errors
- [Medium] usb: dwc2: hcd: unbalanced vbus_supply enable count in
  dwc2_vbus_supply_restore()
- [Medium] usb: dwc2: unbalanced regulator disable on _dwc2_hcd_resume()
  error
--

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

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

This commit stops disabling the external VBUS supply in _dwc2_hcd_suspend()
and replaces the unconditional dwc2_vbus_supply_init() in
_dwc2_hcd_resume() with dwc2_vbus_supply_restore() to avoid unbalanced
regulator enable counts across system suspend and resume.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cae9d7e13d47d..54e3b5698642f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -147,6 +147,20 @@ static int dwc2_vbus_supply_exit(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> +static int dwc2_vbus_supply_restore(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	if (!hsotg->vbus_supply)
> +		return 0;
> +
> +	ret = regulator_is_enabled(hsotg->vbus_supply);
> +	if (!ret)
> +		return regulator_enable(hsotg->vbus_supply);
> +
> +	return ret < 0 ? ret : 0;
> +}

[Severity: Medium]
Can checking regulator_is_enabled() here desynchronize the consumer enable
count on hsotg->vbus_supply from HPRT0_PWR?

The driver tracks whether this consumer enabled hsotg->vbus_supply via
HPRT0_PWR, calling dwc2_vbus_supply_exit() in _dwc2_hcd_stop() and
ClearPortFeature(USB_PORT_FEAT_POWER) whenever HPRT0_PWR is set.
regulator_is_enabled() queries the physical or shared regulator state rather
than this consumer handle's enable_count, returning 1 for always-on
regulators, regulators without an is_enabled() op, or regulators kept on by
another consumer.

If port power was cleared via ClearPortFeature(USB_PORT_FEAT_POWER) before
suspend, hsotg->vbus_supply has enable_count == 0.  When _dwc2_hcd_resume()
takes the HPRT0_CONNSTS == 0 path, dwc2_vbus_supply_restore() sees
regulator_is_enabled() > 0 and skips regulator_enable(), while
_dwc2_hcd_resume() still sets HPRT0_PWR.  A later _dwc2_hcd_stop() then sees
HPRT0_PWR set and calls regulator_disable() with enable_count == 0,
triggering a kernel warning.  Conversely, if hsotg->vbus_supply remained
enabled across suspend but the physical regulator output dropped so
is_enabled() returns 0, regulator_enable() increments enable_count to 2 and
leaks a reference.

[ ... ]

> @@ -4500,9 +4510,14 @@ 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);
> +	/*
> +	 * Restore external vbus supply after resuming the port.
> +	 * Keeps vbus_supply in sync with HPRT0_PWR set here after.
> +	 */
> +	ret = dwc2_vbus_supply_restore(hsotg);

[Severity: Medium]
Does assigning to ret here overwrite an earlier error from
dwc2_exit_partial_power_down() or dwc2_exit_hibernation()?

Those calls store their failure in ret so _dwc2_hcd_resume() can return it
at the end of the function.  When dwc2_vbus_supply_restore() succeeds, ret
is reset to 0 and the controller resume failure is hidden from
hcd_bus_resume().

> +	if (ret)
> +		return ret;

[Severity: Medium]
If dwc2_vbus_supply_restore() fails and returns early here, can HPRT0_PWR
remain set while hsotg->vbus_supply is still disabled?

Earlier in _dwc2_hcd_resume(), dwc2_hcd_reinit() -> dwc2_core_host_init()
(in the DWC2_POWER_DOWN_PARAM_NONE path) and dwc2_exit_hibernation() ->
dwc2_host_exit_hibernation() already set HPRT0_PWR in HPRT0.  Returning
without clearing HPRT0_PWR leaves it out of sync with hsotg->vbus_supply, so
a subsequent _dwc2_hcd_stop() or ClearPortFeature(USB_PORT_FEAT_POWER) sees
HPRT0_PWR set and calls dwc2_vbus_supply_exit(), causing an unbalanced
regulator_disable().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=5

[PATCH v3 6/6] 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).

In __dwc2_lowlevel_hw_disable() in case the dwc2_stm32_hw_disable() fails,
return the error, without disabling other HW resources to avoid possible
inconsistent state (e.g. usb33d supply not disabled, so other resources
are kept in the same state).

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>
---
Notes:
- In case dwc2_stm32_hw_disable() fails in __dwc2_lowlevel_hw_disable()
  there's nothing to roll back, so return with directly. Alternatively
  the error could be ignored, to continue disablement of other resources.
  Same behavior pre-exists already with phy shutdown/exit/power_off calls
  in case of error. This could be addressed in another patch if needed.
- In host mode with remote wakeup enabled (and a descendant has been
  enabled to wakeup), e.g. phy_off_for_suspend = false the power domain
  isn't expected to be turned off on stm32, so registers shouldn't be lost.
---
Changes in v3:
- roll back in case regulator_disable() fails in dwc2_stm32_hw_disable(),
  as commented by Sashiko.
- Add note since Sashiko review on v2.

Changes in v2:
- fix probe error path in case devm_regulator_get or dwc2_stm32_hw_enable
  fails as raised by Sashiko. Clear activate_stm_id_vb_detection flag so
  the dwc2_stm32_hw_disable() routine won't call regulator_disable()
  respectively with the ERR_PTR returned or unbalanced.
- update commit message to describe error handling on HW disable.
---
 drivers/usb/dwc2/platform.c | 182 ++++++++++++++++++++++++++------------------
 1 file changed, 107 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index b65b9898bac0..d61b088cbc26 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -91,6 +91,89 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
+static void dwc2_stm32_hw_activate_id_vb_detection(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+
+	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);
+}
+
+static int dwc2_stm32_hw_enable(struct dwc2_hsotg *hsotg)
+{
+	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;
+
+	dwc2_stm32_hw_activate_id_vb_detection(hsotg);
+
+	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) {
+		dwc2_stm32_hw_activate_id_vb_detection(hsotg);
+		dwc2_force_dr_mode(hsotg);
+		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 +212,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 +263,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 +447,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 +639,26 @@ 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);
+			/* skips low level dwc2_stm32_hw_disable() in case of error */
+			hsotg->params.activate_stm_id_vb_detection = 0;
 			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");
+			hsotg->params.activate_stm_id_vb_detection = 0;
+			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 +728,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 +749,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);
@@ -717,31 +774,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 v3 6/6] usb: dwc2: platform: fix stm32mp15 detection logic power management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc2: NULL deref in dwc2_stm32_hw_disable() on
  dwc2_init_params() error
--

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

usb: dwc2: platform: fix stm32mp15 detection logic power management

This commit moves STM32MP15 ID/VBUS detection logic enable and disable
handling into __dwc2_lowlevel_hw_enable() and __dwc2_lowlevel_hw_disable()
so the detection resources follow the low-level hardware power state.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index b65b9898bac0d..d61b088cbc265 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c

[ ... ]

> @@ -541,33 +639,26 @@ 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);
> +			/* skips low level dwc2_stm32_hw_disable() in case of error */
> +			hsotg->params.activate_stm_id_vb_detection = 0;
>  			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");
> +			hsotg->params.activate_stm_id_vb_detection = 0;
> +			dev_err_probe(hsotg->dev, retval, "dwc2_stm32_hw_enable failed\n");
>  			goto error;
>  		}

[Severity: Medium]
Can a failure in dwc2_init_params() right before this block cause a NULL
pointer dereference on hsotg->usb33d in regulator_disable()?

In dwc2_init_params(), set_params(hsotg) sets
hsotg->params.activate_stm_id_vb_detection = true on STM32MP15 before
calling dwc2_limit_speed(hsotg):

drivers/usb/dwc2/params.c:dwc2_init_params() {
    ...
	if (set_params) {
		set_params(hsotg);
	} else {
    ...
	}

	ret = dwc2_limit_speed(hsotg);
	if (ret)
		return ret;
    ...
}

If dwc2_limit_speed() fails, dwc2_driver_probe() jumps to error before
devm_regulator_get() runs, and dwc2_lowlevel_hw_disable() ->
dwc2_stm32_hw_disable() passes hsotg->usb33d == NULL to
regulator_disable().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=6