From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3DF08347BC5 for ; Tue, 20 Jan 2026 08:18:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768897132; cv=none; b=L9x9XNf42KotyCzxgozjQyEw4twVIabYaYM8ZDzEqJFY9RLwpt1seYiD7Dw4HueHRz3F3e3Ir28vXjDvUHiTv8QU2QNT1I3ME7j3X6skszkRuWI1nnVvB+kb0CCnAS7uj4bMOT4YE/5heCD1vMT277jgZrDgg5lg/7/EXp70npQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768897132; c=relaxed/simple; bh=qyvzLqcm1Y8bkz0o1wMAZLeFnBMWPFkiUHTVVHF4rys=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=iTscaOB1tOv66x13Y7PBJBi02MLXd8WEDPGiF3FTGFawAXa9LjikHS88WaTkyxy11Nkxgy2171Js1J+iBa1hY/kGCfZPuweNIb0Oqx+/RmtK8iNprhXu+Bl+vBY8rNwVj0TT3Rzgp3qyNIeXaMKpIBqpNIZYno3MB8h17HGSYNA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=PNkYY7XN; arc=none smtp.client-ip=209.85.218.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="PNkYY7XN" Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-b86ff9ff9feso58630566b.1 for ; Tue, 20 Jan 2026 00:18:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1768897128; x=1769501928; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=TJbdw+xcDYuOTpLf6yNuSRnoYdJpj1gSQsdBtW0Vq6o=; b=PNkYY7XNtNYF0GzHt0GUf83gwU2vahRqOJ3chslV0fwZ9/xNzFF8D/XjJ/5EsDPEC1 ThQu+tgt6xp0UQezP9N5esqpd/WR1WiheRm3okKBtsUsFYR6MPSDekmA+lIlss/vP97q yBvgf0pxKWwtarDcRLGrBP5trdfASoW/aYoivJZzaALNfTXbaXEU6CbrwgFPlKfl580m DddFKFc7aPYfrk3vL870STFs5iDc92C674MnVvvMwZhBgYfeuPijyiIwbFm0wyPzdOeA SrUQI9j+Vrr5GABQS2REWBPzIOQW3e5vd8YKmSnCLLggLvQxyYtDXqf/btBAaV+deGes G7mg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768897128; x=1769501928; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=TJbdw+xcDYuOTpLf6yNuSRnoYdJpj1gSQsdBtW0Vq6o=; b=fznX1wZdVcDqAoi187j+EnhaKCDBc8gYVb7bbEMBliA07hKFkOGyqeqylDTHLqzqSR NUXigpyphH8WFdgsSvBjHjhNnPP99JJaC27R3mzuXtJUCq7gPINouMGKI6BOM8s3Xavr TKvt26cYq4+af+wNM7nkypKgiwzVnLfLkSAM4FR8JNx1pQJJKXRlHZkJe80GdPwAAUod wvH6j8o9V83gHLg+LUc2+hetNrKRwQiFwZWi3vYN3r+s5POOEBRfX5+9TsuLjBt8gmVj 2z7RJMNc0KgXzhBAllGR3eJfG2ldUdH+gfnvA+2qh2Am+pTKAqgufQ4bh75F+JVARucr azDQ== X-Forwarded-Encrypted: i=1; AJvYcCWTB5iX4NNYi/fQ4BYutom9wZVkEzlojWelt89Mzwg0idW0jiALOhNrwLhnf8Xu4LKEYU8Rz3Q=@vger.kernel.org X-Gm-Message-State: AOJu0YzxPmskg3FaNWqYeltTSwRhPkTgA+5xEnHnfP3ZeFygX+hZCNUN zrgAo6IPH1S58u0A+a1i2dSyteJpEx6H8Z0wMqv3Xd7yxbZ4OMNW28mM X-Gm-Gg: AY/fxX6Yq5zjx4OJ6LSDO1LoA+3YswKPVEFBRwiBN48Ai34BNbRklQZcjoMLZK3397u BWmT9Wdij1D4y/h/IiGkzebX11r/U+ImbOlds8Bx25URgUKO3C48qZDt07wu4BhRLdXnfckaekd x4IGF3nF0XnP24sjB1e3o6SAKAllVEiTszacEjLZk7oa2aANy9w/p+dj6JGGYT0+aIDth9tLvrT 9KkQcCCkmdM7nleFJ22kpnnBJsvTBu6Z64heIVE2O1yt08ulvhepSC+Wz0vRl8fxYzuSMRlZJJ0 R7asDGAV2twSSFQKPphGa8i+l+GdEv8kKVHeqt0cOZIn+MQB64ZJmpBBYP+lheyH5IbO3/leklE xd0DTxld7aGyuKtTzSmeTPZuf68Q60NH1mdu2AARcr0UggS2Cxh7YCi8ZLCkTZVlZu5IkxdrrMy cIn8s= X-Received: by 2002:a17:907:3c8a:b0:b87:6f58:a844 with SMTP id a640c23a62f3a-b8792b3a359mr743051066b.0.1768897127986; Tue, 20 Jan 2026 00:18:47 -0800 (PST) Received: from skbuf ([2a02:2f04:d501:d900:619a:24df:1726:f869]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-b8795c38208sm1344223966b.71.2026.01.20.00.18.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 20 Jan 2026 00:18:47 -0800 (PST) Date: Tue, 20 Jan 2026 10:18:44 +0200 From: Vladimir Oltean To: "Russell King (Oracle)" Cc: Jakub Kicinski , 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 Message-ID: <20260120081844.7e6aq2urhxrylywi@skbuf> References: <20260119192125.1245102-1-kuba@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 --- 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 --- 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 --