Re: [PATCH 2/2] usb: dwc3: Keep the host bus suspended, not torn down, across suspend-to-idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: dwc3: dwc3_google_suspend() resets core in s2idle when
  wakeup is off
- [High] usb: dwc3: dwc3_host_core_exit_needed() breaks parent glue
  resets in s2idle
- [High] usb: dwc3: device_set_awake_path() fails to keep non-wakeup
  genpd on in s2idle
--

commit 2b3424195227cb5706b93edc71af29a9569a4ddf
Author: Arun Iyengar <aiyengar1990@gmail.com>

usb: dwc3: Keep the host bus suspended, not torn down, across suspend-to-idle

This commit skips tearing down the DWC3 core in host mode during
suspend-to-idle when full reinitialization is not required, and marks the
device as being on the awake path to keep its power domain active.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2b31f146e43fc..8b335ba7cde4d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[ ... ]
> @@ -2539,6 +2540,29 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +/*
> + * In host mode, system suspend suspends the bus: xhci-plat puts the root ports
> + * and with them every attached device into U3/suspend, and the USB core
> + * expects them to keep that power session (see
> + * Documentation/driver-api/usb/persist.rst). Tearing the core down here ends
> + * all power sessions: the devices then have to go through a reset-resume, and
> + * a device that takes longer than the USB core allows to come back (it has to
> + * recover its link from scratch, a hub behind the port is reset and switches
> + * its own ports off) is disconnected, e.g. with a mounted filesystem.
> + *
> + * Only do that when the core will lose its state anyway or the system goes
> + * into a state that powers it down: keep it for wakeup, and for
> + * suspend-to-idle, where nothing is powered off behind the drivers' back.
> + */
> +static bool dwc3_host_core_exit_needed(struct dwc3 *dwc)
> +{
> +	if (dwc->needs_full_reinit)
> +		return true;
> +	if (device_may_wakeup(dwc->dev))
> +		return false;
> +	return pm_suspend_target_state != PM_SUSPEND_TO_IDLE;
> +}

[Severity: High]
Does skipping dwc3_core_exit() in PM_SUSPEND_TO_IDLE break parent glue
drivers that reset or power down the hardware in their own .suspend()
callbacks?

Parent glue drivers that populate a child snps,dwc3 device via
of_platform_populate() bind it to dwc3_probe(), where needs_full_reinit is
false.  During PM_SUSPEND_TO_IDLE, device .suspend() callbacks still run:
for example, dwc3_of_simple_suspend() asserts simple->resets when
simple->need_reset is set (such as on rockchip,rk3399-dwc3),
dwc3_meson_g12a_suspend() powers off and exits the PHYs before pulsing
reset on resume, and dwc3_exynos_suspend() disables the vdd33/vdd10
regulators.

Because dwc3_suspend_common() now leaves host_core_exited as false on
those platforms, dwc3_resume_common() skips dwc3_core_init_for_resume()
and dwc3_set_prtcap() and passes power_lost = false to xhci-plat after the
parent glue driver has reset or power-cycled the controller, leaving the
host controller uninitialized.

> +
>  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	u32 reg;
> @@ -2569,12 +2593,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		dwc3_core_exit(dwc);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
> -		if (!PMSG_IS_AUTO(msg) &&
> -		    (!device_may_wakeup(dwc->dev) || dwc->needs_full_reinit)) {
> +		dwc->host_core_exited = false;
> +		if (!PMSG_IS_AUTO(msg) && dwc3_host_core_exit_needed(dwc)) {
>  			dwc3_core_exit(dwc);
> +			dwc->host_core_exited = true;
>  			break;
>  		}

[Severity: High]
Does dwc3_google_suspend() also need to check !google->dwc.host_core_exited
instead of device_may_wakeup()?

drivers/usb/dwc3/dwc3-google.c:dwc3_google_suspend() {
	if (google->dwc.current_dr_role == DWC3_GCTL_PRTCAP_HOST) {
		/*
		 * Follow dwc3_suspend_common() guidelines for deciding between
		 * a full teardown and hibernation.
		 */
		if (PMSG_IS_AUTO(msg) || device_may_wakeup(google->dev)) {
			...
			return 0;
		}
	}

	reset_control_bulk_assert(google->num_rsts, google->rsts);
	clk_bulk_disable_unprepare(google->num_clks, google->clks);
}

When wakeup is disabled during PM_SUSPEND_TO_IDLE, dwc3_suspend_common()
now keeps the core active and sets host_core_exited to false, but
dwc3_google_suspend() still falls through and asserts google->rsts.  On
resume, dwc3_google_resume() deasserts the resets and dwc3_resume_common()
sees host_core_exited == false, skipping dwc3_core_init_for_resume() and
dwc3_set_prtcap() on a reset controller.

>  
> +		/*
> +		 * The core and its PHYs stay powered to keep the suspended bus:
> +		 * keep the power domain on as well.
> +		 */
> +		if (!PMSG_IS_AUTO(msg))
> +			device_set_awake_path(dwc->dev);

[Severity: High]
What happens on SoCs where the DWC3 power domain does not set
GENPD_FLAG_ACTIVE_WAKEUP?

drivers/pmdomain/core.c:genpd_finish_suspend() {
	if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
	    !device_out_band_wakeup(dev))
		return 0;
	...
	genpd_sync_power_off(genpd, true, 0);
}

genpd_finish_suspend() still runs during PM_SUSPEND_TO_IDLE and only
honors device_awake_path() when the domain sets GENPD_FLAG_ACTIVE_WAKEUP.
On platforms whose DWC3 power domain does not set that flag (for example,
RK3568_PD_PIPE on RK3566/RK3568, where usb_host0_xhci and usb_host1_xhci
bind to dwc3_probe() with needs_full_reinit == false), genpd still stops
clocks and powers off the domain during suspend-to-idle, and
dwc3_resume_common() then skips dwc3_core_init_for_resume() because
host_core_exited is false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-dwc3-s2idle-v1-0-2ef4cc3f8130@gmail.com?part=2