Re: [PATCH net-next 2/2] net: pcs: xpcs: select operating mode for 10G-baseR capable PCS

[netdev-bot+sashiko@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] The new 10GBASE-R check in xpcs_switch_interface_mode()
  (drivers/net/pcs/pcs-xpcs.c:714-724) reads PCS MMD 3 register 3.8…
- [Medium] When MDIO_STAT2 bit 0 is set, the default branch of
  xpcs_switch_interface_mode() (pcs-xpcs.c:740-743) writes…

--

net: pcs: xpcs: select operating mode for 10G-baseR capable PCS

This patch makes xpcs_switch_interface_mode() read the PCS MDIO_STAT2
register. When the 10GBASE-R capability bit is set, it programs
SR_XS_PCS_CTRL2:PCS_TYPE_SEL: SGMII, 1000BASE-X and 2500BASE-X get a
reserved value, and every other interface gets the 10GBASE-R type. The
aim is to make the TC9564 XPCS work with a QCA8081 PHY without adding
another quirk.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 49abf5ea6f5cc..5316cf9c31f19 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -705,10 +705,47 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
>  static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs,
>  				      phy_interface_t interface)
>  {
> +	int mdio_stat2, ret;
> +
>  	/* Wangxun provides a full alternative implementation to handle quirks */
>  	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
>  		return txgbe_xpcs_switch_mode(xpcs, interface);
>  
> +	mdio_stat2 = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
> +	if (mdio_stat2 < 0)
> +		return mdio_stat2;
> +
> +	/*
> +	 * If this XPCS supports 10Gbase-R then that will be the default
> +	 * operating mode. There are several interface modes where this default
> +	 * is unhelpful. Change the operating mode for interfaces were we know
> +	 * the default is wrong, and restore the default otherwise.
> +	 */
> +	if (mdio_stat2 & MDIO_PCS_STAT2_10GBR) {

[Severity: Medium]
Can a missing PCS MMD be treated as 10GBASE-R capable here? The code tests
bit 0 of MDIO_STAT2 but does not check the device-present field in bits
15:14. An absent MMD 3 that reads back as 0xffff would pass the test.

The driver already expects MMD 3 to be absent on some parts.
xpcs_read_ids() falls back to MMD 31 when the IDs read as all zeros or all
ones:

    if (!id || id == 0xffffffff) {

On SJA1105R/S, the read side returns 0xffff for any MMD other than
VEND1/VEND2:

drivers/net/dsa/sja1105/sja1105_mdio.c:sja1105_pcs_mdio_read_c45() {
    ...
	if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
		return 0xffff;
    ...
}

The write side rejects the new CTRL2 write:

drivers/net/dsa/sja1105/sja1105_mdio.c:sja1105_pcs_mdio_write_c45() {
    ...
	if (mdio != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
		return -EINVAL;
    ...
}

So for an SGMII port on SJA1105R/S:

xpcs_pre_config()
  xpcs_switch_interface_mode()
    xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2)       -> 0xffff
    xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2, 4)   -> -EINVAL

xpcs_switch_interface_mode() then returns without setting
xpcs->interface. xpcs_pre_config() would then log "switch interface
failed: -EINVAL" on every major config.

On SJA1110, sja1110_pcs_mdio_read_c45() and sja1110_pcs_mdio_write_c45()
don't filter by MMD, so MMD 3 accesses go to bank 0x300 of the PCS
window. If that location reads with bit 0 set, would 0x0004 be written
into undocumented switch register space?

Would it make sense to also require that
(mdio_stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL? Another
option is to run this only for descs whose compat tables include 10G
modes.

> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_SGMII:
> +		case PHY_INTERFACE_MODE_1000BASEX:
> +		case PHY_INTERFACE_MODE_2500BASEX:

[ ... ]

> +			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
> +					 MDIO_PCS_CTRL2_TYPE + 1);
> +			break;
> +		default:
> +			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
> +					 MDIO_PCS_CTRL2_10GBR);
> +			break;

[Severity: Medium]
Is 10GBASE-R the right PCS type for every interface that reaches this
default branch? The comment says "restore the default otherwise".
However, synopsys_xpcs_compat[] also lists 25GBASER, XLGMII, 50GBASER,
LAUI and 100GBASEP for DW_XPCS_ID. All of those would end up here with
PCS_TYPE_SEL set to 0 (10GBASE-R).

Before this patch, the generic path never wrote PCS CTRL2. The only CTRL2
writes were in the Wangxun code.

Meta fbnic is an in-tree user that reaches this branch:

- It doesn't use the Wangxun PMA ID.
- fbnic_phylink.c only enables 100GBASEP, 50GBASER, LAUI and 25GBASER.
- fbnic_mdio_read_pcs() doesn't intercept MDIO_STAT2, so the gating bit
  comes from the real PCS register.

fbnic_mdio_write_pcs() passes the CTRL2 write straight to hardware:

drivers/net/ethernet/meta/fbnic/fbnic_mdio.c:fbnic_mdio_write_pcs() {
    ...
	fbnic_wr32(fbd, FBNIC_PCS_PAGE(addr) + regnum, val);
}

If the fbnic PCS reports the 10GBR bit in STAT2, would every major config
force the PCS type to 10GBASE-R while the link runs at 25G, 50G or 100G?
I couldn't confirm from the tree whether that bit reads as 1 on fbnic.

> +		}
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	xpcs->interface = interface;
>  
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014223100.304486-1-elder%40riscstar.com