Skip to contentSkip to conversation

net: dsa: cleanup EEE (part 1)

PATCH Applied Russell King (Oracle) 6 participants 23 messages Dec 10, 2024 – Dec 12, 2024

Subsystems (9)
lore
Share & tools
lore
#0 lore raw

cover net: dsa: cleanup EEE (part 1)

Hi,

First part of DSA EEE cleanups.

Patch 1 removes a useless test that is always false. dp->pl will always be set for user ports, so !dp->pl in the EEE methods will always be false.

Patch 2 adds support for a new DSA support_eee() method, which tells DSA whether the DSA driver supports EEE, and thus whether the ethtool set_eee() and get_eee() methods should return -EOPNOTSUPP.

Patch 3 adds a trivial implementation for this new method which indicates that EEE is supported.

Patches 4..8 adds implementations for .supports_eee() to all drivers that support EEE in some form.

Patch 9 switches the core DSA code to require a .supports_eee() implementation if DSA is supported. Any DSA driver that doesn't implement this method after this patch will not support the ethtool EEE methods.

Part 2 will remove the (now) useless .get_mac_eee() DSA operation.

 drivers/net/dsa/b53/b53_common.c       | 13 +++++++------
 drivers/net/dsa/b53/b53_priv.h         |  1 +
 drivers/net/dsa/bcm_sf2.c              |  1 +
 drivers/net/dsa/microchip/ksz_common.c | 20 +++++---------------
 drivers/net/dsa/mt7530.c               |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c       |  1 +
 drivers/net/dsa/qca/qca8k-8xxx.c       |  1 +
 include/net/dsa.h                      |  2 ++
 net/dsa/port.c                         | 16 ++++++++++++++++
 net/dsa/user.c                         | 12 ++++++++++--
 10 files changed, 45 insertions(+), 23 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/9 net: dsa: remove check for dp->pl in EEE methods

When user ports are initialised, a phylink instance is always created, and so dp->pl will always be non-NULL. The EEE methods are only used for user ports, so checking for dp->pl to be NULL makes no sense. No other phylink-calling method implements similar checks in DSA. Remove this unnecessary check.

---
 net/dsa/user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/dsa/user.c +2 -2
index 06c30a9e29ff..0640247b8f0a 100644
@@ -1229,7 +1229,7 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
 	int ret;
 
 	/* Port's PHY and MAC both need to be EEE capable */
-	if (!dev->phydev || !dp->pl)
+	if (!dev->phydev)
 		return -ENODEV;
 
 	if (!ds->ops->set_mac_eee)
@@ -1249,7 +1249,7 @@ static int dsa_user_get_eee(struct net_device *dev, struct ethtool_keee *e)
 	int ret;
 
 	/* Port's PHY and MAC both need to be EEE capable */
-	if (!dev->phydev || !dp->pl)
+	if (!dev->phydev)
 		return -ENODEV;
 
 	if (!ds->ops->get_mac_eee)
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 1/9
#2 lore raw

Re: [PATCH net-next 1/9] net: dsa: remove check for dp->pl in EEE methods

Quoted 7 lines · Russell King (Oracle)

When user ports are initialised, a phylink instance is always created, and so dp->pl will always be non-NULL. The EEE methods are only used for user ports, so checking for dp->pl to be NULL makes no sense. No other phylink-calling method implements similar checks in DSA. Remove this unnecessary check.

Signature · 1 line
Florian
#3 lore raw

PATCH 2/9 net: dsa: add hook to determine whether EEE is supported

In reply to Russell King (Oracle)

Add a hook to determine whether the switch supports EEE. This will return false if the switch does not, or true if it does. If the method is not implemented, we assume (currently) that the switch supports EEE.

---
 include/net/dsa.h | 1 +
 net/dsa/user.c    | 8 ++++++++
 2 files changed, 9 insertions(+)
