netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] DSA tagger-owned storage fixups
@ 2021-12-14  1:45 Vladimir Oltean
  2021-12-14  1:45 ` [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect Vladimir Oltean
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-12-14  1:45 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Ansuel Smith

It seems that the DSA tagger-owned storage changes were insufficiently
tested and do not work in all cases. Specifically, the NXP Bluebox 3
(arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dts) got broken by
these changes, because
(a) I forgot that DSA_TAG_PROTO_SJA1110 exists and differs from
    DSA_TAG_PROTO_SJA1105
(b) the Bluebox 3 uses a DSA switch tree with 2 switches, and the
    tagger-owned storage patches don't cover that use case well, it
    seems

Therefore, I'm sorry to say that there needs to be an API fixup: tagging
protocol drivers will from now on connect to individual switches from a
tree, rather than to the tree as a whole. This is more robust against
various ordering constraints in the DSA probe and teardown paths, and is
also symmetrical with the connection API exposed to the switch drivers
themselves, which is also per switch.

With these changes, the Bluebox 3 also works fine.

Vladimir Oltean (3):
  net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto
    disconnect
  net: dsa: sja1105: fix broken connection with the sja1110 tagger
  net: dsa: make tagging protocols connect to individual switches from a
    tree

 drivers/net/dsa/sja1105/sja1105_main.c | 16 +++---
 include/linux/dsa/sja1105.h            |  3 +-
 include/net/dsa.h                      |  5 +-
 net/dsa/dsa2.c                         | 44 +++++++----------
 net/dsa/dsa_priv.h                     |  1 +
 net/dsa/switch.c                       | 52 ++++++++++++++++++--
 net/dsa/tag_ocelot_8021q.c             | 53 ++++++--------------
 net/dsa/tag_sja1105.c                  | 67 +++++++++-----------------
 8 files changed, 119 insertions(+), 122 deletions(-)

-- 
2.25.1


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

* [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect
  2021-12-14  1:45 [PATCH net-next 0/3] DSA tagger-owned storage fixups Vladimir Oltean
@ 2021-12-14  1:45 ` Vladimir Oltean
  2021-12-14  4:34   ` Florian Fainelli
  2021-12-14  1:45 ` [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger Vladimir Oltean
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-12-14  1:45 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Ansuel Smith

The method was meant to zeroize ds->tagger_data but got the wrong
pointer. Fix this.

Fixes: c79e84866d2a ("net: dsa: tag_sja1105: convert to tagger-owned data")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/tag_sja1105.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 93d2484b2480..b7095a033fc4 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -756,7 +756,7 @@ static void sja1105_disconnect(struct dsa_switch_tree *dst)
 			kthread_destroy_worker(priv->xmit_worker);
 
 		kfree(priv);
-		dp->ds->priv = NULL;
+		dp->ds->tagger_data = NULL;
 	}
 }
 
-- 
2.25.1


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

