Re: [PATCH net-next v6 1/7] net: phylink: use act_link_an_mode to determine PHY

[Choong Yong Liang <yong.liang.choong@linux.intel.com>]

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