[PATCH net v2 3/3] net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

[netdev-bot+sashiko at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Misattributed Fixes: tag and inconsistent stable range.
- [Medium] Setting STMMAC_FLAG_SERDES_SUPPORTS_2500M makes
  stmmac_mac_select_pcs() (stmmac_main.c:1031-1033) return the dwmac…

--

net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

This sets STMMAC_FLAG_SERDES_SUPPORTS_2500M in qcom_ethqos_probe() when the
optional SerDes phy is present, so that stmmac_integrated_pcs_init() adds
PHY_INTERFACE_MODE_2500BASEX to the integrated PCS supported_interfaces
bitmap.

A couple of questions about this one.

> Fixes: 61e9be0efbe8 ("net: stmmac: qcom-ethqos: add support for 2.5G BASEX mode")
> Cc: stable at vger.kernel.org # 7.0+

[Severity: Medium]
Is this the right commit to blame?  The behaviour being corrected is the
flag gate in stmmac_integrated_pcs_init():

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:stmmac_integrated_pcs_init() {
	...
	/* Only allow 2500BASE-X if the SerDes has support. */
	if (priv->plat->flags & STMMAC_FLAG_SERDES_SUPPORTS_2500M)
		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
			  spcs->pcs.supported_interfaces);
	...
}

git show 61e9be0efbe8 only touches dwmac-qcom-ethqos.c (it adds
qcom_ethqos_speed_mode_2500() and the PHY_INTERFACE_MODE_2500BASEX switch
case); it does not touch stmmac_pcs.c or include/linux/stmmac.h.

git blame points both the gate above and the

	#define STMMAC_FLAG_SERDES_SUPPORTS_2500M	BIT(16)

definition in include/linux/stmmac.h at 68cff4fff61f ("net: stmmac: add
BASE-X support to integrated PCS"), which is also where the flag first
appeared, and no in-tree platform ever set it.  Should the Fixes tag name
68cff4fff61f instead?

There is also a mismatch between the two tags: 61e9be0efbe8 shipped in
v6.11, so the Fixes tag nominates trees that predate the code being fixed,
while the Cc annotation says 7.0+.  Which range is intended?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a..0e07225a70ca 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -786,6 +786,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		plat_dat->host_dma_width = data->dma_addr_width;
>  
>  	if (ethqos->serdes_phy) {
> +		plat_dat->flags |= STMMAC_FLAG_SERDES_SUPPORTS_2500M;

[Severity: Medium]
Does setting this flag leave auto-negotiation enabled for 2500BASE-X on
ethqos?

With 2500BASEX now in the integrated PCS supported_interfaces,
stmmac_mac_select_pcs() starts returning the integrated PCS for that
interface instead of NULL (ethqos installs no plat->select_pcs):

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_select_pcs() {
	...
	if (priv->integrated_pcs &&
	    test_bit(interface, priv->integrated_pcs->pcs.supported_interfaces))
		return &priv->integrated_pcs->pcs;
	...
}

That newly routes ethqos through dwmac_integrated_pcs_config():

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:dwmac_integrated_pcs_config() {
	bool changed = false, ane = true;

	/* Only configure the advertisement and allow AN in BASE-X mode if
	 * the core supports TBI/RTBI. ...
	 */
	if (phy_interface_mode_is_8023z(interface) &&
	    spcs->support_tbi_rtbi) {
		ane = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED;
	...
	dwmac_ctrl_ane(spcs->base, 0, ane,
		       spcs->priv->hw->reverse_sgmii_enable);
}

ethqos is the non-TBI case, per the comment in
stmmac_integrated_pcs_init():

	/* If the PCS supports extended status, then it supports BASE-X AN
	 * with a TBI interface to the SerDes. Otherwise, we can support
	 * BASE-X without AN using SGMII, which is required for qcom-ethqos.
	 */

So support_tbi_rtbi is false, the branch is skipped, ane stays true, and
dwmac_ctrl_ane() programs GMAC_AN_CTRL_ANE|GMAC_AN_CTRL_RAN even though
dwmac_integrated_pcs_inband_caps() reported only LINK_INBAND_DISABLE for
802.3z on this core and phylink therefore passed an out-of-band neg_mode.

That also leaves two layers writing GMAC_AN_CTRL in opposite directions:
pcs_config() sets ANE, while the glue clears it later at link up:

drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c:ethqos_fix_mac_speed_sgmii() {
	...
	ethqos_pcs_set_inband(ethqos, interface == PHY_INTERFACE_MODE_SGMII);
	...
}

Is correctness then dependent on a mac_link_up() always following
pcs_config()?  If the interface is configured while the PHY link is down,
or on a repeated major_config without a link-up, the PCS looks like it is
left with AN enabled and restarted on a 2500BASE-X link whose SerDes does
no AN.

Should the ane initialisation in dwmac_integrated_pcs_config() be fixed
first, and should the now-redundant ethqos_pcs_set_inband() write be
mentioned in the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SJ2PR11MB8401F5C3A075DE2A0013E063E4872%40SJ2PR11MB8401.namprd11.prod.outlook.com