Message-ID: <0486c877-cbb4-411b-9bd6-9b10306c47a6@lunn.ch>
Date: Wed, 12 Mar 2025 23:25:30 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Hamish Martin <Hamish.Martin@...iedtelesis.co.nz>
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

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

Thanks for the additional details. These should go into the commit
message.

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

Yes, it does seem like the PHY is broken. 

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

Which is what you would expect, since EEE is negotiated. But
implicitly changing the link modes advertised is not what you would
expect.

By the way, what PHY is this? I don't remember seeing any errata for
Linux PHY drivers resembling this.

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

I actually think you need to do more testing. Assuming the PHY is not
even more broken than we think, it should not matter if it advertises
EEE mode for link modes which are not advertised. The link partner
should ignore them. It would be good if you tested out various EEE
combinations from both link partners sides.

However, setting EEE advertisement and then always setting link mode
advertisement does seem like a good workaround. It would however be
good to get some sort of feedback from the PHY silicon vendor about
this odd behaviour.

	Andrew

Powered by blists - more mailing lists