include/net/dsa.h +1 -0
index 72ae65e7246a..aaa75bbaa0ea 100644
@@ -988,6 +988,7 @@ struct dsa_switch_ops {
 	/*
 	 * Port's MAC EEE settings
 	 */
+	bool	(*support_eee)(struct dsa_switch *ds, int port);
 	int	(*set_mac_eee)(struct dsa_switch *ds, int port,
 			       struct ethtool_keee *e);
 	int	(*get_mac_eee)(struct dsa_switch *ds, int port,
net/dsa/user.c +8 -0
index 0640247b8f0a..b54f61605a57 100644
@@ -1228,6 +1228,10 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
 	struct dsa_switch *ds = dp->ds;
 	int ret;
 
+	/* Check whether the switch supports EEE */
+	if (ds->ops->support_eee && !ds->ops->support_eee(ds, dp->index))
+		return -EOPNOTSUPP;
+
 	/* Port's PHY and MAC both need to be EEE capable */
 	if (!dev->phydev)
 		return -ENODEV;
@@ -1248,6 +1252,10 @@ static int dsa_user_get_eee(struct net_device *dev, struct ethtool_keee *e)
 	struct dsa_switch *ds = dp->ds;
 	int ret;
 
+	/* Check whether the switch supports EEE */
+	if (ds->ops->support_eee && !ds->ops->support_eee(ds, dp->index))
+		return -EOPNOTSUPP;
+
 	/* Port's PHY and MAC both need to be EEE capable */
 	if (!dev->phydev)
 		return -ENODEV;
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 2/9
#4 lore raw

Re: [PATCH net-next 2/9] net: dsa: add hook to determine whether EEE is supported

Quoted 6 lines · Russell King (Oracle)

Add a hook to determine whether the switch supports EEE. This will return false if the switch does not, or true if it does. If the method is not implemented, we assume (currently) that the switch supports EEE.

Signature · 1 line
Florian
#5 lore raw

PATCH 3/9 net: dsa: provide implementation of .support_eee()

In reply to Russell King (Oracle)

Provide a trivial implementation for the .support_eee() method which switch drivers can use to simply indicate that they support EEE on all their user ports.

---
 include/net/dsa.h |  1 +
 net/dsa/port.c    | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
include/net/dsa.h +1 -0
index aaa75bbaa0ea..4aeedb296d67 100644
@@ -1384,5 +1384,6 @@ static inline bool dsa_user_dev_check(const struct net_device *dev)
 
 netdev_tx_t dsa_enqueue_skb(struct sk_buff *skb, struct net_device *dev);
 void dsa_port_phylink_mac_change(struct dsa_switch *ds, int port, bool up);
+bool dsa_supports_eee(struct dsa_switch *ds, int port);
 
 #endif
net/dsa/port.c +16 -0
index ee0aaec4c8e0..5c9d1798e830 100644
@@ -1575,6 +1575,22 @@ void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp,
 	cpu_dp->tag_ops = tag_ops;
 }
 
+/* dsa_supports_eee - indicate that EEE is supported
+ * @ds: pointer to &struct dsa_switch
+ * @port: port index
+ *
+ * A default implementation for the .support_eee() DSA operations member,
+ * which drivers can use to indicate that they support EEE on all of their
+ * user ports.
+ *
+ * Returns: true
+ */
+bool dsa_supports_eee(struct dsa_switch *ds, int port)
+{
+	return true;
+}
+EXPORT_SYMBOL_GPL(dsa_supports_eee);
+
 static void dsa_port_phylink_mac_config(struct phylink_config *config,
 					unsigned int mode,
 					const struct phylink_link_state *state)
Signature · 1 line
2.30.2
#7 lore raw

PATCH 4/9 net: dsa: b53/bcm_sf2: implement .support_eee() method

In reply to Russell King (Oracle)

Implement the .support_eee() method to indicate that EEE is not supported by two switch variants, rather than making these checks in the .set_mac_eee() and .get_mac_eee() methods.

---
 drivers/net/dsa/b53/b53_common.c | 13 +++++++------
 drivers/net/dsa/b53/b53_priv.h   |  1 +
 drivers/net/dsa/bcm_sf2.c        |  1 +
 3 files changed, 9 insertions(+), 6 deletions(-)
drivers/net/dsa/b53/b53_common.c +7 -6
index 285785c942b0..0561b60f668f 100644
@@ -2224,13 +2224,16 @@ int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy)
 }
 EXPORT_SYMBOL(b53_eee_init);
 
