[PATCH net-next 0/2] Improvements to the DSA tag_8021q cross-chip notifiers

[PATCH net-next 0/2] Improvements to the DSA tag_8021q cross-chip notifiers

[Vladimir Oltean]

This series improves cross-chip notifier error messages and addresses a
benign error message seen during reboot on a system with disjoint DSA
trees.

Vladimir Oltean (2):
  net: dsa: print more information when a cross-chip notifier fails
  net: dsa: tag_8021q: don't broadcast during setup/teardown

 net/dsa/dsa2.c      |  3 +++
 net/dsa/dsa_priv.h  |  4 ++--
 net/dsa/port.c      | 32 ++++++++++++++++++++++----------
 net/dsa/tag_8021q.c | 21 +++++++++++----------
 4 files changed, 38 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH net-next 1/2] net: dsa: print more information when a cross-chip notifier fails

[Vladimir Oltean]

Currently this error message does not say a lot:

[   32.693498] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
[   32.699725] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
[   32.705931] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
[   32.712139] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
[   32.718347] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
[   32.724554] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT

but in this form, it is immediately obvious (at least to me) what the
problem is, even without further looking at the code:

[   12.345566] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 1088 deletion: -ENOENT
[   12.353804] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 2112 deletion: -ENOENT
[   12.362019] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 1089 deletion: -ENOENT
[   12.370246] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 2113 deletion: -ENOENT
[   12.378466] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 1090 deletion: -ENOENT
[   12.386683] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 2114 deletion: -ENOENT

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

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 831d50d28d59..ee1c6bfcb386 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -426,7 +426,9 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
 
 	err = dsa_broadcast(DSA_NOTIFIER_BRIDGE_LEAVE, &info);
 	if (err)
-		pr_err("DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE\n");
+		dev_err(dp->ds->dev,
+			"port %d failed to notify DSA_NOTIFIER_BRIDGE_LEAVE: %pe\n",
+			dp->index, ERR_PTR(err));
 
 	dsa_port_switchdev_unsync_attrs(dp);
 }
@@ -525,8 +527,9 @@ void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag)
 
 	err = dsa_port_notify(dp, DSA_NOTIFIER_LAG_LEAVE, &info);
 	if (err)
-		pr_err("DSA: failed to notify DSA_NOTIFIER_LAG_LEAVE: %d\n",
-		       err);
+		dev_err(dp->ds->dev,
+			"port %d failed to notify DSA_NOTIFIER_LAG_LEAVE: %pe\n",
+			dp->index, ERR_PTR(err));
 
 	dsa_lag_unmap(dp->ds->dst, lag);
 }
@@ -1306,7 +1309,9 @@ void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr)
 
 	err = dsa_port_notify(dp, DSA_NOTIFIER_HSR_LEAVE, &info);
 	if (err)
-		pr_err("DSA: failed to notify DSA_NOTIFIER_HSR_LEAVE\n");
+		dev_err(dp->ds->dev,
+			"port %d failed to notify DSA_NOTIFIER_HSR_LEAVE: %pe\n",
+			dp->index, ERR_PTR(err));
 }
 
 int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid)
@@ -1333,6 +1338,7 @@ void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid)
 
 	err = dsa_broadcast(DSA_NOTIFIER_TAG_8021Q_VLAN_DEL, &info);
 	if (err)
-		pr_err("DSA: failed to notify tag_8021q VLAN deletion: %pe\n",
-		       ERR_PTR(err));
+		dev_err(dp->ds->dev,
+			"port %d failed to notify tag_8021q VLAN %d deletion: %pe\n",
+			dp->index, vid, ERR_PTR(err));
 }
-- 
2.25.1

[PATCH net-next 2/2] net: dsa: tag_8021q: don't broadcast during setup/teardown

[Vladimir Oltean]

