netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern
@ 2021-08-09 19:03 Vladimir Oltean
  2021-08-09 19:03 ` [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:03 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
	Kurt Kanzenbach, Woojung Huh, UNGLinuxDriver, Sean Wang,
	Landen Chao, DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister

The DSA core and drivers currently iterate too much through the port
list of a switch. For example, this snippet:

	for (port = 0; port < ds->num_ports; port++) {
		if (!dsa_is_cpu_port(ds, port))
			continue;

		ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
	}

iterates through ds->num_ports once, and then calls dsa_is_cpu_port to
filter out the other types of ports. But that function has a hidden call
to dsa_to_port() in it, which contains:

	list_for_each_entry(dp, &dst->ports, list)
		if (dp->ds == ds && dp->index == p)
			return dp;

where the only thing we wanted to know in the first place was whether
dp->type == DSA_PORT_TYPE_CPU or not.

So it seems that the problem is that we are not iterating with the right
variable. We have an "int port" but in fact need a "struct dsa_port *dp".

This has started being an issue since this patch series:
https://patchwork.ozlabs.org/project/netdev/cover/20191020031941.3805884-1-vivien.didelot@gmail.com/

The currently proposed set of changes iterates like this:

	dsa_switch_for_each_cpu_port(cpu_dp, ds)
		err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
						   tag_ops->proto);

which iterates directly over ds->dst->ports, which is a list of struct
dsa_port *dp. This makes it much easier and more efficient to check
dp->type.

As a nice side effect, with the proposed driver API, driver writers are
now encouraged to use more efficient patterns, and not only due to less
iterations through the port list. For example, something like this:

	for (port = 0; port < ds->num_ports; port++)
		do_something();

probably does not need to do_something() for the ports that are disabled
in the device tree. But adding extra code for that would look like this:

	for (port = 0; port < ds->num_ports; port++) {
		if (!dsa_is_unused_port(ds, port))
			continue;

		do_something();
	}

and therefore, it is understandable that some driver writers may decide
to not bother. This patch series introduces a "dsa_switch_for_each_available_port"
macro which comes at no extra cost in terms of lines of code / number of
braces to the driver writer, but it has the "dsa_is_unused_port" check
embedded within it.

I changed as much code as I could, probably not all, but a start anyway.

Vladimir Oltean (4):
  net: dsa: introduce a dsa_port_is_unused helper
  net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers
  net: dsa: b53: express b53_for_each_port in terms of
    dsa_switch_for_each_port

 drivers/net/dsa/b53/b53_common.c              |  26 ++-
 drivers/net/dsa/b53/b53_priv.h                |   6 +-
 drivers/net/dsa/bcm_sf2.c                     |   8 +-
 drivers/net/dsa/hirschmann/hellcreek.c        |  27 +--
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   |  19 +-
 drivers/net/dsa/microchip/ksz9477.c           |  19 +-
 drivers/net/dsa/microchip/ksz_common.c        |  19 +-
 drivers/net/dsa/mt7530.c                      |  58 +++---
 drivers/net/dsa/mv88e6xxx/chip.c              |  37 ++--
 drivers/net/dsa/mv88e6xxx/hwtstamp.c          |  10 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  12 +-
 drivers/net/dsa/ocelot/felix.c                |  79 +++-----
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  10 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c      |  14 +-
 drivers/net/dsa/qca8k.c                       |  32 ++--
 drivers/net/dsa/sja1105/sja1105_main.c        | 176 ++++++++----------
 drivers/net/dsa/sja1105/sja1105_mdio.c        |  12 +-
 drivers/net/dsa/xrs700x/xrs700x.c             |  37 ++--
 include/net/dsa.h                             |  43 ++++-
 net/dsa/dsa.c                                 |  22 +--
 net/dsa/dsa2.c                                |  11 +-
 net/dsa/port.c                                |   7 +-
 net/dsa/switch.c                              |  41 ++--
 net/dsa/tag_8021q.c                           |  29 ++-
 24 files changed, 360 insertions(+), 394 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper
  2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
@ 2021-08-09 19:03 ` Vladimir Oltean
  2021-08-10  9:34   ` Florian Fainelli
  2021-08-09 19:03 ` [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:03 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
	Kurt Kanzenbach, Woojung Huh, UNGLinuxDriver, Sean Wang,
	Landen Chao, DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister

Similar to the existing dsa_port_is_{cpu,user,dsa} helpers which operate
directly on a struct dsa_port *dp, let's introduce the equivalent of
dsa_is_unused_port. We will use this to create a more efficient iterator
over the available ports of a switch.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index cd7dc74d0d4c..d05c71a92715 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -431,6 +431,11 @@ static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
 	return NULL;
 }
 
