Re: [PATCH v2 4/5] usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: dwc2: state desync leaves VBUS regulator permanently off
--

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

usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

This commit removes the disable and enable calls for the external vbus in
the bus suspend and resume routines. It aims to avoid an unbalanced regulator
enable count that occurs when registers are lost during system suspend.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index c966c08cd7d82..263713ebd4801 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c

[ ... ]

> @@ -4500,10 +4496,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>  		spin_lock_irqsave(&hsotg->lock, flags);
>  	}
>  
> -	/* Enable external vbus supply after resuming the port. */
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> -	dwc2_vbus_supply_init(hsotg);
> -

[Severity: Medium]
Will this leave the physical regulator permanently off if the port power was 
cleared before system suspend?

If the port is powered off by the hub driver before suspend, the external
regulator count is 0 and HPRT0_PWR is 0. During system resume, if controller
registers are lost, _dwc2_hcd_resume() unconditionally sets HPRT0_PWR to 1
further down in this function:

    dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
                    HPRT0_ENACHG, HPRT0);

By removing the dwc2_vbus_supply_init() call here, the physical regulator
remains off, but the HPRT0_PWR bit is forced to 1, causing a state desync.

When the hub driver later tries to power on the port in dwc2_hcd_hub_control(),
it checks the current state:

    case USB_PORT_FEAT_POWER:
        ...
        hprt0 = dwc2_read_hprt0(hsotg);
        pwr = hprt0 & HPRT0_PWR;
        hprt0 |= HPRT0_PWR;
        dwc2_writel(hsotg, hprt0, HPRT0);
        if (!pwr)
            dwc2_vbus_supply_init(hsotg);
        break;

Since HPRT0_PWR is already 1, the check for !pwr evaluates to false, and the
external regulator is silently skipped.

Does this lock the port in an permanently unpowered state where it is unable
to detect or enumerate devices? Could this be avoided by only setting HPRT0_PWR
during resume if the regulator is also being enabled, or by restoring the
exact prior port power state instead of unconditionally setting it?

>  	/* Wait for controller to correctly update D+/D- level */
>  	usleep_range(3000, 5000);
>  	spin_lock_irqsave(&hsotg->lock, flags);

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