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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aW8M9ZiiftGBQIRM@shell.armlinux.org.uk>
Date: Tue, 20 Jan 2026 05:04:53 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jakub Kicinski <kuba@...nel.org>
Cc: linux-phy@...ts.infradead.org, davem@...emloft.net,
	maxime.chevallier@...tlin.com, alexandre.torgue@...s.st.com,
	mohd.anwar@....qualcomm.com, neil.armstrong@...aro.org,
	hkallweit1@...il.com, mcoquelin.stm32@...il.com,
	netdev@...r.kernel.org, edumazet@...gle.com,
	linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	vkoul@...nel.org, andrew@...n.ch, pabeni@...hat.com,
	andrew+netdev@...n.ch, linux-stm32@...md-mailman.stormreply.com
Subject: Re: [net-next,05/14] net: stmmac: add stmmac core serdes support

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!