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

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.

[ ... ]

[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