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

[Tommaso Merciai]

Hi Prabhakar,
Thanks for your patch.

On Wed, Oct 07, 2026 at 08:06:54PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> 
> On OTG channels of SoCs without ADP control, the VBUS regulator is
> acquired with devm_regulator_get_exclusive() and its output is switched
> by rcar_gen3_enable_vbus_ctrl() using regulator_hardware_enable() once
> the port role is known.
> 
> The helper acquiring the regulator has the following problems:
> 
> - The internal regulator is unconditionally enabled at probe, so VBUS
>   is driven before the port role is known. If the port is connected to
>   an external host this can back-power it.
> 
> - regulator_get_exclusive() hands the consumer an enable reference if
>   the regulator is already enabled, for example by the bootloader. The
>   unconditional regulator_enable() then takes a second reference while
>   only one is released on unbind, leaving the regulator enabled and
>   triggering the WARN_ON() in _regulator_put().
> 
> Fix this by making sure the driver always holds exactly one reference
> to the regulator: reuse the reference handed over by the exclusive get
> if the regulator is already enabled, and call regulator_enable()
> otherwise. Register the disable action in both cases, then turn off the
> physical VBUS output with regulator_hardware_enable() until the port
> role is established. Enabling a disabled regulator to take the
> reference may briefly drive VBUS during probe; this cannot be avoided
> with the regulator API.
> 
> Rename the regulator helper to reflect that it acquires the regulator
> without directly enabling its physical VBUS output.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>

Tested on RZ/G3E.
Tested-by: Tommaso Merciai <tommaso.merciai.xr@xxxxxxxxxxxxxx>

Kind regards,
Tommaso

> ---
> v1->v2:
> - regulator_enable() is only called if the regulator is not already enabled,
>   ensuring exactly one reference is held.
> - Dropped enable parameter from the function
> - Renamed function rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable()
>   to rcar_gen3_phy_usb2_vbus_regulator_get_exclusive()
> - Updated commit message
> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 35 +++++++++++++++++-------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index b5ba751805aa..3481505dea2e 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -1114,8 +1114,7 @@ static void rcar_gen3_phy_usb2_vbus_disable_action(void *data)
>  	regulator_disable(vbus);
>  }
>  
> -static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_gen3_chan *channel,
> -								  bool enable)
> +static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive(struct rcar_gen3_chan *channel)
>  {
>  	struct device *dev = channel->dev;
>  	int ret;
> @@ -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;
> +
> +	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);
>  }
>  
>  static int rcar_gen3_phy_usb2_vbus_regulator_register(struct rcar_gen3_chan *channel)
> @@ -1151,7 +1168,6 @@ static int rcar_gen3_phy_usb2_vbus_regulator_register(struct rcar_gen3_chan *cha
>  	struct device *dev = channel->dev;
>  	struct regulator_config rcfg = { .dev = dev, };
>  	struct regulator_dev *rdev;
> -	bool enable = false;
>  
>  	rcfg.of_node = of_get_available_child_by_name(dev->of_node,
>  						      "vbus-regulator");
> @@ -1165,10 +1181,9 @@ static int rcar_gen3_phy_usb2_vbus_regulator_register(struct rcar_gen3_chan *cha
>  					     "Failed to create vbus-regulator\n");
>  
>  		channel->otg_internal_reg = true;
> -		enable = true;
>  	}
>  
> -	return rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(channel, enable);
> +	return rcar_gen3_phy_usb2_vbus_regulator_get_exclusive(channel);
>  }
>  
>  static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> -- 
> 2.55.0
>