Re: [net-next,05/14] net: stmmac: add stmmac core serdes support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: add stmmac core serdes support

This commit adds SerDes PHY support to the stmmac core driver, moving it
from platform glue into the integrated PCS driver. The SerDes is managed
through phy_init/phy_exit for lifecycle and phy_power_on/phy_power_off
for the power state.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> index 2f826fe7229b..4d1902f3a58f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c

[ ... ]

> static int dwmac_integrated_pcs_enable(struct phylink_pcs *pcs)
> {
> 	struct stmmac_pcs *spcs = phylink_pcs_to_stmmac_pcs(pcs);
> +	struct stmmac_priv *priv = spcs->priv;
> +	int ret;
>
> -	stmmac_mac_irq_modify(spcs->priv, 0, spcs->int_mask);
> +	ret = dwmac_serdes_power_on(priv);
> +	if (ret)
> +		return ret;
> +
> +	if (spcs->interface != PHY_INTERFACE_MODE_NA) {
> +		ret = dwmac_serdes_set_mode(priv, spcs->interface);
> +		if (ret)
> +			return ret;
                        ^^^^^^^^^^

If dwmac_serdes_set_mode() fails here, doesn't this leave the SerDes in
a powered-on state? dwmac_serdes_power_on() succeeded at this point but
there's no corresponding dwmac_serdes_power_off() call before returning
the error.

Looking at phylink_major_config(), it appears to discard the return
value from phylink_pcs_enable(), so the caller won't know to call
pcs_disable() to clean up the power state.

Should this be something like:

    if (spcs->interface != PHY_INTERFACE_MODE_NA) {
        ret = dwmac_serdes_set_mode(priv, spcs->interface);
        if (ret) {
            dwmac_serdes_power_off(priv);
            return ret;
        }
    }

> +	}
> +
> +	stmmac_mac_irq_modify(priv, 0, spcs->int_mask);
>
> 	return 0;
> }

[ ... ]