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

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

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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@bp.renesas.com>
---
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


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

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

[sashiko-bot]

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

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

[Lad, Prabhakar]

Hi,

On Wed, Oct 7, 2026 at 8:20 PM <sashiko-bot@kernel.org> wrote:
>
> 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?
>
If there is intermittent I2C communication that would mean the system
is already in a bad state. Handling such a case will just add more
dead code!

> 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?
>
As mentioned in the comment above the regulators currently used in the
SoCs implement enable/disable callbacks; adding these checks would
mean adding more dead code.

Cheers,
Prabhakar

> 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

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@bp.renesas.com>
> 
> 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@bp.renesas.com>

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

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
> 

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