linux-kernel - Re: [PATCH net-next v6 1/7] net: phylink: use act_link_an

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] [day] [month] [year] [list]

Message-ID: <c25c2da7-18fa-46b7-b992-fb5b17a01749@linux.intel.com>
Date: Wed, 5 Feb 2025 14:50:31 +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>,
 Andrew Halaney <ahalaney@...hat.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 v6 1/7] net: phylink: use act_link_an_mode to
 determine PHY

On 4/2/2025 8:04 pm, Russell King (Oracle) wrote:
> On Tue, Feb 04, 2025 at 02:10:14PM +0800, Choong Yong Liang wrote:
>> When the interface mode is SGMII and act_link_an_mode is MLO_AN_INBAND,
>> switching to the 2500BASE-X interface mode will trigger
>> phylink_major_config, and act_link_an_mode will be updated to MLO_AN_PHY
>> in phylink_pcs_neg_mode when the PCS does not support in-band mode.
>> The MAC and PCS will configure according to the interface mode
>> and act_link_an_mode.
> 
> act_link_an_mode must only ever be updated by phylink_major_config()
> since it defines state for the currently configured mode, and must
> stay in sync with how the hardware has been programmed at all times.
> 
>> However, when the interface goes link down and then link up again, the MAC
>> will attempt to attach the PHY.
> 
> Why is the MAC trying to disconnect and reconnect the PHY on link
> changes? Do you really mean "link down" and "link up" as in "connection
> with the link partner" or do you mean administratively taking the
> interface down and up (which is a completely different thing.)
> 

The "link down" and "link up" I mention in this part refer to using the 
command:
ifconfig <interface> down/up

>> The interface mode remains as 2500BASE-X,
>> but cfg_link_an_mode still holds MLO_AN_INBAND. This causes a failure to
>> attach the PHY.
> 
> Hmm.
> 
> pl->link_interface is the configured setting from firmware etc and doesn't
> change.
> 
> pl->cfg_link_an_mode is the configured mode from firmware etc which was
> passed to phylink_create(), and again doesn't change.
> 
> So there should be no difference unless something weird is going on,
> which as you're talking about stmmac, could be the case.
> 

Thank you for pointing that out.

I think I was confused between pl->link_interface and
pl->link_config.interface. The function phylink_attach_phy() uses
pl->link_interface, whereas phylink_expects_phy() uses
pl->link_config.interface.

When the interface switches from SGMII to 2500BASE-X,
pl->link_config.interface is updated by phylink_major_config().
So, when the STMMAC link goes down and comes up again,
it is blocked by phylink_expects_phy().
At this point, pl->cfg_link_an_mode is MLO_AN_INBAND and 
pl->link_config.interface is 2500BASE-X.

Since phylink_expects_phy() is trying to achieve the same checking 
condition as phylink_attach_phy(), it should use pl->link_interface for the 
check as well.

Does that make sense to you?

> More information needed, but as this patch currently stands, I deem it
> to be incorrect, sorry.
>