Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: Keep OTG VBUS disabled until role is known

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: renesas: Probe fails with -EOPNOTSUPP for generic VBUS regulators
- [Medium] phy: renesas: dangling regulator enable reference on regulator_is_enabled failure
--

commit a95e43cc86d79f16f418ebb877687c2d3024260e
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

phy: renesas: rcar-gen3-usb2: Keep OTG VBUS disabled until role is known

This commit addresses an issue on SoCs without ADP control where the VBUS
regulator was unconditionally enabled at probe, potentially back-powering an
external host. It reuses the reference handed over by an exclusive get if the
regulator is already enabled, registers a devm cleanup action, and uses
regulator_hardware_enable() to turn off the physical VBUS output until the
port role is known.

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index b5ba751805aae..3481505dea2e8 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
[ ... ]
> @@ -1133,17 +1132,35 @@ static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_ge
>  		return 0;
>  	}
>  
> -	if (enable) {
> +	ret = regulator_is_enabled(channel->vbus);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
If devm_regulator_get_exclusive() acquired a regulator that was already
enabled (e.g. by the bootloader), the regulator core implicitly sets the
enable_count to 1. 

If regulator_is_enabled() then fails due to an intermittent I2C communication
error and we return early here, does this leak the enable reference since we
bypassed the registration of the devm_add_action_or_reset() disable action
below? 

This unbalanced count could trigger a WARN_ON() in the regulator core during
regulator_put() when devres tears down the probe state.

> +
> +	if (!ret) {
>  		ret = regulator_enable(channel->vbus);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	if (regulator_is_enabled(channel->vbus))
> -		return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> -						channel->vbus);
> +	/*
> +	 * An exclusive get of an enabled regulator already holds a reference, so
> +	 * add a cleanup action to disable it when the device is released.
> +	 */
> +	ret = devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> +				       channel->vbus);
> +	if (ret)
> +		return ret;
>  
> -	return 0;
> +	/*
> +	 * The regulator is enabled only so that the regulator core holds a
> +	 * reference to it. Do not drive VBUS until the port's role is known;
> +	 * rcar_gen3_enable_vbus_ctrl() switches it from then on.
> +	 *
> +	 * The VBUS regulator used on these SoCs implement the .enable/.disable
> +	 * ops, so regulator_hardware_enable() cannot fail with -EOPNOTSUPP here
> +	 * and no special handling for it is needed.
> +	 */
> +	return regulator_hardware_enable(channel->vbus, false);

[Severity: High]
Does this actually fail probe unconditionally with -EOPNOTSUPP for generic
VBUS regulators?

Upstream device trees like r9a07g044c2-hummingboard-ripple.dts use
renesas,rzg2l-usb2-phy (which sets no_adp_ctrl = true) and map vbus-supply to
a generic GPIO regulator-fixed (vbus1).

For these generic regulators, fixed_voltage_ops does not provide explicit
.enable/.disable callbacks since GPIOs are handled internally by the regulator
core. In this case, regulator_hardware_enable() returns -EOPNOTSUPP and would
cause rcar_gen3_phy_usb2_probe() to unconditionally fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007190654.452376-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1

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