[PATCH net-next 0/6] Improvement for DSA cross-chip setups

[PATCH net-next 0/6] Improvement for DSA cross-chip setups

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This series improves some aspects in multi-switch DSA tree topologies:
- better device tree validation
- better handling of MTU changes
- better handling of multicast addresses
- removal of some unused code

Vladimir Oltean (6):
  net: dsa: assert uniqueness of dsa,member properties
  net: dsa: export the dsa_port_is_{user,cpu,dsa} helpers
  net: dsa: execute dsa_switch_mdb_add only for routing port in
    cross-chip topologies
  net: dsa: calculate the largest_mtu across all ports in the tree
  net: dsa: targeted MTU notifiers should only match on one port
  net: dsa: remove cross-chip support from the MRP notifiers

 include/net/dsa.h  | 15 ++++++++
 net/dsa/dsa2.c     | 22 ++++--------
 net/dsa/dsa_priv.h |  4 +--
 net/dsa/port.c     |  4 +--
 net/dsa/slave.c    | 22 ++++++------
 net/dsa/switch.c   | 87 ++++++++--------------------------------------
 6 files changed, 53 insertions(+), 101 deletions(-)

-- 
2.25.1

[PATCH net-next 1/6] net: dsa: assert uniqueness of dsa,member properties

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The cross-chip notifiers work by comparing each ds->index against the
info->sw_index value from the notifier. The ds->index is retrieved from
the device tree dsa,member property.

If a single tree cross-chip topology does not declare unique switch IDs,
this will result in hard-to-debug issues/voodoo effects such as the
cross-chip notifier for one switch port also matching the port with the
same number from another switch.

Check in dsa_switch_parse_member_of() whether the DSA switch tree
contains a DSA switch with the index we're preparing to add, before
actually adding it.

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

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index b71e87909f0e..ba244fbd9646 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1259,6 +1259,13 @@ static int dsa_switch_parse_member_of(struct dsa_switch *ds,
 	if (!ds->dst)
 		return -ENOMEM;
 
+	if (dsa_switch_find(ds->dst->index, ds->index)) {
+		dev_err(ds->dev,
+			"A DSA switch with index %d already exists in tree %d\n",
+			ds->index, ds->dst->index);
+		return -EEXIST;
+	}
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH net-next 1/6] net: dsa: assert uniqueness of dsa,member properties

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The cross-chip notifiers work by comparing each ds->index against the
> info->sw_index value from the notifier. The ds->index is retrieved from
> the device tree dsa,member property.
> 
> If a single tree cross-chip topology does not declare unique switch IDs,
> this will result in hard-to-debug issues/voodoo effects such as the
> cross-chip notifier for one switch port also matching the port with the
> same number from another switch.
> 
> Check in dsa_switch_parse_member_of() whether the DSA switch tree
> contains a DSA switch with the index we're preparing to add, before
> actually adding it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH net-next 1/6] net: dsa: assert uniqueness of dsa,member properties

[Andrew Lunn]

On Fri, Jun 18, 2021 at 09:30:12PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The cross-chip notifiers work by comparing each ds->index against the
> info->sw_index value from the notifier. The ds->index is retrieved from
> the device tree dsa,member property.
> 
> If a single tree cross-chip topology does not declare unique switch IDs,
> this will result in hard-to-debug issues/voodoo effects such as the
> cross-chip notifier for one switch port also matching the port with the
> same number from another switch.
> 
> Check in dsa_switch_parse_member_of() whether the DSA switch tree
> contains a DSA switch with the index we're preparing to add, before
> actually adding it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

    Andrew

[PATCH net-next 2/6] net: dsa: export the dsa_port_is_{user,cpu,dsa} helpers

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The difference between dsa_is_user_port and dsa_port_is_user is that the
former needs to look up the list of ports of the DSA switch tree in
order to find the struct dsa_port, while the latter directly receives it
as an argument.

