[PATCH net-next v2 0/6] net: eee network driver cleanups

[PATCH net-next v2 0/6] net: eee network driver cleanups

[Russell King (Oracle)]

Hi,

Since commit d1420bb99515 ("net: phy: improve generic EEE ethtool
functions") changed phylib to set eee->eee_active and eee->eee_enabled,
overriding anything that drivers have set these to prior to calling
phy_ethtool_get_eee().

Therefore, drivers setting these members becomes redundant, since
phylib overwrites the values they set. This series finishes off
Heiner's work in the referenced commit by removing these redundant
writes in various drivers and any associated code or structure members
that become unnecessary.

v2: Address Andrew's comment on fec_main.c

 drivers/net/dsa/b53/b53_common.c                     | 6 ------
 drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c  | 4 ----
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c     | 5 +++--
 drivers/net/ethernet/broadcom/genet/bcmgenet.c       | 8 +++-----
 drivers/net/ethernet/broadcom/genet/bcmmii.c         | 5 +++--
 drivers/net/ethernet/freescale/fec_main.c            | 4 ----
 drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h    | 1 -
 drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c   | 2 --
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
 10 files changed, 9 insertions(+), 29 deletions(-)
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 1/6] net: stmmac: remove eee_enabled/eee_active in stmmac_ethtool_op_get_eee()

[Russell King (Oracle)]

stmmac_ethtool_op_get_eee() sets both eee_enabled and eee_active, and
then goes on to call phylink_ethtool_get_eee().

phylink_ethtool_get_eee() will return -EOPNOTSUPP if there is no PHY
present, otherwise calling 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.

Thus, when there is no PHY, stmmac_ethtool_op_get_eee() will fail with
-EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to
userspace. When there is a PHY, eee_enabled and eee_active will be
overwritten by phylib, making the setting of these members in
stmmac_ethtool_op_get_eee() entirely unnecessary.

Remove this code, thus simplifying stmmac_ethtool_op_get_eee().

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index bbecb3b89535..411c3ac8cb17 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -859,8 +859,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
 	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 2/6] net: sxgbe: remove eee_enabled/eee_active in sxgbe_get_eee()

[Russell King (Oracle)]

sxgbe_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 sxgbe_get_eee() is redundant.
Remove this, and remove the priv->eee_active member which then becomes
a write-only variable.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h  | 1 -
 drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c | 2 --
 drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c    | 1 -
 3 files changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
index d14e0cfc3a6b..1458939c3bf5 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h
@@ -503,7 +503,6 @@ struct sxgbe_priv_data {
 	bool tx_path_in_lpi_mode;
 	int lpi_irq;
 	int eee_enabled;
-	int eee_active;
 	int tx_lpi_timer;
 };
 
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
index d93b628b7046..4a439b34114d 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c
@@ -140,8 +140,6 @@ static int sxgbe_get_eee(struct net_device *dev,
 	if (!priv->hw_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->eee_enabled = priv->eee_enabled;
-	edata->eee_active = priv->eee_active;
 	edata->tx_lpi_timer = priv->tx_lpi_timer;
 
 	return phy_ethtool_get_eee(dev->phydev, edata);
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
index 71439825ea4e..ecbe3994f2b1 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
@@ -130,7 +130,6 @@ bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
 		if (phy_init_eee(ndev->phydev, true))
 			return false;
 
-		priv->eee_active = 1;
 		timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0);
 		priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer);
 		add_timer(&priv->eee_ctrl_timer);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 3/6] net: fec: remove eee_enabled/eee_active in fec_enet_get_eee()

[Russell King (Oracle)]

fec_enet_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 fec_enet_get_eee() is redundant.
Remove this, and remove the setting of fep->eee.eee_active member which
becomes a write-only variable.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/freescale/fec_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 63707e065141..42bdc01a304e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3139,8 +3139,6 @@ static int fec_enet_eee_mode_set(struct net_device *ndev, bool enable)
 	}
 
 	p->tx_lpi_enabled = enable;
-	p->eee_enabled = enable;
-	p->eee_active = enable;
 
 	writel(sleep_cycle, fep->hwp + FEC_LPI_SLEEP);
 	writel(wake_cycle, fep->hwp + FEC_LPI_WAKE);
@@ -3160,8 +3158,6 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_keee *edata)
 	if (!netif_running(ndev))
 		return -ENETDOWN;
 
-	edata->eee_enabled = p->eee_enabled;
-	edata->eee_active = p->eee_active;
 	edata->tx_lpi_timer = p->tx_lpi_timer;
 	edata->tx_lpi_enabled = p->tx_lpi_enabled;
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 4/6] net: bcmgenet: remove eee_enabled/eee_active in bcmgenet_get_eee()