-int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
+bool b53_support_eee(struct dsa_switch *ds, int port)
 {
 	struct b53_device *dev = ds->priv;
 
-	if (is5325(dev) || is5365(dev))
-		return -EOPNOTSUPP;
+	return !is5325(dev) && !is5365(dev);
+}
+EXPORT_SYMBOL(b53_support_eee);
 
+int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e)
+{
 	return 0;
 }
 EXPORT_SYMBOL(b53_get_mac_eee);
@@ -2240,9 +2243,6 @@ int b53_set_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;
 
-	if (is5325(dev) || is5365(dev))
-		return -EOPNOTSUPP;
-
 	p->eee_enabled = e->eee_enabled;
 	b53_eee_enable_set(ds, port, e->eee_enabled);
 
@@ -2298,6 +2298,7 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.phylink_get_caps	= b53_phylink_get_caps,
 	.port_enable		= b53_enable_port,
 	.port_disable		= b53_disable_port,
+	.support_eee		= b53_support_eee,
 	.get_mac_eee		= b53_get_mac_eee,
 	.set_mac_eee		= b53_set_mac_eee,
 	.port_bridge_join	= b53_br_join,
drivers/net/dsa/b53/b53_priv.h +1 -0
index 05141176daf5..99e5cfc98ae8 100644
@@ -384,6 +384,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
 void b53_disable_port(struct dsa_switch *ds, int port);
 void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
+bool b53_support_eee(struct dsa_switch *ds, int port);
 int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e);
 int b53_set_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e);
 
drivers/net/dsa/bcm_sf2.c +1 -0
index 43bde1f583ff..a53fb6191e6b 100644
@@ -1232,6 +1232,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.set_wol		= bcm_sf2_sw_set_wol,
 	.port_enable		= bcm_sf2_port_setup,
 	.port_disable		= bcm_sf2_port_disable,
+	.support_eee		= b53_support_eee,
 	.get_mac_eee		= b53_get_mac_eee,
 	.set_mac_eee		= b53_set_mac_eee,
 	.port_bridge_join	= b53_br_join,
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 4/9
#8 lore raw

Re: [PATCH net-next 4/9] net: dsa: b53/bcm_sf2: implement .support_eee() method

Quoted 5 lines · Russell King (Oracle)

Implement the .support_eee() method to indicate that EEE is not supported by two switch variants, rather than making these checks in the .set_mac_eee() and .get_mac_eee() methods.

Signature · 1 line
Florian
#9 lore raw

PATCH 5/9 net: dsa: mt753x: implement .support_eee() method

In reply to Russell King (Oracle)

Implement the .support_eee() method by using the generic helper as all user ports support EEE.

---
 drivers/net/dsa/mt7530.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/dsa/mt7530.c +1 -0
index 086b8b3d5b40..9605febd3573 100644
@@ -3238,6 +3238,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
 	.port_mirror_add	= mt753x_port_mirror_add,
 	.port_mirror_del	= mt753x_port_mirror_del,
 	.phylink_get_caps	= mt753x_phylink_get_caps,
+	.support_eee		= dsa_supports_eee,
 	.get_mac_eee		= mt753x_get_mac_eee,
 	.set_mac_eee		= mt753x_set_mac_eee,
 	.conduit_state_change	= mt753x_conduit_state_change,
Signature · 1 line
2.30.2
#12 lore raw

PATCH 6/9 net: dsa: qca8k: implement .support_eee() method

In reply to Russell King (Oracle)

Implement the .support_eee() method by using the generic helper as all user ports support EEE.

---
 drivers/net/dsa/qca/qca8k-8xxx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/dsa/qca/qca8k-8xxx.c +1 -0
index 59b4a7240b58..ec74e3c2b0e9 100644
@@ -2016,6 +2016,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
 	.set_ageing_time	= qca8k_set_ageing_time,
+	.support_eee		= dsa_supports_eee,
 	.get_mac_eee		= qca8k_get_mac_eee,
 	.set_mac_eee		= qca8k_set_mac_eee,
 	.port_enable		= qca8k_port_enable,
Signature · 1 line
2.30.2
#14 lore raw

PATCH 7/9 net: dsa: mv88e6xxx: implement .support_eee() method

In reply to Russell King (Oracle)

Implement the .support_eee() method by using the generic helper as all user ports support EEE.

