Re: [PATCH net v1 1/2] net: phy: Introduce phy_update_eee() to update eee_cfg values

From: Choong Yong Liang
Date: Wed Nov 13 2024 - 23:35:54 EST




On 14/11/2024 5:48 am, Heiner Kallweit wrote:
"relies on" may be the wrong term here. There's an API definition,
and phy_ethtool_get_eee() takes care of the PHY-related kernel part,
provided that the MAC driver uses phylib.
I say "PHY-related part", because tx_lpi_timer is something relevant
for the MAC only. Therefore phylib stores the master config timer value
only, not the actual value.
The MAC driver should populate tx_lpi_timer in the get_eee() callback,
in addition to what phy_ethtool_get_eee() populates.
This may result in the master config value being overwritten with actual
value in cases where the MAC doesn't support the master config value.

One (maybe there are more) special case of tx_lpi_timer handling is
Realtek chips, as they store the LPI timer in bytes. Means whenever
the link speed changes, the actual timer value also changes implicitly.

Few values exist twice: As a master config value, and as status.
struct phy_device has the status values:
@eee_enabled: Flag indicating whether the EEE feature is enabled
@enable_tx_lpi: When True, MAC should transmit LPI to PHY

And master config values are in struct eee_cfg:

struct eee_config {
u32 tx_lpi_timer;
bool tx_lpi_enabled;
bool eee_enabled;
};

And yes, it may be a little misleading that eee_enabled exists twice,
you have to be careful which one you're referring to.

ethtool handles the master config values, only "active" is a status
information.

So the MAC driver should:
- provide a link change handler in e.g. phy_connect_direct()
- this handler should:
- use phydev->enable_tx_lpi to set whether MAC transmits LPI or not
- use phydev->eee_cfg.tx_lpi_timer to set the timer (if the config
value is set)

Important note:
This describes how MAC drivers *should* behave. Some don't get it right.
So part of your confusion may be caused by misbehaving MAC drivers.
One example of a MAC driver bug is what I wrote earlier about
stmmac_ethtool_op_get_eee().

And what I write here refers to plain phylib, I don't cover phylink as
additional layer.


Thank you for your detailed explanation. It has been very helpful and has clarified how the code behaves.

Based on your and Andrew's input, I agree that phy_update_eee() is not needed.

I will ensure that our implementation follows these guidelines and will address any potential issues with misbehaving MAC drivers.

Thank you again for your valuable insights.