[Russell King (Oracle)]

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.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
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(-)

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);
 }
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 5/6] net: bcmasp: remove eee_enabled/eee_active in bcmasp_get_eee()

[Russell King (Oracle)]

bcmasp_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 bcmasp_get_eee() is redundant, and
can be removed. This also makes intf->eee.eee_active unnecessary, so
remove this and use a local variable where appropriate.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c | 4 ----
 drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c    | 5 +++--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
index 2851bed153e6..484fc2b5626f 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_ethtool.c
@@ -360,7 +360,6 @@ void bcmasp_eee_enable_set(struct bcmasp_intf *intf, bool enable)
 	umac_wl(intf, reg, UMC_EEE_CTRL);
 
 	intf->eee.eee_enabled = enable;
-	intf->eee.eee_active = enable;
 }
 
 static int bcmasp_get_eee(struct net_device *dev, struct ethtool_keee *e)
@@ -371,8 +370,6 @@ static int bcmasp_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 = umac_rl(intf, UMC_EEE_LPI_TIMER);
 
@@ -399,7 +396,6 @@ static int bcmasp_set_eee(struct net_device *dev, struct ethtool_keee *e)
 		}
 
 		umac_wl(intf, e->tx_lpi_timer, UMC_EEE_LPI_TIMER);
-		intf->eee.eee_active = ret >= 0;
 		intf->eee.tx_lpi_enabled = e->tx_lpi_enabled;
 		bcmasp_eee_enable_set(intf, true);
 	}
diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index 53e542881255..3a15f269c7d1 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -607,6 +607,7 @@ static void bcmasp_adj_link(struct net_device *dev)
 	struct phy_device *phydev = dev->phydev;
 	u32 cmd_bits = 0, reg;
 	int changed = 0;
+	bool active;
 
 	if (intf->old_link != phydev->link) {
 		changed = 1;
@@ -658,8 +659,8 @@ static void bcmasp_adj_link(struct net_device *dev)
 		reg |= cmd_bits;
 		umac_wl(intf, reg, UMC_CMD);
 
-		intf->eee.eee_active = phy_init_eee(phydev, 0) >= 0;
-		bcmasp_eee_enable_set(intf, intf->eee.eee_active);
+		active = phy_init_eee(phydev, 0) >= 0;
+		bcmasp_eee_enable_set(intf, active);
 	}
 
 	reg = rgmii_rl(intf, RGMII_OOB_CNTRL);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Russell King (Oracle)]

b53_get_mac_eee() sets both eee_enabled and eee_active, and then
returns zero.

dsa_slave_get_eee(), which calls this function, will then continue to
call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there
is no PHY present, otherwise calling 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.

Thus, when there is no PHY, dsa_slave_get_eee() will fail with
-EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to
userspace. When there is a PHY, eee_enabled and eee_active will be
overwritten by phylib, making the setting of these members in
b53_get_mac_eee() entirely unnecessary.

Remove this code, thus simplifying b53_get_mac_eee().

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/dsa/b53/b53_common.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index adc93abf4551..9e4c9bd6abcc 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init);
 int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
 {
 	struct b53_device *dev = ds->priv;
-	struct ethtool_keee *p = &dev->ports[port].eee;
-	u16 reg;
 
 	if (is5325(dev) || is5365(dev))
 		return -EOPNOTSUPP;
 
-	b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, &reg);
-	e->eee_enabled = p->eee_enabled;
-	e->eee_active = !!(reg & BIT(port));
-
 	return 0;
 }
 EXPORT_SYMBOL(b53_get_mac_eee);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 3/6] net: fec: remove eee_enabled/eee_active in fec_enet_get_eee()

[Andrew Lunn]

On Sun, Feb 04, 2024 at 12:13:12PM +0000, Russell King (Oracle) wrote:
> fec_enet_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 fec_enet_get_eee() is redundant.
> Remove this, and remove the setting of fep->eee.eee_active member which
> becomes a write-only variable.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 1/6] net: stmmac: remove eee_enabled/eee_active in stmmac_ethtool_op_get_eee()

[Serge Semin]

On Sun, Feb 04, 2024 at 12:13:02PM +0000, Russell King (Oracle) wrote:
> stmmac_ethtool_op_get_eee() sets both eee_enabled and eee_active, and
> then goes on to call phylink_ethtool_get_eee().
> 
> phylink_ethtool_get_eee() will return -EOPNOTSUPP if there is no PHY
> present, otherwise calling 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.
> 
> Thus, when there is no PHY, stmmac_ethtool_op_get_eee() will fail with
> -EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to
> userspace. When there is a PHY, eee_enabled and eee_active will be
> overwritten by phylib, making the setting of these members in
> stmmac_ethtool_op_get_eee() entirely unnecessary.
> 
> Remove this code, thus simplifying stmmac_ethtool_op_get_eee().

