Re: [PATCH v2 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: missing host registers init on resume
- [Medium] usb: dwc2: race condition in dwc2_resume between global interrupt enable and lx_state update
--

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

usb: dwc2: fix recovery after PM domain off

This commit addresses a freeze during platform low power transitions
by modifying how registers are recovered after the PM domain turns off.
Instead of backing up registers that cause hangs during clock gating,
it re-initializes the core via dwc2_core_init() and restores the canary
detection logic later in the resume sequence.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..5aa5526720783 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
[ ... ]
> @@ -786,6 +748,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.
> +	 */
> +	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;

[Severity: High]
Does this adequately initialize the host registers? dwc2_core_init()
only performs global initialization and doesn't configure host-specific
registers like HCFG, FIFO sizing, or the GUSBCFG_TOUTCAL canary.

While _dwc2_hcd_resume() does call dwc2_core_host_init() via
dwc2_hcd_reinit(), it unconditionally skips this step if the PHY
detects a connected device:

drivers/usb/dwc2/hcd.c:_dwc2_hcd_resume() {
    ...
    hprt0 = dwc2_read_hprt0(hsotg);

    /*
     * Added port connection status checking which prevents exiting from
     * Partial Power Down mode from _dwc2_hcd_resume() if not in Partial
     * Power Down mode.
     */
    if (hprt0 & HPRT0_CONNSTS) {
        set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
        hsotg->lx_state = DWC2_L0;
        goto unlock;
    }
    ...
}

Will this cause host registers to never be initialized if a device is
connected during resume, potentially breaking host mode?

Also, since the GUSBCFG_TOUTCAL canary is never restored to 7, will
subsequent resumes repeatedly misinterpret the canary as missing and trigger
redundant resets?

> +			dwc2_enable_global_interrupts(dwc2);
> +		}
> +		/* Also ensure device mode resume takes place in dwc2_hsotg_resume() */
> +		if (dwc2_is_device_mode(dwc2))
> +			dwc2->lx_state = DWC2_L0;

[Severity: Medium]
Can this introduce a race condition? Global interrupts are unmasked before
the lockless update of lx_state to DWC2_L0.

If an interrupt fires immediately, the handler (dwc2_handle_common_intr)
could take the lock and read a stale DWC2_L2 state for dwc2->lx_state.
Could this lead to incorrect state transitions or a controller freeze?

> +	}
> +
>  	if (dwc2_is_device_mode(dwc2))

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