Re: [net-next,PATCH v2 2/5] net: ethernet: stmmac: stm32: support the phy-supply regulator binding

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] `phy_power_on()` unconditionally returns 0, masking regulator errors, creating dead code, and causing unbalanced disables.
- [Medium] `stm32mp1_suspend` fails to restore the PHY power state if `clk_prepare_enable` fails, leaving the device in an inconsistent state.
- [Medium] `stm32_dwmac_parse_data()` improperly masks real errors from `devm_regulator_get_optional()`, treating them as missing regulators.
--

--- Patch [2]: [net-next,PATCH v2 2/5] net: ethernet: stmmac: stm32: support the phy-supply regulator binding ---
commit 82bd267f7e4d9d57647e83b70366ec6a5796b345
Author: Christophe Roullier <[email protected]>

net: ethernet: stmmac: stm32: support the phy-supply regulator binding

Configure the phy regulator if defined by the "phy-supply" DT phandle.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> index e1b260ed4790b..4597f605ec1bb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c

[ ... ]

> @@ -423,6 +425,16 @@ static int stm32_dwmac_parse_data(struct stm32_dwmac *dwmac,
>  	if (IS_ERR(dwmac->regmap))
>  		return PTR_ERR(dwmac->regmap);
>  
> +	dwmac->regulator = devm_regulator_get_optional(dev, "phy");
> +	if (IS_ERR(dwmac->regulator)) {
> +		if (PTR_ERR(dwmac->regulator) == -EPROBE_DEFER) {
> +			dev_dbg(dev, "phy regulator is not available yet, deferred probing\n");
> +			return -EPROBE_DEFER;
> +		}
> +		dev_dbg(dev, "no regulator found\n");
> +		dwmac->regulator = NULL;
> +	}
> +

[Severity: Medium]
Should this code propagate legitimate errors other than -EPROBE_DEFER and
-ENODEV?

The devm_regulator_get_optional() function returns -ENODEV if the regulator
is absent, but it can also return real errors like -ENOMEM or -EINVAL. By
treating all other errors as "no regulator found" and setting the regulator
to NULL, couldn't this hide actual initialization failures?

>  	if (dwmac->ops->is_mp2)
>  		return 0;

[ ... ]

> @@ -522,6 +534,27 @@ static int stm32_dwmac_resume(struct device *dev, void *bsp_priv)
>  	return stm32_dwmac_init(priv->plat);
>  }
>  
> +static int phy_power_on(struct stm32_dwmac *bsp_priv, bool enable)
> +{
> +	int ret;
> +	struct device *dev = bsp_priv->dev;
> +
> +	if (!bsp_priv->regulator)
> +		return 0;
> +
> +	if (enable) {
> +		ret = regulator_enable(bsp_priv->regulator);
> +		if (ret)
> +			dev_err(dev, "fail to enable phy-supply\n");
> +	} else {
> +		ret = regulator_disable(bsp_priv->regulator);
> +		if (ret)
> +			dev_err(dev, "fail to disable phy-supply\n");
> +	}
> +
> +	return 0;
> +}
> +

[Severity: Medium]
Should this function return ret instead of unconditionally returning 0?

Because it always returns 0, the caller stm32_dwmac_probe() can never see
an error, rendering its if (ret) check dead code. Furthermore, if
regulator_enable() fails, the probe will continue with an unpowered PHY,
and the teardown path might later attempt to disable a regulator that was
never successfully enabled.

[ ... ]

> @@ -614,16 +653,26 @@ static void stm32_dwmac_remove(struct platform_device *pdev)
>  		dev_pm_clear_wake_irq(&pdev->dev);
>  		device_init_wakeup(&pdev->dev, false);
>  	}
> +
> +	phy_power_on(priv->plat->bsp_priv, false);
>  }
>  
>  static int stm32mp1_suspend(struct stm32_dwmac *dwmac)
>  {
> +	/* Keep the PHY up if we use Wake-on-Lan. */
> +	if (!device_may_wakeup(dwmac->dev))
> +		phy_power_on(dwmac, false);
> +
>  	return clk_prepare_enable(dwmac->clk_ethstp);
>  }

[Severity: Medium]
Is there a missing rollback path here if clk_prepare_enable() fails?

If the clock enablement fails, the system suspend is aborted and the PM
core will not call the driver's resume callback. Since the PHY was already
powered off above, wouldn't this leave the device in an inconsistent,
non-functional state with an unpowered PHY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2