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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] usb: dwc2: resource leak on dwc2_stm32_hw_disable error
- [High] usb: dwc2: stm32 GGPIO registers not restored on resume in host mode
- [High] usb: dwc2: platform: resource leak and hardware state corruption on regulator_disable failure
- [Medium] usb: dwc2: hardware left in inconsistent state on regulator_disable failure
- [Medium] usb: dwc2: unlocked RMW of GUSBCFG in dwc2_force_mode() races with IRQ handler
--

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

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

This patch moves the STM32 detection logic enable and disable procedures
into dedicated routines. These routines are now called from the low-level
hardware enable and disable functions, ensuring that the detection
registers are properly handled during power management transitions.

Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5aa5526720783..5841efef6b351 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)
[ ... ]
> +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);

[Severity: Medium]
Since dwc2_force_mode() performs an unlocked read-modify-write on the
GUSBCFG register, can this race with a concurrent interrupt handler
(such as dwc2_port_intr() running under hsotg->lock) and silently
overwrite the handler's changes?

[ ... ]
> +	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;

[Severity: Medium]
If regulator_disable() fails and returns an error, the OTG detection
registers GOTGCTL and GGPIO have already been modified. Does this leave
the hardware in a permanently inconsistent state with its OTG detection
broken, since the register modifications are not rolled back on this
error path?

[ ... ]
> @@ -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);

[Severity: High]
If dwc2_stm32_hw_disable() fails (for example, due to the
regulator_disable failure), this early return abruptly halts the
teardown. When this happens during dwc2_driver_remove(), does this
bypass the disablement of the remaining PHY, clock, and bulk regulator
resources, causing a permanent leak?

[ ... ]
> @@ -716,31 +764,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);

[Severity: High]
This STM32 hardware enable logic was previously executed unconditionally
on resume, but is now moved inside __dwc2_lowlevel_hw_enable().

In host mode with remote wakeup enabled, dwc2_host_can_poweroff_phy()
returns false, which skips __dwc2_lowlevel_hw_disable() during suspend
and leaves phy_off_for_suspend = false.

During dwc2_resume(), because phy_off_for_suspend is false,
__dwc2_lowlevel_hw_enable() is skipped entirely.

If the controller's power domain was disabled during suspend (losing
register values), doesn't skipping the STM32 hardware enable leave the
GGPIO register uninitialized, breaking VBUS and ID detection on resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=5