Re: net: rtl8169: EEE seems to be ok

On 27.11.2024 11:49, Russell King (Oracle) wrote:

On Tue, Nov 26, 2024 at 12:51:36PM +0000, Russell King (Oracle) wrote:

quoted

In doing this, I came across the fact that the addition of phylib
managed EEE support has actually broken a huge number of drivers -
phylib will now overwrite all members of struct ethtool_keee whether
the netdev driver wants it or not. This leads to weird scenarios where
doing a get_eee() op followed by a set_eee() op results in e.g.
tx_lpi_timer being zeroed, because the MAC driver doesn't know it needs
to initialise phylib's phydev->eee_cfg.tx_lpi_timer member. This mess
really needs urgently addressing, and I believe it came about because
Andrew's patches were only partly merged via another party - I guess
highlighting the inherent danger of "thou shalt limit your patch series
to no more than 15 patches" when one has a subsystem who's in-kernel
API is changing.

Looking at the rtl8169 driver, it looks pretty similar to the Marvell
situation. The value stored in tp->tx_lpi_timer is apparently,
according to r8169_get_tx_lpi_timer_us(), a value in bytes, not in a
unit of time. So it's dependent on the negotiated speed, and thus we
can't read it to set the initial phydev->eee_cfg.tx_lpi_timer state,
because in the _probe() function, the PHY may not have negotiated a
speed.

Right, hw stores the tx_lpi_timer in bytes. Driver's default value is
one frame plus a few bytes. It doesn't use phydev->eee_cfg.tx_lpi_timer.
set_eee() op isn't implemented for tx_lpi_timer, because no one ever
asked for it and I'm not aware of any good use case.

However, this driver writes keee->tx_lpi_timer after
phy_ethtool_get_eee() which means that it overrides phylib, so hasn't
been broken.

Therefore, I think rtl8169 is fine.