Re: [PATCH net-next 2/9] net: phy: dp83867: add regulator supply management

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] The supplies are enabled from the PHY driver's ->probe callback,
  which is structurally too late for the use case the commit message…
- [Medium] dp83867_power_on() (drivers/net/phy/dp83867.c) treats a 0
  return from devm_regulator_get_enable_optional() as "this rail was…
- [Medium] dp83867_power_on() propagates every non--ENODEV error out of
  dp83867_probe(), including -EPROBE_DEFER, which…

--

net: phy: dp83867: add regulator supply management

This adds dp83867_power_on(), called from dp83867_probe(), which walks four
supply names, enables each one via devm_regulator_get_enable_optional(),
and sleeps 200 ms if any of them returned success.

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 88255e92b4cdb..e03b39fd080d1 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -719,9 +720,40 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +	static const char * const supply_names[] = {
> +		"vdda-2p5", "vdd-1p0", "vdda-1p8", "vddio",
> +	};
> +	struct device *dev = &phydev->mdio.dev;
> +	u32 count = 0;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(supply_names); i++) {
> +		ret = devm_regulator_get_enable_optional(dev, supply_names[i]);
> +		if (!ret)
> +			count++;
> +		else if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "failed to enable %s supply\n",
> +					     supply_names[i]);
> +	}

[Severity: Medium]
This propagates -EPROBE_DEFER out of dp83867_probe(), which
devm_regulator_get_enable_optional() returns when the phandle resolves but
the regulator provider has not registered yet.

Can that deferral end up with genphy bound instead?  In
phy_attach_direct(), if the MAC attaches the PHY while the dp83867 probe is
still deferred:

drivers/net/phy/phy_device.c:phy_attach_direct() {
	...
	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;

		phydev->is_genphy_driven = 1;
	}
	...
	if (phydev->is_genphy_driven) {
		err = d->driver->probe(d);
		if (err >= 0)
			err = device_bind_driver(d);
	...
}

Once genphy is bound, does the deferred retry still get a chance to bind
dp83867, or does __device_attach() return early because dev->driver is
already set?  If it is the latter, the port links with none of the
DP83867-specific setup (RGMII internal tx/rx delays, tx/rx FIFO depth, IO
impedance, clk-output-sel, rxctrl strap quirk) and nothing is logged.

Would it be worth mentioning the new probe-deferral point in the commit
message?

> +
> +	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
> +	if (count)
> +		fsleep(200000);
> +
> +	return 0;
> +}

[Severity: Medium]
Does count actually track "newly enabled" supplies?  The commit message says

  "Absent supplies are silently skipped, so boards that do not describe
  them are unaffected."

and

  "When any supply is newly enabled the driver sleeps for 200 ms"

but with CONFIG_REGULATOR=n the stub never returns -ENODEV:

include/linux/regulator/consumer.h {
	static inline int devm_regulator_get_enable_optional(struct device *dev,
							     const char *id)
	{
		return 0;
	}
}

so count becomes 4 on every probe, including boards that describe no
supplies at all, and fsleep(200000) takes the msleep() branch:

include/linux/delay.h:fsleep() {
	...
	else
		msleep(DIV_ROUND_UP(usecs, USEC_PER_MSEC));
}

The driver has no depends on REGULATOR and no IS_ENABLED(CONFIG_REGULATOR)
guard, so isn't this a plain 200 ms stall per DP83867 probe for all existing
users?

The CONFIG_REGULATOR=y case looks similar for an already-on rail:

drivers/regulator/devres.c:_devm_regulator_get_enable() {
	r = _devm_regulator_get(dev, id, get_type);
	if (IS_ERR(r))
		return PTR_ERR(r);

	ret = regulator_enable(r);
	...
}

regulator_enable() is reference counted and returns 0 for a rail already
enabled by the bootloader, by regulator-always-on/boot-on, or by another
consumer, so no ramp happens yet the 200 ms "post power-up stabilization"
delay is still paid.  Should the delay instead be gated on something that
reflects an actual off to on transition?

>  static int dp83867_probe(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867;
> +	int ret;
> +
> +	ret = dp83867_power_on(phydev);
> +	if (ret)
> +		return ret;

[Severity: High]
Can the rails ever be enabled from here on the boards this targets?  The
commit message describes boards that "use GPIO-controlled regulators for the
DP83867 power rails", but drv->probe() only runs after the PHY has already
answered MDIO ID reads during bus enumeration:

of_mdiobus_register()
  __of_mdiobus_parse_phys()
    of_mdiobus_register_phy()
      fwnode_mdiobus_register_phy()
        get_phy_device()
          get_phy_c22_id()

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy() {
	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
		phy = get_phy_device(bus, addr, is_c45);
	else
		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
	...
}

Unless the DT child carries an explicit ethernet-phy-idAAAA.BBBB compatible,
an unpowered DP83867 does not answer and the address is simply skipped:

drivers/net/mdio/of_mdio.c:__of_mdiobus_parse_phys() {
	...
	if (rc == -ENODEV)
		dev_err(&mdio->dev,
			"MDIO device at address %d is missing.\n",
			addr);
	...
}

There is no deferred retry for that address, so dp83867_probe() and
dp83867_power_on() never run and the GPIO regulator is never enabled.

For the case where the PHY is found (rail already on at boot, or the ID
hard-coded in DT), reset release and register writes happen before the
supplies are touched:

drivers/net/phy/phy_device.c:phy_device_register() {
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

	/* Run all of the fixups for this PHY */
	err = phy_scan_fixups(phydev);
	...
}

drivers/net/phy/phy_device.c:phy_probe() {
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

	if (phydev->drv->probe) {
		err = phydev->drv->probe(phydev);
	...
}

Nothing in the new code re-asserts reset or re-applies configuration after
the rails stabilise, so does the part end up programmed while unpowered and
then left at POR/strap defaults?

Grepping for regulator under drivers/net/mdio/ and in
drivers/net/phy/mdio_bus.c turns up only the mdio-sun4i controller's own
supply, so should rail control for a device that must answer MDIO during
enumeration be handled before/at bus registration, or the rails described as
regulator-always-on/boot-on in DT, rather than in the PHY driver probe?

>  
>  	dp83867 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83867),
>  			       GFP_KERNEL);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-shikra_ethernet-v1-0-a50765996035%40oss.qualcomm.com