dsa_is_user_port is already in widespread use and has its place, so
there isn't any chance of converting all callers to a single form.
But being able to do:
	dsa_port_is_user(dp)
instead of
	dsa_is_user_port(dp->ds, dp->index)

is much more efficient too, especially when the "dp" comes from an
iterator over the DSA switch tree - this reduces the complexity from
quadratic to linear.

Move these helpers from dsa2.c to include/net/dsa.h so that others can
use them too.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h | 15 +++++++++++++++
 net/dsa/dsa2.c    | 15 ---------------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 289d68e82da0..ea47783d5695 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -409,6 +409,21 @@ static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
 	return NULL;
 }
 
+static inline bool dsa_port_is_dsa(struct dsa_port *port)
+{
+	return port->type == DSA_PORT_TYPE_DSA;
+}
+
+static inline bool dsa_port_is_cpu(struct dsa_port *port)
+{
+	return port->type == DSA_PORT_TYPE_CPU;
+}
+
+static inline bool dsa_port_is_user(struct dsa_port *dp)
+{
+	return dp->type == DSA_PORT_TYPE_USER;
+}
+
 static inline bool dsa_is_unused_port(struct dsa_switch *ds, int p)
 {
 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_UNUSED;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ba244fbd9646..9000a8c84baf 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -219,21 +219,6 @@ static void dsa_tree_put(struct dsa_switch_tree *dst)
 		kref_put(&dst->refcount, dsa_tree_release);
 }
 
-static bool dsa_port_is_dsa(struct dsa_port *port)
-{
-	return port->type == DSA_PORT_TYPE_DSA;
-}
-
-static bool dsa_port_is_cpu(struct dsa_port *port)
-{
-	return port->type == DSA_PORT_TYPE_CPU;
-}
-
-static bool dsa_port_is_user(struct dsa_port *dp)
-{
-	return dp->type == DSA_PORT_TYPE_USER;
-}
-
 static struct dsa_port *dsa_tree_find_port_by_node(struct dsa_switch_tree *dst,
 						   struct device_node *dn)
 {
-- 
2.25.1

Re: [PATCH net-next 2/6] net: dsa: export the dsa_port_is_{user,cpu,dsa} helpers

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The difference between dsa_is_user_port and dsa_port_is_user is that the
> former needs to look up the list of ports of the DSA switch tree in
> order to find the struct dsa_port, while the latter directly receives it
> as an argument.
> 
> dsa_is_user_port is already in widespread use and has its place, so
> there isn't any chance of converting all callers to a single form.
> But being able to do:
> 	dsa_port_is_user(dp)
> instead of
> 	dsa_is_user_port(dp->ds, dp->index)
> 
> is much more efficient too, especially when the "dp" comes from an
> iterator over the DSA switch tree - this reduces the complexity from
> quadratic to linear.
> 
> Move these helpers from dsa2.c to include/net/dsa.h so that others can
> use them too.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH net-next 2/6] net: dsa: export the dsa_port_is_{user,cpu,dsa} helpers

[Andrew Lunn]

On Fri, Jun 18, 2021 at 09:30:13PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The difference between dsa_is_user_port and dsa_port_is_user is that the
> former needs to look up the list of ports of the DSA switch tree in
> order to find the struct dsa_port, while the latter directly receives it
> as an argument.
> 
> dsa_is_user_port is already in widespread use and has its place, so
> there isn't any chance of converting all callers to a single form.
> But being able to do:
> 	dsa_port_is_user(dp)
> instead of
> 	dsa_is_user_port(dp->ds, dp->index)
> 
> is much more efficient too, especially when the "dp" comes from an
> iterator over the DSA switch tree - this reduces the complexity from
> quadratic to linear.
> 
> Move these helpers from dsa2.c to include/net/dsa.h so that others can
> use them too.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

    Andrew

[PATCH net-next 3/6] net: dsa: execute dsa_switch_mdb_add only for routing port in cross-chip topologies

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Currently, the notifier for adding a multicast MAC address matches on
the targeted port and on all DSA links in the system, be they upstream
or downstream links.

