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

[netdev-bot+sashiko]

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

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

- [High] The rail enable sequence is placed in the PHY driver's probe
  (dp83867_probe() -> dp83867_power_on(),…
- [Medium] In dp83867_power_on() (drivers/net/phy/dp83867.c:723-748) the
  variable `count` is incremented for every return value of 0 from…
- [Low] dp83867_power_on() wraps its entire body in `#ifdef CONFIG_OF`
  (drivers/net/phy/dp83867.c:725-746) although nothing inside uses an OF…

--

net: phy: dp83867: add regulator supply management

This adds dp83867_power_on(), called from dp83867_probe(), which walks four
supply names with devm_regulator_get_enable_optional() and, if any of them
returned success, sleeps 200 ms for the datasheet post power-up
stabilisation.  A few questions about the new function below.

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

[ ... ]

> @@ -719,9 +720,41 @@ static int dp83867_resume(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int dp83867_power_on(struct phy_device *phydev)
> +{
> +#ifdef CONFIG_OF

[Severity: Low]
Is CONFIG_OF the right predicate here?  Nothing inside the guard uses an OF
API, only devm_regulator_get_enable_optional(), dev_err_probe() and
fsleep().

On CONFIG_OF=n the whole body disappears and dp83867_power_on() becomes
"return 0", so supplies that the regulator core would resolve through the
non-DT lookup path (regulator_lookup_by_name() in _regulator_get(), taken
when dev_of_node(dev) is NULL) are never enabled, and errors including
-EPROBE_DEFER are never propagated.

The rest of this file uses #if IS_ENABLED(CONFIG_OF_MDIO) for its genuinely
OF-dependent code (dp83867_of_init_io_impedance()).  Would keying this on
the regulator API instead, or dropping the guard entirely since the
!CONFIG_REGULATOR stub already makes the calls harmless, be more accurate?

> +	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]);
> +	}
> +
> +	/* Datasheet section 6.6 suggests a 200ms post power-up stabilization */
> +	if (count)
> +		fsleep(200000);
> +#endif

[Severity: Medium]
Can count reliably mean "a rail was newly powered up" here?

With CONFIG_REGULATOR=n the API is a stub that can never return -ENODEV:

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

so all four lookups "succeed", count == 4, and every DP83867 probe pays
fsleep(200000), which for usecs >= USLEEP_RANGE_UPPER_BOUND lands in
msleep(200).  DP83867_PHY in drivers/net/phy/Kconfig has no
depends on REGULATOR, so this is a valid configuration.

With CONFIG_REGULATOR=y, _devm_regulator_get_enable() in
drivers/regulator/devres.c returns the result of regulator_enable():

	ret = regulator_enable(r);
	if (!ret)
		ret = devm_add_action_or_reset(dev, &regulator_action_disable, r);

and regulator_enable() also returns 0 for a rail that is already on
(always-on/boot-on, enabled by the bootloader, or shared with another
consumer), so the 200 ms wait is taken on every probe or re-bind merely
because the rails are described in DT, with no off->on transition.

Both cases seem to disagree with the commit message:

> 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 before
> returning.

Should the delay instead be gated on something that actually distinguishes
an off->on transition?

> +	return 0;
> +}
> +
>  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 this call site ever power up a DP83867 whose rails are off at boot,
which is the board class the commit message describes?

phy_driver->probe() runs only after the PHY has been identified over MDIO.
For a DT node without an explicit "ethernet-phy-idXXXX.YYYY" compatible,
fwnode_mdiobus_register_phy() takes the scanning path:

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);
}

and get_phy_device() -> get_phy_c22_id() gives up on an unpowered PHY:

drivers/net/phy/phy_device.c:get_phy_c22_id() {
	/* If the phy_id is mostly Fs, there is no device there */
	if ((*phy_id & 0x1fffffff) == 0x1fffffff)
		return -ENODEV;
}

With the rails off no phy_device with the TI ID is created, so this driver
never binds and dp83867_power_on() never runs.  In the case where the driver
does bind after a scan, the rails were already on, so the enable is a no-op
and the 200 ms is dead time on every bind.  Would this power-up need to
happen before MDIO identification (MDIO bus / reset-and-power stage) to have
the intended effect?  The only configuration where the new code can power a
dark PHY appears to be a DT node carrying an explicit phy-id compatible so
the ID read is skipped, and the binding example added earlier in the series
in Documentation/devicetree/bindings/net/ti,dp83867.yaml uses the scanned
form.

There is a second consequence of using the devm form here.
devm_regulator_get_enable_optional() installs regulator_action_disable() via
devres, so unbinding the PHY driver, or a later failure in dp83867_probe(),
disables these rails while the mdio device stays registered.  Does that
leave subsequent register access (generic PHY fallback, re-scan, ethtool)
talking to a powered-off device?

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

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