Re: [PATCH v6 08/10] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The return value of `regulator_is_enabled()` can be a negative error code (e.g., `-EIO`), but it is evaluated as a boolean `if (regulator_is_enabled(channel->vbus))`. This incorrectly treats errors as an "enabled" state, registering an inappropriate cleanup action.
- [High] The logic relies on a hardware state query (`regulator_is_enabled`) to balance a software reference count (`regulator_enable`), which can silently skip cleanup and trigger a kernel `WARN_ON`.

Pre-existing issues:
- [High] The patch fails to resolve the `WARN_ON` for an unbalanced `enable_count` when the regulator is both initially physically enabled (e.g., by the bootloader) AND explicitly enabled by the driver. It also introduces a leak on the error path.
--

--- Patch [8]: [PATCH v6 08/10] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator ---
commit 435f169ade89ebb67ba9731bcbcda0ddecbbf95d
Author: Biju Das <[email protected]>
phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

This patch modifies the device managed action registration for the VBUS
regulator disable action. It attempts to ensure the cleanup action is
registered based on the actual enabled state of the regulator rather than
only when an explicit enable is requested.

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index d06fb52ed5f1b..ef38c3b365d42 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -905,15 +905,17 @@ static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_ge
>  	if (IS_ERR(channel->vbus))
>  		return PTR_ERR(channel->vbus);
>  
> -	if (!enable)
> -		return 0;
> +	if (enable) {
> +		ret = regulator_enable(channel->vbus);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
This is a pre-existing issue, but does this logic properly account for a
regulator that is already physically enabled when
devm_regulator_get_exclusive() is called?

If the regulator is initially on (e.g., left on by the bootloader), the
consumer inherits this state and enable_count is initialized to 1. If this
code then explicitly calls regulator_enable(), the count increments to 2.
Because only a single devm_add_action_or_reset() is registered later in
this function, the count will only drop from 2 to 1 on device removal,
which would trigger the very WARN_ON(regulator->enable_count) this patch is
intended to fix.

Additionally, if regulator_enable() fails and returns early here, it
bypasses the cleanup registration entirely. This would permanently leak the
inherited enable_count of 1 and trigger a WARN_ON during the probe failure
unwind path.

> -	ret = regulator_enable(channel->vbus);
> -	if (ret)
> -		return ret;
> +	if (regulator_is_enabled(channel->vbus))

[Severity: High]
Can regulator_is_enabled() return a negative error code (such as -EIO)?

If it returns an error, this condition will evaluate to true. The driver
would then incorrectly register the disable action even though the software
enable_count might be 0. During unbind, executing regulator_disable() will
underflow the enable_count and trigger a WARN_ON in the regulator core.

[Severity: High]
Are there scenarios where relying on a hardware state query to balance a
software reference count misses transient hardware states or delays?

If the driver explicitly requests enable = true, regulator_enable() will
successfully increment the software reference counter. However, if the
hardware has a delayed ramp-up or temporarily reports 0 when
regulator_is_enabled() is called, this check will fail. The driver will
skip registering the disable action, which leaks the software enable_count
and triggers a WARN_ON on unbind.

> +		return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> +						channel->vbus);
>  
> -	return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> -					channel->vbus);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy