net: rtl8169: EEE seems to be ok

[Heiner Kallweit]

On 27.11.2024 11:49, Russell King (Oracle) wrote:
> On Tue, Nov 26, 2024 at 12:51:36PM +0000, Russell King (Oracle) wrote:
>> 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.
>