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

[sashiko-bot@kernel.org]

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