Right. and AFAICS stmmac_priv::{eee_active,eee_enabled} will be in
sync with the ethtool_eee::{eee_active,eee_enabled} values since they
are basically determined by means of the genphy_c45_eee_is_active()
method too. But damn it wasn't that easy to figure out... Anyway
thanks for the patch:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index bbecb3b89535..411c3ac8cb17 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -859,8 +859,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
>  	if (!priv->dma_cap.eee)
>  		return -EOPNOTSUPP;
>  
> -	edata->eee_enabled = priv->eee_enabled;
> -	edata->eee_active = priv->eee_active;
>  	edata->tx_lpi_timer = priv->tx_lpi_timer;
>  	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
>  
> -- 
> 2.30.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Vladimir Oltean]

On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote:
> b53_get_mac_eee() sets both eee_enabled and eee_active, and then
> returns zero.
> 
> dsa_slave_get_eee(), which calls this function, will then continue to
> call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there
> is no PHY present, otherwise calling phy_ethtool_get_eee() which in
> turn will call genphy_c45_ethtool_get_eee().

Nitpick: If you need to resend, the function name changed to
dsa_user_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.
> 
> Thus, when there is no PHY, dsa_slave_get_eee() will fail with

Here too.

> -EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to
> userspace. When there is a PHY, eee_enabled and eee_active will be
> overwritten by phylib, making the setting of these members in
> b53_get_mac_eee() entirely unnecessary.
> 
> Remove this code, thus simplifying b53_get_mac_eee().
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/dsa/b53/b53_common.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index adc93abf4551..9e4c9bd6abcc 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init);
>  int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
>  {
>  	struct b53_device *dev = ds->priv;
> -	struct ethtool_keee *p = &dev->ports[port].eee;
> -	u16 reg;
>  
>  	if (is5325(dev) || is5365(dev))
>  		return -EOPNOTSUPP;
>  
> -	b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, &reg);
> -	e->eee_enabled = p->eee_enabled;
> -	e->eee_active = !!(reg & BIT(port));
> -

I know next to nothing about EEE and especially the implementation on
Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
completely redundant? Is it actually in the system's best interest to
ignore it?

>  	return 0;
>  }
>  EXPORT_SYMBOL(b53_get_mac_eee);
> -- 
> 2.30.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Russell King (Oracle)]

On Tue, Feb 06, 2024 at 01:20:24PM +0200, Vladimir Oltean wrote:
> On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote:
> > b53_get_mac_eee() sets both eee_enabled and eee_active, and then
> > returns zero.
> > 
> > dsa_slave_get_eee(), which calls this function, will then continue to
> > call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there
> > is no PHY present, otherwise calling phy_ethtool_get_eee() which in
> > turn will call genphy_c45_ethtool_get_eee().
> 
> Nitpick: If you need to resend, the function name changed to
> dsa_user_get_eee().

Thanks.

> > @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init);
> >  int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
> >  {
> >  	struct b53_device *dev = ds->priv;
> > -	struct ethtool_keee *p = &dev->ports[port].eee;
> > -	u16 reg;
> >  
> >  	if (is5325(dev) || is5365(dev))
> >  		return -EOPNOTSUPP;
> >  
> > -	b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, &reg);
> > -	e->eee_enabled = p->eee_enabled;
> > -	e->eee_active = !!(reg & BIT(port));
> > -
> 
> I know next to nothing about EEE and especially the implementation on
> Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
> completely redundant? Is it actually in the system's best interest to
> ignore it?

That's a review comment that should have been made when the original
change to phylib was done, because it's already ignored in kernels
today since the commit changing phylib that I've referenced in this
series - since e->eee_enabled and e->eee_active will be overwritten by
phylib.

If we need B53_EEE_LPI_INDICATE to do something, then we need to have
a discussion about it, and decide how that fits in with the EEE
interface, and how to work around phylib's implementation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Vladimir Oltean]

On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote:
> > I know next to nothing about EEE and especially the implementation on
> > Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
> > completely redundant? Is it actually in the system's best interest to
> > ignore it?
> 
> That's a review comment that should have been made when the original
> change to phylib was done, because it's already ignored in kernels
> today since the commit changing phylib that I've referenced in this
> series - since e->eee_enabled and e->eee_active will be overwritten by
> phylib.

That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE
ethtool functions") is dated November 2018, and my involvement with the
kernel started in March 2019. So it would have been a bit difficult for
me to make this observation back then.

> If we need B53_EEE_LPI_INDICATE to do something, then we need to have
> a discussion about it, and decide how that fits in with the EEE
> interface, and how to work around phylib's implementation.

Hopefully Florian or Doug can quickly clarify whether this is the case
or not.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Florian Fainelli]

