| Message ID | E1tFv3F-005yhT-AA@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: phylink managed EEE support | expand |
On Tue, Nov 26, 2024 at 12:52:21PM +0000, Russell King (Oracle) wrote: > @@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); > static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > const struct eee_config *old_cfg) > { > - if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled || > + bool enable_tx_lpi; > + > + if (!phydev->link) > + return; > + > + enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active; > + > + if (phydev->enable_tx_lpi != enable_tx_lpi || > phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) { I'm wondering whether this should be: if (phydev->enable_tx_lpi != enable_tx_lpi || (phydev->enable_tx_lpi && phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer)) { The argument for this change would be to avoid cycling the link when the LPI timer changes but we're not using LPI. The argument against this change would be that then we don't program the hardware, and if the driver reads the initial value from hardware and is unbound/rebound, we'll lose that update whereas before the phylib changes, it would have been preserved. The problem, however, are drivers where the LPI timer is dependent on the speed. Any thoughts?
On Wed, Nov 27, 2024 at 11:12:28AM +0000, Russell King (Oracle) wrote: > On Tue, Nov 26, 2024 at 12:52:21PM +0000, Russell King (Oracle) wrote: > > @@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); > > static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > const struct eee_config *old_cfg) > > { > > - if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled || > > + bool enable_tx_lpi; > > + > > + if (!phydev->link) > > + return; > > + > > + enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active; > > + > > + if (phydev->enable_tx_lpi != enable_tx_lpi || > > phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) { > > I'm wondering whether this should be: > > if (phydev->enable_tx_lpi != enable_tx_lpi || > (phydev->enable_tx_lpi && > phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer)) { > > The argument for this change would be to avoid cycling the link when the > LPI timer changes but we're not using LPI. > > The argument against this change would be that then we don't program the > hardware, and if the driver reads the initial value from hardware and > is unbound/rebound, we'll lose that update whereas before the phylib > changes, it would have been preserved. > > The problem, however, are drivers where the LPI timer is dependent on > the speed. > > Any thoughts? So far, i was not able to find any. But, like I said before, to get optimal performance and power saving balance, the lpi_timer should be link speed specific... at least, some day with appropriate user interface.
On Wed, Nov 27, 2024 at 11:12:28AM +0000, Russell King (Oracle) wrote: > On Tue, Nov 26, 2024 at 12:52:21PM +0000, Russell King (Oracle) wrote: > > @@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); > > static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, > > const struct eee_config *old_cfg) > > { > > - if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled || > > + bool enable_tx_lpi; > > + > > + if (!phydev->link) > > + return; > > + > > + enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active; > > + > > + if (phydev->enable_tx_lpi != enable_tx_lpi || > > phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) { > > I'm wondering whether this should be: > > if (phydev->enable_tx_lpi != enable_tx_lpi || > (phydev->enable_tx_lpi && > phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer)) { > > The argument for this change would be to avoid cycling the link when the > LPI timer changes but we're not using LPI. > > The argument against this change would be that then we don't program the > hardware, and if the driver reads the initial value from hardware and > is unbound/rebound, we'll lose that update whereas before the phylib > changes, it would have been preserved. unbound/rebound is a pretty unusual use case. I would not consider that a strong argument against it. This is the case where we don't need to perform negotiation. So it is going to be a fast operation compared to when we do need negotiation. So i wounder if we really need to care? Donald Knuth, Premature optimisation is the root of all evil, etc... Andrew
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index 96d0b3a5a9d3..944ae98ad110 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1530,7 +1530,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev, return ret; data->eee_enabled = is_enabled; - data->eee_active = ret; + data->eee_active = phydev->eee_active; linkmode_copy(data->supported, phydev->supported_eee); return 0; diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index a660a80f34b7..0d20b534122b 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -990,14 +990,14 @@ static int phy_check_link_status(struct phy_device *phydev) phydev->state = PHY_RUNNING; err = genphy_c45_eee_is_active(phydev, NULL, NULL, NULL); - if (err <= 0) - phydev->enable_tx_lpi = false; - else - phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled; + phydev->eee_active = err > 0; + phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && + phydev->eee_active; phy_link_up(phydev); } else if (!phydev->link && phydev->state != PHY_NOLINK) { phydev->state = PHY_NOLINK; + phydev->eee_active = false; phydev->enable_tx_lpi = false; phy_link_down(phydev); } @@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); static void phy_ethtool_set_eee_noneg(struct phy_device *phydev, const struct eee_config *old_cfg) { - if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled || + bool enable_tx_lpi; + + if (!phydev->link) + return; + + enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active; + + if (phydev->enable_tx_lpi != enable_tx_lpi || phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) { - phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg); - if (phydev->link) { - phydev->link = false; - phy_link_down(phydev); - phydev->link = true; - phy_link_up(phydev); - } + phydev->enable_tx_lpi = false; + phydev->link = false; + phy_link_down(phydev); + phydev->enable_tx_lpi = enable_tx_lpi; + phydev->link = true; + phy_link_up(phydev); } } diff --git a/include/linux/phy.h b/include/linux/phy.h index 77c6d6451638..563c46205685 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -602,6 +602,7 @@ struct macsec_ops; * @supported_eee: supported PHY EEE linkmodes * @advertising_eee: Currently advertised EEE linkmodes * @enable_tx_lpi: When True, MAC should transmit LPI to PHY + * @eee_active: phylib private state, indicating that EEE has been negotiated * @eee_cfg: User configuration of EEE * @lp_advertising: Current link partner advertised linkmodes * @host_interfaces: PHY interface modes supported by host @@ -723,6 +724,7 @@ struct phy_device { /* Energy efficient ethernet modes which should be prohibited */ __ETHTOOL_DECLARE_LINK_MODE_MASK(eee_broken_modes); bool enable_tx_lpi; + bool eee_active; struct eee_config eee_cfg; /* Host supported PHY interface types. Should be ignored if empty. */
When phy_ethtool_set_eee_noneg() detects a change in the LPI parameters, it attempts to update phylib state and trigger the link to cycle so the MAC sees the updated parameters. However, in doing so, it sets phydev->enable_tx_lpi depending on whether the EEE configuration allows the MAC to generate LPI without taking into account the result of negotiation. This can be demonstrated with a 1000base-T FD interface by: # ethtool --set-eee eno0 advertise 8 # cause EEE to be not negotiated # ethtool --set-eee eno0 tx-lpi off # ethtool --set-eee eno0 tx-lpi on This results in being true, despite EEE not having been negotiated and: # ethtool --show-eee eno0 EEE status: enabled - inactive Tx LPI: 250 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Fix this by keeping track of whether EEE was negotiated via a new eee_active member in struct phy_device, and include this state in the decision whether phydev->enable_tx_lpi should be set. Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes") Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy-c45.c | 2 +- drivers/net/phy/phy.c | 30 ++++++++++++++++++------------ include/linux/phy.h | 2 ++ 3 files changed, 21 insertions(+), 13 deletions(-)