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

On 5/2/26 10:56 AM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

This is a good observation, and for the reasons pointed out,
it might not be "safe".

The comment above the assignment explains that what's going
on is intentional (to get a behavior that comes from writing
a reserved value), and was done in way that tries to avoid an
eventual problem.

The next version of the code will still do this the same way, but
we would welcome a suggestion for a better way to handle this.

> 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?

The next version of the code reworks how this is done.  The code
will preserve the special behavior used for Wanxun hardware, but
will update the operating mode in a way similar to what's done
in the current patch otherwise.

					-Alex

> 
> 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
>