Currently, on my board with multiple sja1105 switches in disjoint trees
described in commit f66a6a69f97a ("net: dsa: permit cross-chip bridging
between all trees in the system"), rebooting the board triggers the
following benign warnings:

[   12.345566] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 1088 deletion: -ENOENT
[   12.353804] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 2112 deletion: -ENOENT
[   12.362019] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 1089 deletion: -ENOENT
[   12.370246] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 2113 deletion: -ENOENT
[   12.378466] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 1090 deletion: -ENOENT
[   12.386683] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 2114 deletion: -ENOENT

Basically switch 1 calls dsa_tag_8021q_unregister, and switch 1's TX and
RX VLANs cannot be found on switch 2's CPU port.

But why would switch 2 even attempt to delete switch 1's TX and RX
tag_8021q VLANs from its CPU port? Well, because we use dsa_broadcast,
and it is supposed that it had added those VLANs in the first place
(because in dsa_port_tag_8021q_vlan_match, all CPU ports match
regardless of their tree index or switch index).

The two trees probe asynchronously, and when switch 1 probed, it called
dsa_broadcast which did not notify the tree of switch 2, because that
didn't probe yet. But during unbind, switch 2's tree _is_ probed, so it
_is_ notified of the deletion.

Before jumping to introduce a synchronization mechanism between the
probing across disjoint switch trees, let's take a step back and see
whether we _need_ to do that in the first place.

The RX and TX VLANs of switch 1 would be needed on switch 2's CPU port
only if switch 1 and 2 were part of a cross-chip bridge. And
dsa_tag_8021q_bridge_join takes care precisely of that (but if probing
was synchronous, the bridge_join would just end up bumping the VLANs'
refcount, because they are already installed by the setup path).

Since by the time the ports are bridged, all DSA trees are already set
up, and we don't need the tag_8021q VLANs of one switch installed on the
other switches during probe time, the answer is that we don't need to
fix the synchronization issue.

So make the setup and teardown code paths call dsa_port_notify, which
notifies only the local tree, and the bridge code paths call
dsa_broadcast, which let the other trees know as well.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c      |  3 +++
 net/dsa/dsa_priv.h  |  4 ++--
 net/dsa/port.c      | 14 ++++++++++----
 net/dsa/tag_8021q.c | 21 +++++++++++----------
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8150e16aaa55..dcd67801eca4 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -49,6 +49,9 @@ int dsa_tree_notify(struct dsa_switch_tree *dst, unsigned long e, void *v)
  * Can be used to notify the switching fabric of events such as cross-chip
  * bridging between disjoint trees (such as islands of tagger-compatible
  * switches bridged by an incompatible middle switch).
+ *
+ * WARNING: this function is not reliable during probe time, because probing
+ * between trees is asynchronous and not all DSA trees might have probed.
  */
 int dsa_broadcast(unsigned long e, void *v)
 {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 9575cabd3ec3..d9cc1ffcca10 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -261,8 +261,8 @@ int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
 void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
-int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid);
-void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid);
+int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast);
+void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
 
 static inline bool dsa_port_offloads_bridge_port(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ee1c6bfcb386..979042a64d1a 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1314,7 +1314,7 @@ void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr)
 			dp->index, ERR_PTR(err));
 }
 
-int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid)
+int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid, bool broadcast)
 {
 	struct dsa_notifier_tag_8021q_vlan_info info = {
 		.tree_index = dp->ds->dst->index,
@@ -1323,10 +1323,13 @@ int dsa_port_tag_8021q_vlan_add(struct dsa_port *dp, u16 vid)
 		.vid = vid,
 	};
 
-	return dsa_broadcast(DSA_NOTIFIER_TAG_8021Q_VLAN_ADD, &info);
+	if (broadcast)
+		return dsa_broadcast(DSA_NOTIFIER_TAG_8021Q_VLAN_ADD, &info);
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_TAG_8021Q_VLAN_ADD, &info);
 }
 
