Message-ID: <9455a623aaeb08999eec9202459d266f22432c00.camel@alliedtelesis.co.nz>
Date: Wed, 12 Mar 2025 21:16:19 +0000
From: Hamish Martin <Hamish.Martin@...iedtelesis.co.nz>
To: "andrew@...n.ch" <andrew@...n.ch>
CC: "przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] igb: Prevent IPCFGN write resetting autoneg
 advertisement register

On Wed, 2025-03-12 at 14:05 +0100, Andrew Lunn wrote:
> On Wed, Mar 12, 2025 at 04:22:50PM +1300, Hamish Martin wrote:
> > An issue is observed on the i210 when autonegotiation advertisement
> > is set
> > to a specific subset of the supported speeds but the requested
> > settings
> > are not correctly set in the Copper Auto-Negotiation Advertisement
> > Register
> > (Page 0, Register 4).
> > Initially, the advertisement register is correctly set by the
> > driver code
> > (in igb_phy_setup_autoneg()) but this register's contents are
> > modified as a
> > result of a later write to the IPCNFG register in
> > igb_set_eee_i350(). It is
> > unclear what the mechanism is for the write of the IPCNFG register
> > to lead
> > to the change in the autoneg advertisement register.
> > The issue can be observed by, for example, restricting the
> > advertised speed
> > to just 10MFull. The expected result would be that the link would
> > come up
> > at 10MFull, but actually the phy ends up advertising a full suite
> > of speeds
> > and the link will come up at 100MFull.
> > 
> > The problem is avoided by ensuring that the write to the IPCNFG
> > register
> > occurs before the write to the autoneg advertisement register.
> 
> When you set the advertisement for only 10BaseT Full, what EEE
> settings are applied? It could be that calling igb_set_eee_i350() to
> advertise EEE for 100BaseT Full and 1000BaseT Full, while only
> advertising link mode 10BaseT causes the change to the autoneg
> register.
> 
> Please try only advertising EEE modes which fit with the basic link
> mode advertising.
> 
>      Andrew
Hi Andrew,

Thanks for your feedback. I'll try and give more detail about what's
happening with a concrete example.

If we start with the device in a state where it is advertising:
1000BaseT Full
100baseT Full
100baseT Half
10baseT Full
10baseT Half
I see the following settings in the autoneg related registers:
0.4 = 0x0de1 (PHY_AUTONEG_ADV)
0.9 = 0x0200 (PHY_1000T_CTRL)

EEE is disabled.

If I then adjust the advertisement to only advertise 1000BaseT Full and
100baseT Full with:
# ethtool -s eth0 advertise 0x28
I see the following writes to the registers:
1. In igb_phy_setup_autoneg() PHY_AUTONEG_ADV is written with 0x0101
(the correct value)
2. Later in igb_phy_setup_autoneg() PHY_1000T_CTRL is written with
0x0200 (correct)
3. Autoneg gets restarted in igb_copper_link_autoneg() with PHY_CONTROL
(0.0) being written with 0x1340
(everything looks fine up until here)
4. Now we reach igb_set_eee_i350(). Here we read in IPCNFG and it has
value 0xf. EEE is disabled so we hit the 'else' case and remove
E1000_IPCNFG_EEE_1G_AN and E1000_IPCNFG_EEE_100M_AN from the 'ipcnfg'
value. We then write this back as 0x3. At this point, if you re-read
PHY_AUTONEG_ADV you will see it's contents has been reset to 0x0de1.

If you run the same example above but with EEE enabled (ethtool --set-
eee eth0 eee on; ethtool -s eth0 advertise 0x28) the issue is not seen.
In this case the contents of IPCNFG are written back unmodified as 0xf.
This seems important to avoid the bug.

It seems that any case where EEE is disabled will lead to the
undesirable behaviour where the contents of PHY_AUTONEG_ADV is reset to
0x0de1. The key trigger for this appears to be changes to either or
both of EEE_100M_AN and EEE_1G_AN in IPCNFG. The datasheet does note
that "Changing value of bit causes link drop and re-negotiation" - what
I see is a lot more than that but there does at least seem to be some
acknowledgement of a possible link to the issue I see.

It seems the intent of the code in the else case that modifies IPCNFG
is to make EEE really really "off". But perhaps we could just avoid
altering IPCNFG altogether in the EEE disabled case and rely on
controlling EEE with the EEER register only? If I make that change
instead my issues go away.

What's your opinion on that less invasive fix (i.e remove "ipcnfg &=
~(E1000_IPCNFG_EEE_1G_AN | E1000_IPCNFG_EEE_100M_AN);" )? Is it
sufficient to rely on the EEER settings to control disabling EEE with
the IPCNFG register still set to advertise those modes?

Thanks,
Hamish M

Powered by blists - more mailing lists