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

[sashiko-bot@kernel.org]

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