Re: [PATCH v4 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:
- [Medium] usb: dwc2: premature IRQ enable and incomplete cleanup in
  device resume
- [Low] usb: dwc2: stale commit message note on HPRT0_CONNSTS in host
  resume
--

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

usb: dwc2: fix recovery after PM domain off

This commit removes the critical register backup from dwc2_suspend() and
instead reinitializes the DWC2 controller, HCD, and gadget state during
dwc2_resume() when GUSBCFG.TOUTCAL indicates that registers were lost.

> Host resume observes HPRT0_CONNSTS cleared after register loss, so it
> does not skip the recovery path. Ordinary resume and USB-reset behavior
> remain unchanged.

[Severity: Low]
Is this note about HPRT0_CONNSTS still accurate?

dwc2_hcd_restore() now sets hsotg->lx_state = DWC2_L0 during platform
resume, so when _dwc2_hcd_resume() runs later it returns early at the
hsotg->lx_state != DWC2_L2 check before reading HPRT0:

drivers/usb/dwc2/hcd.c:_dwc2_hcd_resume() {
    ...
	if (hsotg->lx_state != DWC2_L2)
		goto unlock;

	hprt0 = dwc2_read_hprt0(hsotg);
    ...
	if (hprt0 & HPRT0_CONNSTS) {
		hsotg->lx_state = DWC2_L0;
		goto unlock;
	}
    ...
}

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6c8a9f05fff00..225442471aa95 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5200,11 +5200,12 @@ 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;
> +	int ep;
>  
> -	if (hsotg->lx_state == DWC2_L2)
> +	if (!need_restore && hsotg->lx_state == DWC2_L2)
>  		return 0;
>  
>  	if (hsotg->driver) {
> @@ -5212,6 +5213,19 @@ int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
>  			 hsotg->driver->driver.name);
>  
>  		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (need_restore) {
> +			hsotg->hibernated = 0;
> +			hsotg->in_ppd = 0;
> +			hsotg->bus_suspended = false;
> +			dwc2_hsotg_disconnect(hsotg);
> +			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);
> +			}
> +			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +		}
>  		dwc2_hsotg_core_init_disconnected(hsotg, false);
>  		if (hsotg->enabled) {
>  			/* Enable ACG feature in device mode,if supported */

[ ... ]

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..72214593f4312 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c

[ ... ]

> @@ -786,8 +749,31 @@ 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) {
> +			if (dwc2_is_device_mode(dwc2)) {
> +				ret = dwc2_core_init(dwc2, true);
> +				if (ret)
> +					return ret;
> +				dwc2_enable_global_interrupts(dwc2);
> +			} else {
> +				ret = dwc2_hcd_restore(dwc2);
> +				if (ret)
> +					return ret;
> +			}
> +		}
> +	}
> +
>  	if (dwc2_is_device_mode(dwc2))
> -		ret = dwc2_hsotg_resume(dwc2);
> +		ret = dwc2_hsotg_resume(dwc2, need_restore);
>  
>  	return ret;
>  }

[Severity: Medium]
Can enabling global interrupts here race with the state cleanup in
dwc2_hsotg_resume()?

dwc2_core_init() unmasks GINTMSK_COMMON (including GINTSTS_USBSUSP and
GINTSTS_WKUPINT), and dwc2_enable_global_interrupts() sets
GAHBCFG_GLBL_INTR_EN before dwc2_hsotg_resume() takes hsotg->lock and
clears hsotg->hibernated, hsotg->in_ppd, hsotg->bus_suspended, and
hsotg->connected.  In addition, dwc2_hsotg_disconnect() drops and
reacquires hsotg->lock before dwc2_hsotg_core_init_disconnected()
reinitializes the device registers:

dwc2_resume()
  dwc2_core_init(dwc2, true)
  dwc2_enable_global_interrupts(dwc2)
  <hardirq>
    dwc2_handle_common_intr()
      ...
        dwc2_handle_usb_suspend_intr()
          dwc2_gadget_enter_clock_gating()  <-- sees connected == 1, gates clk
  dwc2_hsotg_resume(dwc2, true)
    spin_lock_irqsave(&hsotg->lock, flags)
    ...
    dwc2_hsotg_ep_disable()
      dwc2_readl(hsotg, DIEPCTL(index))     <-- hangs with clock gated

If GINTSTS_USBSUSP fires while hsotg->connected is still 1 and DSTS_SUSPSTS
is set after reset, dwc2_handle_usb_suspend_intr() enters clock gating or
partial power down right before dwc2_hsotg_ep_disable() accesses endpoint
registers, hanging the platform.  Similarly, an early wakeup or common
interrupt while hsotg->hibernated or hsotg->in_ppd is still 1 will run the
hibernation or partial-power-down exit paths using stale register backups.

Also, unlike dwc2_hcd_restore(), the device-mode restore path does not
clear gr_backup.valid, dr_backup.valid, or hr_backup.valid, and if
hsotg->driver is NULL, dwc2_hsotg_resume() skips the need_restore cleanup
entirely, leaving hibernated, in_ppd, bus_suspended, and lx_state stale.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=1