* [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger
  2021-12-14  1:45 [PATCH net-next 0/3] DSA tagger-owned storage fixups Vladimir Oltean
  2021-12-14  1:45 ` [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect Vladimir Oltean
@ 2021-12-14  1:45 ` Vladimir Oltean
  2021-12-14  4:34   ` Florian Fainelli
  2021-12-14  1:45 ` [PATCH net-next 3/3] net: dsa: make tagging protocols connect to individual switches from a tree Vladimir Oltean
  2021-12-14 13:00 ` [PATCH net-next 0/3] DSA tagger-owned storage fixups patchwork-bot+netdevbpf
  3 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-12-14  1:45 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Ansuel Smith

The driver was incorrectly converted assuming that "sja1105" is the only
tagger supported by this driver. This results in SJA1110 switches
failing to probe:

sja1105 spi1.0: Unable to connect to tag protocol "sja1110": -EPROTONOSUPPORT
sja1105: probe of spi1.2 failed with error -93

Add DSA_TAG_PROTO_SJA1110 to the list of supported taggers by the
sja1105 driver. The sja1105_tagger_data structure format is common for
the two tagging protocols.

Fixes: c79e84866d2a ("net: dsa: tag_sja1105: convert to tagger-owned data")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 16 ++++++++--------
 include/linux/dsa/sja1105.h            |  3 ++-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 9171fbea588c..b513713be610 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2708,17 +2708,17 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
 static int sja1105_connect_tag_protocol(struct dsa_switch *ds,
 					enum dsa_tag_protocol proto)
 {
+	struct sja1105_private *priv = ds->priv;
 	struct sja1105_tagger_data *tagger_data;
 
-	switch (proto) {
-	case DSA_TAG_PROTO_SJA1105:
-		tagger_data = sja1105_tagger_data(ds);
-		tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
-		tagger_data->meta_tstamp_handler = sja1110_process_meta_tstamp;
-		return 0;
-	default:
+	if (proto != priv->info->tag_proto)
 		return -EPROTONOSUPPORT;
-	}
+
+	tagger_data = sja1105_tagger_data(ds);
+	tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
+	tagger_data->meta_tstamp_handler = sja1110_process_meta_tstamp;
+
+	return 0;
 }
 
 /* The MAXAGE setting belongs to the L2 Forwarding Parameters table,
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index e9cb1ae6d742..159e43171ccc 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -70,7 +70,8 @@ struct sja1105_skb_cb {
 static inline struct sja1105_tagger_data *
 sja1105_tagger_data(struct dsa_switch *ds)
 {
-	BUG_ON(ds->dst->tag_ops->proto != DSA_TAG_PROTO_SJA1105);
+	BUG_ON(ds->dst->tag_ops->proto != DSA_TAG_PROTO_SJA1105 &&
+	       ds->dst->tag_ops->proto != DSA_TAG_PROTO_SJA1110);
 
 	return ds->tagger_data;
 }
-- 
2.25.1


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

* [PATCH net-next 3/3] net: dsa: make tagging protocols connect to individual switches from a tree
  2021-12-14  1:45 [PATCH net-next 0/3] DSA tagger-owned storage fixups Vladimir Oltean
  2021-12-14  1:45 ` [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect Vladimir Oltean
  2021-12-14  1:45 ` [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger Vladimir Oltean
@ 2021-12-14  1:45 ` Vladimir Oltean
  2021-12-14 13:00 ` [PATCH net-next 0/3] DSA tagger-owned storage fixups patchwork-bot+netdevbpf
  3 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-12-14  1:45 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Florian Fainelli, Ansuel Smith

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>
---
 include/net/dsa.h          |  5 ++-
 net/dsa/dsa2.c             | 44 ++++++++++---------------
 net/dsa/dsa_priv.h         |  1 +
 net/dsa/switch.c           | 52 +++++++++++++++++++++++++++--
 net/dsa/tag_ocelot_8021q.c | 53 +++++++++---------------------
 net/dsa/tag_sja1105.c      | 67 +++++++++++++-------------------------
 6 files changed, 109 insertions(+), 113 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 64d71968aa91..f16959444ae1 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -82,15 +82,14 @@ enum dsa_tag_protocol {
 };
 
 struct dsa_switch;
-struct dsa_switch_tree;
 
 struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
 	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
 			     int *offset);
-	int (*connect)(struct dsa_switch_tree *dst);
-	void (*disconnect)(struct dsa_switch_tree *dst);
+	int (*connect)(struct dsa_switch *ds);
+	void (*disconnect)(struct dsa_switch *ds);
 	unsigned int needed_headroom;
 	unsigned int needed_tailroom;
 	const char *name;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cf6566168620..c18b22c0bf55 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -248,12 +248,8 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
 
 static void dsa_tree_free(struct dsa_switch_tree *dst)
 {
-	if (dst->tag_ops) {
-		if (dst->tag_ops->disconnect)
-			dst->tag_ops->disconnect(dst);
-
+	if (dst->tag_ops)
 		dsa_tag_driver_put(dst->tag_ops);
-	}
 	list_del(&dst->list);
 	kfree(dst);
 }
@@ -841,17 +837,29 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
 	}
 
 connect:
+	if (tag_ops->connect) {
+		err = tag_ops->connect(ds);
+		if (err)
+			return err;
+	}
+
 	if (ds->ops->connect_tag_protocol) {
 		err = ds->ops->connect_tag_protocol(ds, tag_ops->proto);
 		if (err) {
 			dev_err(ds->dev,
 				"Unable to connect to tag protocol \"%s\": %pe\n",
 				tag_ops->name, ERR_PTR(err));
-			return err;
+			goto disconnect;
 		}
 	}
 
 	return 0;
+
+disconnect:
+	if (tag_ops->disconnect)
+		tag_ops->disconnect(ds);
+
+	return err;
 }
 
 static int dsa_switch_setup(struct dsa_switch *ds)
@@ -1160,13 +1168,6 @@ static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst,
 
 	dst->tag_ops = tag_ops;
 
-	/* Notify the new tagger about the connection to this tree */
-	if (tag_ops->connect) {
-		err = tag_ops->connect(dst);
-		if (err)
-			goto out_revert;
-	}
-
 	/* Notify the switches from this tree about the connection
 	 * to the new tagger
 	 */
@@ -1176,16 +1177,14 @@ static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst,
 		goto out_disconnect;
 
 	/* Notify the old tagger about the disconnection from this tree */
-	if (old_tag_ops->disconnect)
-		old_tag_ops->disconnect(dst);
+	info.tag_ops = old_tag_ops;
+	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DISCONNECT, &info);
 
 	return 0;
 
 out_disconnect:
-	/* Revert the new tagger's connection to this tree */
-	if (tag_ops->disconnect)
-		tag_ops->disconnect(dst);
-out_revert:
+	info.tag_ops = tag_ops;
+	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DISCONNECT, &info);
 	dst->tag_ops = old_tag_ops;
 
 	return err;
@@ -1318,7 +1317,6 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
 	struct dsa_switch_tree *dst = ds->dst;
 	const struct dsa_device_ops *tag_ops;
 	enum dsa_tag_protocol default_proto;
-	int err;
 
 	/* Find out which protocol the switch would prefer. */
 	default_proto = dsa_get_tag_protocol(dp, master);
@@ -1366,12 +1364,6 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
 		 */
 		dsa_tag_driver_put(tag_ops);
 	} else {
-		if (tag_ops->connect) {
-			err = tag_ops->connect(dst);
-			if (err)
-				return err;
-		}
-
 		dst->tag_ops = tag_ops;
 	}
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 0db2b26b0c83..edfaae7b5967 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -38,6 +38,7 @@ enum {
 	DSA_NOTIFIER_MTU,
 	DSA_NOTIFIER_TAG_PROTO,
 	DSA_NOTIFIER_TAG_PROTO_CONNECT,
+	DSA_NOTIFIER_TAG_PROTO_DISCONNECT,
 	DSA_NOTIFIER_MRP_ADD,
 	DSA_NOTIFIER_MRP_DEL,
 	DSA_NOTIFIER_MRP_ADD_RING_ROLE,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 06948f536829..393f2d8a860a 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -647,15 +647,58 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
 	return 0;
 }
 
-static int dsa_switch_connect_tag_proto(struct dsa_switch *ds,
-					struct dsa_notifier_tag_proto_info *info)
+/* We use the same cross-chip notifiers to inform both the tagger side, as well
+ * as the switch side, of connection and disconnection events.
+ * Since ds->tagger_data is owned by the tagger, it isn't a hard error if the
+ * switch side doesn't support connecting to this tagger, and therefore, the
+ * fact that we don't disconnect the tagger side doesn't constitute a memory
+ * leak: the tagger will still operate with persistent per-switch memory, just
+ * with the switch side unconnected to it. What does constitute a hard error is
+ * when the switch side supports connecting but fails.
+ */
+static int
+dsa_switch_connect_tag_proto(struct dsa_switch *ds,
+			     struct dsa_notifier_tag_proto_info *info)
 {
 	const struct dsa_device_ops *tag_ops = info->tag_ops;
+	int err;
+
+	/* Notify the new tagger about the connection to this switch */
+	if (tag_ops->connect) {
+		err = tag_ops->connect(ds);
+		if (err)
+			return err;
+	}
 
 	if (!ds->ops->connect_tag_protocol)
 		return -EOPNOTSUPP;
 
-	return ds->ops->connect_tag_protocol(ds, tag_ops->proto);
+	/* Notify the switch about the connection to the new tagger */
+	err = ds->ops->connect_tag_protocol(ds, tag_ops->proto);
+	if (err) {
+		/* Revert the new tagger's connection to this tree */
+		if (tag_ops->disconnect)
+			tag_ops->disconnect(ds);
+		return err;
+	}
+
+	return 0;
+}
+
+static int
+dsa_switch_disconnect_tag_proto(struct dsa_switch *ds,
+				struct dsa_notifier_tag_proto_info *info)
+{
+	const struct dsa_device_ops *tag_ops = info->tag_ops;
+
+	/* Notify the tagger about the disconnection from this switch */
+	if (tag_ops->disconnect && ds->tagger_data)
+		tag_ops->disconnect(ds);
+
+	/* No need to notify the switch, since it shouldn't have any
+	 * resources to tear down
+	 */
+	return 0;
 }
 
 static int dsa_switch_mrp_add(struct dsa_switch *ds,
@@ -780,6 +823,9 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_TAG_PROTO_CONNECT:
 		err = dsa_switch_connect_tag_proto(ds, info);
 		break;
+	case DSA_NOTIFIER_TAG_PROTO_DISCONNECT:
+		err = dsa_switch_disconnect_tag_proto(ds, info);
+		break;
 	case DSA_NOTIFIER_MRP_ADD:
 		err = dsa_switch_mrp_add(ds, info);
 		break;
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index fe451f4de7ba..68982b2789a5 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -81,55 +81,34 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
 	return skb;
 }
 
-static void ocelot_disconnect(struct dsa_switch_tree *dst)
+static void ocelot_disconnect(struct dsa_switch *ds)
 {
-	struct ocelot_8021q_tagger_private *priv;
-	struct dsa_port *dp;
-
-	list_for_each_entry(dp, &dst->ports, list) {
-		priv = dp->ds->tagger_data;
-
-		if (!priv)
-			continue;
+	struct ocelot_8021q_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 ocelot_connect(struct dsa_switch_tree *dst)
+static int ocelot_connect(struct dsa_switch *ds)
 {
 	struct ocelot_8021q_tagger_private *priv;
-	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;
-		}
-
-		priv->xmit_worker = kthread_create_worker(0, "felix_xmit");
-		if (IS_ERR(priv->xmit_worker)) {
-			err = PTR_ERR(priv->xmit_worker);
-			goto out;
-		}
-
-		dp->ds->tagger_data = priv;
+	priv->xmit_worker = kthread_create_worker(0, "felix_xmit");
+	if (IS_ERR(priv->xmit_worker)) {
+		err = PTR_ERR(priv->xmit_worker);
+		kfree(priv);
+		return err;
 	}
 
-	return 0;
+	ds->tagger_data = priv;
 
-out:
-	ocelot_disconnect(dst);
-	return err;
+	return 0;
 }
 
 static const struct dsa_device_ops ocelot_8021q_netdev_ops = {
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 = {
-- 
2.25.1


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

* Re: [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect
  2021-12-14  1:45 ` [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect Vladimir Oltean
@ 2021-12-14  4:34   ` Florian Fainelli
  0 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2021-12-14  4:34 UTC (permalink / raw)
  To: Vladimir Oltean, netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Ansuel Smith



On 12/13/2021 5:45 PM, Vladimir Oltean wrote:
> The method was meant to zeroize ds->tagger_data but got the wrong
> pointer. Fix this.
> 
> Fixes: c79e84866d2a ("net: dsa: tag_sja1105: convert to tagger-owned data")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

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

* Re: [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger
  2021-12-14  1:45 ` [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger Vladimir Oltean
@ 2021-12-14  4:34   ` Florian Fainelli
  0 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2021-12-14  4:34 UTC (permalink / raw)
  To: Vladimir Oltean, netdev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
	Ansuel Smith



On 12/13/2021 5:45 PM, Vladimir Oltean wrote:
> The driver was incorrectly converted assuming that "sja1105" is the only
> tagger supported by this driver. This results in SJA1110 switches
> failing to probe:
> 
> sja1105 spi1.0: Unable to connect to tag protocol "sja1110": -EPROTONOSUPPORT
> sja1105: probe of spi1.2 failed with error -93
> 
> Add DSA_TAG_PROTO_SJA1110 to the list of supported taggers by the
> sja1105 driver. The sja1105_tagger_data structure format is common for
> the two tagging protocols.
> 
> Fixes: c79e84866d2a ("net: dsa: tag_sja1105: convert to tagger-owned data")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

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

* Re: [PATCH net-next 0/3] DSA tagger-owned storage fixups
  2021-12-14  1:45 [PATCH net-next 0/3] DSA tagger-owned storage fixups Vladimir Oltean
                   ` (2 preceding siblings ...)
  2021-12-14  1:45 ` [PATCH net-next 3/3] net: dsa: make tagging protocols connect to individual switches from a tree Vladimir Oltean
@ 2021-12-14 13:00 ` patchwork-bot+netdevbpf
  3 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-12-14 13:00 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, davem, kuba, andrew, vivien.didelot, f.fainelli, ansuelsmth

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 14 Dec 2021 03:45:33 +0200 you wrote:
> It seems that the DSA tagger-owned storage changes were insufficiently
> tested and do not work in all cases. Specifically, the NXP Bluebox 3
> (arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dts) got broken by
> these changes, because
> (a) I forgot that DSA_TAG_PROTO_SJA1110 exists and differs from
>     DSA_TAG_PROTO_SJA1105
> (b) the Bluebox 3 uses a DSA switch tree with 2 switches, and the
>     tagger-owned storage patches don't cover that use case well, it
>     seems
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect
    https://git.kernel.org/netdev/net-next/c/e2f01bfe1406
  - [net-next,2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger
    https://git.kernel.org/netdev/net-next/c/c8a2a011cd04
  - [net-next,3/3] net: dsa: make tagging protocols connect to individual switches from a tree
    https://git.kernel.org/netdev/net-next/c/7f2973149c22

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



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

end of thread, other threads:[~2021-12-14 13:00 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-14  1:45 [PATCH net-next 0/3] DSA tagger-owned storage fixups Vladimir Oltean
2021-12-14  1:45 ` [PATCH net-next 1/3] net: dsa: tag_sja1105: fix zeroization of ds->priv on tag proto disconnect Vladimir Oltean
2021-12-14  4:34   ` Florian Fainelli
2021-12-14  1:45 ` [PATCH net-next 2/3] net: dsa: sja1105: fix broken connection with the sja1110 tagger Vladimir Oltean
2021-12-14  4:34   ` Florian Fainelli
2021-12-14  1:45 ` [PATCH net-next 3/3] net: dsa: make tagging protocols connect to individual switches from a tree Vladimir Oltean
2021-12-14 13:00 ` [PATCH net-next 0/3] DSA tagger-owned storage fixups patchwork-bot+netdevbpf

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).