| Message ID | E1rVpvs-002Pe6-1w@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: eee network driver cleanups | expand |
On Fri, Feb 02, 2024 at 09:34:00AM +0000, Russell King (Oracle) wrote: > bcmgenet_get_eee() sets edata->eee_active and edata->eee_enabled from > its own copy, and then calls phy_ethtool_get_eee() which in turn will > call genphy_c45_ethtool_get_eee(). > > genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active > with its own interpretation from the PHYs settings and negotiation > result. > > Therefore, setting these members in bcmgenet_get_eee() is redundant, > and can be removed. This also makes priv->eee.eee_active unnecessary, > so remove this and use a local variable where appropriate. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 2/2/2024 1:34 AM, Russell King (Oracle) wrote: > bcmgenet_get_eee() sets edata->eee_active and edata->eee_enabled from > its own copy, and then calls phy_ethtool_get_eee() which in turn will > call genphy_c45_ethtool_get_eee(). > > genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active > with its own interpretation from the PHYs settings and negotiation > result. > > Therefore, setting these members in bcmgenet_get_eee() is redundant, > and can be removed. This also makes priv->eee.eee_active unnecessary, > so remove this and use a local variable where appropriate. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> Is not there an opportunity for no longer overriding eee_enabled as well since genphy_c45_ethtool_get_eee() will set that variable too?
On 2/2/2024 5:17 PM, Florian Fainelli wrote: > > > On 2/2/2024 1:34 AM, Russell King (Oracle) wrote: >> bcmgenet_get_eee() sets edata->eee_active and edata->eee_enabled from >> its own copy, and then calls phy_ethtool_get_eee() which in turn will >> call genphy_c45_ethtool_get_eee(). >> >> genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active >> with its own interpretation from the PHYs settings and negotiation >> result. >> >> Therefore, setting these members in bcmgenet_get_eee() is redundant, >> and can be removed. This also makes priv->eee.eee_active unnecessary, >> so remove this and use a local variable where appropriate. >> >> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > > Is not there an opportunity for no longer overriding eee_enabled as well > since genphy_c45_ethtool_get_eee() will set that variable too? Scratch that comment, you are doing it in the getter.
On Fri, Feb 02, 2024 at 05:21:57PM -0800, Florian Fainelli wrote: > > > On 2/2/2024 5:17 PM, Florian Fainelli wrote: > > > > > > On 2/2/2024 1:34 AM, Russell King (Oracle) wrote: > > > bcmgenet_get_eee() sets edata->eee_active and edata->eee_enabled from > > > its own copy, and then calls phy_ethtool_get_eee() which in turn will > > > call genphy_c45_ethtool_get_eee(). > > > > > > genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active > > > with its own interpretation from the PHYs settings and negotiation > > > result. > > > > > > Therefore, setting these members in bcmgenet_get_eee() is redundant, > > > and can be removed. This also makes priv->eee.eee_active unnecessary, > > > so remove this and use a local variable where appropriate. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > > > > Is not there an opportunity for no longer overriding eee_enabled as well > > since genphy_c45_ethtool_get_eee() will set that variable too? > > Scratch that comment, you are doing it in the getter. Also, priv->eee.eee_enabled is used in bcmmii.c, so we can't get rid of it in the setter.
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 051c31fb17c2..7396e2823e32 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -1313,7 +1313,6 @@ void bcmgenet_eee_enable_set(struct net_device *dev, bool enable, } priv->eee.eee_enabled = enable; - priv->eee.eee_active = enable; priv->eee.tx_lpi_enabled = tx_lpi_enabled; } @@ -1328,8 +1327,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e) if (!dev->phydev) return -ENODEV; - e->eee_enabled = p->eee_enabled; - e->eee_active = p->eee_active; e->tx_lpi_enabled = p->tx_lpi_enabled; e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER); @@ -1340,6 +1337,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) { struct bcmgenet_priv *priv = netdev_priv(dev); struct ethtool_keee *p = &priv->eee; + bool active; if (GENET_IS_V1(priv)) return -EOPNOTSUPP; @@ -1352,9 +1350,9 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e) if (!p->eee_enabled) { bcmgenet_eee_enable_set(dev, false, false); } else { - p->eee_active = phy_init_eee(dev->phydev, false) >= 0; + active = phy_init_eee(dev->phydev, false) >= 0; bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER); - bcmgenet_eee_enable_set(dev, p->eee_active, e->tx_lpi_enabled); + bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled); } return phy_ethtool_set_eee(dev->phydev, e); diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c index 97ea76d443ab..cbbe004621bc 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c @@ -30,6 +30,7 @@ static void bcmgenet_mac_config(struct net_device *dev) struct bcmgenet_priv *priv = netdev_priv(dev); struct phy_device *phydev = dev->phydev; u32 reg, cmd_bits = 0; + bool active; /* speed */ if (phydev->speed == SPEED_1000) @@ -88,9 +89,9 @@ static void bcmgenet_mac_config(struct net_device *dev) } bcmgenet_umac_writel(priv, reg, UMAC_CMD); - priv->eee.eee_active = phy_init_eee(phydev, 0) >= 0; + active = phy_init_eee(phydev, 0) >= 0; bcmgenet_eee_enable_set(dev, - priv->eee.eee_enabled && priv->eee.eee_active, + priv->eee.eee_enabled && active, priv->eee.tx_lpi_enabled); }
bcmgenet_get_eee() sets edata->eee_active and edata->eee_enabled from its own copy, and then calls phy_ethtool_get_eee() which in turn will call genphy_c45_ethtool_get_eee(). genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active with its own interpretation from the PHYs settings and negotiation result. Therefore, setting these members in bcmgenet_get_eee() is redundant, and can be removed. This also makes priv->eee.eee_active unnecessary, so remove this and use a local variable where appropriate. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 8 +++----- drivers/net/ethernet/broadcom/genet/bcmmii.c | 5 +++-- 2 files changed, 6 insertions(+), 7 deletions(-)