net: dsa: make tagging protocols connect to individual switches from a tree

On the NXP Bluebox 3 board which uses a multi-switch setup with sja1105,
the mechanism through which the tagger connects to the switch tree is
broken, due to improper DSA code design. At the time when tag_ops->connect()
is called in dsa_port_parse_cpu(), DSA hasn't finished "touching" all
the ports, so it doesn't know how large the tree is and how many ports
it has. It has just seen the first CPU port by this time. As a result,
this function will call the tagger's ->connect method too early, and the
tagger will connect only to the first switch from the tree.

This could be perhaps addressed a bit more simply by just moving the
tag_ops->connect(dst) call a bit later (for example in dsa_tree_setup),
but there is already a design inconsistency at present: on the switch
side, the notification is on a per-switch basis, but on the tagger side,
it is on a per-tree basis. Furthermore, the persistent storage itself is
per switch (ds->tagger_data). And the tagger connect and disconnect
procedures (at least the ones that exist currently) could see a fair bit
of simplification if they didn't have to iterate through the switches of
a tree.

To fix the issue, this change transforms tag_ops->connect(dst) into
tag_ops->connect(ds) and moves it somewhere where we already iterate
over all switches of a tree. That is in dsa_switch_setup_tag_protocol(),
which is a good placement because we already have there the connection
call to the switch side of things.

As for the dsa_tree_bind_tag_proto() method (called from the code path
that changes the tag protocol), things are a bit more complicated
because we receive the tree as argument, yet when we unwind on errors,
it would be nice to not call tag_ops->disconnect(ds) where we didn't
previously call tag_ops->connect(ds). We didn't have this problem before
because the tag_ops connection operations passed the entire dst before,
and this is more fine grained now. To solve the error rewind case using
the new API, we have to create yet one more cross-chip notifier for
disconnection, and stay connected with the old tag protocol to all the
switches in the tree until we've succeeded to connect with the new one
as well. So if something fails half way, the whole tree is still
connected to the old tagger. But there may still be leaks if the tagger
fails to connect to the 2nd out of 3 switches in a tree: somebody needs
to tell the tagger to disconnect from the first switch. Nothing comes
for free, and this was previously handled privately by the tagging
protocol driver before, but now we need to emit a disconnect cross-chip
notifier for that, because DSA has to take care of the unwind path. We
assume that the tagging protocol has connected to a switch if it has set
ds->tagger_data to something, otherwise we avoid calling its
disconnection method in the error rewind path.

The rest of the changes are in the tagging protocol drivers, and have to
do with the replacement of dst with ds. The iteration is removed and the
error unwind path is simplified, as mentioned above.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

1 files changed, 23 insertions, 44 deletions

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index b7095a033fc4..72d5e0ef8dcf 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -741,65 +741,44 @@ static void sja1110_flow_dissect(const struct sk_buff *skb, __be16 *proto,
 	*proto = ((__be16 *)skb->data)[(VLAN_HLEN / 2) - 1];
 }
 
-static void sja1105_disconnect(struct dsa_switch_tree *dst)
+static void sja1105_disconnect(struct dsa_switch *ds)
 {
-	struct sja1105_tagger_private *priv;
-	struct dsa_port *dp;
-
-	list_for_each_entry(dp, &dst->ports, list) {
-		priv = dp->ds->tagger_data;
-
-		if (!priv)
-			continue;
+	struct sja1105_tagger_private *priv = ds->tagger_data;
 
-		if (priv->xmit_worker)
-			kthread_destroy_worker(priv->xmit_worker);
-
-		kfree(priv);
-		dp->ds->tagger_data = NULL;
-	}
+	kthread_destroy_worker(priv->xmit_worker);
+	kfree(priv);
+	ds->tagger_data = NULL;
 }
 
-static int sja1105_connect(struct dsa_switch_tree *dst)
+static int sja1105_connect(struct dsa_switch *ds)
 {
 	struct sja1105_tagger_data *tagger_data;
 	struct sja1105_tagger_private *priv;
 	struct kthread_worker *xmit_worker;
-	struct dsa_port *dp;
 	int err;
 
-	list_for_each_entry(dp, &dst->ports, list) {
-		if (dp->ds->tagger_data)
-			continue;
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
 
-		priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-		if (!priv) {
-			err = -ENOMEM;
-			goto out;
-		}
-
-		spin_lock_init(&priv->meta_lock);
-
-		xmit_worker = kthread_create_worker(0, "dsa%d:%d_xmit",
-						    dst->index, dp->ds->index);
-		if (IS_ERR(xmit_worker)) {
-			err = PTR_ERR(xmit_worker);
-			goto out;
-		}
+	spin_lock_init(&priv->meta_lock);
 
-		priv->xmit_worker = xmit_worker;
-		/* Export functions for switch driver use */
-		tagger_data = &priv->data;
-		tagger_data->rxtstamp_get_state = sja1105_rxtstamp_get_state;
-		tagger_data->rxtstamp_set_state = sja1105_rxtstamp_set_state;
-		dp->ds->tagger_data = priv;
+	xmit_worker = kthread_create_worker(0, "dsa%d:%d_xmit",
+					    ds->dst->index, ds->index);
+	if (IS_ERR(xmit_worker)) {
+		err = PTR_ERR(xmit_worker);
+		kfree(priv);
+		return err;
 	}
 
-	return 0;
+	priv->xmit_worker = xmit_worker;
+	/* Export functions for switch driver use */
+	tagger_data = &priv->data;
+	tagger_data->rxtstamp_get_state = sja1105_rxtstamp_get_state;
+	tagger_data->rxtstamp_set_state = sja1105_rxtstamp_set_state;
+	ds->tagger_data = priv;
 
-out:
-	sja1105_disconnect(dst);
-	return err;
+	return 0;
 }
 
 static const struct dsa_device_ops sja1105_netdev_ops = {