Message-ID: <20260120081844.7e6aq2urhxrylywi@skbuf>
Date: Tue, 20 Jan 2026 10:18:44 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Jakub Kicinski <kuba@...nel.org>, 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 Tue, Jan 20, 2026 at 05:04:53AM +0000, Russell King (Oracle) wrote:
> 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.

Isn't it sufficient to set pl->pcs to NULL when pcs_enable() fails and
after calling pcs_disable(), though?

I had to deal with the same issue when preparing patches that integrate
SerDes support into the Lynx PCS.

I had these patches (please pardon the unadapted commit messages for the
present situation):

-- >8 --
Subject: [PATCH] net: phylink: handle return code from phylink_pcs_enable()

I am trying to make phylink_pcs_ops :: pcs_enable() something that is
handled sufficiently carefully by phylink, such that we can expect that
when we return an error code here, no other phylink_pcs_ops call is
being made. This way, the API can be considered sufficiently reliable to
allocate memory in pcs_enable() which is freed in pcs_disable().

Currently this does not take place. The pcs_enable() method has an int
return code, which is ignored. If the PCS returns an error, the
initialization of the phylink instance is not stopped, but continues on
like a train, most likely triggering faults somewhere else.

Like this:

$ ip link set endpmac2 up
fsl_dpaa2_eth dpni.1 endpmac2: configuring for c73/10gbase-kr link mode
fsl_dpaa2_eth dpni.1 endpmac2: pcs_enable() failed: -ENOMEM // added by me
Unable to handle kernel paging request at virtual address fffffffffffffff4
Call trace:
 mtip_backplane_get_state+0x34/0x2b4
 lynx_pcs_get_state+0x30/0x180
 phylink_resolve+0x2c0/0x764
 process_scheduled_works+0x228/0x330
 worker_thread+0x28c/0x450

Do a minimal handling of the error by clearing pl->pcs, so that we lose
access to its ops, and thus are unable to call anything else (which
would be invalid anyway).

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/phylink.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 32ffa4f9e5b2..a8459116b701 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1315,8 +1315,15 @@ static void phylink_major_config(struct phylink *pl, bool restart,
 		}
 	}
 
-	if (pl->pcs_state == PCS_STATE_STARTING || pcs_changed)
-		phylink_pcs_enable(pl->pcs);
+	if (pl->pcs_state == PCS_STATE_STARTING || pcs_changed) {
+		err = phylink_pcs_enable(pl->pcs);
+		if (err < 0) {
+			phylink_err(pl, "pcs_enable() failed: %pe\n",
+				    ERR_PTR(err));
+			pl->pcs = NULL;
+			return;
+		}
+	}
 
 	err = phylink_pcs_config(pl->pcs, pl->pcs_neg_mode, state,
 				 !!(pl->link_config.pause & MLO_PAUSE_AN));
-- >8 --

-- >8 --
Subject: [PATCH] net: phylink: suppress pcs->ops->pcs_get_state() calls after
 phylink_stop()

I am attempting to make phylink_pcs_ops :: pcs_disable() treated
sufficiently carefully by phylink so as to be able to free memory
allocations from this PCS callback, and do not suffer from faults
attempting to access that memory later from other phylink_pcs callbacks.

Currently, nothing prevents this situation from happening:

$ ip link set endpmac2 up
$ ip link set endpmac2 down
$ ethtool endpmac2
Unable to handle kernel paging request at virtual address 0000100000000034
Call trace:
 __mutex_lock+0xb8/0x574
 __mutex_lock_slowpath+0x14/0x20
 mutex_lock+0x24/0x58
 mtip_backplane_get_state+0x44/0x24c
 lynx_pcs_get_state+0x30/0x180
 phylink_ethtool_ksettings_get+0x178/0x218
 dpaa2_eth_get_link_ksettings+0x54/0xa4
 __ethtool_get_link_ksettings+0x68/0xa8
 linkmodes_prepare_data+0x44/0xc4
 ethnl_default_doit+0x118/0x39c
 genl_rcv_msg+0x29c/0x314
 netlink_rcv_skb+0x11c/0x134
 genl_rcv+0x34/0x4c

However, the case where "ethtool endpmac2" is executed as the first
thing (before the interface is brought up) does not crash. What's
different is that second situation is that phylink_major_config() did
not run yet, so pl->pcs is still NULL inside phylink_mac_pcs_get_state().
In plain English, "as long as the PCS is disabled, the link is naturally
down, no need to ask".

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/phylink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a8459116b701..f78d0e0f7cfb 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2527,6 +2527,7 @@ void phylink_stop(struct phylink *pl)
 	pl->pcs_state = PCS_STATE_DOWN;

 	phylink_pcs_disable(pl->pcs);
+	pl->pcs = NULL;
 }
 EXPORT_SYMBOL_GPL(phylink_stop);

-- >8 --

Powered by blists - more mailing lists