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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The generic workaround unconditionally overwrites `MDIO_CTRL2`, breaking Wangxun hardware.
- [High] Transitioning from SGMII/1000BASEX to 10GBASE-R leaves MDIO_CTRL2 in a broken state, preventing 10G modes from functioning.
- [Medium] Synthesizing a hardware value by adding 1 to a UAPI bitmask macro is brittle and semantically incorrect.
--

commit 797f904fd928c0648fed4442dd4dbb70a939a7f7
Author: Daniel Thompson <daniel@riscstar.com>

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

The patch attempts to solve an issue where 10Gbase-R support suppresses modal
switching for 1000base-X and slower speeds on XPCS. It sets the PCS_TYPE_SEL
in MDIO_CTRL2 to a reserved value when detecting the relevant PHY interface
and XPCS feature support.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index e69fa2f0a0e8..b2c84b7e1e11 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -747,6 +747,40 @@ static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
>  	xpcs->need_reset = false;
>  }
>  
> +static int xpcs_config_operating_mode(struct dw_xpcs *xpcs, int an_mode)
> +{
> +	int mdio_stat2, ret;
> +
> +	switch (an_mode) {
> +	case DW_AN_C37_SGMII:
> +	case DW_AN_C37_1000BASEX:
> +	case DW_2500BASEX:
> +		mdio_stat2 = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
> +		if (mdio_stat2 < 0)
> +			return mdio_stat2;
> +
> +		/*
> +		 * If this XPCS supports 10Gbase-R then it will be the default
> +		 * which prevents 1000base-X and slower from working correctly.
> +		 *
> +		 * Why are we writing MDIO_PCS_CTRL2_TYPE + 1? We want the modal
> +		 * behaviour that comes when we pick a reserved value. XPCS
> +		 * allocates extra bits to this field and allocates values from
> +		 * 15 down so MDIO_PCS_CTRL2_TYPE + 1 is the value likely to
> +		 * be allocated last (and hopefully never).
> +		 */
> +		if (mdio_stat2 & MDIO_PCS_STAT2_10GBR) {
> +			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
> +					 MDIO_PCS_CTRL2_TYPE + 1);

Is it safe to calculate the reserved value by adding 1 to the bitmask macro
MDIO_PCS_CTRL2_TYPE?

MDIO_PCS_CTRL2_TYPE is defined in <uapi/linux/mdio.h> as a bitmask (0x0003),
not a maximum integer value. While this evaluates to 4 today, if the IEEE
standard expands the PCS type selection field (e.g. to a 4-bit mask 0x000F),
this expression will evaluate to 16, overflowing the field. Could this use an
explicit local constant for the targeted proprietary reserved value instead?

Also, could this cause issues if the interface is dynamically switched back
to a 10G mode (e.g., DW_10GBASER)?

If the interface switches from SGMII to DW_10GBASER,
xpcs_config_operating_mode() hits an empty switch case and does not update
the register. Because xpcs_switch_interface_mode() only sets
xpcs->need_reset = true when switching to SGMII, no soft reset is triggered,
leaving MDIO_CTRL2 stuck at the reserved value 4 and breaking 10GBASE-R
modal behavior.

> +			if (ret < 0)
> +				return ret;
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -919,6 +953,10 @@ static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
>  	if (!compat)
>  		return -ENODEV;
>  
> +	ret = xpcs_config_operating_mode(xpcs, compat->an_mode);

Does this unconditionally overwrite MDIO_CTRL2 and break Wangxun hardware?

For Wangxun devices, xpcs_pre_config() calls xpcs_switch_interface_mode(),
which delegates to txgbe_xpcs_switch_mode(). This explicitly configures
MDIO_CTRL2 to MDIO_PCS_CTRL2_10GBX (1) for SGMII and 1000BASE-X.

Later, xpcs_do_config() calls xpcs_config_operating_mode() here. Because
Wangxun devices support 10GBASE-R, it writes the reserved value 4 to
MDIO_CTRL2. This clobbers the required Wangxun-specific 10GBX state,
forcing the hardware into a reserved state and breaking the link.

> +	if (ret < 0)
> +		return ret;
> +
>  	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
>  		/* Wangxun devices need backplane CL37 AN enabled for
>  		 * SGMII and 1000base-X

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260501155421.3329862-1-elder@riscstar.com?part=2