---
 drivers/net/dsa/mv88e6xxx/chip.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/dsa/mv88e6xxx/chip.c +1 -0
index 3a792f79270d..1d78974ea2a3 100644
@@ -7074,6 +7074,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 	.port_max_mtu		= mv88e6xxx_get_max_mtu,
 	.port_change_mtu	= mv88e6xxx_change_mtu,
+	.support_eee		= dsa_supports_eee,
 	.get_mac_eee		= mv88e6xxx_get_mac_eee,
 	.set_mac_eee		= mv88e6xxx_set_mac_eee,
 	.get_eeprom_len		= mv88e6xxx_get_eeprom_len,
Signature · 1 line
2.30.2
#16 lore raw

PATCH 8/9 net: dsa: ksz: implement .support_eee() method

In reply to Russell King (Oracle)

Implement the .support_eee() method by reusing the ksz_validate_eee() method as a template, renaming the function, changing the return type and values, and removing it from the ksz_set_mac_eee() and ksz_get_mac_eee() methods.

---
 drivers/net/dsa/microchip/ksz_common.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)
drivers/net/dsa/microchip/ksz_common.c +5 -15
index f5822c57be32..94f9aa983ff6 100644
@@ -3454,12 +3454,12 @@ static int ksz_max_mtu(struct dsa_switch *ds, int port)
 	return -EOPNOTSUPP;
 }
 
-static int ksz_validate_eee(struct dsa_switch *ds, int port)
+static bool ksz_support_eee(struct dsa_switch *ds, int port)
 {
 	struct ksz_device *dev = ds->priv;
 
 	if (!dev->info->internal_phy[port])
-		return -EOPNOTSUPP;
+		return false;
 
 	switch (dev->chip_id) {
 	case KSZ8563_CHIP_ID:
@@ -3471,21 +3471,15 @@ static int ksz_validate_eee(struct dsa_switch *ds, int port)
 	case KSZ9896_CHIP_ID:
 	case KSZ9897_CHIP_ID:
 	case LAN9646_CHIP_ID:
-		return 0;
+		return true;
 	}
 
-	return -EOPNOTSUPP;
+	return false;
 }
 
 static int ksz_get_mac_eee(struct dsa_switch *ds, int port,
 			   struct ethtool_keee *e)
 {
-	int ret;
-
-	ret = ksz_validate_eee(ds, port);
-	if (ret)
-		return ret;
-
 	/* There is no documented control of Tx LPI configuration. */
 	e->tx_lpi_enabled = true;
 
@@ -3501,11 +3495,6 @@ static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
 			   struct ethtool_keee *e)
 {
 	struct ksz_device *dev = ds->priv;
-	int ret;
-
-	ret = ksz_validate_eee(ds, port);
-	if (ret)
-		return ret;
 
 	if (!e->tx_lpi_enabled) {
 		dev_err(dev->dev, "Disabling EEE Tx LPI is not supported\n");
@@ -4651,6 +4640,7 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.cls_flower_add		= ksz_cls_flower_add,
 	.cls_flower_del		= ksz_cls_flower_del,
 	.port_setup_tc		= ksz_setup_tc,
+	.support_eee		= ksz_support_eee,
 	.get_mac_eee		= ksz_get_mac_eee,
 	.set_mac_eee		= ksz_set_mac_eee,
 	.port_get_default_prio	= ksz_port_get_default_prio,
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 8/9
#17 lore raw

Re: [PATCH net-next 8/9] net: dsa: ksz: implement .support_eee() method

Quoted 6 lines · Russell King (Oracle)

Implement the .support_eee() method by reusing the ksz_validate_eee() method as a template, renaming the function, changing the return type and values, and removing it from the ksz_set_mac_eee() and ksz_get_mac_eee() methods.

Signature · 1 line
Florian
#18 lore raw

PATCH 9/9 net: dsa: require .support_eee() method to be implemented

In reply to Russell King (Oracle)

Now that we have updated all drivers, switch DSA to require an implementation of the .support_eee() method for EEE to be usable, rather than defaulting to being permissive when not implemented.

---
 net/dsa/user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/dsa/user.c +2 -2
index b54f61605a57..4239083c18bf 100644
@@ -1229,7 +1229,7 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
 	int ret;
 
 	/* Check whether the switch supports EEE */
-	if (ds->ops->support_eee && !ds->ops->support_eee(ds, dp->index))
+	if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index))
 		return -EOPNOTSUPP;
 
 	/* Port's PHY and MAC both need to be EEE capable */
