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

[sashiko-bot@kernel.org]

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