On 2/6/2024 5:29 AM, Vladimir Oltean wrote:
> On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote:
>>> I know next to nothing about EEE and especially the implementation on
>>> Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
>>> completely redundant? Is it actually in the system's best interest to
>>> ignore it?
>>
>> That's a review comment that should have been made when the original
>> change to phylib was done, because it's already ignored in kernels
>> today since the commit changing phylib that I've referenced in this
>> series - since e->eee_enabled and e->eee_active will be overwritten by
>> phylib.
> 
> That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE
> ethtool functions") is dated November 2018, and my involvement with the
> kernel started in March 2019. So it would have been a bit difficult for
> me to make this observation back then.
> 
>> If we need B53_EEE_LPI_INDICATE to do something, then we need to have
>> a discussion about it, and decide how that fits in with the EEE
>> interface, and how to work around phylib's implementation.
> 
> Hopefully Florian or Doug can quickly clarify whether this is the case
> or not.

Russell's replacement is actually a better one because it will return a 
stable state. B53_EEE_LPI_INDICATE would indicate when the switch port's 
built-in PHY asserts the LPI signal to its MAC, which could be transient 
AFAICT.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Vladimir Oltean]

On Tue, Feb 06, 2024 at 08:25:17PM -0800, Florian Fainelli wrote:
> 
> 
> On 2/6/2024 5:29 AM, Vladimir Oltean wrote:
> > On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote:
> > > > I know next to nothing about EEE and especially the implementation on
> > > > Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE
> > > > completely redundant? Is it actually in the system's best interest to
> > > > ignore it?
> > > 
> > > That's a review comment that should have been made when the original
> > > change to phylib was done, because it's already ignored in kernels
> > > today since the commit changing phylib that I've referenced in this
> > > series - since e->eee_enabled and e->eee_active will be overwritten by
> > > phylib.
> > 
> > That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE
> > ethtool functions") is dated November 2018, and my involvement with the
> > kernel started in March 2019. So it would have been a bit difficult for
> > me to make this observation back then.
> > 
> > > If we need B53_EEE_LPI_INDICATE to do something, then we need to have
> > > a discussion about it, and decide how that fits in with the EEE
> > > interface, and how to work around phylib's implementation.
> > 
> > Hopefully Florian or Doug can quickly clarify whether this is the case
> > or not.
> 
> Russell's replacement is actually a better one because it will return a
> stable state. B53_EEE_LPI_INDICATE would indicate when the switch port's
> built-in PHY asserts the LPI signal to its MAC, which could be transient
> AFAICT.
> -- 
> Florian

Thanks, Florian.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

[Vladimir Oltean]

On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote:
> b53_get_mac_eee() sets both eee_enabled and eee_active, and then
> returns zero.
> 
> dsa_slave_get_eee(), which calls this function, will then continue to
> call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there
> is no PHY present, otherwise calling 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.
> 
> Thus, when there is no PHY, dsa_slave_get_eee() will fail with
> -EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to
> userspace. When there is a PHY, eee_enabled and eee_active will be
> overwritten by phylib, making the setting of these members in
> b53_get_mac_eee() entirely unnecessary.
> 
> Remove this code, thus simplifying b53_get_mac_eee().
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

I see the series was put in "Changes Requested", possibly due to my
clarification question. Let's see if I can change that.

---
pw-bot: under-review

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next v2 0/6] net: eee network driver cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 4 Feb 2024 12:12:43 +0000 you wrote:
> Hi,
> 
> Since commit d1420bb99515 ("net: phy: improve generic EEE ethtool
> functions") changed phylib to set eee->eee_active and eee->eee_enabled,
> overriding anything that drivers have set these to prior to calling
> phy_ethtool_get_eee().
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/6] net: stmmac: remove eee_enabled/eee_active in stmmac_ethtool_op_get_eee()
    https://git.kernel.org/netdev/net-next/c/0cb6daf549f9
  - [net-next,v2,2/6] net: sxgbe: remove eee_enabled/eee_active in sxgbe_get_eee()
    https://git.kernel.org/netdev/net-next/c/d0d8c548789d
  - [net-next,v2,3/6] net: fec: remove eee_enabled/eee_active in fec_enet_get_eee()
    https://git.kernel.org/netdev/net-next/c/b573cb0a5586
  - [net-next,v2,4/6] net: bcmgenet: remove eee_enabled/eee_active in bcmgenet_get_eee()
    https://git.kernel.org/netdev/net-next/c/409359c1c2ef
  - [net-next,v2,5/6] net: bcmasp: remove eee_enabled/eee_active in bcmasp_get_eee()
    https://git.kernel.org/netdev/net-next/c/0cbfdfe3fb80
  - [net-next,v2,6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()
    https://git.kernel.org/netdev/net-next/c/3465df5533af

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel