| Message ID | 20230327170201.2036708-6-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: Rework EEE | expand |
On Mon, Mar 27, 2023 at 07:01:43PM +0200, Andrew Lunn wrote: > The MAC driver changes its EEE hardware configuration in its > adjust_link callback. This is called when auto-neg completes. If > set_eee is called with a change to tx_lpi_enabled which does not > trigger an auto-neg, it is necessary to call the adjust_link callback > so that the MAC is reconfigured to take this change into account. > > When setting phydev->eee_active, take tx_lpi_enabled into account, so > the MAC drivers don't need to consider tx_lpi_enabled. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> Hmm.. > @@ -1619,11 +1619,20 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data) > > mutex_lock(&phydev->lock); > ret = genphy_c45_ethtool_set_eee(phydev, data); > - if (!ret) > + if (ret >= 0) { > + if (ret == 0) { > + /* auto-neg not triggered */ > + if (phydev->tx_lpi_enabled != data->tx_lpi_enabled) { > + phydev->tx_lpi_enabled = data->tx_lpi_enabled; > + if (phydev->link) > + phy_link_up(phydev); > + } > + } > phydev->tx_lpi_enabled = data->tx_lpi_enabled; So we set eee_active depending on tx_lpi_enabled: > + phydev->eee_active = (err & phydev->tx_lpi_enabled); However, if tx_lpi_enabled changes state in this function, we don't update phydev->eee_active, but it's phydev->eee_active that gets passed back to MAC drivers. Is that intentional?
On Mon, Mar 27, 2023 at 07:02:01PM +0100, Russell King (Oracle) wrote: > On Mon, Mar 27, 2023 at 07:01:43PM +0200, Andrew Lunn wrote: > > The MAC driver changes its EEE hardware configuration in its > > adjust_link callback. This is called when auto-neg completes. If > > set_eee is called with a change to tx_lpi_enabled which does not > > trigger an auto-neg, it is necessary to call the adjust_link callback > > so that the MAC is reconfigured to take this change into account. > > > > When setting phydev->eee_active, take tx_lpi_enabled into account, so > > the MAC drivers don't need to consider tx_lpi_enabled. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Hmm.. > > > @@ -1619,11 +1619,20 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data) > > > > mutex_lock(&phydev->lock); > > ret = genphy_c45_ethtool_set_eee(phydev, data); > > - if (!ret) > > + if (ret >= 0) { > > + if (ret == 0) { > > + /* auto-neg not triggered */ > > + if (phydev->tx_lpi_enabled != data->tx_lpi_enabled) { > > + phydev->tx_lpi_enabled = data->tx_lpi_enabled; > > + if (phydev->link) > > + phy_link_up(phydev); > > + } > > + } > > phydev->tx_lpi_enabled = data->tx_lpi_enabled; > > So we set eee_active depending on tx_lpi_enabled: > > > + phydev->eee_active = (err & phydev->tx_lpi_enabled); > > However, if tx_lpi_enabled changes state in this function, we don't > update phydev->eee_active, but it's phydev->eee_active that gets > passed back to MAC drivers. Is that intentional? No, that is a bug. I don't think phy_check_link_status() can be called here, since we might not be in state RUNNING. So a bit of refactoring is required to add a function which can update phydev->eee_active. Thanks Andrew
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index fee514b96ab1..84e859eae64b 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1431,6 +1431,8 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee); * * Description: it reportes the Supported/Advertisement/LP Advertisement * capabilities. + * Returns either error code, 0 if there was no change, or positive if + * there was a change which triggered auto-neg. */ int genphy_c45_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data) @@ -1464,9 +1466,12 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev, ret = genphy_c45_an_config_eee_aneg(phydev); if (ret < 0) return ret; - if (ret > 0) - return phy_restart_aneg(phydev); - + if (ret > 0) { + ret = phy_restart_aneg(phydev); + if (ret < 0) + return ret; + return 1; + } return 0; } EXPORT_SYMBOL(genphy_c45_ethtool_set_eee); diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 7d9205c3f235..b074ac5d1de1 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -933,7 +933,7 @@ static int phy_check_link_status(struct phy_device *phydev) if (err < 0) phydev->eee_active = false; else - phydev->eee_active = err; + phydev->eee_active = (err & phydev->tx_lpi_enabled); phy_link_up(phydev); } else if (!phydev->link && phydev->state != PHY_NOLINK) { phydev->state = PHY_NOLINK; @@ -1619,11 +1619,20 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data) mutex_lock(&phydev->lock); ret = genphy_c45_ethtool_set_eee(phydev, data); - if (!ret) + if (ret >= 0) { + if (ret == 0) { + /* auto-neg not triggered */ + if (phydev->tx_lpi_enabled != data->tx_lpi_enabled) { + phydev->tx_lpi_enabled = data->tx_lpi_enabled; + if (phydev->link) + phy_link_up(phydev); + } + } phydev->tx_lpi_enabled = data->tx_lpi_enabled; + } mutex_unlock(&phydev->lock); - return ret; + return (ret < 0 ? ret : 0); } EXPORT_SYMBOL(phy_ethtool_set_eee);
The MAC driver changes its EEE hardware configuration in its adjust_link callback. This is called when auto-neg completes. If set_eee is called with a change to tx_lpi_enabled which does not trigger an auto-neg, it is necessary to call the adjust_link callback so that the MAC is reconfigured to take this change into account. When setting phydev->eee_active, take tx_lpi_enabled into account, so the MAC drivers don't need to consider tx_lpi_enabled. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- drivers/net/phy/phy-c45.c | 11 ++++++++--- drivers/net/phy/phy.c | 15 ++++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-)