Re: [PATCH v3 5/6] 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 3 potential
issues to consider:
- [Medium] usb: dwc2: _dwc2_hcd_resume() overwrites ret and drops
  power-down exit errors
- [Medium] usb: dwc2: hcd: unbalanced vbus_supply enable count in
  dwc2_vbus_supply_restore()
- [Medium] usb: dwc2: unbalanced regulator disable on _dwc2_hcd_resume()
  error
--

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

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

This commit stops disabling the external VBUS supply in _dwc2_hcd_suspend()
and replaces the unconditional dwc2_vbus_supply_init() in
_dwc2_hcd_resume() with dwc2_vbus_supply_restore() to avoid unbalanced
regulator enable counts across system suspend and resume.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cae9d7e13d47d..54e3b5698642f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -147,6 +147,20 @@ static int dwc2_vbus_supply_exit(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> +static int dwc2_vbus_supply_restore(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	if (!hsotg->vbus_supply)
> +		return 0;
> +
> +	ret = regulator_is_enabled(hsotg->vbus_supply);
> +	if (!ret)
> +		return regulator_enable(hsotg->vbus_supply);
> +
> +	return ret < 0 ? ret : 0;
> +}

[Severity: Medium]
Can checking regulator_is_enabled() here desynchronize the consumer enable
count on hsotg->vbus_supply from HPRT0_PWR?

The driver tracks whether this consumer enabled hsotg->vbus_supply via
HPRT0_PWR, calling dwc2_vbus_supply_exit() in _dwc2_hcd_stop() and
ClearPortFeature(USB_PORT_FEAT_POWER) whenever HPRT0_PWR is set.
regulator_is_enabled() queries the physical or shared regulator state rather
than this consumer handle's enable_count, returning 1 for always-on
regulators, regulators without an is_enabled() op, or regulators kept on by
another consumer.

If port power was cleared via ClearPortFeature(USB_PORT_FEAT_POWER) before
suspend, hsotg->vbus_supply has enable_count == 0.  When _dwc2_hcd_resume()
takes the HPRT0_CONNSTS == 0 path, dwc2_vbus_supply_restore() sees
regulator_is_enabled() > 0 and skips regulator_enable(), while
_dwc2_hcd_resume() still sets HPRT0_PWR.  A later _dwc2_hcd_stop() then sees
HPRT0_PWR set and calls regulator_disable() with enable_count == 0,
triggering a kernel warning.  Conversely, if hsotg->vbus_supply remained
enabled across suspend but the physical regulator output dropped so
is_enabled() returns 0, regulator_enable() increments enable_count to 2 and
leaks a reference.

[ ... ]

> @@ -4500,9 +4510,14 @@ 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);
> +	/*
> +	 * Restore external vbus supply after resuming the port.
> +	 * Keeps vbus_supply in sync with HPRT0_PWR set here after.
> +	 */
> +	ret = dwc2_vbus_supply_restore(hsotg);

[Severity: Medium]
Does assigning to ret here overwrite an earlier error from
dwc2_exit_partial_power_down() or dwc2_exit_hibernation()?

Those calls store their failure in ret so _dwc2_hcd_resume() can return it
at the end of the function.  When dwc2_vbus_supply_restore() succeeds, ret
is reset to 0 and the controller resume failure is hidden from
hcd_bus_resume().

> +	if (ret)
> +		return ret;

[Severity: Medium]
If dwc2_vbus_supply_restore() fails and returns early here, can HPRT0_PWR
remain set while hsotg->vbus_supply is still disabled?

Earlier in _dwc2_hcd_resume(), dwc2_hcd_reinit() -> dwc2_core_host_init()
(in the DWC2_POWER_DOWN_PARAM_NONE path) and dwc2_exit_hibernation() ->
dwc2_host_exit_hibernation() already set HPRT0_PWR in HPRT0.  Returning
without clearing HPRT0_PWR leaves it out of sync with hsotg->vbus_supply, so
a subsequent _dwc2_hcd_stop() or ClearPortFeature(USB_PORT_FEAT_POWER) sees
HPRT0_PWR set and calls dwc2_vbus_supply_exit(), causing an unbalanced
regulator_disable().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-dwc2-low-power-fixes-stm32-v3-0-68d2a0e9bab6@foss.st.com?part=5