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

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Jakub Kicinski <kuba@kernel.org>
Cc: linux-phy@lists.infradead.org, davem@davemloft.net,
	maxime.chevallier@bootlin.com, alexandre.torgue@foss.st.com,
	mohd.anwar@oss.qualcomm.com, neil.armstrong@linaro.org,
	hkallweit1@gmail.com, mcoquelin.stm32@gmail.com,
	netdev@vger.kernel.org, edumazet@google.com,
	linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, vkoul@kernel.org,
	andrew@lunn.ch, pabeni@redhat.com, andrew+netdev@lunn.ch,
	linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [net-next,05/14] net: stmmac: add stmmac core serdes support
Date: Tue, 20 Jan 2026 05:04:53 +0000	[thread overview]
Message-ID: <aW8M9ZiiftGBQIRM@shell.armlinux.org.uk> (raw)
In-Reply-To: <20260119192125.1245102-1-kuba@kernel.org>

On Mon, Jan 19, 2026 at 11:21:25AM -0800, Jakub Kicinski wrote:
> 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.

This AI analysis is incorrect.

By the time phylink_pcs_enable() has been called, the PCS is already
plumbed in to phylink. It _will_ have phylink_pcs_disable() called on
it at some point in the future, either by having the PCS displaced
by another in a subsequent phylink_major_config(), or by a driver
calling phylink_stop().

If we clean up here, then we will call dwmac_serdes_power_off() twice.

Yes, it's not "nice" but that's the way phylink is right now, and
without reworking phylink to record that pcs_enable() has failed
to avoid a subsequent pcs_disable(), and to stop the major config
(which then potentially causes a whole bunch of other issues). I
don't even want to think about that horrid scenario at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!