@@ -1253,7 +1253,7 @@ static int dsa_user_get_eee(struct net_device *dev, struct ethtool_keee *e)
 	int ret;
 
 	/* Check whether the switch supports EEE */
-	if (ds->ops->support_eee && !ds->ops->support_eee(ds, dp->index))
+	if (!ds->ops->support_eee || !ds->ops->support_eee(ds, dp->index))
 		return -EOPNOTSUPP;
 
 	/* Port's PHY and MAC both need to be EEE capable */
Signature · 1 line
2.30.2
maintainer Reviewed-by on v1 9/9
#19 lore raw

Re: [PATCH net-next 9/9] net: dsa: require .support_eee() method to be implemented

Quoted 5 lines · Russell King (Oracle)

Now that we have updated all drivers, switch DSA to require an implementation of the .support_eee() method for EEE to be usable, rather than defaulting to being permissive when not implemented.

Signature · 1 line
Florian
maintainer Reviewed-by on the v1 series
#20 lore raw
In reply to Russell King (Oracle)
Quoted 18 lines · Russell King (Oracle)

Hi,

First part of DSA EEE cleanups.

Patch 1 removes a useless test that is always false. dp->pl will always be set for user ports, so !dp->pl in the EEE methods will always be false.

Patch 2 adds support for a new DSA support_eee() method, which tells DSA whether the DSA driver supports EEE, and thus whether the ethtool set_eee() and get_eee() methods should return -EOPNOTSUPP.

Patch 3 adds a trivial implementation for this new method which indicates that EEE is supported.

Patches 4..8 adds implementations for .supports_eee() to all drivers that support EEE in some form.

Quoted 6 lines

Patch 9 switches the core DSA code to require a .supports_eee() implementation if DSA is supported. Any DSA driver that doesn't implement this method after this patch will not support the ethtool EEE methods.

Part 2 will remove the (now) useless .get_mac_eee() DSA operation.

maintainer
#21 lore raw
In reply to Russell King (Oracle)
Quoted 1 line · Russell King (Oracle)

Part 2 will remove the (now) useless .get_mac_eee() DSA operation.

Thanks for the work. For part 2, could you also include changes to Documentation/networking/dsa/ to explain the new usage pattern?

#22 lore raw
In reply to Russell King (Oracle)
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Tue, 10 Dec 2024 14:17:52 +0000 you wrote:

Hi,

First part of DSA EEE cleanups.

Patch 1 removes a useless test that is always false. dp->pl will always be set for user ports, so !dp->pl in the EEE methods will always be false.

[...]

Here is the summary with links:
- [net-next,1/9] net: dsa: remove check for dp->pl in EEE methods

    https://git.kernel.org/netdev/net-next/c/66c366392e55
  - [net-next,2/9] net: dsa: add hook to determine whether EEE is supported
    https://git.kernel.org/netdev/net-next/c/9723a77318b7
  - [net-next,3/9] net: dsa: provide implementation of .support_eee()
    https://git.kernel.org/netdev/net-next/c/99379f587278
  - [net-next,4/9] net: dsa: b53/bcm_sf2: implement .support_eee() method
    https://git.kernel.org/netdev/net-next/c/c86692fc2cb7
  - [net-next,5/9] net: dsa: mt753x: implement .support_eee() method
    https://git.kernel.org/netdev/net-next/c/7eb4f3d9fe17
  - [net-next,6/9] net: dsa: qca8k: implement .support_eee() method
    https://git.kernel.org/netdev/net-next/c/fe3ef44385b2
  - [net-next,7/9] net: dsa: mv88e6xxx: implement .support_eee() method
    https://git.kernel.org/netdev/net-next/c/eb3126e720e7
  - [net-next,8/9] net: dsa: ksz: implement .support_eee() method
    https://git.kernel.org/netdev/net-next/c/801fd546c1ca
  - [net-next,9/9] net: dsa: require .support_eee() method to be implemented
    https://git.kernel.org/netdev/net-next/c/88325a291a0c

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html