Re: [PATCH net-next v2 2/5] net: phy: mxl-gpy: add MxL86211C VDDP supply

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Probe regression for existing MxL86211C users.
- [Medium] mxl86211c_probe() gets the VDDP supply with
  devm_regulator_get(), reads its voltage, programs the MDIO/MDC/MDINT…
- [Low] Baseline decision: invalid (kept only because
  requires_validation=true).
- [Low] Baseline decision: invalid for the final state of the series
  (kept only because requires_validation=true).

--

net: phy: mxl-gpy: add MxL86211C VDDP supply

This commit adds mxl86211c_probe(). It gets the VDDP supply, reads its
voltage and sets the MDIO/MDC/MDINT pad level in VSPEC1_PM_CTRL to 1.8 V
or 3.3 V. The MxL86211C entry in gpy_drivers[] now uses this probe
instead of gpy_probe().

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index 089a1a9e5bc61..fbac7c49b4759 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c

[ ... ]

> +static int mxl86211c_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct gpy_priv *priv;
> +	struct regulator *vddp;
> +	int ret;
> +
> +	vddp = devm_regulator_get(dev, "vddp");
> +	if (IS_ERR(vddp))
> +		return dev_err_probe(dev, PTR_ERR(vddp),
> +				     "failed to get VDDP supply\n");
> +	if (!vddp)
> +		return dev_err_probe(dev, -EOPNOTSUPP,
> +				     "VDDP supply requires regulator support\n");
> +
> +	ret = mxl86211c_configure_mdio_voltage(phydev, vddp);
> +	if (ret)
> +		return ret;

[Severity: High]
Will this break probe for existing MxL86211C users whose firmware has no
vddp-supply property? gpy_probe() has handled the MxL86211C since commit
9d844da693d6. DT nodes written before this series won't have the
property, and neither will PHYs matched only by MDIO ID.

With CONFIG_REGULATOR=y and full constraints, devm_regulator_get() falls
back to the dummy regulator. That regulator has empty ops, no fixed_uV
and no supply, so regulator_get_voltage() reaches the last branch of
regulator_get_voltage_rdev():

drivers/regulator/core.c:regulator_get_voltage_rdev() {
    ...
	} else if (rdev->supply_name) {
		return -EPROBE_DEFER;
	} else {
		return -EINVAL;
	}
    ...
}

mxl86211c_configure_mdio_voltage() passes that -EINVAL back, and
mxl86211c_probe() fails without printing any message.

Without full constraints, devm_regulator_get() returns ERR_PTR(-ENODEV).
With CONFIG_REGULATOR=n, the stub returns NULL, which becomes -EOPNOTSUPP
here. So probe fails in all three cases.

When probe fails, the PHY no longer binds to mxl-gpy and phylib falls
back to genphy/genphy_c45. The PHY then loses the SGMII/2500BASE-X
possible_interfaces setup, in-band AN handling, WoL, LEDs, hwmon and the
SGMII re-AN workarounds.

No in-tree DTS sets vddp-supply for this PHY. Could this use
devm_regulator_get_optional() instead, and leave the hardware/strap
default in place on -ENODEV?

The requirement is still there at the end of the series, because the
later mxl86211c_config_init() also fails when the voltage can't be read.

[Severity: Low]
The earlier binding patch in this series ("dt-bindings: net: maxlinear:
add MxL86211C VDDP supply") documents vddp-supply and makes it required
in maxlinear,gpy2xx.yaml. However, the schema's select only matches nodes
that carry the explicit compatible:

select:
  properties:
    compatible:
      contains:
        const: ethernet-phy-idc133.5400

A PHY node matched only by MDIO ID is not checked against this schema,
but mxl86211c_probe() still requires the supply for it. Should the
binding and the driver agree on which nodes need vddp-supply?

> +
> +	ret = gpy_probe(phydev);
> +	if (ret)
> +		return ret;
> +
> +	priv = phydev->priv;
> +	priv->vddp = vddp;
> +
> +	return 0;
> +}

[Severity: Medium]
Should the VDDP supply be enabled here? mxl86211c_probe() stores the
handle in priv->vddp but never calls regulator_enable() or uses
devm_regulator_get_enable(). As a result, rdev->use_count stays at 0 for
this consumer. No later patch in the series enables it either.

Suppose VDDP is a switchable regulator that is on at boot but not marked
regulator-always-on. Then regulator_late_cleanup() will turn it off:

drivers/regulator/core.c:regulator_late_cleanup() {
    ...
	if (rdev->use_count)
		goto unlock;
    ...
	if (have_full_constraints()) {
		...
		rdev_info(rdev, "disabling\n");
		ret = _regulator_do_disable(rdev);
    ...
}

On a shared rail, another consumer could also turn it off by dropping its
last enable reference. In either case the MDIO/MDC/MDINT pads lose power
while the driver is still bound.

regulator_get_voltage() also works on a disabled regulator, so the read
in mxl86211c_configure_mdio_voltage() does not show that the pads are
powered.

[ ... ]

> @@ -1406,7 +1467,7 @@ static struct phy_driver gpy_drivers[] = {
>  		.name		= "Maxlinear Ethernet MxL86211C",
>  		.get_features	= genphy_c45_pma_read_abilities,
>  		.config_init	= gpy21x_config_init,
> -		.probe		= gpy_probe,
> +		.probe		= mxl86211c_probe,

[Severity: Low]
At this commit, only mxl86211c_probe() sets the pad voltage. .config_init
is still gpy21x_config_init, which doesn't touch VSPEC1_PM_CTRL.

phy_detach() can assert a hardware reset, and phy_init_hw() releases it.
After that only .config_init runs again. Wouldn't VSPEC1_PM_CTRL_MDIO_VOL
then go back to its hardware default?

The next patch in the series, "net: phy: mxl-gpy: release MxL86211C
Super Isolate", adds mxl86211c_config_init(), which calls
mxl86211c_configure_mdio_voltage() again. So this only matters for
bisection.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016112033.949026-1-robert.marko%40sartura.hr