+static inline bool dsa_port_is_unused(struct dsa_port *port)
+{
+	return port->type == DSA_PORT_TYPE_UNUSED;
+}
+
 static inline bool dsa_port_is_dsa(struct dsa_port *port)
 {
 	return port->type == DSA_PORT_TYPE_DSA;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
  2021-08-09 19:03 ` [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
@ 2021-08-09 19:03 ` Vladimir Oltean
  2021-08-10  3:33   ` DENG Qingfang
  2021-08-10  9:37   ` Florian Fainelli
  2021-08-09 19:03 ` [RFC PATCH net-next 3/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers Vladimir Oltean
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:03 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
	Kurt Kanzenbach, Woojung Huh, UNGLinuxDriver, Sean Wang,
	Landen Chao, DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister

Ever since Vivien's conversion of the ds->ports array into a dst->ports
list, and the introduction of dsa_to_port, iterations through the ports
of a switch became quadratic whenever dsa_to_port was needed.

dsa_to_port can either be called directly, or indirectly through the
dsa_is_{user,cpu,dsa,unused}_port helpers.

Introduce a basic switch port iterator macro, dsa_switch_for_each_port,
that works with the iterator variable being a struct dsa_port *dp
directly, and not an int i. It is an expensive variable to go from i to
dp, but cheap to go from dp to i.

This macro iterates through the entire ds->dst->ports list and filters
by the ports belonging just to the switch provided as argument.

While at it, provide some more flavors of that macro which filter for
specific types of ports: at the moment just user ports and CPU ports.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h   | 19 +++++++++++++++----
 net/dsa/dsa.c       | 22 +++++++++++-----------
 net/dsa/dsa2.c      | 11 +++++------
 net/dsa/port.c      |  7 ++++---
 net/dsa/switch.c    | 41 ++++++++++++++++++-----------------------
 net/dsa/tag_8021q.c | 29 +++++++++++++----------------
 6 files changed, 66 insertions(+), 63 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d05c71a92715..4639a82bab66 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -471,14 +471,25 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
 }
 
+#define dsa_switch_for_each_port(_dp, _ds) \
+	list_for_each_entry((_dp), &(_ds)->dst->ports, list) \
+		if ((_dp)->ds == (_ds))
+
+#define dsa_switch_for_each_user_port(_dp, _ds) \
+	dsa_switch_for_each_port((_dp), (_ds)) \
+		if (dsa_port_is_user((_dp)))
+
+#define dsa_switch_for_each_cpu_port(_dp, _ds) \
+	dsa_switch_for_each_port((_dp), (_ds)) \
+		if (dsa_port_is_cpu((_dp)))
+
 static inline u32 dsa_user_ports(struct dsa_switch *ds)
 {
+	struct dsa_port *dp;
 	u32 mask = 0;
-	int p;
 
-	for (p = 0; p < ds->num_ports; p++)
-		if (dsa_is_user_port(ds, p))
-			mask |= BIT(p);
+	dsa_switch_for_each_user_port(dp, ds)
+		mask |= BIT(dp->index);
 
 	return mask;
 }
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5e73332a9707..8104f2382988 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -280,23 +280,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 }
 
 #ifdef CONFIG_PM_SLEEP
-static bool dsa_is_port_initialized(struct dsa_switch *ds, int p)
+static bool dsa_port_is_initialized(const struct dsa_port *dp)
 {
-	const struct dsa_port *dp = dsa_to_port(ds, p);
-
 	return dp->type == DSA_PORT_TYPE_USER && dp->slave;
 }
 
 int dsa_switch_suspend(struct dsa_switch *ds)
 {
-	int i, ret = 0;
+	struct dsa_port *dp;
+	int ret = 0;
 
 	/* Suspend slave network devices */
-	for (i = 0; i < ds->num_ports; i++) {
-		if (!dsa_is_port_initialized(ds, i))
+	dsa_switch_for_each_port(dp, ds) {
+		if (!dsa_port_is_initialized(dp))
 			continue;
 
-		ret = dsa_slave_suspend(dsa_to_port(ds, i)->slave);
+		ret = dsa_slave_suspend(dp->slave);
 		if (ret)
 			return ret;
 	}
@@ -310,7 +309,8 @@ EXPORT_SYMBOL_GPL(dsa_switch_suspend);
 
 int dsa_switch_resume(struct dsa_switch *ds)
 {
-	int i, ret = 0;
+	struct dsa_port *dp;
+	int ret = 0;
 
 	if (ds->ops->resume)
 		ret = ds->ops->resume(ds);
@@ -319,11 +319,11 @@ int dsa_switch_resume(struct dsa_switch *ds)
 		return ret;
 
 	/* Resume slave network devices */
-	for (i = 0; i < ds->num_ports; i++) {
-		if (!dsa_is_port_initialized(ds, i))
+	dsa_switch_for_each_port(dp, ds) {
+		if (!dsa_port_is_initialized(dp))
 			continue;
 
-		ret = dsa_slave_resume(dsa_to_port(ds, i)->slave);
+		ret = dsa_slave_resume(dp->slave);
 		if (ret)
 			return ret;
 	}
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8150e16aaa55..13794578a1d0 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -707,16 +707,15 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
 {
 	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
 	struct dsa_switch_tree *dst = ds->dst;
-	int port, err;
+	struct dsa_port *cpu_dp;
+	int err;
 
 	if (tag_ops->proto == dst->default_proto)
 		return 0;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (!dsa_is_cpu_port(ds, port))
-			continue;
-
-		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
+	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
+		err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
+						   tag_ops->proto);
 		if (err) {
 			dev_err(ds->dev, "Unable to use tag protocol \"%s\": %pe\n",
 				tag_ops->name, ERR_PTR(err));
diff --git a/net/dsa/port.c b/net/dsa/port.c
index eedd9881e1ba..05b902d69863 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -540,7 +540,8 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
 					      struct netlink_ext_ack *extack)
 {
 	struct dsa_switch *ds = dp->ds;
-	int err, i;
+	struct dsa_port *other_dp;
+	int err;
 
 	/* VLAN awareness was off, so the question is "can we turn it on".
 	 * We may have had 8021q uppers, those need to go. Make sure we don't
@@ -582,10 +583,10 @@ static bool dsa_port_can_apply_vlan_filtering(struct dsa_port *dp,
 	 * different ports of the same switch device and one of them has a
 	 * different setting than what is being requested.
 	 */
-	for (i = 0; i < ds->num_ports; i++) {
+	dsa_switch_for_each_port(other_dp, ds) {
 		struct net_device *other_bridge;
 
-		other_bridge = dsa_to_port(ds, i)->bridge_dev;
+		other_bridge = other_dp->bridge_dev;
 		if (!other_bridge)
 			continue;
 		/* If it's the same bridge, it also has same
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index fd1a1c6bf9cf..5ddcf37ecfa5 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -17,14 +17,11 @@
 static unsigned int dsa_switch_fastest_ageing_time(struct dsa_switch *ds,
 						   unsigned int ageing_time)
 {
-	int i;
-
-	for (i = 0; i < ds->num_ports; ++i) {
-		struct dsa_port *dp = dsa_to_port(ds, i);
+	struct dsa_port *dp;
 
+	dsa_switch_for_each_port(dp, ds)
 		if (dp->ageing_time && dp->ageing_time < ageing_time)
 			ageing_time = dp->ageing_time;
-	}
 
 	return ageing_time;
 }
@@ -117,7 +114,8 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 	bool unset_vlan_filtering = br_vlan_enabled(info->br);
 	struct dsa_switch_tree *dst = ds->dst;
 	struct netlink_ext_ack extack = {0};
-	int err, port;
+	struct dsa_port *dp;
+	int err;
 
 	if (dst->index == info->tree_index && ds->index == info->sw_index &&
 	    ds->ops->port_bridge_leave)
@@ -138,10 +136,10 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 	 * VLAN-aware bridge.
 	 */
 	if (unset_vlan_filtering && ds->vlan_filtering_is_global) {
-		for (port = 0; port < ds->num_ports; port++) {
+		dsa_switch_for_each_port(dp, ds) {
 			struct net_device *bridge_dev;
 
-			bridge_dev = dsa_to_port(ds, port)->bridge_dev;
+			bridge_dev = dp->bridge_dev;
 
 			if (bridge_dev && br_vlan_enabled(bridge_dev)) {
 				unset_vlan_filtering = false;
@@ -566,38 +564,35 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
 				       struct dsa_notifier_tag_proto_info *info)
 {
 	const struct dsa_device_ops *tag_ops = info->tag_ops;
-	int port, err;
+	struct dsa_port *dp, *cpu_dp;
+	int err;
 
 	if (!ds->ops->change_tag_protocol)
 		return -EOPNOTSUPP;
 
 	ASSERT_RTNL();
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (!dsa_is_cpu_port(ds, port))
-			continue;
-
-		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
+	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
+		err = ds->ops->change_tag_protocol(ds, cpu_dp->index,
+						   tag_ops->proto);
 		if (err)
 			return err;
 
-		dsa_port_set_tag_protocol(dsa_to_port(ds, port), tag_ops);
+		dsa_port_set_tag_protocol(cpu_dp, tag_ops);
 	}
 
 	/* Now that changing the tag protocol can no longer fail, let's update
 	 * the remaining bits which are "duplicated for faster access", and the
 	 * bits that depend on the tagger, such as the MTU.
 	 */
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_user_port(ds, port)) {
-			struct net_device *slave;
+	dsa_switch_for_each_user_port(dp, ds) {
+		struct net_device *slave;
 
-			slave = dsa_to_port(ds, port)->slave;
-			dsa_slave_setup_tagger(slave);
+		slave = dp->slave;
+		dsa_slave_setup_tagger(slave);
 
-			/* rtnl_mutex is held in dsa_tree_change_tag_proto */
-			dsa_slave_change_mtu(slave, slave->mtu);
-		}
+		/* rtnl_mutex is held in dsa_tree_change_tag_proto */
+		dsa_slave_change_mtu(slave, slave->mtu);
 	}
 
 	return 0;
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 654697ebb6f3..b29f4eb9aed1 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -322,15 +322,13 @@ int dsa_switch_tag_8021q_vlan_del(struct dsa_switch *ds,
  * +-+-----+-+-----+-+-----+-+-----+-+    +-+-----+-+-----+-+-----+-+-----+-+
  *   swp0    swp1    swp2    swp3           swp0    swp1    swp2    swp3
  */
-static bool dsa_tag_8021q_bridge_match(struct dsa_switch *ds, int port,
+static bool dsa_tag_8021q_bridge_match(struct dsa_port *dp,
 				       struct dsa_notifier_bridge_info *info)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port);
-
 	/* Don't match on self */
-	if (ds->dst->index == info->tree_index &&
-	    ds->index == info->sw_index &&
-	    port == info->port)
+	if (dp->ds->dst->index == info->tree_index &&
+	    dp->ds->index == info->sw_index &&
+	    dp->index == info->port)
 		return false;
 
 	if (dsa_port_is_user(dp))
@@ -344,8 +342,9 @@ int dsa_tag_8021q_bridge_join(struct dsa_switch *ds,
 {
 	struct dsa_switch *targeted_ds;
 	struct dsa_port *targeted_dp;
+	struct dsa_port *dp;
 	u16 targeted_rx_vid;
-	int err, port;
+	int err;
 
 	if (!ds->tag_8021q_ctx)
 		return 0;
@@ -354,11 +353,10 @@ int dsa_tag_8021q_bridge_join(struct dsa_switch *ds,
 	targeted_dp = dsa_to_port(targeted_ds, info->port);
 	targeted_rx_vid = dsa_8021q_rx_vid(targeted_ds, info->port);
 
-	for (port = 0; port < ds->num_ports; port++) {
-		struct dsa_port *dp = dsa_to_port(ds, port);
-		u16 rx_vid = dsa_8021q_rx_vid(ds, port);
+	dsa_switch_for_each_port(dp, ds) {
+		u16 rx_vid = dsa_8021q_rx_vid(ds, dp->index);
 
-		if (!dsa_tag_8021q_bridge_match(ds, port, info))
+		if (!dsa_tag_8021q_bridge_match(dp, info))
 			continue;
 
 		/* Install the RX VID of the targeted port in our VLAN table */
@@ -380,8 +378,8 @@ int dsa_tag_8021q_bridge_leave(struct dsa_switch *ds,
 {
 	struct dsa_switch *targeted_ds;
 	struct dsa_port *targeted_dp;
+	struct dsa_port *dp;
 	u16 targeted_rx_vid;
-	int port;
 
 	if (!ds->tag_8021q_ctx)
 		return 0;
@@ -390,11 +388,10 @@ int dsa_tag_8021q_bridge_leave(struct dsa_switch *ds,
 	targeted_dp = dsa_to_port(targeted_ds, info->port);
 	targeted_rx_vid = dsa_8021q_rx_vid(targeted_ds, info->port);
 
-	for (port = 0; port < ds->num_ports; port++) {
-		struct dsa_port *dp = dsa_to_port(ds, port);
-		u16 rx_vid = dsa_8021q_rx_vid(ds, port);
+	dsa_switch_for_each_port(dp, ds) {
+		u16 rx_vid = dsa_8021q_rx_vid(ds, dp->index);
 
-		if (!dsa_tag_8021q_bridge_match(ds, port, info))
+		if (!dsa_tag_8021q_bridge_match(dp, info))
 			continue;
 
 		/* Remove the RX VID of the targeted port from our VLAN table */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH net-next 3/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers
  2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
  2021-08-09 19:03 ` [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
  2021-08-09 19:03 ` [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
@ 2021-08-09 19:03 ` Vladimir Oltean
  2021-08-09 19:03 ` [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port Vladimir Oltean
  2021-08-09 19:31 ` [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
  4 siblings, 0 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:03 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
	Kurt Kanzenbach, Woojung Huh, UNGLinuxDriver, Sean Wang,
	Landen Chao, DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister

Since the DSA conversion from the ds->ports array into the dst->ports
list, the DSA API has encouraged driver writers to write inefficient
code.

Currently, switch drivers which want to filter by a specific type of
port when iterating, like {!unused, user, cpu, dsa}, use the
dsa_is_*_port helper. Under the hood, this uses dsa_to_port which
iterates again through dst->ports. But the driver iterates through the
port list already, so the complexity is quadratic for the typical case
of a single-switch tree.

Many drivers also call dsa_to_port many times while iterating and do not
even cache the result, probably unaware of the hidden complexity of this
function.

When drivers need to iterate between pairs of {to, from} ports, and use
any of the functions above, the complexity is even higher.

Use the newly introduced DSA port iterators, and also introduce some
more which are not directly needed by the DSA core.

Note that the b53_br_{join,leave} functions have been converted to use
the dp-based iterators, but to preserve the original behavior, the
dev->enabled_ports check from b53_for_each_port has been split out and
open-coded. This will be addressed in the next patch.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/b53/b53_common.c              |  22 ++-
 drivers/net/dsa/bcm_sf2.c                     |   8 +-
 drivers/net/dsa/hirschmann/hellcreek.c        |  27 +--
 .../net/dsa/hirschmann/hellcreek_hwtstamp.c   |  19 +-
 drivers/net/dsa/microchip/ksz9477.c           |  19 +-
 drivers/net/dsa/microchip/ksz_common.c        |  19 +-
 drivers/net/dsa/mt7530.c                      |  58 +++---
 drivers/net/dsa/mv88e6xxx/chip.c              |  37 ++--
 drivers/net/dsa/mv88e6xxx/hwtstamp.c          |  10 +-
 drivers/net/dsa/mv88e6xxx/port.c              |  12 +-
 drivers/net/dsa/ocelot/felix.c                |  79 +++-----
 drivers/net/dsa/ocelot/felix_vsc9959.c        |  10 +-
 drivers/net/dsa/ocelot/seville_vsc9953.c      |  14 +-
 drivers/net/dsa/qca8k.c                       |  32 ++--
 drivers/net/dsa/sja1105/sja1105_main.c        | 176 ++++++++----------
 drivers/net/dsa/sja1105/sja1105_mdio.c        |  12 +-
 drivers/net/dsa/xrs700x/xrs700x.c             |  37 ++--
 include/net/dsa.h                             |  19 ++
 18 files changed, 284 insertions(+), 326 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index bd1417a66cbf..ccd93147d994 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1851,6 +1851,7 @@ int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
 	s8 cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
+	struct dsa_port *dp;
 	u16 pvlan, reg;
 	unsigned int i;
 
@@ -1873,8 +1874,13 @@ int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 
 	b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(port), &pvlan);
 
-	b53_for_each_port(dev, i) {
-		if (dsa_to_port(ds, i)->bridge_dev != br)
+	dsa_switch_for_each_port(dp, ds) {
+		i = dp->index;
+
+		if (!(dev->enabled_ports & BIT(i)))
+			continue;
+
+		if (dp->bridge_dev != br)
 			continue;
 
 		/* Add this local port to the remote port VLAN control
@@ -1903,14 +1909,20 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 	struct b53_device *dev = ds->priv;
 	struct b53_vlan *vl = &dev->vlans[0];
 	s8 cpu_port = dsa_to_port(ds, port)->cpu_dp->index;
-	unsigned int i;
 	u16 pvlan, reg, pvid;
+	struct dsa_port *dp;
+	unsigned int i;
 
 	b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(port), &pvlan);
 
-	b53_for_each_port(dev, i) {
+	dsa_switch_for_each_port(dp, ds) {
+		i = dp->index;
+
+		if (!(dev->enabled_ports & BIT(i)))
+			continue;
+
 		/* Don't touch the remaining ports */
-		if (dsa_to_port(ds, i)->bridge_dev != br)
+		if (dp->bridge_dev != br)
 			continue;
 
 		b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(i), &reg);
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 6ce9ec1283e0..2f78fb88ed9e 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -913,7 +913,7 @@ static void bcm_sf2_enable_acb(struct dsa_switch *ds)
 static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	unsigned int port;
+	struct dsa_port *dp;
 
 	bcm_sf2_intr_disable(priv);
 
@@ -921,10 +921,8 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 	 * port, the other ones have already been disabled during
 	 * bcm_sf2_sw_setup
 	 */
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_user_port(ds, port) || dsa_is_cpu_port(ds, port))
-			bcm_sf2_port_disable(ds, port);
-	}
+	dsa_switch_for_each_available_port(dp, ds)
+		bcm_sf2_port_disable(ds, dp->index);
 
 	if (!priv->wol_ports_mask)
 		clk_disable_unprepare(priv->clk);
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 9fdcc4bde480..c457b521942d 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -345,7 +345,7 @@ static int hellcreek_vlan_prepare(struct dsa_switch *ds, int port,
 				  struct netlink_ext_ack *extack)
 {
 	struct hellcreek *hellcreek = ds->priv;
-	int i;
+	struct dsa_port *dp;
 
 	dev_dbg(hellcreek->dev, "VLAN prepare for port %d\n", port);
 
@@ -353,11 +353,8 @@ static int hellcreek_vlan_prepare(struct dsa_switch *ds, int port,
 	 * VLANs are internally used by the driver to ensure port
 	 * separation. Thus, they cannot be used by someone else.
 	 */
-	for (i = 0; i < hellcreek->pdata->num_ports; ++i) {
-		const u16 restricted_vid = hellcreek_private_vid(i);
-
-		if (!dsa_is_user_port(ds, i))
-			continue;
+	dsa_switch_for_each_user_port(dp, ds) {
+		const u16 restricted_vid = hellcreek_private_vid(dp->index);
 
 		if (vlan->vid == restricted_vid) {
 			NL_SET_ERR_MSG_MOD(extack, "VID restricted by driver");
@@ -1301,8 +1298,9 @@ static void hellcreek_teardown_devlink_regions(struct dsa_switch *ds)
 static int hellcreek_setup(struct dsa_switch *ds)
 {
 	struct hellcreek *hellcreek = ds->priv;
+	struct dsa_port *dp;
 	u16 swcfg = 0;
-	int ret, i;
+	int ret;
 
 	dev_dbg(hellcreek->dev, "Set up the switch\n");
 
@@ -1328,12 +1326,8 @@ static int hellcreek_setup(struct dsa_switch *ds)
 	hellcreek_write(hellcreek, swcfg, HR_SWCFG);
 
 	/* Initial vlan membership to reflect port separation */
-	for (i = 0; i < ds->num_ports; ++i) {
-		if (!dsa_is_user_port(ds, i))
-			continue;
-
-		hellcreek_setup_vlan_membership(ds, i, true);
-	}
+	dsa_switch_for_each_user_port(dp, ds)
+		hellcreek_setup_vlan_membership(ds, dp->index, true);
 
 	/* Configure PCP <-> TC mapping */
 	hellcreek_setup_tc_identity_mapping(hellcreek);
@@ -1410,10 +1404,10 @@ hellcreek_port_prechangeupper(struct dsa_switch *ds, int port,
 			      struct netdev_notifier_changeupper_info *info)
 {
 	struct hellcreek *hellcreek = ds->priv;
+	struct dsa_port *dp;
 	bool used = true;
 	int ret = -EBUSY;
 	u16 vid;
-	int i;
 
 	dev_dbg(hellcreek->dev, "Pre change upper for port %d\n", port);
 
@@ -1432,9 +1426,8 @@ hellcreek_port_prechangeupper(struct dsa_switch *ds, int port,
 
 	/* For all ports, check bitmaps */
 	mutex_lock(&hellcreek->vlan_lock);
-	for (i = 0; i < hellcreek->pdata->num_ports; ++i) {
-		if (!dsa_is_user_port(ds, i))
-			continue;
+	dsa_switch_for_each_user_port(dp, ds) {
+		int i = dp->index;
 
 		if (port == i)
 			continue;
diff --git a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
index 40b41c794dfa..1cbae1654dfe 100644
--- a/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
+++ b/drivers/net/dsa/hirschmann/hellcreek_hwtstamp.c
@@ -354,13 +354,12 @@ long hellcreek_hwtstamp_work(struct ptp_clock_info *ptp)
 {
 	struct hellcreek *hellcreek = ptp_to_hellcreek(ptp);
 	struct dsa_switch *ds = hellcreek->ds;
-	int i, restart = 0;
+	struct dsa_port *dp;
+	int restart = 0;
 
-	for (i = 0; i < ds->num_ports; i++) {
+	dsa_switch_for_each_user_port(dp, ds) {
 		struct hellcreek_port_hwtstamp *ps;
-
-		if (!dsa_is_user_port(ds, i))
-			continue;
+		int i = dp->index;
 
 		ps = &hellcreek->ports[i].port_hwtstamp;
 
@@ -459,15 +458,11 @@ static void hellcreek_hwtstamp_port_setup(struct hellcreek *hellcreek, int port)
 int hellcreek_hwtstamp_setup(struct hellcreek *hellcreek)
 {
 	struct dsa_switch *ds = hellcreek->ds;
-	int i;
+	struct dsa_port *dp;
 
 	/* Initialize timestamping ports. */
-	for (i = 0; i < ds->num_ports; ++i) {
-		if (!dsa_is_user_port(ds, i))
-			continue;
-
-		hellcreek_hwtstamp_port_setup(hellcreek, i);
-	}
+	dsa_switch_for_each_user_port(dp, ds)
+		hellcreek_hwtstamp_port_setup(hellcreek, dp->index);
 
 	/* Select the synchronized clock as the source timekeeper for the
 	 * timestamps and enable inline timestamping.
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 854e25f43fa7..a7435c581e49 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1266,11 +1266,14 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
+	struct dsa_port *dp;
 	struct ksz_port *p;
 	int i;
 
-	for (i = 0; i < dev->port_cnt; i++) {
-		if (dsa_is_cpu_port(ds, i) && (dev->cpu_ports & (1 << i))) {
+	dsa_switch_for_each_port(dp, ds) {
+		i = dp->index;
+
+		if (dsa_port_is_cpu(dp) && (dev->cpu_ports & (1 << i))) {
 			phy_interface_t interface;
 			const char *prev_msg;
 			const char *prev_mode;
@@ -1609,18 +1612,22 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 
 int ksz9477_switch_register(struct ksz_device *dev)
 {
-	int ret, i;
 	struct phy_device *phydev;
+	struct dsa_switch *ds;
+	struct dsa_port *dp;
+	int ret;
 
 	ret = ksz_switch_register(dev, &ksz9477_dev_ops);
 	if (ret)
 		return ret;
 
-	for (i = 0; i < dev->phy_port_cnt; ++i) {
-		if (!dsa_is_user_port(dev->ds, i))
+	ds = dev->ds;
+
+	dsa_switch_for_each_user_port(dp, ds) {
+		if (dp->index >= dev->phy_port_cnt)
 			continue;
 
-		phydev = dsa_to_port(dev->ds, i)->slave->phydev;
+		phydev = dp->slave->phydev;
 
 		/* The MAC actually cannot run in 1000 half-duplex mode. */
 		phy_remove_link_mode(phydev,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 1542bfb8b5e5..2b188f998115 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -68,28 +68,23 @@ static void ksz_mib_read_work(struct work_struct *work)
 {
 	struct ksz_device *dev = container_of(work, struct ksz_device,
 					      mib_read.work);
+	struct dsa_switch *ds = dev->ds;
 	struct ksz_port_mib *mib;
+	struct dsa_port *dp;
 	struct ksz_port *p;
-	int i;
-
-	for (i = 0; i < dev->port_cnt; i++) {
-		if (dsa_is_unused_port(dev->ds, i))
-			continue;
 
-		p = &dev->ports[i];
+	dsa_switch_for_each_available_port(dp, ds) {
+		p = &dev->ports[dp->index];
 		mib = &p->mib;
 		mutex_lock(&mib->cnt_mutex);
 
 		/* Only read MIB counters when the port is told to do.
 		 * If not, read only dropped counters when link is not up.
 		 */
-		if (!p->read) {
-			const struct dsa_port *dp = dsa_to_port(dev->ds, i);
+		if (!p->read && !netif_carrier_ok(dp->slave))
+			mib->cnt_ptr = dev->reg_mib_cnt;
 
-			if (!netif_carrier_ok(dp->slave))
-				mib->cnt_ptr = dev->reg_mib_cnt;
-		}
-		port_r_cnt(dev, i);
+		port_r_cnt(dev, dp->index);
 		p->read = false;
 		mutex_unlock(&mib->cnt_mutex);
 	}
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 53e6150e95b6..010d4bb7794f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1195,25 +1195,27 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 port_bitmap = BIT(MT7530_CPU_PORT);
-	int i;
+	struct dsa_port *dp;
 
 	mutex_lock(&priv->reg_mutex);
 
-	for (i = 0; i < MT7530_NUM_PORTS; i++) {
+	dsa_switch_for_each_user_port(dp, ds) {
+		if (dp->index == port)
+			continue;
+
+		if (dp->bridge_dev != bridge)
+			continue;
+
 		/* Add this port to the port matrix of the other ports in the
 		 * same bridge. If the port is disabled, port matrix is kept
 		 * and not being setup until the port becomes enabled.
 		 */
-		if (dsa_is_user_port(ds, i) && i != port) {
-			if (dsa_to_port(ds, i)->bridge_dev != bridge)
-				continue;
-			if (priv->ports[i].enable)
-				mt7530_set(priv, MT7530_PCR_P(i),
-					   PCR_MATRIX(BIT(port)));
-			priv->ports[i].pm |= PCR_MATRIX(BIT(port));
+		if (priv->ports[dp->index].enable)
+			mt7530_set(priv, MT7530_PCR_P(dp->index),
+				   PCR_MATRIX(BIT(port)));
+		priv->ports[dp->index].pm |= PCR_MATRIX(BIT(port));
 
-			port_bitmap |= BIT(i);
-		}
+		port_bitmap |= BIT(dp->index);
 	}
 
 	/* Add the all other ports to this port matrix. */
@@ -1236,7 +1238,7 @@ mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
 {
 	struct mt7530_priv *priv = ds->priv;
 	bool all_user_ports_removed = true;
-	int i;
+	struct dsa_port *dp;
 
 	/* This is called after .port_bridge_leave when leaving a VLAN-aware
 	 * bridge. Don't set standalone ports to fallback mode.
@@ -1255,9 +1257,8 @@ mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
 	mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
 		   G0_PORT_VID_DEF);
 
-	for (i = 0; i < MT7530_NUM_PORTS; i++) {
-		if (dsa_is_user_port(ds, i) &&
-		    dsa_port_is_vlan_filtering(dsa_to_port(ds, i))) {
+	dsa_switch_for_each_user_port(dp, ds) {
+		if (dsa_port_is_vlan_filtering(dp)) {
 			all_user_ports_removed = false;
 			break;
 		}
@@ -1307,26 +1308,31 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 			 struct net_device *bridge)
 {
 	struct mt7530_priv *priv = ds->priv;
-	int i;
+	struct dsa_port *dp;
 
 	mutex_lock(&priv->reg_mutex);
 
-	for (i = 0; i < MT7530_NUM_PORTS; i++) {
+	dsa_switch_for_each_user_port(dp, ds) {
 		/* Remove this port from the port matrix of the other ports
 		 * in the same bridge. If the port is disabled, port matrix
 		 * is kept and not being setup until the port becomes enabled.
 		 * And the other port's port matrix cannot be broken when the
 		 * other port is still a VLAN-aware port.
 		 */
-		if (dsa_is_user_port(ds, i) && i != port &&
-		   !dsa_port_is_vlan_filtering(dsa_to_port(ds, i))) {
-			if (dsa_to_port(ds, i)->bridge_dev != bridge)
-				continue;
-			if (priv->ports[i].enable)
-				mt7530_clear(priv, MT7530_PCR_P(i),
-					     PCR_MATRIX(BIT(port)));
-			priv->ports[i].pm &= ~PCR_MATRIX(BIT(port));
-		}
+		if (dp->index == port)
+			continue;
+
+		if (dsa_port_is_vlan_filtering(dp))
+			continue;
+
+		if (dp->bridge_dev != bridge)
+			continue;
+
+		if (priv->ports[dp->index].enable)
+			mt7530_clear(priv, MT7530_PCR_P(dp->index),
+				     PCR_MATRIX(BIT(port)));
+
+		priv->ports[dp->index].pm &= ~PCR_MATRIX(BIT(port));
 	}
 
 	/* Set the cpu port to be the only one in the port matrix of
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c45ca2473743..1525415aca46 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1635,9 +1635,11 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
 static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 					u16 vid)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct mv88e6xxx_chip *chip = ds->priv;
 	struct mv88e6xxx_vtu_entry vlan;
-	int i, err;
+	struct dsa_port *other_dp;
+	int err;
 
 	/* DSA and CPU ports have to be members of multiple vlans */
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
@@ -1650,27 +1652,20 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 	if (!vlan.valid)
 		return 0;
 
-	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
-		if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
-			continue;
-
-		if (!dsa_to_port(ds, i)->slave)
-			continue;
-
-		if (vlan.member[i] ==
+	dsa_switch_for_each_user_port(other_dp, ds) {
+		if (vlan.member[other_dp->index] ==
 		    MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_NON_MEMBER)
 			continue;
 
-		if (dsa_to_port(ds, i)->bridge_dev ==
-		    dsa_to_port(ds, port)->bridge_dev)
+		if (other_dp->bridge_dev == dp->bridge_dev)
 			break; /* same bridge, check next VLAN */
 
-		if (!dsa_to_port(ds, i)->bridge_dev)
+		if (!other_dp->bridge_dev)
 			continue;
 
 		dev_err(ds->dev, "p%d: hw VLAN %d already used by port %d in %s\n",
-			port, vlan.vid, i,
-			netdev_name(dsa_to_port(ds, i)->bridge_dev));
+			port, vlan.vid, other_dp->index,
+			netdev_name(other_dp->bridge_dev));
 		return -EOPNOTSUPP;
 	}
 
@@ -1996,16 +1991,14 @@ static int mv88e6xxx_port_add_broadcast(struct mv88e6xxx_chip *chip, int port,
 
 static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 {
+	struct dsa_switch *ds = chip->ds;
+	struct dsa_port *dp;
 	int port;
 	int err;
 
-	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
-		struct dsa_port *dp = dsa_to_port(chip->ds, port);
+	dsa_switch_for_each_available_port(dp, ds) {
 		struct net_device *brport;
 
-		if (dsa_is_unused_port(chip->ds, port))
-			continue;
-
 		brport = dsa_port_to_bridge_port(dp);
 		if (brport && !br_port_flag_is_set(brport, BR_BCAST_FLOOD))
 			/* Skip bridged user ports where broadcast
@@ -3077,6 +3070,7 @@ static void mv88e6xxx_teardown(struct dsa_switch *ds)
 static int mv88e6xxx_setup(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	struct dsa_port *dp;
 	u8 cmode;
 	int err;
 	int i;
@@ -3113,9 +3107,8 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	}
 
 	/* Setup Switch Port Registers */
-	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
-		if (dsa_is_unused_port(ds, i))
-			continue;
+	dsa_switch_for_each_available_port(dp, ds) {
+		i = dp->index;
 
 		/* Prevent the use of an invalid port. */
 		if (mv88e6xxx_is_invalid_port(chip, i)) {
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
index 8f74ffc7a279..73153249b4c0 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
@@ -452,13 +452,11 @@ long mv88e6xxx_hwtstamp_work(struct ptp_clock_info *ptp)
 	struct mv88e6xxx_chip *chip = ptp_to_chip(ptp);
 	struct dsa_switch *ds = chip->ds;
 	struct mv88e6xxx_port_hwtstamp *ps;
-	int i, restart = 0;
+	struct dsa_port *dp;
+	int restart = 0;
 
-	for (i = 0; i < ds->num_ports; i++) {
-		if (!dsa_is_user_port(ds, i))
-			continue;
-
-		ps = &chip->port_hwtstamp[i];
+	dsa_switch_for_each_user_port(dp, ds) {
+		ps = &chip->port_hwtstamp[dp->index];
 		if (test_bit(MV88E6XXX_HWTSTAMP_TX_IN_PROGRESS, &ps->state))
 			restart |= mv88e6xxx_txtstamp_work(chip, ps);
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index f77e2ee64a60..2c300ab70002 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1381,13 +1381,13 @@ static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, int port,
 static int mv88e6393x_port_policy_write_all(struct mv88e6xxx_chip *chip,
 					    u16 pointer, u8 data)
 {
-	int err, port;
-
-	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
-		if (dsa_is_unused_port(chip->ds, port))
-			continue;
+	struct dsa_switch *ds = chip->ds;
+	struct dsa_port *dp;
+	int err;
 
-		err = mv88e6393x_port_policy_write(chip, port, pointer, data);
+	dsa_switch_for_each_available_port(dp, ds) {
+		err = mv88e6393x_port_policy_write(chip, dp->index, pointer,
+						   data);
 		if (err)
 			return err;
 	}
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 9505f9b3ac90..f4bd43545f04 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -266,12 +266,12 @@ static void felix_8021q_cpu_port_deinit(struct ocelot *ocelot, int port)
  */
 static int felix_setup_mmio_filtering(struct felix *felix)
 {
-	unsigned long user_ports = 0, cpu_ports = 0;
+	unsigned long user_ports = dsa_user_ports(felix->ds);
+	unsigned long cpu_ports = dsa_cpu_ports(felix->ds);
 	struct ocelot_vcap_filter *redirect_rule;
 	struct ocelot_vcap_filter *tagging_rule;
 	struct ocelot *ocelot = &felix->ocelot;
-	struct dsa_switch *ds = felix->ds;
-	int port, ret;
+	int ret;
 
 	tagging_rule = kzalloc(sizeof(struct ocelot_vcap_filter), GFP_KERNEL);
 	if (!tagging_rule)
@@ -283,13 +283,6 @@ static int felix_setup_mmio_filtering(struct felix *felix)
 		return -ENOMEM;
 	}
 
-	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		if (dsa_is_user_port(ds, port))
-			user_ports |= BIT(port);
-		if (dsa_is_cpu_port(ds, port))
-			cpu_ports |= BIT(port);
-	}
-
 	tagging_rule->key_type = OCELOT_VCAP_KEY_ETYPE;
 	*(__be16 *)tagging_rule->key.etype.etype.value = htons(ETH_P_1588);
 	*(__be16 *)tagging_rule->key.etype.etype.mask = htons(0xffff);
@@ -388,14 +381,12 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
 	unsigned long cpu_flood;
-	int port, err;
+	struct dsa_port *dp;
+	int err;
 
 	felix_8021q_cpu_port_init(ocelot, cpu);
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds) {
 		/* This overwrites ocelot_init():
 		 * Do not forward BPDU frames to the CPU port module,
 		 * for 2 reasons:
@@ -408,7 +399,7 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
 		 */
 		ocelot_write_gix(ocelot,
 				 ANA_PORT_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA(0),
-				 ANA_PORT_CPU_FWD_BPDU_CFG, port);
+				 ANA_PORT_CPU_FWD_BPDU_CFG, dp->index);
 	}
 
 	/* In tag_8021q mode, the CPU port module is unused, except for PTP
@@ -439,7 +430,8 @@ static void felix_teardown_tag_8021q(struct dsa_switch *ds, int cpu)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
-	int err, port;
+	struct dsa_port *dp;
+	int err;
 
 	err = felix_teardown_mmio_filtering(felix);
 	if (err)
@@ -448,17 +440,14 @@ static void felix_teardown_tag_8021q(struct dsa_switch *ds, int cpu)
 
 	dsa_tag_8021q_unregister(ds);
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds) {
 		/* Restore the logic from ocelot_init:
 		 * do not forward BPDU frames to the front ports.
 		 */
 		ocelot_write_gix(ocelot,
 				 ANA_PORT_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA(0xffff),
 				 ANA_PORT_CPU_FWD_BPDU_CFG,
-				 port);
+				 dp->index);
 	}
 
 	felix_8021q_cpu_port_deinit(ocelot, cpu);
@@ -1178,7 +1167,8 @@ static int felix_setup(struct dsa_switch *ds)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
-	int port, err;
+	struct dsa_port *dp;
+	int err;
 
 	err = felix_init_structs(felix, ds->num_ports);
 	if (err)
@@ -1197,31 +1187,24 @@ static int felix_setup(struct dsa_switch *ds)
 		}
 	}
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		ocelot_init_port(ocelot, port);
+	dsa_switch_for_each_available_port(dp, ds) {
+		ocelot_init_port(ocelot, dp->index);
 
 		/* Set the default QoS Classification based on PCP and DEI
 		 * bits of vlan tag.
 		 */
-		felix_port_qos_map_init(ocelot, port);
+		felix_port_qos_map_init(ocelot, dp->index);
 	}
 
 	err = ocelot_devlink_sb_register(ocelot);
 	if (err)
 		goto out_deinit_ports;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (!dsa_is_cpu_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_cpu_port(dp, ds)
 		/* The initial tag protocol is NPI which always returns 0, so
 		 * there's no real point in checking for errors.
 		 */
-		felix_set_tag_protocol(ds, port, felix->tag_proto);
-	}
+		felix_set_tag_protocol(ds, dp->index, felix->tag_proto);
 
 	ds->mtu_enforcement_ingress = true;
 	ds->assisted_learning_on_cpu_port = true;
@@ -1229,12 +1212,8 @@ static int felix_setup(struct dsa_switch *ds)
 	return 0;
 
 out_deinit_ports:
-	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		ocelot_deinit_port(ocelot, port);
-	}
+	dsa_switch_for_each_available_port(dp, ds)
+		ocelot_deinit_port(ocelot, dp->index);
 
 	ocelot_deinit_timestamp(ocelot);
 	ocelot_deinit(ocelot);
@@ -1250,25 +1229,17 @@ static void felix_teardown(struct dsa_switch *ds)
 {
 	struct ocelot *ocelot = ds->priv;
 	struct felix *felix = ocelot_to_felix(ocelot);
-	int port;
-
-	for (port = 0; port < ds->num_ports; port++) {
-		if (!dsa_is_cpu_port(ds, port))
-			continue;
+	struct dsa_port *dp;
 
-		felix_del_tag_protocol(ds, port, felix->tag_proto);
-	}
+	dsa_switch_for_each_cpu_port(dp, ds)
+		felix_del_tag_protocol(ds, dp->index, felix->tag_proto);
 
 	ocelot_devlink_sb_unregister(ocelot);
 	ocelot_deinit_timestamp(ocelot);
 	ocelot_deinit(ocelot);
 
-	for (port = 0; port < ocelot->num_phys_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		ocelot_deinit_port(ocelot, port);
-	}
+	dsa_switch_for_each_available_port(dp, ds)
+		ocelot_deinit_port(ocelot, dp->index);
 
 	if (felix->info->mdio_bus_free)
 		felix->info->mdio_bus_free(ocelot);
diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index f966a253d1c7..5b61da586102 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1034,12 +1034,14 @@ static const struct ocelot_ops vsc9959_ops = {
 static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
+	struct dsa_switch *ds = felix->ds;
 	struct enetc_mdio_priv *mdio_priv;
 	struct device *dev = ocelot->dev;
 	void __iomem *imdio_regs;
 	struct resource res;
 	struct enetc_hw *hw;
 	struct mii_bus *bus;
+	struct dsa_port *dp;
 	int port;
 	int rc;
 
@@ -1091,13 +1093,11 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 
 	felix->imdio = bus;
 
-	for (port = 0; port < felix->info->num_ports; port++) {
-		struct ocelot_port *ocelot_port = ocelot->ports[port];
+	dsa_switch_for_each_available_port(dp, ds) {
+		struct ocelot_port *ocelot_port = ocelot->ports[dp->index];
 		struct mdio_device *pcs;
 		struct lynx_pcs *lynx;
-
-		if (dsa_is_unused_port(felix->ds, port))
-			continue;
+		int port = dp->index;
 
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index deae923c8b7a..fe7828c402bd 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1085,9 +1085,10 @@ static const struct ocelot_ops vsc9953_ops = {
 static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 {
 	struct felix *felix = ocelot_to_felix(ocelot);
+	struct dsa_switch *ds = felix->ds;
 	struct device *dev = ocelot->dev;
+	struct dsa_port *dp;
 	struct mii_bus *bus;
-	int port;
 	int rc;
 
 	felix->pcs = devm_kcalloc(dev, felix->info->num_ports,
@@ -1118,15 +1119,12 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 
 	felix->imdio = bus;
 
-	for (port = 0; port < felix->info->num_ports; port++) {
-		struct ocelot_port *ocelot_port = ocelot->ports[port];
-		int addr = port + 4;
+	dsa_switch_for_each_available_port(dp, ds) {
+		struct ocelot_port *ocelot_port = ocelot->ports[dp->index];
+		int addr = dp->index + 4;
 		struct mdio_device *pcs;
 		struct lynx_pcs *lynx;
 
-		if (dsa_is_unused_port(felix->ds, port))
-			continue;
-
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
 			continue;
 
@@ -1140,7 +1138,7 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
 			continue;
 		}
 
-		felix->pcs[port] = lynx;
+		felix->pcs[dp->index] = lynx;
 
 		dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
 	}
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 1f63f50f73f1..f2c1369c2bff 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -940,6 +940,7 @@ static int
 qca8k_setup(struct dsa_switch *ds)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
+	struct dsa_port *dp;
 	int ret, i;
 	u32 mask;
 
@@ -1009,9 +1010,11 @@ qca8k_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Setup connection between CPU port & user ports */
-	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+	dsa_switch_for_each_port(dp, ds) {
+		i = dp->index;
+
 		/* CPU port gets connected to all user ports of the switch */
-		if (dsa_is_cpu_port(ds, i)) {
+		if (dsa_port_is_cpu(dp)) {
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(QCA8K_CPU_PORT),
 					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 			if (ret)
@@ -1019,7 +1022,7 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 
 		/* Individual user ports get connected to CPU port only */
-		if (dsa_is_user_port(ds, i)) {
+		if (dsa_port_is_user(dp)) {
 			int shift = 16 * (i % 2);
 
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
@@ -1509,21 +1512,23 @@ qca8k_port_bridge_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	int port_mask = BIT(QCA8K_CPU_PORT);
-	int i, ret;
+	struct dsa_port *dp;
+	int ret;
 
-	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
-		if (dsa_to_port(ds, i)->bridge_dev != br)
+	dsa_switch_for_each_port(dp, ds) {
+		if (dp->bridge_dev != br)
 			continue;
+
 		/* Add this port to the portvlan mask of the other ports
 		 * in the bridge
 		 */
 		ret = qca8k_reg_set(priv,
-				    QCA8K_PORT_LOOKUP_CTRL(i),
+				    QCA8K_PORT_LOOKUP_CTRL(dp->index),
 				    BIT(port));
 		if (ret)
 			return ret;
-		if (i != port)
-			port_mask |= BIT(i);
+		if (dp->index != port)
+			port_mask |= BIT(dp->index);
 	}
 
 	/* Add all other ports to this ports portvlan mask */
@@ -1537,16 +1542,17 @@ static void
 qca8k_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-	int i;
+	struct dsa_port *dp;
 
-	for (i = 1; i < QCA8K_NUM_PORTS; i++) {
-		if (dsa_to_port(ds, i)->bridge_dev != br)
+	dsa_switch_for_each_port(dp, ds) {
+		if (dp->bridge_dev != br)
 			continue;
+
 		/* Remove this port to the portvlan mask of the other ports
 		 * in the bridge
 		 */
 		qca8k_reg_clear(priv,
-				QCA8K_PORT_LOOKUP_CTRL(i),
+				QCA8K_PORT_LOOKUP_CTRL(dp->index),
 				BIT(port));
 	}
 
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 2bb35da0af12..42d6ab656afd 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -207,10 +207,7 @@ static int sja1105_init_mac_settings(struct sja1105_private *priv)
 
 	mac = table->entries;
 
-	list_for_each_entry(dp, &ds->dst->ports, list) {
-		if (dp->ds != ds)
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds) {
 		mac[dp->index] = default_mac;
 
 		/* Let sja1105_bridge_stp_state_set() keep address learning
@@ -239,7 +236,7 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv)
 	struct sja1105_xmii_params_entry *mii;
 	struct dsa_switch *ds = priv->ds;
 	struct sja1105_table *table;
-	int i;
+	struct dsa_port *dp;
 
 	table = &priv->static_config.tables[BLK_IDX_XMII_PARAMS];
 
@@ -259,11 +256,9 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv)
 
 	mii = table->entries;
 
-	for (i = 0; i < ds->num_ports; i++) {
+	dsa_switch_for_each_available_port(dp, ds) {
 		sja1105_mii_role_t role = XMII_MAC;
-
-		if (dsa_is_unused_port(priv->ds, i))
-			continue;
+		int i = dp->index;
 
 		switch (priv->phy_mode[i]) {
 		case PHY_INTERFACE_MODE_INTERNAL:
@@ -331,8 +326,9 @@ static int sja1105_init_mii_settings(struct sja1105_private *priv)
 static int sja1105_init_static_fdb(struct sja1105_private *priv)
 {
 	struct sja1105_l2_lookup_entry *l2_lookup;
+	struct dsa_switch *ds = priv->ds;
 	struct sja1105_table *table;
-	int port;
+	struct dsa_port *dp;
 
 	table = &priv->static_config.tables[BLK_IDX_L2_LOOKUP];
 
@@ -363,9 +359,8 @@ static int sja1105_init_static_fdb(struct sja1105_private *priv)
 	l2_lookup[0].index = SJA1105_MAX_L2_LOOKUP_COUNT - 1;
 
 	/* Flood multicast to every port by default */
-	for (port = 0; port < priv->ds->num_ports; port++)
-		if (!dsa_is_unused_port(priv->ds, port))
-			l2_lookup[0].destports |= BIT(port);
+	dsa_switch_for_each_available_port(dp, ds)
+		l2_lookup[0].destports |= BIT(dp->index);
 
 	return 0;
 }
@@ -405,20 +400,16 @@ static int sja1105_init_l2_lookup_params(struct sja1105_private *priv)
 	struct dsa_switch *ds = priv->ds;
 	int port, num_used_ports = 0;
 	struct sja1105_table *table;
+	struct dsa_port *dp;
 	u64 max_fdb_entries;
 
-	for (port = 0; port < ds->num_ports; port++)
-		if (!dsa_is_unused_port(ds, port))
-			num_used_ports++;
+	dsa_switch_for_each_available_port(dp, ds)
+		num_used_ports++;
 
 	max_fdb_entries = SJA1105_MAX_L2_LOOKUP_COUNT / num_used_ports;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds)
 		default_l2_lookup_params.maxaddrp[port] = max_fdb_entries;
-	}
 
 	table = &priv->static_config.tables[BLK_IDX_L2_LOOKUP_PARAMS];
 
@@ -461,7 +452,7 @@ static int sja1105_init_static_vlan(struct sja1105_private *priv)
 		.vlanid = SJA1105_DEFAULT_VLAN,
 	};
 	struct dsa_switch *ds = priv->ds;
-	int port;
+	struct dsa_port *dp;
 
 	table = &priv->static_config.tables[BLK_IDX_VLAN_LOOKUP];
 
@@ -477,15 +468,14 @@ static int sja1105_init_static_vlan(struct sja1105_private *priv)
 
 	table->entry_count = 1;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
+	dsa_switch_for_each_available_port(dp, ds) {
+		int port = dp->index;
 
 		pvid.vmemb_port |= BIT(port);
 		pvid.vlan_bc |= BIT(port);
 		pvid.tag_port &= ~BIT(port);
 
-		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) {
+		if (dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)) {
 			priv->tag_8021q_pvid[port] = SJA1105_DEFAULT_VLAN;
 			priv->bridge_pvid[port] = SJA1105_DEFAULT_VLAN;
 		}
@@ -498,12 +488,12 @@ static int sja1105_init_static_vlan(struct sja1105_private *priv)
 static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 {
 	struct sja1105_l2_forwarding_entry *l2fwd;
+	struct dsa_port *dp, *from_dp, *to_dp;
 	struct dsa_switch *ds = priv->ds;
 	struct dsa_switch_tree *dst;
 	struct sja1105_table *table;
 	struct dsa_link *dl;
-	int port, tc;
-	int from, to;
+	int tc;
 
 	table = &priv->static_config.tables[BLK_IDX_L2_FORWARDING];
 
@@ -525,24 +515,21 @@ static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 	 * rules and the VLAN PCP to ingress queue mapping.
 	 * Set up the ingress queue mapping first.
 	 */
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds)
 		for (tc = 0; tc < SJA1105_NUM_TC; tc++)
-			l2fwd[port].vlan_pmap[tc] = tc;
-	}
+			l2fwd[dp->index].vlan_pmap[tc] = tc;
 
 	/* Then manage the forwarding domain for user ports. These can forward
 	 * only to the always-on domain (CPU port and DSA links)
 	 */
-	for (from = 0; from < ds->num_ports; from++) {
-		if (!dsa_is_user_port(ds, from))
-			continue;
+	dsa_switch_for_each_user_port(from_dp, ds) {
+		int from = from_dp->index;
 
-		for (to = 0; to < ds->num_ports; to++) {
-			if (!dsa_is_cpu_port(ds, to) &&
-			    !dsa_is_dsa_port(ds, to))
+		dsa_switch_for_each_port(to_dp, ds) {
+			int to = to_dp->index;
+
+			if (!dsa_port_is_cpu(to_dp) &&
+			    !dsa_port_is_dsa(to_dp))
 				continue;
 
 			l2fwd[from].bc_domain |= BIT(to);
@@ -556,13 +543,14 @@ static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 	 * always-on domain). These can send packets to any enabled port except
 	 * themselves.
 	 */
-	for (from = 0; from < ds->num_ports; from++) {
-		if (!dsa_is_cpu_port(ds, from) && !dsa_is_dsa_port(ds, from))
+	dsa_switch_for_each_port(from_dp, ds) {
+		int from = from_dp->index;
+
+		if (!dsa_port_is_cpu(from_dp) && !dsa_port_is_dsa(from_dp))
 			continue;
 
-		for (to = 0; to < ds->num_ports; to++) {
-			if (dsa_is_unused_port(ds, to))
-				continue;
+		dsa_switch_for_each_available_port(to_dp, ds) {
+			int to = to_dp->index;
 
 			if (from == to)
 				continue;
@@ -585,6 +573,8 @@ static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 	dst = ds->dst;
 
 	list_for_each_entry(dl, &dst->rtable, list) {
+		int from, to;
+
 		if (dl->dp->ds != ds || dl->link_dp->cpu_dp == dl->dp->cpu_dp)
 			continue;
 
@@ -604,24 +594,17 @@ static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 	/* Finally, manage the egress flooding domain. All ports start up with
 	 * flooding enabled, including the CPU port and DSA links.
 	 */
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		priv->ucast_egress_floods |= BIT(port);
-		priv->bcast_egress_floods |= BIT(port);
+	dsa_switch_for_each_available_port(dp, ds) {
+		priv->ucast_egress_floods |= BIT(dp->index);
+		priv->bcast_egress_floods |= BIT(dp->index);
 	}
 
 	/* Next 8 entries define VLAN PCP mapping from ingress to egress.
 	 * Create a one-to-one mapping.
 	 */
 	for (tc = 0; tc < SJA1105_NUM_TC; tc++) {
-		for (port = 0; port < ds->num_ports; port++) {
-			if (dsa_is_unused_port(ds, port))
-				continue;
-
-			l2fwd[ds->num_ports + tc].vlan_pmap[port] = tc;
-		}
+		dsa_switch_for_each_available_port(dp, ds)
+			l2fwd[ds->num_ports + tc].vlan_pmap[dp->index] = tc;
 
 		l2fwd[ds->num_ports + tc].type_egrpcp2outputq = true;
 	}
@@ -634,7 +617,8 @@ static int sja1110_init_pcp_remapping(struct sja1105_private *priv)
 	struct sja1110_pcp_remapping_entry *pcp_remap;
 	struct dsa_switch *ds = priv->ds;
 	struct sja1105_table *table;
-	int port, tc;
+	struct dsa_port *dp;
+	int tc;
 
 	table = &priv->static_config.tables[BLK_IDX_PCP_REMAPPING];
 
@@ -657,13 +641,9 @@ static int sja1110_init_pcp_remapping(struct sja1105_private *priv)
 	pcp_remap = table->entries;
 
 	/* Repeat the configuration done for vlan_pmap */
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
+	dsa_switch_for_each_available_port(dp, ds)
 		for (tc = 0; tc < SJA1105_NUM_TC; tc++)
-			pcp_remap[port].egrpcp[tc] = tc;
-	}
+			pcp_remap[dp->index].egrpcp[tc] = tc;
 
 	return 0;
 }
@@ -999,7 +979,8 @@ static int sja1105_init_l2_policing(struct sja1105_private *priv)
 	struct sja1105_l2_policing_entry *policing;
 	struct dsa_switch *ds = priv->ds;
 	struct sja1105_table *table;
-	int port, tc;
+	struct dsa_port *dp;
+	int tc;
 
 	table = &priv->static_config.tables[BLK_IDX_L2_POLICING];
 
@@ -1019,9 +1000,10 @@ static int sja1105_init_l2_policing(struct sja1105_private *priv)
 	policing = table->entries;
 
 	/* Setup shared indices for the matchall policers */
-	for (port = 0; port < ds->num_ports; port++) {
-		int mcast = (ds->num_ports * (SJA1105_NUM_TC + 1)) + port;
-		int bcast = (ds->num_ports * SJA1105_NUM_TC) + port;
+	dsa_switch_for_each_available_port(dp, ds) {
+		int mcast = (ds->num_ports * (SJA1105_NUM_TC + 1)) + dp->index;
+		int bcast = (ds->num_ports * SJA1105_NUM_TC) + dp->index;
+		int port = dp->index;
 
 		for (tc = 0; tc < SJA1105_NUM_TC; tc++)
 			policing[port * SJA1105_NUM_TC + tc].sharindx = port;
@@ -1033,10 +1015,11 @@ static int sja1105_init_l2_policing(struct sja1105_private *priv)
 	}
 
 	/* Setup the matchall policer parameters */
-	for (port = 0; port < ds->num_ports; port++) {
+	dsa_switch_for_each_available_port(dp, ds) {
 		int mtu = VLAN_ETH_FRAME_LEN + ETH_FCS_LEN;
+		int port = dp->index;
 
-		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+		if (dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))
 			mtu += VLAN_HLEN;
 
 		policing[port].smax = 65535; /* Burst size in bytes */
@@ -1994,16 +1977,17 @@ static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 {
 	struct sja1105_l2_forwarding_entry *l2_fwd;
 	struct sja1105_private *priv = ds->priv;
-	int i, rc;
+	struct dsa_port *dp;
+	int rc;
 
 	l2_fwd = priv->static_config.tables[BLK_IDX_L2_FORWARDING].entries;
 
-	for (i = 0; i < ds->num_ports; i++) {
-		/* Add this port to the forwarding matrix of the
-		 * other ports in the same bridge, and viceversa.
-		 */
-		if (!dsa_is_user_port(ds, i))
-			continue;
+	/* Add this port to the forwarding matrix of the
+	 * other ports in the same bridge, and viceversa.
+	 */
+	dsa_switch_for_each_user_port(dp, ds) {
+		int i = dp->index;
+
 		/* For the ports already under the bridge, only one thing needs
 		 * to be done, and that is to add this port to their
 		 * reachability domain. So we can perform the SPI write for
@@ -2015,8 +1999,10 @@ static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 		 */
 		if (i == port)
 			continue;
-		if (dsa_to_port(ds, i)->bridge_dev != br)
+
+		if (dp->bridge_dev != br)
 			continue;
+
 		sja1105_port_allow_traffic(l2_fwd, i, port, member);
 		sja1105_port_allow_traffic(l2_fwd, port, i, member);
 
@@ -2364,6 +2350,7 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_table *table;
 	struct sja1105_rule *rule;
+	struct dsa_port *dp;
 	u16 tpid, tpid2;
 	int rc;
 
@@ -2433,11 +2420,8 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
 	l2_lookup_params = table->entries;
 	l2_lookup_params->shared_learn = !priv->vlan_aware;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		rc = sja1105_commit_pvid(ds, port);
+	dsa_switch_for_each_available_port(dp, ds) {
+		rc = sja1105_commit_pvid(ds, dp->index);
 		if (rc)
 			return rc;
 	}
@@ -2751,17 +2735,14 @@ static int sja1105_setup(struct dsa_switch *ds)
 static void sja1105_teardown(struct dsa_switch *ds)
 {
 	struct sja1105_private *priv = ds->priv;
-	int port;
+	struct dsa_port *dp;
 
 	rtnl_lock();
 	dsa_tag_8021q_unregister(ds);
 	rtnl_unlock();
 
-	for (port = 0; port < ds->num_ports; port++) {
-		struct sja1105_port *sp = &priv->ports[port];
-
-		if (!dsa_is_user_port(ds, port))
-			continue;
+	dsa_switch_for_each_user_port(dp, ds) {
+		struct sja1105_port *sp = &priv->ports[dp->index];
 
 		if (sp->xmit_worker)
 			kthread_destroy_worker(sp->xmit_worker);
@@ -3282,7 +3263,8 @@ static int sja1105_probe(struct spi_device *spi)
 	struct sja1105_private *priv;
 	size_t max_xfer, max_msg;
 	struct dsa_switch *ds;
-	int rc, port;
+	struct dsa_port *dp;
+	int rc;
 
 	if (!dev->of_node) {
 		dev_err(dev, "No DTS bindings for SJA1105 driver\n");
@@ -3387,14 +3369,10 @@ static int sja1105_probe(struct spi_device *spi)
 	}
 
 	/* Connections between dsa_port and sja1105_port */
-	for (port = 0; port < ds->num_ports; port++) {
-		struct sja1105_port *sp = &priv->ports[port];
-		struct dsa_port *dp = dsa_to_port(ds, port);
+	dsa_switch_for_each_user_port(dp, ds) {
+		struct sja1105_port *sp = &priv->ports[dp->index];
 		struct net_device *slave;
 
-		if (!dsa_is_user_port(ds, port))
-			continue;
-
 		dp->priv = sp;
 		sp->dp = dp;
 		sp->data = tagger_data;
@@ -3416,10 +3394,10 @@ static int sja1105_probe(struct spi_device *spi)
 	return 0;
 
 out_destroy_workers:
-	while (port-- > 0) {
-		struct sja1105_port *sp = &priv->ports[port];
+	dsa_switch_for_each_port_continue_reverse(dp, ds) {
+		struct sja1105_port *sp = &priv->ports[dp->index];
 
-		if (!dsa_is_user_port(ds, port))
+		if (!dsa_port_is_user(dp))
 			continue;
 
 		kthread_destroy_worker(sp->xmit_worker);
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
index 5acf6742da4d..00fb2e589ee8 100644
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
@@ -390,9 +390,9 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 {
 	struct sja1105_mdio_private *mdio_priv;
 	struct dsa_switch *ds = priv->ds;
+	struct dsa_port *dp;
 	struct mii_bus *bus;
 	int rc = 0;
-	int port;
 
 	if (!priv->info->pcs_mdio_read || !priv->info->pcs_mdio_write)
 		return 0;
@@ -420,12 +420,10 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 		return rc;
 	}
 
-	for (port = 0; port < ds->num_ports; port++) {
+	dsa_switch_for_each_available_port(dp, ds) {
 		struct mdio_device *mdiodev;
 		struct dw_xpcs *xpcs;
-
-		if (dsa_is_unused_port(ds, port))
-			continue;
+		int port = dp->index;
 
 		if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
 		    priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
@@ -455,7 +453,9 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 	return 0;
 
 out_pcs_free:
-	for (port = 0; port < ds->num_ports; port++) {
+	dsa_switch_for_each_port_continue_reverse(dp, ds) {
+		int port = dp->index;
+
 		if (!priv->xpcs[port])
 			continue;
 
diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 130abb0f1438..7465be144cc9 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -297,7 +297,6 @@ static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
 static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
 {
 	struct xrs700x *priv = ds->priv;
-	unsigned int val = 0;
 	int i = 0;
 	int ret;
 
@@ -316,12 +315,8 @@ static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
 	}
 
 	/* Mirror BPDU to CPU port */
-	for (i = 0; i < ds->num_ports; i++) {
-		if (dsa_is_cpu_port(ds, i))
-			val |= BIT(i);
-	}
-
-	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_FWD_MIRROR(port, 0), val);
+	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_FWD_MIRROR(port, 0),
+			   dsa_cpu_ports(ds));
 	if (ret)
 		return ret;
 
@@ -340,8 +335,8 @@ static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
 static int xrs700x_port_add_hsrsup_ipf(struct dsa_switch *ds, int port,
 				       int fwdport)
 {
+	unsigned int val = dsa_cpu_ports(ds);
 	struct xrs700x *priv = ds->priv;
-	unsigned int val = 0;
 	int i = 0;
 	int ret;
 
@@ -360,11 +355,6 @@ static int xrs700x_port_add_hsrsup_ipf(struct dsa_switch *ds, int port,
 	}
 
 	/* Mirror HSR/PRP supervision to CPU port */
-	for (i = 0; i < ds->num_ports; i++) {
-		if (dsa_is_cpu_port(ds, i))
-			val |= BIT(i);
-	}
-
 	ret = regmap_write(priv->regmap, XRS_ETH_ADDR_FWD_MIRROR(port, 1), val);
 	if (ret)
 		return ret;
@@ -505,28 +495,27 @@ static void xrs700x_mac_link_up(struct dsa_switch *ds, int port,
 static int xrs700x_bridge_common(struct dsa_switch *ds, int port,
 				 struct net_device *bridge, bool join)
 {
-	unsigned int i, cpu_mask = 0, mask = 0;
+	unsigned int cpu_mask = 0, mask = 0;
 	struct xrs700x *priv = ds->priv;
+	struct dsa_port *dp;
 	int ret;
 
-	for (i = 0; i < ds->num_ports; i++) {
-		if (dsa_is_cpu_port(ds, i))
-			continue;
-
-		cpu_mask |= BIT(i);
+	dsa_switch_for_each_user_port(dp, ds) {
+		cpu_mask |= BIT(dp->index);
 
-		if (dsa_to_port(ds, i)->bridge_dev == bridge)
+		if (dp->bridge_dev == bridge)
 			continue;
 
-		mask |= BIT(i);
+		mask |= BIT(dp->index);
 	}
 
-	for (i = 0; i < ds->num_ports; i++) {
-		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+	dsa_switch_for_each_port(dp, ds) {
+		if (dp->bridge_dev != bridge)
 			continue;
 
 		/* 1 = Disable forwarding to the port */
-		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(i), mask);
+		ret = regmap_write(priv->regmap, XRS_PORT_FWD_MASK(dp->index),
+				   mask);
 		if (ret)
 			return ret;
 	}
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4639a82bab66..379ad8183639 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -475,6 +475,14 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
 	list_for_each_entry((_dp), &(_ds)->dst->ports, list) \
 		if ((_dp)->ds == (_ds))
 
+#define dsa_switch_for_each_port_continue_reverse(_dp, _ds) \
+	list_for_each_entry_continue_reverse((_dp), &(_ds)->dst->ports, list) \
+		if ((_dp)->ds == (_ds))
+
+#define dsa_switch_for_each_available_port(_dp, _ds) \
+	dsa_switch_for_each_port((_dp), (_ds)) \
+		if (!dsa_port_is_unused((_dp)))
+
 #define dsa_switch_for_each_user_port(_dp, _ds) \
 	dsa_switch_for_each_port((_dp), (_ds)) \
 		if (dsa_port_is_user((_dp)))
@@ -494,6 +502,17 @@ static inline u32 dsa_user_ports(struct dsa_switch *ds)
 	return mask;
 }
 
+static inline u32 dsa_cpu_ports(struct dsa_switch *ds)
+{
+	struct dsa_port *dp;
+	u32 mask = 0;
+
+	dsa_switch_for_each_cpu_port(dp, ds)
+		mask |= BIT(dp->index);
+
+	return mask;
+}
+
 /* Return the local port used to reach an arbitrary switch device */
 static inline unsigned int dsa_routing_port(struct dsa_switch *ds, int device)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port
  2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
                   ` (2 preceding siblings ...)
  2021-08-09 19:03 ` [RFC PATCH net-next 3/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers Vladimir Oltean
@ 2021-08-09 19:03 ` Vladimir Oltean
  2021-08-10  9:39   ` Florian Fainelli
  2021-08-09 19:31 ` [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
  4 siblings, 1 reply; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:03 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, Vladimir Oltean,
	Kurt Kanzenbach, Woojung Huh, UNGLinuxDriver, Sean Wang,
	Landen Chao, DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister

Merging the two allows us to remove the open-coded
"dev->enabled_ports & BIT(i)" check from b53_br_join and b53_br_leave,
while still avoiding a quadratic iteration through the switch's ports.

Sadly I don't know if it's possible to completely get rid of
b53_for_each_port and replace it with dsa_switch_for_each_available_port,
especially for the platforms that use pdata and not OF bindings.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/b53/b53_common.c | 20 ++++++++++----------
 drivers/net/dsa/b53/b53_priv.h   |  6 +++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index ccd93147d994..5351d1f65ed9 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -498,6 +498,7 @@ static int b53_fast_age_vlan(struct b53_device *dev, u16 vid)
 void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 {
 	struct b53_device *dev = ds->priv;
+	struct dsa_port *dp;
 	unsigned int i;
 	u16 pvlan;
 
@@ -505,7 +506,9 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 	 * on a per-port basis such that we only have Port i and IMP in
 	 * the same VLAN.
 	 */
-	b53_for_each_port(dev, i) {
+	b53_for_each_port(dp, dev) {
+		i = dp->index;
+
 		b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(i), &pvlan);
 		pvlan |= BIT(cpu_port);
 		b53_write16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(i), pvlan);
@@ -739,6 +742,7 @@ int b53_configure_vlan(struct dsa_switch *ds)
 {
 	struct b53_device *dev = ds->priv;
 	struct b53_vlan vl = { 0 };
+	struct dsa_port *dp;
 	struct b53_vlan *v;
 	int i, def_vid;
 	u16 vid;
@@ -761,7 +765,9 @@ int b53_configure_vlan(struct dsa_switch *ds)
 	 * entry. Do this only when the tagging protocol is not
 	 * DSA_TAG_PROTO_NONE
 	 */
-	b53_for_each_port(dev, i) {
+	b53_for_each_port(dp, dev) {
+		i = dp->index;
+
 		v = &dev->vlans[def_vid];
 		v->members |= BIT(i);
 		if (!b53_vlan_port_needs_forced_tagged(ds, i))
@@ -1874,12 +1880,9 @@ int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 
 	b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(port), &pvlan);
 
-	dsa_switch_for_each_port(dp, ds) {
+	b53_for_each_port(dp, dev) {
 		i = dp->index;
 
-		if (!(dev->enabled_ports & BIT(i)))
-			continue;
-
 		if (dp->bridge_dev != br)
 			continue;
 
@@ -1915,12 +1918,9 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 
 	b53_read16(dev, B53_PVLAN_PAGE, B53_PVLAN_PORT_MASK(port), &pvlan);
 
-	dsa_switch_for_each_port(dp, ds) {
+	b53_for_each_port(dp, dev) {
 		i = dp->index;
 
-		if (!(dev->enabled_ports & BIT(i)))
-			continue;
-
 		/* Don't touch the remaining ports */
 		if (dp->bridge_dev != br)
 			continue;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 9bf8319342b0..aec4b1176be9 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -145,10 +145,10 @@ struct b53_device {
 	struct b53_port *ports;
 };
 
-#define b53_for_each_port(dev, i) \
-	for (i = 0; i < B53_N_PORTS; i++) \
-		if (dev->enabled_ports & BIT(i))
 
+#define b53_for_each_port(_dp, _dev) \
+	dsa_switch_for_each_port((_dp), (_dev)->ds) \
+		if ((_dev)->enabled_ports & BIT((_dp)->index))
 
 static inline int is5325(struct b53_device *dev)
 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern
  2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
                   ` (3 preceding siblings ...)
  2021-08-09 19:03 ` [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port Vladimir Oltean
@ 2021-08-09 19:31 ` Vladimir Oltean
  4 siblings, 0 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-09 19:31 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, Jakub Kicinski, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot, Kurt Kanzenbach, Woojung Huh,
	UNGLinuxDriver, Sean Wang, Landen Chao, DENG Qingfang,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

FWIW, I also have a small test: on my Turris MOX with 25 mv88e6xxx user
ports, I have a script which sets up the bridge for my network:

-----------------------------[ cut here ]-----------------------------
# cat /etc/init.d/S50bridge
#!/bin/bash

ip link del br0
ip link add br0 type bridge vlan_filtering 1
for eth in lan1 lan2 lan3 lan4 lan5 lan6 lan7 lan8 lan9 lan10 lan11 lan12 lan13 lan14 lan15 lan16 lan17 lan18 lan19 lan20 lan21 lan22 lan23 lan24 sfp; do
        ip link set ${eth} master br0
done

# For TFTP, the server is connected to sfp
# bridge vlan add dev sfp vid 100
bridge vlan add dev lan19 vid 100

# lan1-lan24 are the for board access to TFTP. Use VLAN 100.
# FIXME: I removed lan19 from this list and now use it for TFTP server
for eth in lan1 lan2 lan3 lan4 lan5 lan6 lan7 lan8 lan9 lan10 lan11 lan12 lan13 lan14 lan15 lan16 lan17 lan18 lan20 lan21 lan22 lan23 lan24; do
        bridge vlan del dev ${eth} vid 1
        bridge vlan add dev ${eth} vid 100 pvid untagged
done

ip link set br0 up
-----------------------------[ cut here ]-----------------------------

Before this series:

time /etc/init.d/S50bridge
real    0m8.809s
user    0m0.253s
sys     0m2.614s

time ip link del br0
real    0m4.509s
user    0m0.006s
sys     0m1.184s

After:

time /etc/init.d/S50bridge
real    0m8.270s
user    0m0.199s
sys     0m2.468s

time ip link del br0
real    0m3.964s
user    0m0.000s
sys     0m1.024s

So there is a (small, but still visible) improvement - note that
mv88e6xxx is heavily limited by MDIO access anyway, and that would be
the predominant latency.

Also, I noticed that I forgot an unused "int port" variable in
felix_vsc9959.c variable, that I'm sure the kernel test robot will
complain about. So this is just RFC for now.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-09 19:03 ` [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
@ 2021-08-10  3:33   ` DENG Qingfang
  2021-08-10  9:41     ` Florian Fainelli
  2021-08-10  9:37   ` Florian Fainelli
  1 sibling, 1 reply; 16+ messages in thread
From: DENG Qingfang @ 2021-08-10  3:33 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, Jakub Kicinski, David S. Miller, Florian Fainelli,
	Andrew Lunn, Vivien Didelot, Vladimir Oltean, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Mon, Aug 09, 2021 at 10:03:18PM +0300, Vladimir Oltean wrote:
> Ever since Vivien's conversion of the ds->ports array into a dst->ports
> list, and the introduction of dsa_to_port, iterations through the ports
> of a switch became quadratic whenever dsa_to_port was needed.

So, what is the benefit of a linked list here? Do we allow users to
insert/delete a dsa_port at runtime? If not, how about using a
dynamically allocated array instead?

> 
> dsa_to_port can either be called directly, or indirectly through the
> dsa_is_{user,cpu,dsa,unused}_port helpers.
> 
> Introduce a basic switch port iterator macro, dsa_switch_for_each_port,
> that works with the iterator variable being a struct dsa_port *dp
> directly, and not an int i. It is an expensive variable to go from i to
> dp, but cheap to go from dp to i.
> 
> This macro iterates through the entire ds->dst->ports list and filters
> by the ports belonging just to the switch provided as argument.
> 
> While at it, provide some more flavors of that macro which filter for
> specific types of ports: at the moment just user ports and CPU ports.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper
  2021-08-09 19:03 ` [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
@ 2021-08-10  9:34   ` Florian Fainelli
  0 siblings, 0 replies; 16+ messages in thread
From: Florian Fainelli @ 2021-08-10  9:34 UTC (permalink / raw)
  To: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Vivien Didelot, Vladimir Oltean, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister



On 8/9/2021 12:03 PM, Vladimir Oltean wrote:
> Similar to the existing dsa_port_is_{cpu,user,dsa} helpers which operate
> directly on a struct dsa_port *dp, let's introduce the equivalent of
> dsa_is_unused_port. We will use this to create a more efficient iterator
> over the available ports of a switch.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-09 19:03 ` [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
  2021-08-10  3:33   ` DENG Qingfang
@ 2021-08-10  9:37   ` Florian Fainelli
  1 sibling, 0 replies; 16+ messages in thread
From: Florian Fainelli @ 2021-08-10  9:37 UTC (permalink / raw)
  To: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Vivien Didelot, Vladimir Oltean, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister



On 8/9/2021 12:03 PM, Vladimir Oltean wrote:
> Ever since Vivien's conversion of the ds->ports array into a dst->ports
> list, and the introduction of dsa_to_port, iterations through the ports
> of a switch became quadratic whenever dsa_to_port was needed.
> 
> dsa_to_port can either be called directly, or indirectly through the
> dsa_is_{user,cpu,dsa,unused}_port helpers.
> 
> Introduce a basic switch port iterator macro, dsa_switch_for_each_port,
> that works with the iterator variable being a struct dsa_port *dp
> directly, and not an int i. It is an expensive variable to go from i to
> dp, but cheap to go from dp to i.
> 
> This macro iterates through the entire ds->dst->ports list and filters
> by the ports belonging just to the switch provided as argument.
> 
> While at it, provide some more flavors of that macro which filter for
> specific types of ports: at the moment just user ports and CPU ports.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port
  2021-08-09 19:03 ` [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port Vladimir Oltean
@ 2021-08-10  9:39   ` Florian Fainelli
  2021-08-10 13:14     ` Vladimir Oltean
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Fainelli @ 2021-08-10  9:39 UTC (permalink / raw)
  To: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Vivien Didelot, Vladimir Oltean, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	DENG Qingfang, Matthias Brugger, Claudiu Manoil,
	Alexandre Belloni, George McCollister



On 8/9/2021 12:03 PM, Vladimir Oltean wrote:
> Merging the two allows us to remove the open-coded
> "dev->enabled_ports & BIT(i)" check from b53_br_join and b53_br_leave,
> while still avoiding a quadratic iteration through the switch's ports.
> 
> Sadly I don't know if it's possible to completely get rid of
> b53_for_each_port and replace it with dsa_switch_for_each_available_port,
> especially for the platforms that use pdata and not OF bindings.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

We should really be able to eliminate b53_for_each_port() entirely, let 
me try to submit a patch doing that when I come back from vacation or 
you can do it, and if there are bugs, I will address them.
-- 
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-10  3:33   ` DENG Qingfang
@ 2021-08-10  9:41     ` Florian Fainelli
  2021-08-10 11:35       ` Vladimir Oltean
  0 siblings, 1 reply; 16+ messages in thread
From: Florian Fainelli @ 2021-08-10  9:41 UTC (permalink / raw)
  To: DENG Qingfang, Vladimir Oltean
  Cc: netdev, Jakub Kicinski, David S. Miller, Andrew Lunn,
	Vivien Didelot, Vladimir Oltean, Kurt Kanzenbach, Woojung Huh,
	UNGLinuxDriver, Sean Wang, Landen Chao, Matthias Brugger,
	Claudiu Manoil, Alexandre Belloni, George McCollister



On 8/9/2021 8:33 PM, DENG Qingfang wrote:
> On Mon, Aug 09, 2021 at 10:03:18PM +0300, Vladimir Oltean wrote:
>> Ever since Vivien's conversion of the ds->ports array into a dst->ports
>> list, and the introduction of dsa_to_port, iterations through the ports
>> of a switch became quadratic whenever dsa_to_port was needed.
> 
> So, what is the benefit of a linked list here? Do we allow users to
> insert/delete a dsa_port at runtime? If not, how about using a
> dynamically allocated array instead?

The goal was to flatten the space while doing cross switch operations, 
which would have otherwise required iterating over dsa_switch instances 
within a dsa_switch_tree, and then over dsa_port within each dsa_switch.
-- 
Florian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-10  9:41     ` Florian Fainelli
@ 2021-08-10 11:35       ` Vladimir Oltean
  2021-08-10 16:35         ` Vladimir Oltean
  0 siblings, 1 reply; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-10 11:35 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: DENG Qingfang, Vladimir Oltean, netdev, Jakub Kicinski,
	David S. Miller, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Tue, Aug 10, 2021 at 02:41:07AM -0700, Florian Fainelli wrote:
> On 8/9/2021 8:33 PM, DENG Qingfang wrote:
> > On Mon, Aug 09, 2021 at 10:03:18PM +0300, Vladimir Oltean wrote:
> > > Ever since Vivien's conversion of the ds->ports array into a dst->ports
> > > list, and the introduction of dsa_to_port, iterations through the ports
> > > of a switch became quadratic whenever dsa_to_port was needed.
> > 
> > So, what is the benefit of a linked list here? Do we allow users to
> > insert/delete a dsa_port at runtime? If not, how about using a
> > dynamically allocated array instead?
> 
> The goal was to flatten the space while doing cross switch operations, which
> would have otherwise required iterating over dsa_switch instances within a
> dsa_switch_tree, and then over dsa_port within each dsa_switch.

To expand on that: technically dsa_port_touch() _does_ happen at
runtime, since multiple switches in a cross-chip tree probe
asynchronously. To use a dynamically allocated array would mean to
preallocate the sum of all DSA switch ports' worth of memory, and to
preallocate an index for each DSA switch within that single array.
Overall a list is simpler.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port
  2021-08-10  9:39   ` Florian Fainelli
@ 2021-08-10 13:14     ` Vladimir Oltean
  0 siblings, 0 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-10 13:14 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Vladimir Oltean, netdev, Jakub Kicinski, David S. Miller,
	Andrew Lunn, Vivien Didelot, Kurt Kanzenbach, Woojung Huh,
	UNGLinuxDriver, Sean Wang, Landen Chao, DENG Qingfang,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Tue, Aug 10, 2021 at 02:39:08AM -0700, Florian Fainelli wrote:
> On 8/9/2021 12:03 PM, Vladimir Oltean wrote:
> > Merging the two allows us to remove the open-coded
> > "dev->enabled_ports & BIT(i)" check from b53_br_join and b53_br_leave,
> > while still avoiding a quadratic iteration through the switch's ports.
> > 
> > Sadly I don't know if it's possible to completely get rid of
> > b53_for_each_port and replace it with dsa_switch_for_each_available_port,
> > especially for the platforms that use pdata and not OF bindings.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> We should really be able to eliminate b53_for_each_port() entirely, let me
> try to submit a patch doing that when I come back from vacation or you can
> do it, and if there are bugs, I will address them.

I will let you do it when you come back, until then I believe the
existing conversion has a fairly low risk of introducing any bugs.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-10 11:35       ` Vladimir Oltean
@ 2021-08-10 16:35         ` Vladimir Oltean
  2021-08-10 17:04           ` DENG Qingfang
  0 siblings, 1 reply; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-10 16:35 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: DENG Qingfang, Vladimir Oltean, netdev, Jakub Kicinski,
	David S. Miller, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Tue, Aug 10, 2021 at 02:35:32PM +0300, Vladimir Oltean wrote:
> On Tue, Aug 10, 2021 at 02:41:07AM -0700, Florian Fainelli wrote:
> > On 8/9/2021 8:33 PM, DENG Qingfang wrote:
> > > On Mon, Aug 09, 2021 at 10:03:18PM +0300, Vladimir Oltean wrote:
> > > > Ever since Vivien's conversion of the ds->ports array into a dst->ports
> > > > list, and the introduction of dsa_to_port, iterations through the ports
> > > > of a switch became quadratic whenever dsa_to_port was needed.
> > > 
> > > So, what is the benefit of a linked list here? Do we allow users to
> > > insert/delete a dsa_port at runtime? If not, how about using a
> > > dynamically allocated array instead?
> > 
> > The goal was to flatten the space while doing cross switch operations, which
> > would have otherwise required iterating over dsa_switch instances within a
> > dsa_switch_tree, and then over dsa_port within each dsa_switch.
> 
> To expand on that: technically dsa_port_touch() _does_ happen at
> runtime, since multiple switches in a cross-chip tree probe
> asynchronously. To use a dynamically allocated array would mean to
> preallocate the sum of all DSA switch ports' worth of memory, and to
> preallocate an index for each DSA switch within that single array.
> Overall a list is simpler.

If I were to guess where Qingfang was hinting at, is that the receive
path now needs to iterate over a list, whereas before it simply indexed
an array:

static inline struct net_device *dsa_master_find_slave(struct net_device *dev,
						       int device, int port)
{
	struct dsa_port *cpu_dp = dev->dsa_ptr;
	struct dsa_switch_tree *dst = cpu_dp->dst;
	struct dsa_port *dp;

	list_for_each_entry(dp, &dst->ports, list)
		if (dp->ds->index == device && dp->index == port &&
		    dp->type == DSA_PORT_TYPE_USER)
			return dp->slave;

	return NULL;
}

I will try in the following days to make a prototype implementation of
converting back the linked list into an array and see if there is any
justifiable performance improvement.

[ even if this would make the "multiple CPU ports in LAG" implementation
  harder ]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-10 16:35         ` Vladimir Oltean
@ 2021-08-10 17:04           ` DENG Qingfang
  2021-08-11 17:32             ` Vladimir Oltean
  0 siblings, 1 reply; 16+ messages in thread
From: DENG Qingfang @ 2021-08-10 17:04 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Florian Fainelli, Vladimir Oltean, netdev, Jakub Kicinski,
	David S. Miller, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Tue, Aug 10, 2021 at 07:35:33PM +0300, Vladimir Oltean wrote:
> If I were to guess where Qingfang was hinting at, is that the receive
> path now needs to iterate over a list, whereas before it simply indexed
> an array:
> 
> static inline struct net_device *dsa_master_find_slave(struct net_device *dev,
> 						       int device, int port)
> {
> 	struct dsa_port *cpu_dp = dev->dsa_ptr;
> 	struct dsa_switch_tree *dst = cpu_dp->dst;
> 	struct dsa_port *dp;
> 
> 	list_for_each_entry(dp, &dst->ports, list)
> 		if (dp->ds->index == device && dp->index == port &&
> 		    dp->type == DSA_PORT_TYPE_USER)
> 			return dp->slave;
> 
> 	return NULL;
> }
> 
> I will try in the following days to make a prototype implementation of
> converting back the linked list into an array and see if there is any
> justifiable performance improvement.
> 
> [ even if this would make the "multiple CPU ports in LAG" implementation
>   harder ]

Yes, you got my point.

There is RTL8390M series SoC, which has 52+ ports but a weak CPU (MIPS
34kc 700MHz). In that case the linear lookup time and the potential cache
miss could make a difference.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core
  2021-08-10 17:04           ` DENG Qingfang
@ 2021-08-11 17:32             ` Vladimir Oltean
  0 siblings, 0 replies; 16+ messages in thread
From: Vladimir Oltean @ 2021-08-11 17:32 UTC (permalink / raw)
  To: DENG Qingfang
  Cc: Florian Fainelli, Vladimir Oltean, netdev, Jakub Kicinski,
	David S. Miller, Andrew Lunn, Vivien Didelot, Kurt Kanzenbach,
	Woojung Huh, UNGLinuxDriver, Sean Wang, Landen Chao,
	Matthias Brugger, Claudiu Manoil, Alexandre Belloni,
	George McCollister

On Wed, Aug 11, 2021 at 01:04:47AM +0800, DENG Qingfang wrote:
> On Tue, Aug 10, 2021 at 07:35:33PM +0300, Vladimir Oltean wrote:
> > If I were to guess where Qingfang was hinting at, is that the receive
> > path now needs to iterate over a list, whereas before it simply indexed
> > an array:
> > 
> > static inline struct net_device *dsa_master_find_slave(struct net_device *dev,
> > 						       int device, int port)
> > {
> > 	struct dsa_port *cpu_dp = dev->dsa_ptr;
> > 	struct dsa_switch_tree *dst = cpu_dp->dst;
> > 	struct dsa_port *dp;
> > 
> > 	list_for_each_entry(dp, &dst->ports, list)
> > 		if (dp->ds->index == device && dp->index == port &&
> > 		    dp->type == DSA_PORT_TYPE_USER)
> > 			return dp->slave;
> > 
> > 	return NULL;
> > }
> > 
> > I will try in the following days to make a prototype implementation of
> > converting back the linked list into an array and see if there is any
> > justifiable performance improvement.
> > 
> > [ even if this would make the "multiple CPU ports in LAG" implementation
> >   harder ]
> 
> Yes, you got my point.
> 
> There is RTL8390M series SoC, which has 52+ ports but a weak CPU (MIPS
> 34kc 700MHz). In that case the linear lookup time and the potential cache
> miss could make a difference.

Then I am not in a position to make relevant performance tests for that
scenario.

I have been testing with the following setup: an NXP LS1028A switch
(ocelot/felix driver) using IPv4 forwarding of 64 byte UDP datagrams
sent by a data generator. 2 ports at 1Gbps each, 100% port load. IP
forwarding takes place between 1 port and the other.
Generator port A sends from 192.168.100.1 to 192.168.200.1
Generator port B sends from 192.168.200.1 to 192.168.100.1

Flow control is enabled on all switch ports, the user ports and the CPU port
(I don't really have a setup that I can test in any meaningful way
without flow control).

The script I run on the board to set things up for IP forwarding is:

ip link set eno2 down && echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
ip link set swp0 address a0:00:00:00:00:02
ip link set swp1 address a0:00:00:00:00:04
for eth in swp0 swp1; do
	ip link set ${eth} up
done
ip addr add 192.168.100.2/24 dev swp0
ip addr add 192.168.200.2/24 dev swp1
echo 1 > /proc/sys/net/ipv4/ip_forward
arp -s 192.168.100.1 00:01:02:03:04:05 dev swp0
arp -s 192.168.200.1 00:01:02:03:04:06 dev swp1
ethtool --config-nfc eno2 flow-type ip4 dst-ip 192.168.200.1 action 0
ethtool --config-nfc eno2 flow-type ip4 dst-ip 192.168.100.1 action 1
ethtool -K eno2 gro on rx-gro-list on
ethtool -K swp0 gro on rx-gro-list on
ethtool -K swp1 gro on rx-gro-list on


The DSA patch I used on top of today's net-next was:

-----------------------------[ cut here ]-----------------------------
From 7733f643dd61431a93da5a8f5118848cdc037562 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 11 Aug 2021 00:27:07 +0300
Subject: [PATCH] net: dsa: setup a linear port cache for faster receive path

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h  |  5 +++++
 net/dsa/dsa2.c     | 46 ++++++++++++++++++++++++++++++++++++++++++----
 net/dsa/dsa_priv.h |  9 ++++-----
 3 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 3203b200cc38..2a9ea4f57910 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -153,9 +153,14 @@ struct dsa_switch_tree {
 	struct net_device **lags;
 	unsigned int lags_len;
 
+	struct dsa_port **port_cache;
+
 	/* Track the largest switch index within a tree */
 	unsigned int last_switch;
 
+	/* Track the largest port count in a switch within a tree */
+	unsigned int max_num_ports;
+
 	/* Track the bridges with forwarding offload enabled */
 	unsigned long fwd_offloading_bridges;
 };
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 0b7497dd60c3..3d2b92dbd603 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -941,6 +941,39 @@ static void dsa_tree_teardown_lags(struct dsa_switch_tree *dst)
 	kfree(dst->lags);
 }
 
+static int dsa_tree_setup_port_cache(struct dsa_switch_tree *dst)
+{
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (dst->last_switch < dp->ds->index)
+			dst->last_switch = dp->ds->index;
+		if (dst->max_num_ports < dp->ds->num_ports)
+			dst->max_num_ports = dp->ds->num_ports;
+	}
+
+	dst->port_cache = kcalloc((dst->last_switch + 1) * dst->max_num_ports,
+				  sizeof(struct dsa_port *), GFP_KERNEL);
+	if (!dst->port_cache)
+		return -ENOMEM;
+
+	list_for_each_entry(dp, &dst->ports, list)
+		dst->port_cache[dp->ds->index * dst->max_num_ports + dp->index] = dp;
+
+	return 0;
+}
+
+static void dsa_tree_teardown_port_cache(struct dsa_switch_tree *dst)
+{
+	int i;
+
+	for (i = 0; i < dst->max_num_ports * dst->last_switch; i++)
+		dst->port_cache[i] = NULL;
+
+	kfree(dst->port_cache);
+	dst->port_cache = NULL;
+}
+
 static int dsa_tree_setup(struct dsa_switch_tree *dst)
 {
 	bool complete;
@@ -956,10 +989,14 @@ static int dsa_tree_setup(struct dsa_switch_tree *dst)
 	if (!complete)
 		return 0;
 
-	err = dsa_tree_setup_cpu_ports(dst);
+	err = dsa_tree_setup_port_cache(dst);
 	if (err)
 		return err;
 
+	err = dsa_tree_setup_cpu_ports(dst);
+	if (err)
+		goto teardown_port_cache;
+
 	err = dsa_tree_setup_switches(dst);
 	if (err)
 		goto teardown_cpu_ports;
@@ -984,6 +1021,8 @@ static int dsa_tree_setup(struct dsa_switch_tree *dst)
 	dsa_tree_teardown_switches(dst);
 teardown_cpu_ports:
 	dsa_tree_teardown_cpu_ports(dst);
+teardown_port_cache:
+	dsa_tree_teardown_port_cache(dst);
 
 	return err;
 }
@@ -1003,6 +1042,8 @@ static void dsa_tree_teardown(struct dsa_switch_tree *dst)
 
 	dsa_tree_teardown_cpu_ports(dst);
 
+	dsa_tree_teardown_port_cache(dst);
+
 	list_for_each_entry_safe(dl, next, &dst->rtable, list) {
 		list_del(&dl->list);
 		kfree(dl);
@@ -1301,9 +1342,6 @@ static int dsa_switch_parse_member_of(struct dsa_switch *ds,
 		return -EEXIST;
 	}
 
-	if (ds->dst->last_switch < ds->index)
-		ds->dst->last_switch = ds->index;
-
 	return 0;
 }
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 6310a15afe21..5c27f66fd62a 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -188,12 +188,11 @@ static inline struct net_device *dsa_master_find_slave(struct net_device *dev,
 	struct dsa_switch_tree *dst = cpu_dp->dst;
 	struct dsa_port *dp;
 
-	list_for_each_entry(dp, &dst->ports, list)
-		if (dp->ds->index == device && dp->index == port &&
-		    dp->type == DSA_PORT_TYPE_USER)
-			return dp->slave;
+	dp = dst->port_cache[device * dst->max_num_ports + port];
+	if (!dp || dp->type != DSA_PORT_TYPE_USER)
+		return NULL;
 
-	return NULL;
+	return dp->slave;
 }
 
 /* netlink.c */
-----------------------------[ cut here ]-----------------------------

The results I got were:

Before the patch:

684 Kpps = 459 Mbps

perf record -e cycles -C 0 sleep 10 && perf report
    10.17%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_pci_remove
     6.13%  ksoftirqd/0      [kernel.kallsyms]  [k] eth_type_trans
     5.48%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_poll
     4.99%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_alloc
     4.56%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_gro_receive
     2.89%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_start_xmit
     2.77%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_flow_dissect
     2.75%  ksoftirqd/0      [kernel.kallsyms]  [k] __siphash_aligned
     2.55%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_core
     2.48%  ksoftirqd/0      [kernel.kallsyms]  [k] build_skb
     2.47%  ksoftirqd/0      [kernel.kallsyms]  [k] take_page_off_buddy
     2.01%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gro_receive
     1.86%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_slave_xmit
     1.76%  ksoftirqd/0      [kernel.kallsyms]  [k] __dev_queue_xmit
     1.68%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_zcopy_clear
     1.62%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_build_skb
     1.60%  ksoftirqd/0      [kernel.kallsyms]  [k] __build_skb_around
     1.50%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_hard_start_xmit
     1.49%  ksoftirqd/0      [kernel.kallsyms]  [k] sch_direct_xmit
     1.42%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_get_hash
     1.29%  ksoftirqd/0      [kernel.kallsyms]  [k] __local_bh_enable_ip
     1.26%  ksoftirqd/0      [kernel.kallsyms]  [k] udp_gro_receive
     1.23%  ksoftirqd/0      [kernel.kallsyms]  [k] udp4_gro_receive
     1.21%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_segment_list
     1.13%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_drivername
     1.05%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_shutdown
     1.05%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gso_segment
     1.01%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_switch_rcv
     0.98%  ksoftirqd/0      [kernel.kallsyms]  [k] ocelot_rcv
     0.91%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_gro_receive
     0.87%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_xmit
     0.85%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_free_bulk
     0.84%  ksoftirqd/0      [kernel.kallsyms]  [k] do_csum
     0.84%  ksoftirqd/0      [kernel.kallsyms]  [k] memmove
     0.80%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_rcv
     0.79%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_list_core
     0.78%  ksoftirqd/0      [kernel.kallsyms]  [k] netif_skb_features
     0.76%  ksoftirqd/0      [kernel.kallsyms]  [k] netif_receive_skb_list_internal
     0.76%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_all
     0.74%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_pick_tx

perf record -e cache-misses -C 0 sleep 10 && perf report
     7.22%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_zcopy_clear
     6.46%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gro_receive
     6.41%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_pci_remove
     6.20%  ksoftirqd/0      [kernel.kallsyms]  [k] take_page_off_buddy
     6.13%  ksoftirqd/0      [kernel.kallsyms]  [k] build_skb
     5.06%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gso_segment
     4.47%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_gro_receive
     4.28%  ksoftirqd/0      [kernel.kallsyms]  [k] memmove
     3.77%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_poll
     3.73%  ksoftirqd/0      [kernel.kallsyms]  [k] __copy_skb_header
     3.46%  ksoftirqd/0      [kernel.kallsyms]  [k] eth_type_trans
     3.06%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_all
     2.76%  ksoftirqd/0      [kernel.kallsyms]  [k] ip_send_check
     2.36%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_get_hash
     2.24%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_alloc
     2.23%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_core
     1.68%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_pick_tx
     1.56%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_segment_list
     1.55%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_slave_xmit
     1.54%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_headers_offset_update
     1.51%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_start_xmit
     1.48%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_core_pick_tx
     1.30%  ksoftirqd/0      [kernel.kallsyms]  [k] __dev_queue_xmit
     1.14%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_rcv
     1.05%  ksoftirqd/0      [kernel.kallsyms]  [k] __build_skb_around
     1.05%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_pull_rcsum
     1.03%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_xmit
     0.98%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_flow_dissect
     0.89%  ksoftirqd/0      [kernel.kallsyms]  [k] ocelot_xmit
     0.84%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_build_skb
     0.73%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_free_bulk
     0.63%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_refill_rx_ring
     0.55%  ksoftirqd/0      [kernel.kallsyms]  [k] netif_skb_features
     0.46%  ksoftirqd/0      [kernel.kallsyms]  [k] dma_unmap_page_attrs
     0.46%  ksoftirqd/0      [kernel.kallsyms]  [k] fib_table_lookup
     0.36%  ksoftirqd/0      [kernel.kallsyms]  [k] fib_lookup_good_nhc
     0.33%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_data
     0.33%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_tx_vid
     0.32%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_flip_rx_buff
     0.29%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_consume_skb

After the patch:

650 Kpps = 426 Mbps

perf record -e cycles -C 0 sleep 10 && perf report
     9.34%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_pci_remove
     7.70%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_poll
     5.49%  ksoftirqd/0      [kernel.kallsyms]  [k] eth_type_trans
     4.62%  ksoftirqd/0      [kernel.kallsyms]  [k] take_page_off_buddy
     4.55%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_alloc
     4.36%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_gro_receive
     3.22%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_zcopy_clear
     2.59%  ksoftirqd/0      [kernel.kallsyms]  [k] __siphash_aligned
     2.51%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_start_xmit
     2.37%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_flow_dissect
     2.20%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_core
     1.84%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_free_bulk
     1.69%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gro_receive
     1.65%  ksoftirqd/0      [kernel.kallsyms]  [k] __dev_queue_xmit
     1.63%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_slave_xmit
     1.60%  ksoftirqd/0      [kernel.kallsyms]  [k] build_skb
     1.45%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_build_skb
     1.41%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_hard_start_xmit
     1.39%  ksoftirqd/0      [kernel.kallsyms]  [k] sch_direct_xmit
     1.39%  ksoftirqd/0      [kernel.kallsyms]  [k] __build_skb_around
     1.28%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_get_hash
     1.16%  ksoftirqd/0      [kernel.kallsyms]  [k] __local_bh_enable_ip
     1.14%  ksoftirqd/0      [kernel.kallsyms]  [k] udp4_gro_receive
     1.12%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_all
     1.09%  ksoftirqd/0      [kernel.kallsyms]  [k] ocelot_rcv
     1.08%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_drivername
     1.08%  ksoftirqd/0      [kernel.kallsyms]  [k] dma_unmap_page_attrs
     1.08%  ksoftirqd/0      [kernel.kallsyms]  [k] udp_gro_receive
     1.05%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_shutdown
     1.04%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_consume_skb
     1.03%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_segment_list
     0.90%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_gro_receive
     0.86%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gso_segment
     0.83%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_switch_rcv
     0.77%  ksoftirqd/0      [kernel.kallsyms]  [k] memmove
     0.75%  ksoftirqd/0      [kernel.kallsyms]  [k] do_csum
     0.73%  ksoftirqd/0      [kernel.kallsyms]  [k] netif_skb_features
     0.71%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_rcv
     0.69%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_list_core
     0.67%  ksoftirqd/0      [kernel.kallsyms]  [k] netif_receive_skb_list_internal

perf record -e cache-misses -C 0 sleep 10 && perf report
    12.38%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_zcopy_clear
     9.34%  ksoftirqd/0      [kernel.kallsyms]  [k] take_page_off_buddy
     8.62%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_pci_remove
     5.61%  ksoftirqd/0      [kernel.kallsyms]  [k] memmove
     5.44%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gro_receive
     4.61%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_poll
     4.20%  ksoftirqd/0      [kernel.kallsyms]  [k] inet_gso_segment
     3.58%  ksoftirqd/0      [kernel.kallsyms]  [k] dev_gro_receive
     3.19%  ksoftirqd/0      [kernel.kallsyms]  [k] build_skb
     3.11%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_all
     2.80%  ksoftirqd/0      [kernel.kallsyms]  [k] __copy_skb_header
     2.79%  ksoftirqd/0      [kernel.kallsyms]  [k] eth_type_trans
     2.31%  ksoftirqd/0      [kernel.kallsyms]  [k] ip_send_check
     1.95%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_get_hash
     1.64%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_alloc
     1.54%  ksoftirqd/0      [kernel.kallsyms]  [k] __netif_receive_skb_core
     1.52%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_slave_xmit
     1.51%  ksoftirqd/0      [kernel.kallsyms]  [k] kmem_cache_free_bulk
     1.49%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_pick_tx
     1.42%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_headers_offset_update
     1.34%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_segment_list
     1.20%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_build_skb
     1.19%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_start_xmit
     1.09%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_xmit
     0.94%  ksoftirqd/0      [kernel.kallsyms]  [k] netdev_core_pick_tx
     0.90%  ksoftirqd/0      [kernel.kallsyms]  [k] __dev_queue_xmit
     0.87%  ksoftirqd/0      [kernel.kallsyms]  [k] ocelot_xmit
     0.85%  ksoftirqd/0      [kernel.kallsyms]  [k] __skb_flow_dissect
     0.68%  ksoftirqd/0      [kernel.kallsyms]  [k] dsa_8021q_rcv
     0.63%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_pull_rcsum
     0.63%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_flip_rx_buff
     0.61%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_consume_skb
     0.50%  ksoftirqd/0      [kernel.kallsyms]  [k] skb_release_data
     0.48%  ksoftirqd/0      [kernel.kallsyms]  [k] dma_unmap_page_attrs
     0.41%  ksoftirqd/0      [kernel.kallsyms]  [k] enetc_refill_rx_ring
     0.37%  ksoftirqd/0      [kernel.kallsyms]  [k] __build_skb_around
     0.33%  ksoftirqd/0      [kernel.kallsyms]  [k] fib_table_lookup
     0.33%  ksoftirqd/0      [kernel.kallsyms]  [k] napi_skb_cache_put
     0.28%  ksoftirqd/0      [kernel.kallsyms]  [k] gro_cells_receive
     0.26%  ksoftirqd/0      [kernel.kallsyms]  [k] bpf_skb_load_helper_16

So the performance seems to be slightly worse with the patch, for
reasons that are not immediately apparent from perf, as far as I can
tell. If I just look at dsa_switch_rcv, I see there is a decrease in CPU
cycles from 1.01% to 0.83%, but that doesn't reflect in the throughput I
see for some reason.

(also please ignore some oddities in the perf reports like
"enetc_pci_remove", this comes from enetc_lock_mdio/enetc_unlock_mdio, I
have no idea why it gets printed like that)

So yeah, I cannot actually change my setup such that the list iteration
is more expensive than this (swp0 is the first element, swp1 is the second).

^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-08-11 17:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 19:03 [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean
2021-08-09 19:03 ` [RFC PATCH net-next 1/4] net: dsa: introduce a dsa_port_is_unused helper Vladimir Oltean
2021-08-10  9:34   ` Florian Fainelli
2021-08-09 19:03 ` [RFC PATCH net-next 2/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from the core Vladimir Oltean
2021-08-10  3:33   ` DENG Qingfang
2021-08-10  9:41     ` Florian Fainelli
2021-08-10 11:35       ` Vladimir Oltean
2021-08-10 16:35         ` Vladimir Oltean
2021-08-10 17:04           ` DENG Qingfang
2021-08-11 17:32             ` Vladimir Oltean
2021-08-10  9:37   ` Florian Fainelli
2021-08-09 19:03 ` [RFC PATCH net-next 3/4] net: dsa: remove the "dsa_to_port in a loop" antipattern from drivers Vladimir Oltean
2021-08-09 19:03 ` [RFC PATCH net-next 4/4] net: dsa: b53: express b53_for_each_port in terms of dsa_switch_for_each_port Vladimir Oltean
2021-08-10  9:39   ` Florian Fainelli
2021-08-10 13:14     ` Vladimir Oltean
2021-08-09 19:31 ` [RFC PATCH net-next 0/4] Remove the "dsa_to_port in a loop" antipattern Vladimir Oltean

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).