-void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid)
+void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid, bool broadcast)
 {
 	struct dsa_notifier_tag_8021q_vlan_info info = {
 		.tree_index = dp->ds->dst->index,
@@ -1336,7 +1339,10 @@ void dsa_port_tag_8021q_vlan_del(struct dsa_port *dp, u16 vid)
 	};
 	int err;
 
-	err = dsa_broadcast(DSA_NOTIFIER_TAG_8021Q_VLAN_DEL, &info);
+	if (broadcast)
+		err = dsa_broadcast(DSA_NOTIFIER_TAG_8021Q_VLAN_DEL, &info);
+	else
+		err = dsa_port_notify(dp, DSA_NOTIFIER_TAG_8021Q_VLAN_DEL, &info);
 	if (err)
 		dev_err(dp->ds->dev,
 			"port %d failed to notify tag_8021q VLAN %d deletion: %pe\n",
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 654697ebb6f3..e6d5f3b4fd89 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -362,12 +362,12 @@ int dsa_tag_8021q_bridge_join(struct dsa_switch *ds,
 			continue;
 
 		/* Install the RX VID of the targeted port in our VLAN table */
-		err = dsa_port_tag_8021q_vlan_add(dp, targeted_rx_vid);
+		err = dsa_port_tag_8021q_vlan_add(dp, targeted_rx_vid, false);
 		if (err)
 			return err;
 
 		/* Install our RX VID into the targeted port's VLAN table */
-		err = dsa_port_tag_8021q_vlan_add(targeted_dp, rx_vid);
+		err = dsa_port_tag_8021q_vlan_add(targeted_dp, rx_vid, false);
 		if (err)
 			return err;
 	}
@@ -398,10 +398,10 @@ int dsa_tag_8021q_bridge_leave(struct dsa_switch *ds,
 			continue;
 
 		/* Remove the RX VID of the targeted port from our VLAN table */
-		dsa_port_tag_8021q_vlan_del(dp, targeted_rx_vid);
+		dsa_port_tag_8021q_vlan_del(dp, targeted_rx_vid, true);
 
 		/* Remove our RX VID from the targeted port's VLAN table */
-		dsa_port_tag_8021q_vlan_del(targeted_dp, rx_vid);
+		dsa_port_tag_8021q_vlan_del(targeted_dp, rx_vid, true);
 	}
 
 	return 0;
@@ -413,7 +413,8 @@ int dsa_tag_8021q_bridge_tx_fwd_offload(struct dsa_switch *ds, int port,
 {
 	u16 tx_vid = dsa_8021q_bridge_tx_fwd_offload_vid(bridge_num);
 
-	return dsa_port_tag_8021q_vlan_add(dsa_to_port(ds, port), tx_vid);
+	return dsa_port_tag_8021q_vlan_add(dsa_to_port(ds, port), tx_vid,
+					   true);
 }
 EXPORT_SYMBOL_GPL(dsa_tag_8021q_bridge_tx_fwd_offload);
 
@@ -423,7 +424,7 @@ void dsa_tag_8021q_bridge_tx_fwd_unoffload(struct dsa_switch *ds, int port,
 {
 	u16 tx_vid = dsa_8021q_bridge_tx_fwd_offload_vid(bridge_num);
 
-	dsa_port_tag_8021q_vlan_del(dsa_to_port(ds, port), tx_vid);
+	dsa_port_tag_8021q_vlan_del(dsa_to_port(ds, port), tx_vid, true);
 }
 EXPORT_SYMBOL_GPL(dsa_tag_8021q_bridge_tx_fwd_unoffload);
 
@@ -450,7 +451,7 @@ static int dsa_tag_8021q_port_setup(struct dsa_switch *ds, int port)
 	 * L2 forwarding rules still take precedence when there are no VLAN
 	 * restrictions, so there are no concerns about leaking traffic.
 	 */
-	err = dsa_port_tag_8021q_vlan_add(dp, rx_vid);
+	err = dsa_port_tag_8021q_vlan_add(dp, rx_vid, true);
 	if (err) {
 		dev_err(ds->dev,
 			"Failed to apply RX VID %d to port %d: %pe\n",
@@ -462,7 +463,7 @@ static int dsa_tag_8021q_port_setup(struct dsa_switch *ds, int port)
 	vlan_vid_add(master, ctx->proto, rx_vid);
 
 	/* Finally apply the TX VID on this port and on the CPU port */
-	err = dsa_port_tag_8021q_vlan_add(dp, tx_vid);
+	err = dsa_port_tag_8021q_vlan_add(dp, tx_vid, true);
 	if (err) {
 		dev_err(ds->dev,
 			"Failed to apply TX VID %d on port %d: %pe\n",
@@ -489,11 +490,11 @@ static void dsa_tag_8021q_port_teardown(struct dsa_switch *ds, int port)
 
 	master = dp->cpu_dp->master;
 
-	dsa_port_tag_8021q_vlan_del(dp, rx_vid);
+	dsa_port_tag_8021q_vlan_del(dp, rx_vid, false);
 
 	vlan_vid_del(master, ctx->proto, rx_vid);
 
-	dsa_port_tag_8021q_vlan_del(dp, tx_vid);
+	dsa_port_tag_8021q_vlan_del(dp, tx_vid, false);
 }
 
 static int dsa_tag_8021q_setup(struct dsa_switch *ds)
-- 
2.25.1

Re: [PATCH net-next 1/2] net: dsa: print more information when a cross-chip notifier fails

[Florian Fainelli]

On 8/11/2021 6:46 AM, Vladimir Oltean wrote:
> Currently this error message does not say a lot:
> 
> [   32.693498] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> [   32.699725] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> [   32.705931] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> [   32.712139] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> [   32.718347] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> [   32.724554] DSA: failed to notify tag_8021q VLAN deletion: -ENOENT
> 
> but in this form, it is immediately obvious (at least to me) what the
> problem is, even without further looking at the code:
> 
> [   12.345566] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 1088 deletion: -ENOENT
> [   12.353804] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 2112 deletion: -ENOENT
> [   12.362019] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 1089 deletion: -ENOENT
> [   12.370246] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 2113 deletion: -ENOENT
> [   12.378466] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 1090 deletion: -ENOENT
> [   12.386683] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 2114 deletion: -ENOENT
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH net-next 2/2] net: dsa: tag_8021q: don't broadcast during setup/teardown

[Florian Fainelli]

On 8/11/2021 6:46 AM, Vladimir Oltean wrote:
> Currently, on my board with multiple sja1105 switches in disjoint trees
> described in commit f66a6a69f97a ("net: dsa: permit cross-chip bridging
> between all trees in the system"), rebooting the board triggers the
> following benign warnings:
> 
> [   12.345566] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 1088 deletion: -ENOENT
> [   12.353804] sja1105 spi2.0: port 0 failed to notify tag_8021q VLAN 2112 deletion: -ENOENT
> [   12.362019] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 1089 deletion: -ENOENT
> [   12.370246] sja1105 spi2.0: port 1 failed to notify tag_8021q VLAN 2113 deletion: -ENOENT
> [   12.378466] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 1090 deletion: -ENOENT
> [   12.386683] sja1105 spi2.0: port 2 failed to notify tag_8021q VLAN 2114 deletion: -ENOENT
> 
> Basically switch 1 calls dsa_tag_8021q_unregister, and switch 1's TX and
> RX VLANs cannot be found on switch 2's CPU port.
> 
> But why would switch 2 even attempt to delete switch 1's TX and RX
> tag_8021q VLANs from its CPU port? Well, because we use dsa_broadcast,
> and it is supposed that it had added those VLANs in the first place
> (because in dsa_port_tag_8021q_vlan_match, all CPU ports match
> regardless of their tree index or switch index).
> 
> The two trees probe asynchronously, and when switch 1 probed, it called
> dsa_broadcast which did not notify the tree of switch 2, because that
> didn't probe yet. But during unbind, switch 2's tree _is_ probed, so it
> _is_ notified of the deletion.
> 
> Before jumping to introduce a synchronization mechanism between the
> probing across disjoint switch trees, let's take a step back and see
> whether we _need_ to do that in the first place.
> 
> The RX and TX VLANs of switch 1 would be needed on switch 2's CPU port
> only if switch 1 and 2 were part of a cross-chip bridge. And
> dsa_tag_8021q_bridge_join takes care precisely of that (but if probing
> was synchronous, the bridge_join would just end up bumping the VLANs'
> refcount, because they are already installed by the setup path).
> 
> Since by the time the ports are bridged, all DSA trees are already set
> up, and we don't need the tag_8021q VLANs of one switch installed on the
> other switches during probe time, the answer is that we don't need to
> fix the synchronization issue.
> 
> So make the setup and teardown code paths call dsa_port_notify, which
> notifies only the local tree, and the bridge code paths call
> dsa_broadcast, which let the other trees know as well.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH net-next 0/2] Improvements to the DSA tag_8021q cross-chip notifiers

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Wed, 11 Aug 2021 16:46:04 +0300 you wrote:
> This series improves cross-chip notifier error messages and addresses a
> benign error message seen during reboot on a system with disjoint DSA
> trees.
> 
> Vladimir Oltean (2):
>   net: dsa: print more information when a cross-chip notifier fails
>   net: dsa: tag_8021q: don't broadcast during setup/teardown
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: dsa: print more information when a cross-chip notifier fails
    https://git.kernel.org/netdev/net-next/c/ab97462beb18
  - [net-next,2/2] net: dsa: tag_8021q: don't broadcast during setup/teardown
    https://git.kernel.org/netdev/net-next/c/724395f4dc95

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