[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 at 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 at 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