netdev - Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <564ede5d-9f53-40be-9305-63f63b384e15@linux.intel.com>
Date: Fri, 7 Feb 2025 17:20:29 +0800
From: Choong Yong Liang <yong.liang.choong@...ux.intel.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Simon Horman <horms@...nel.org>, Jose Abreu <joabreu@...opsys.com>,
 Jose Abreu <Jose.Abreu@...opsys.com>,
 David E Box <david.e.box@...ux.intel.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H . Peter Anvin" <hpa@...or.com>,
 Rajneesh Bhardwaj <irenic.rajneesh@...il.com>,
 David E Box <david.e.box@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin
 <mcoquelin.stm32@...il.com>, Alexandre Torgue
 <alexandre.torgue@...s.st.com>, Jiawen Wu <jiawenwu@...stnetic.com>,
 Mengyuan Lou <mengyuanlou@...-swift.com>,
 Heiner Kallweit <hkallweit1@...il.com>, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Richard Cochran <richardcochran@...il.com>,
 Serge Semin <fancer.lancer@...il.com>, x86@...nel.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 platform-driver-x86@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37
 Auto-negotiation



On 6/2/2025 11:30 pm, Russell King (Oracle) wrote:
> On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote:
>> The xpcs_switch_interface_mode function was introduced to handle
>> interface switching.
>>
>> According to the XPCS datasheet, a soft reset is required to initiate
>> Clause 37 auto-negotiation when the XPCS switches interface modes.
> 
> Hmm. Given that description, taking it literally, claus 37
> auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE
> 802.3 standard.) Are you absolutely sure that this applies to Cisco
> SGMII?
> 
Hi Russell,

Yes, you are correct that Clause 37 auto-negotiation is for 1000BASE-X. 
However, I do not believe it applies to Cisco SGMII. The XPCS implements 
Clause 37 auto-negotiation for both 1000BASE-X and SGMII.

> If the reset is required when switching to SGMII, should it be done
> before or after configuring the XPCS for SGMII?
> 
A soft reset is required before configuring the XPCS for SGMII. Based on 
the existing XPCS handling in the initial state, the xpcs_create() function 
will be called, and then xpcs->need_reset will be set to true. Later on, 
phylink_major_config() will call xpcs_pre_config() to perform the 
xpcs_soft_reset(), and then it will continue with xpcs_config().

I apologize for missing this patch: 
https://patchwork.kernel.org/project/netdevbpf/patch/E1svfMA-005ZI3-Va@rmk-PC.armlinux.org.uk/

I think I should move xpcs_switch_interface_mode() to xpcs_pre_config() and 
just update xpcs->need_reset instead of implementing my own method for 
calling xpcs_soft_reset().

> Also, if the reset is required, what happens if we're already using
> SGMII, but AN has been disabled temporarily and is then re-enabled?
> Is a reset required?
> 
Good point. I cannot find this scenario in the datasheet. Please allow me 
some time to test this scenario. I will update you with the results.

> What about 1000BASE-X when AN is enabled or disabled and then switching
> to SGMII?
> 
According to the datasheet, a soft reset is required.

>> +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat,
>> +					 struct dw_xpcs *xpcs,
>> +					 unsigned int neg_mode)
>> +{
>> +	bool an_c37_enabled;
>> +	int ret, mdio_ctrl;
>> +
>> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
>> +		mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
>> +		if (mdio_ctrl < 0)
>> +			return mdio_ctrl;
>> +
>> +		an_c37_enabled = mdio_ctrl & BMCR_ANENABLE;
>> +		if (!an_c37_enabled) {
> 
> I don't think that we need "an_c37_enabled" here, I think simply:
> 
> 		if (!(mdio_ctrl & BMCR_ANENABLE)) {
> 
> would be sufficient.
> 
>> +			//Perform soft reset to initiate C37 auto-negotiation
>> +			ret = xpcs_soft_reset(xpcs, compat);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +	return 0;
> 
> I'm also wondering (as above) whether this soft reset needs to happen
> _after_ xpcs_config_aneg_c37_sgmii() has done its work - this function
> temporarily disables AN while it's doing its work.
> 
Based on the programming sequence in the datasheet, it is not necessary to 
perform a soft reset after xpcs_config_aneg_c37_sgmii() has completed its work.

> I'm also wondering whether AN being disabled is really a deciding
> factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a
> reset required?)
> 

Thank you for pointing this out. The datasheet only mentions performing a 
soft reset when switching to the 1000BASE-X and SGMII interfaces, and it 
does not specify whether AN needs to be enabled or disabled. I thought 
adding a check would reduce the calls to the soft reset. However, I did not 
consider the scenario of switching from 1000BASE-X with AN enabled to SGMII 
with AN enabled. This scenario might cause regression. I will remove all 
the checks and just perform a soft reset when switching to the SGMII interface.