This leads to a considerable amount of useless traffic.

Consider this daisy chain topology, and a MDB add notifier emitted on
sw0p0. It matches on sw0p0, sw0p3, sw1p3 and sw2p4.

   sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
[  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]
[   x   ] [       ] [       ] [   x   ] [       ]
                                  |
                                  +---------+
                                            |
   sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
[  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
[       ] [       ] [       ] [   x   ] [   x   ]
                                  |
                                  +---------+
                                            |
   sw2p0     sw2p1     sw2p2     sw2p3     sw2p4
[  user ] [  user ] [  user ] [  user ] [  dsa  ]
[       ] [       ] [       ] [       ] [   x   ]

But switch 0 has no reason to send the multicast traffic for that MAC
address on sw0p3, which is how it reaches switches 1 and 2. Those
switches don't expect, according to the user configuration, to receive
this multicast address from switch 1, and they will drop it anyway,
because the only valid destination is the port they received it on.
They only need to configure themselves to deliver that multicast address
_towards_ switch 1, where the MDB entry is installed.

Similarly, switch 1 should not send this multicast traffic towards
sw1p3, because that is how it reaches switch 2.

With this change, the heat map for this MDB notifier changes as follows:

   sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
[  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]
[   x   ] [       ] [       ] [       ] [       ]
                                  |
                                  +---------+
                                            |
   sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
[  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
[       ] [       ] [       ] [       ] [   x   ]
                                  |
                                  +---------+
                                            |
   sw2p0     sw2p1     sw2p2     sw2p3     sw2p4
[  user ] [  user ] [  user ] [  user ] [  dsa  ]
[       ] [       ] [       ] [       ] [   x   ]

Now the mdb notifier behaves the same as the fdb notifier.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/switch.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 9bf8e20ecdf3..8b601ced6b45 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -232,36 +232,15 @@ static int dsa_switch_lag_leave(struct dsa_switch *ds,
 	return 0;
 }
 
-static bool dsa_switch_mdb_match(struct dsa_switch *ds, int port,
-				 struct dsa_notifier_mdb_info *info)
-{
-	if (ds->index == info->sw_index && port == info->port)
-		return true;
-
-	if (dsa_is_dsa_port(ds, port))
-		return true;
-
-	return false;
-}
-
 static int dsa_switch_mdb_add(struct dsa_switch *ds,
 			      struct dsa_notifier_mdb_info *info)
 {
-	int err = 0;
-	int port;
+	int port = dsa_towards_port(ds, info->sw_index, info->port);
 
 	if (!ds->ops->port_mdb_add)
 		return -EOPNOTSUPP;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_switch_mdb_match(ds, port, info)) {
-			err = ds->ops->port_mdb_add(ds, port, info->mdb);
-			if (err)
-				break;
-		}
-	}
-
-	return err;
+	return ds->ops->port_mdb_add(ds, port, info->mdb);
 }
 
 static int dsa_switch_mdb_del(struct dsa_switch *ds,
-- 
2.25.1

Re: [PATCH net-next 3/6] net: dsa: execute dsa_switch_mdb_add only for routing port in cross-chip topologies

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Currently, the notifier for adding a multicast MAC address matches on
> the targeted port and on all DSA links in the system, be they upstream
> or downstream links.
> 
> This leads to a considerable amount of useless traffic.
> 
> Consider this daisy chain topology, and a MDB add notifier emitted on
> sw0p0. It matches on sw0p0, sw0p3, sw1p3 and sw2p4.
> 
>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
> [  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]
> [   x   ] [       ] [       ] [   x   ] [       ]
>                                   |
>                                   +---------+
>                                             |
>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
> [       ] [       ] [       ] [   x   ] [   x   ]
>                                   |
>                                   +---------+
>                                             |
>    sw2p0     sw2p1     sw2p2     sw2p3     sw2p4
> [  user ] [  user ] [  user ] [  user ] [  dsa  ]
> [       ] [       ] [       ] [       ] [   x   ]
> 
> But switch 0 has no reason to send the multicast traffic for that MAC
> address on sw0p3, which is how it reaches switches 1 and 2. Those
> switches don't expect, according to the user configuration, to receive
> this multicast address from switch 1, and they will drop it anyway,
> because the only valid destination is the port they received it on.
> They only need to configure themselves to deliver that multicast address
> _towards_ switch 1, where the MDB entry is installed.
> 
> Similarly, switch 1 should not send this multicast traffic towards
> sw1p3, because that is how it reaches switch 2.
> 
> With this change, the heat map for this MDB notifier changes as follows:
> 
>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
> [  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]
> [   x   ] [       ] [       ] [       ] [       ]
>                                   |
>                                   +---------+
>                                             |
>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
> [       ] [       ] [       ] [       ] [   x   ]
>                                   |
>                                   +---------+
>                                             |
>    sw2p0     sw2p1     sw2p2     sw2p3     sw2p4
> [  user ] [  user ] [  user ] [  user ] [  dsa  ]
> [       ] [       ] [       ] [       ] [   x   ]
> 
> Now the mdb notifier behaves the same as the fdb notifier.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[PATCH net-next 4/6] net: dsa: calculate the largest_mtu across all ports in the tree

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

If we have a cross-chip topology like this:

   sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
[  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]
                                  |
                                  +---------+
                                            |
   sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
[  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]

and we issue the following commands:

1. ip link set sw0p1 mtu 1700
2. ip link set sw1p1 mtu 1600

we notice the following happening:

Command 1. emits a non-targeted MTU notifier for the CPU port (sw0p0)
with the largest_mtu calculated across switch 0, of 1700. This matches
sw0p0, sw0p3 and sw1p4 (all CPU ports and DSA links).
Then, it emits a targeted MTU notifier for the user port (sw0p1), again
with MTU 1700 (this doesn't matter).

Command 2. emits a non-targeted MTU notifier for the CPU port (sw0p0)
with the largest_mtu calculated across switch 1, of 1600. This matches
the same group of ports as above, and decreases the MTU for the CPU port
and the DSA links from 1700 to 1600.

As a result, the sw0p1 user port can no longer communicate with its CPU
port at MTU 1700.

To address this, we should calculate the largest_mtu across all switches
that may share a CPU port, and only emit MTU notifiers with that value.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 798944aa847a..ac2ca5f75af3 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1528,6 +1528,7 @@ int dsa_slave_change_mtu(struct net_device *dev, int new_mtu)
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->dp->ds;
+	struct dsa_port *dp_iter;
 	struct dsa_port *cpu_dp;
 	int port = p->dp->index;
 	int largest_mtu = 0;
@@ -1535,31 +1536,31 @@ int dsa_slave_change_mtu(struct net_device *dev, int new_mtu)
 	int old_master_mtu;
 	int mtu_limit;
 	int cpu_mtu;
-	int err, i;
+	int err;
 
 	if (!ds->ops->port_change_mtu)
 		return -EOPNOTSUPP;
 
-	for (i = 0; i < ds->num_ports; i++) {
+	list_for_each_entry(dp_iter, &ds->dst->ports, list) {
 		int slave_mtu;
 
-		if (!dsa_is_user_port(ds, i))
+		if (!dsa_port_is_user(dp_iter))
 			continue;
 
 		/* During probe, this function will be called for each slave
 		 * device, while not all of them have been allocated. That's
 		 * ok, it doesn't change what the maximum is, so ignore it.
 		 */
-		if (!dsa_to_port(ds, i)->slave)
+		if (!dp_iter->slave)
 			continue;
 
 		/* Pretend that we already applied the setting, which we
 		 * actually haven't (still haven't done all integrity checks)
 		 */
-		if (i == port)
+		if (dp_iter == dp)
 			slave_mtu = new_mtu;
 		else
-			slave_mtu = dsa_to_port(ds, i)->slave->mtu;
+			slave_mtu = dp_iter->slave->mtu;
 
 		if (largest_mtu < slave_mtu)
 			largest_mtu = slave_mtu;
-- 
2.25.1

Re: [PATCH net-next 4/6] net: dsa: calculate the largest_mtu across all ports in the tree

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> If we have a cross-chip topology like this:
> 
>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
> [  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]
>                                   |
>                                   +---------+
>                                             |
>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
> 
> and we issue the following commands:
> 
> 1. ip link set sw0p1 mtu 1700
> 2. ip link set sw1p1 mtu 1600
> 
> we notice the following happening:
> 
> Command 1. emits a non-targeted MTU notifier for the CPU port (sw0p0)
> with the largest_mtu calculated across switch 0, of 1700. This matches
> sw0p0, sw0p3 and sw1p4 (all CPU ports and DSA links).
> Then, it emits a targeted MTU notifier for the user port (sw0p1), again
> with MTU 1700 (this doesn't matter).
> 
> Command 2. emits a non-targeted MTU notifier for the CPU port (sw0p0)
> with the largest_mtu calculated across switch 1, of 1600. This matches
> the same group of ports as above, and decreases the MTU for the CPU port
> and the DSA links from 1700 to 1600.
> 
> As a result, the sw0p1 user port can no longer communicate with its CPU
> port at MTU 1700.
> 
> To address this, we should calculate the largest_mtu across all switches
> that may share a CPU port, and only emit MTU notifiers with that value.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[PATCH net-next 5/6] net: dsa: targeted MTU notifiers should only match on one port

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

dsa_slave_change_mtu() calls dsa_port_mtu_change() twice:
- it sends a cross-chip notifier with the MTU of the CPU port which is
  used to update the DSA links.
- it sends one targeted MTU notifier which is supposed to only match the
  user port on which we are changing the MTU. The "propagate_upstream"
  variable is used here to bypass the cross-chip notifier system from
  switch.c

But due to a mistake, the second, targeted notifier matches not only on
the user port, but also on the DSA link which is a member of the same
switch, if that exists.

And because the DSA links of the entire dst were programmed in a
previous round to the largest_mtu via a "propagate_upstream == true"
notification, then the dsa_port_mtu_change(propagate_upstream == false)
call that is immediately upcoming will break the MTU on the one DSA link
which is chip-wise local to the dp whose MTU is changing right now.

Example given this daisy chain topology:

   sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
[  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]
[   x   ] [       ] [       ] [   x   ] [       ]
                                  |
                                  +---------+
                                            |
   sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
[  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
[       ] [       ] [       ] [       ] [   x   ]

ip link set sw0p1 mtu 9000
ip link set sw1p1 mtu 9000 # at this stage, sw0p1 and sw1p1 can talk
                           # to one another using jumbo frames
ip link set sw0p2 mtu 1500 # this programs the sw0p3 DSA link first to
                           # the largest_mtu of 9000, then reprograms it to
                           # 1500 with the "propagate_upstream == false"
                           # notifier, breaking communication between
                           # sw0p1 and sw1p1

To escape from this situation, make the targeted match really match on a
single port - the user port, and rename the "propagate_upstream"
variable to "targeted_match" to clarify the intention and avoid future
issues.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h | 4 ++--
 net/dsa/port.c     | 4 ++--
 net/dsa/slave.c    | 9 +++++----
 net/dsa/switch.c   | 9 ++++++---
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index b8b17474b72b..b0811253d101 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -84,7 +84,7 @@ struct dsa_notifier_vlan_info {
 
 /* DSA_NOTIFIER_MTU */
 struct dsa_notifier_mtu_info {
-	bool propagate_upstream;
+	bool targeted_match;
 	int sw_index;
 	int port;
 	int mtu;
@@ -200,7 +200,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
-			bool propagate_upstream);
+			bool targeted_match);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 6379d66a6bb3..5c93f1e1a03d 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -567,11 +567,11 @@ int dsa_port_mrouter(struct dsa_port *dp, bool mrouter,
 }
 
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
-			bool propagate_upstream)
+			bool targeted_match)
 {
 	struct dsa_notifier_mtu_info info = {
 		.sw_index = dp->ds->index,
-		.propagate_upstream = propagate_upstream,
+		.targeted_match = targeted_match,
 		.port = dp->index,
 		.mtu = new_mtu,
 	};
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ac2ca5f75af3..5e668e529575 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1586,14 +1586,15 @@ int dsa_slave_change_mtu(struct net_device *dev, int new_mtu)
 			goto out_master_failed;
 
 		/* We only need to propagate the MTU of the CPU port to
-		 * upstream switches.
+		 * upstream switches, so create a non-targeted notifier which
+		 * updates all switches.
 		 */
-		err = dsa_port_mtu_change(cpu_dp, cpu_mtu, true);
+		err = dsa_port_mtu_change(cpu_dp, cpu_mtu, false);
 		if (err)
 			goto out_cpu_failed;
 	}
 
-	err = dsa_port_mtu_change(dp, new_mtu, false);
+	err = dsa_port_mtu_change(dp, new_mtu, true);
 	if (err)
 		goto out_port_failed;
 
@@ -1607,7 +1608,7 @@ int dsa_slave_change_mtu(struct net_device *dev, int new_mtu)
 	if (new_master_mtu != old_master_mtu)
 		dsa_port_mtu_change(cpu_dp, old_master_mtu -
 				    dsa_tag_protocol_overhead(cpu_dp->tag_ops),
-				    true);
+				    false);
 out_cpu_failed:
 	if (new_master_mtu != old_master_mtu)
 		dev_set_mtu(master, old_master_mtu);
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 8b601ced6b45..75f567390a6b 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -52,10 +52,13 @@ static int dsa_switch_ageing_time(struct dsa_switch *ds,
 static bool dsa_switch_mtu_match(struct dsa_switch *ds, int port,
 				 struct dsa_notifier_mtu_info *info)
 {
-	if (ds->index == info->sw_index)
-		return (port == info->port) || dsa_is_dsa_port(ds, port);
+	if (ds->index == info->sw_index && port == info->port)
+		return true;
 
-	if (!info->propagate_upstream)
+	/* Do not propagate to other switches in the tree if the notifier was
+	 * targeted for a single switch.
+	 */
+	if (info->targeted_match)
 		return false;
 
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
-- 
2.25.1

Re: [PATCH net-next 5/6] net: dsa: targeted MTU notifiers should only match on one port

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> dsa_slave_change_mtu() calls dsa_port_mtu_change() twice:
> - it sends a cross-chip notifier with the MTU of the CPU port which is
>   used to update the DSA links.
> - it sends one targeted MTU notifier which is supposed to only match the
>   user port on which we are changing the MTU. The "propagate_upstream"
>   variable is used here to bypass the cross-chip notifier system from
>   switch.c
> 
> But due to a mistake, the second, targeted notifier matches not only on
> the user port, but also on the DSA link which is a member of the same
> switch, if that exists.
> 
> And because the DSA links of the entire dst were programmed in a
> previous round to the largest_mtu via a "propagate_upstream == true"
> notification, then the dsa_port_mtu_change(propagate_upstream == false)
> call that is immediately upcoming will break the MTU on the one DSA link
> which is chip-wise local to the dp whose MTU is changing right now.
> 
> Example given this daisy chain topology:
> 
>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4
> [  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]
> [   x   ] [       ] [       ] [   x   ] [       ]
>                                   |
>                                   +---------+
>                                             |
>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4
> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]
> [       ] [       ] [       ] [       ] [   x   ]
> 
> ip link set sw0p1 mtu 9000
> ip link set sw1p1 mtu 9000 # at this stage, sw0p1 and sw1p1 can talk
>                            # to one another using jumbo frames
> ip link set sw0p2 mtu 1500 # this programs the sw0p3 DSA link first to
>                            # the largest_mtu of 9000, then reprograms it to
>                            # 1500 with the "propagate_upstream == false"
>                            # notifier, breaking communication between
>                            # sw0p1 and sw1p1
> 
> To escape from this situation, make the targeted match really match on a
> single port - the user port, and rename the "propagate_upstream"
> variable to "targeted_match" to clarify the intention and avoid future
> issues.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[PATCH net-next 6/6] net: dsa: remove cross-chip support from the MRP notifiers

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

With MRP hardware assist being supported only by the ocelot switch
family, which by design does not support cross-chip bridging, the
current match functions are at best a guess and have not been confirmed
in any way to do anything relevant in a multi-switch topology.

Drop the code and make the notifiers match only on the targeted switch
port.

Cc: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/switch.c | 53 +++++++-----------------------------------------
 1 file changed, 7 insertions(+), 46 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 75f567390a6b..7e948bf15fe0 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -346,36 +346,16 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
 	return 0;
 }
 
-static bool dsa_switch_mrp_match(struct dsa_switch *ds, int port,
-				 struct dsa_notifier_mrp_info *info)
-{
-	if (ds->index == info->sw_index && port == info->port)
-		return true;
-
-	if (dsa_is_dsa_port(ds, port))
-		return true;
-
-	return false;
-}
-
 static int dsa_switch_mrp_add(struct dsa_switch *ds,
 			      struct dsa_notifier_mrp_info *info)
 {
-	int err = 0;
-	int port;
-
 	if (!ds->ops->port_mrp_add)
 		return -EOPNOTSUPP;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_switch_mrp_match(ds, port, info)) {
-			err = ds->ops->port_mrp_add(ds, port, info->mrp);
-			if (err)
-				break;
-		}
-	}
+	if (ds->index == info->sw_index)
+		return ds->ops->port_mrp_add(ds, info->port, info->mrp);
 
-	return err;
+	return 0;
 }
 
 static int dsa_switch_mrp_del(struct dsa_switch *ds,
@@ -390,39 +370,20 @@ static int dsa_switch_mrp_del(struct dsa_switch *ds,
 	return 0;
 }
 
-static bool
-dsa_switch_mrp_ring_role_match(struct dsa_switch *ds, int port,
-			       struct dsa_notifier_mrp_ring_role_info *info)
-{
-	if (ds->index == info->sw_index && port == info->port)
-		return true;
-
-	if (dsa_is_dsa_port(ds, port))
-		return true;
-
-	return false;
-}
-
 static int
 dsa_switch_mrp_add_ring_role(struct dsa_switch *ds,
 			     struct dsa_notifier_mrp_ring_role_info *info)
 {
 	int err = 0;
-	int port;
 
 	if (!ds->ops->port_mrp_add)
 		return -EOPNOTSUPP;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (dsa_switch_mrp_ring_role_match(ds, port, info)) {
-			err = ds->ops->port_mrp_add_ring_role(ds, port,
-							      info->mrp);
-			if (err)
-				break;
-		}
-	}
+	if (ds->index == info->sw_index)
+		return ds->ops->port_mrp_add_ring_role(ds, info->port,
+						       info->mrp);
 
-	return err;
+	return 0;
 }
 
 static int
-- 
2.25.1

Re: [PATCH net-next 6/6] net: dsa: remove cross-chip support from the MRP notifiers

[Florian Fainelli]

On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> With MRP hardware assist being supported only by the ocelot switch
> family, which by design does not support cross-chip bridging, the
> current match functions are at best a guess and have not been confirmed
> in any way to do anything relevant in a multi-switch topology.
> 
> Drop the code and make the notifiers match only on the targeted switch
> port.
> 
> Cc: Horatiu Vultur <horatiu.vultur@microchip.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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