Re: [PATCH v4 7/9] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

[sashiko-bot@kernel.org]

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

New issues:
- [High] The condition `if (regulator_is_enabled(channel->vbus))` incorrectly evaluates negative error codes as true, leading to unbalanced disables.

Pre-existing issues:
- [High] If the regulator is both pre-enabled by the bootloader and explicitly enabled by the driver, it leaks an enable_count, triggering a WARN_ON during teardown.
- [High] Missing cleanup for the implicit enable inherited from the bootloader if the explicit regulator_enable() fails.
--

--- Patch [7]: [PATCH v4 7/9] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator ---
commit 3ef9475c853028e498f373a0d82121c2440a2aae
Author: Biju Das <biju.das.jz@bp.renesas.com>

phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

This commit modifies the devm action registration for disabling the VBUS
regulator to rely on regulator_is_enabled() rather than checking if
enable was explicitly 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 early return leak the implicit
enable count if the regulator was pre-enabled by the bootloader?

If the bootloader left the physical regulator enabled,
devm_regulator_get_exclusive() initializes the consumer with
enable_count = 1. If regulator_enable() fails here (e.g., due to an I2C
error), the function returns early without registering a devres disable
action.

When devres unwinds the exclusive get and calls regulator_put(), could this
trigger a WARN_ON(regulator->enable_count) since the implicitly acquired
enable_count is never decremented?

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

[Severity: High]
Could this cause a regression since regulator_is_enabled() can return a
negative error code on failure? Will this condition incorrectly evaluate
negative error codes as true?

If the regulator wasn't pre-enabled (enable_count = 0) and enable is false,
but regulator_is_enabled() fails and returns an error (like -EIO), the
condition evaluates to true and registers the devm disable action.

Upon driver removal, the action would execute regulator_disable(), which
could underflow the count and trigger WARN(regulator->enable_count == 0)
in _regulator_disable(). Should this check explicitly for > 0?

[Severity: High]
This is a pre-existing issue, but if the regulator is both pre-enabled by
the bootloader and explicitly enabled by the driver, does this leak an
enable_count?

When the driver registers its internal regulator, it sets enable = true. If
the bootloader left the physical regulator enabled,
devm_regulator_get_exclusive() implicitly sets enable_count = 1.

The explicit enable path then calls regulator_enable(), incrementing
enable_count to 2.

Since only a single disable action is registered here, during driver
removal it reduces the count to 1. Does the subsequent devm cleanup call to
regulator_put() then trigger a WARN_ON(regulator->enable_count)?

> +		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/20260709083108.108370-1-biju.das.jz@bp.renesas.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy