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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mxl-gpy: missing regulator_enable() for VDDP supply
- [High] phy: mxl-gpy: probe regression on missing vddp-supply
--

commit a90a687bf14a0ab1f83d612c5413b01b74244ef5
Author: Robert Marko <robert.marko@sartura.hr>

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

This commit introduces VDDP supply handling for the MxL86211C PHY. It
fetches the supply voltage to properly configure the MDIO, MDC, and MDINT
pads for 1.8V or 3.3V operation.

> 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
[ ... ]
> @@ -390,6 +396,61 @@ static int gpy_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int mxl86211c_configure_mdio_voltage(struct phy_device *phydev,
> +					    struct regulator *vddp)
> +{
> +	int voltage;
> +	int val;
> +
> +	voltage = regulator_get_voltage(vddp);
> +	if (voltage < 0)
> +		return voltage;

[Severity: High]
If the device tree does not provide a vddp-supply, devm_regulator_get()
will return a dummy regulator. Calling regulator_get_voltage() on this dummy
regulator returns -EINVAL, which propagates up and aborts the probe. Will
this break backward compatibility on existing boards that do not define a
vddp-supply?

> +
> +	switch (voltage) {
> +	case 1800000:
> +		val = VSPEC1_PM_CTRL_MDIO_VOL;
> +		break;
> +	case 3300000:
> +		val = 0;
> +		break;
> +	default:
> +		phydev_err(phydev, "unsupported VDDP voltage: %d\n", voltage);
> +		return -EINVAL;
> +	}
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_PM_CTRL,
> +			      VSPEC1_PM_CTRL_MDIO_VOL, val);
> +}
> +
> +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");

[Severity: High]
The VDDP supply is acquired here, but it appears regulator_enable() is never
called. Because the regulator's use-count remains at zero, won't the
regulator core disable the VDDP supply during late init (via
regulator_disable_unused()) unless it is marked as regulator-always-on? 

Disabling this supply would cut power to the PHY's MDIO pads and render the
network interface unusable.

> +	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;
> +
> +	ret = gpy_probe(phydev);
> +	if (ret)
> +		return ret;
> +
> +	priv = phydev->priv;
> +	priv->vddp = vddp;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016112033.949026-1-robert.marko@sartura.hr?part=2