[PATCH net-next 0/4] Cascaded management xmit for SJA1105 DSA driver

[PATCH net-next 0/4] Cascaded management xmit for SJA1105 DSA driver

[Vladimir Oltean]

This patch set adds support for simple daisy chain topologies in the
sja1105 DSA driver:

 +------------+
 |    eth0    |
 |  (conduit) |
 +------------+
       |
       | Switch #1
 +------------+
 |    cpu     |--- user
 |            |--- user
 |    dsa     |--- user
 +------------+
       |
       | Switch #2
 +------------+
 |    dsa     |--- user
 |            |--- user
 |            |--- user
 |            |--- user
 +------------+

Previously we did support multi-switch trees, but not the simple kind.

What is special here is that sending PTP/STP packets out through
switch #2's user ports requires the programming of management routes in
switch #1 as well. Patch 4/4 does that. It requires some extra infra in
the DSA core, handled by patch 3/4. The extra infra requires documenting
an assumption in the dt-bindings: patch 2/4. And patch 1/4 is fixing a
bug I noticed while reviewing the code, but which is pretty hard to
meaningfully trigger, so I am not considering it a 'stable' candidate.

I do not actually have a board with a switch topology as described
above. This patch set was confirmed working by customers on their
boards. I have just regression-tested it on simple, single-switch trees.

Vladimir Oltean (4):
  net: dsa: free routing table on probe failure
  dt-bindings: net: dsa: the adjacent DSA port must appear first in
    "link" property
  net: dsa: populate dp->link_dp for cascade ports
  net: dsa: sja1105: implement management routes for cascaded switches

 .../devicetree/bindings/net/dsa/dsa-port.yaml |   9 +-
 drivers/net/dsa/sja1105/sja1105.h             |  43 ++-
 drivers/net/dsa/sja1105/sja1105_main.c        | 253 ++++++++++++++++--
 include/net/dsa.h                             |   9 +-
 net/dsa/dsa.c                                 |  43 ++-
 5 files changed, 307 insertions(+), 50 deletions(-)

-- 
2.34.1

[PATCH net-next 1/4] net: dsa: free routing table on probe failure

[Vladimir Oltean]

If complete = true, it means that we are the last switch of the tree
which is successfully probing, and we should be setting up all switches
from our probe path.

After "complete" becomes true, dsa_tree_setup_cpu_ports() or any
subsequent function may fail. If that happens, the entire tree setup is
in limbo: the first N-1 switches have successfully finished probing
(doing nothing), and switch N failed to probe, ending the tree setup
process before anything is tangible from the user's PoV.

Because the dsa_switch_tree is still technically referenced by the N-1
switches which succeeded in probing but are practically useless, the
memory pointing to "dst" and to the dst->rtable is not actually
"leaked", it is more like "blocked".

However, we could just as well free the dst->rtable, since a subsequent
attempt of switch N to probe, for whatever reason, _will_ trigger issues.

First, dsa_link_touch() left behind references to ports owned by a now
deallocated struct dsa_switch - the one which failed to probe.

Second, the dst->rtable will be regenerated anyway by a subsequent probe
attempt of switch N.

There was no practical problem observed, this is only a result of code
analysis.

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

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 668c729946ea..a543ddaefdd8 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -862,6 +862,16 @@ static void dsa_tree_teardown_lags(struct dsa_switch_tree *dst)
 	kfree(dst->lags);
 }
 
+static void dsa_tree_teardown_routing_table(struct dsa_switch_tree *dst)
+{
+	struct dsa_link *dl, *next;
+
+	list_for_each_entry_safe(dl, next, &dst->rtable, list) {
+		list_del(&dl->list);
+		kfree(dl);
+	}
+}
+
 static int dsa_tree_setup(struct dsa_switch_tree *dst)
 {
 	bool complete;
@@ -879,7 +889,7 @@ static int dsa_tree_setup(struct dsa_switch_tree *dst)
 
 	err = dsa_tree_setup_cpu_ports(dst);
 	if (err)
-		return err;
+		goto teardown_rtable;
 
 	err = dsa_tree_setup_switches(dst);
 	if (err)
@@ -911,14 +921,14 @@ 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_rtable:
+	dsa_tree_teardown_routing_table(dst);
 
 	return err;
 }
 
 static void dsa_tree_teardown(struct dsa_switch_tree *dst)
 {
-	struct dsa_link *dl, *next;
-
 	if (!dst->setup)
 		return;
 
@@ -932,10 +942,7 @@ static void dsa_tree_teardown(struct dsa_switch_tree *dst)
 
 	dsa_tree_teardown_cpu_ports(dst);
 
-	list_for_each_entry_safe(dl, next, &dst->rtable, list) {
-		list_del(&dl->list);
-		kfree(dl);
-	}
+	dsa_tree_teardown_routing_table(dst);
 
 	pr_info("DSA: tree %d torn down\n", dst->index);
 
-- 
2.34.1

[PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA port must appear first in "link" property

[Vladimir Oltean]

If we don't add something along these lines, it is absolutely impossible
to know, for trees with 3 or more switches, which links represent direct
connections and which don't.

I've studied existing mainline device trees, and it seems that the rule
has been respected thus far. I've actually tested such a 3-switch setup
with the Turris MOX.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
index 480120469953..307c61aadcbc 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
@@ -31,10 +31,11 @@ properties:
 
   link:
     description:
-      Should be a list of phandles to other switch's DSA port. This
-      port is used as the outgoing port towards the phandle ports. The
-      full routing information must be given, not just the one hop
-      routes to neighbouring switches
+      Should be a list of phandles to other switch's DSA port. This port is
+      used as the outgoing port towards the phandle ports. In case of trees
+      with more than 2 switches, the full routing information must be given.
+      The first element of the list must be the directly connected DSA port
+      of the adjacent switch.
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
       maxItems: 1
-- 
2.34.1

[PATCH net-next 3/4] net: dsa: populate dp->link_dp for cascade ports

[Vladimir Oltean]

Drivers may need to walk the tree hop by hop, activity which is
currently impossible. This is because dst->rtable offers no guarantee as
to whether we are looking at a dsa_link that represents a direct
connection or not.

Partially address the long-standing TODO that we have, and do introduce
a link_dp member in struct dsa_port. This will actually represent the
adjacent cascade port.

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

diff --git a/include/net/dsa.h b/include/net/dsa.h
index d7a6c2930277..586efb76f67d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -265,6 +265,7 @@ struct dsa_port {
 
 	const char		*name;
 	struct dsa_port		*cpu_dp;
+	struct dsa_port		*link_dp;
 	u8			mac[ETH_ALEN];
 
 	u8			stp_state;
@@ -332,10 +333,10 @@ dsa_phylink_to_port(struct phylink_config *config)
 	return container_of(config, struct dsa_port, pl_config);
 }
 
-/* TODO: ideally DSA ports would have a single dp->link_dp member,
- * and no dst->rtable nor this struct dsa_link would be needed,
- * but this would require some more complex tree walking,
- * so keep it stupid at the moment and list them all.
+/* TODO: DSA ports do have a dp->link_dp member which represents their direct
+ * connection. However, dsa_routing_port() requires full routing information,
+ * which _could_ be deduced based on just the adjacency, but it requires some
+ * complex tree walking. So keep it stupid at the moment and list them all.
  */
 struct dsa_link {
 	struct dsa_port *dp;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index a543ddaefdd8..8b4ec00de521 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -285,7 +285,8 @@ static struct dsa_port *dsa_tree_find_port_by_node(struct dsa_switch_tree *dst,
 }
 
 static struct dsa_link *dsa_link_touch(struct dsa_port *dp,
-				       struct dsa_port *link_dp)
+				       struct dsa_port *link_dp,
+				       bool adjacent)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_switch_tree *dst;
@@ -307,9 +308,23 @@ static struct dsa_link *dsa_link_touch(struct dsa_port *dp,
 	INIT_LIST_HEAD(&dl->list);
 	list_add_tail(&dl->list, &dst->rtable);
 
+	if (adjacent)
+		dp->link_dp = link_dp;
+
 	return dl;
 }
 
+/**
+ * dsa_port_setup_routing_table(): Set up tree routing table based on
+ *	information from this cascade port
+ * @dp: cascade port
+ *
+ * Parse the device tree node for the "link" array of phandles to other cascade
+ * ports, creating routing table elements from this source to each destination
+ * list element found. One assumption is being made, which is backed by the
+ * device tree bindings: that the first "link" element is the directly
+ * connected cascade port.
+ */
 static bool dsa_port_setup_routing_table(struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
@@ -317,6 +332,7 @@ static bool dsa_port_setup_routing_table(struct dsa_port *dp)
 	struct device_node *dn = dp->dn;
 	struct of_phandle_iterator it;
 	struct dsa_port *link_dp;
+	bool adjacent = true;
 	struct dsa_link *dl;
 	int err;
 
@@ -327,11 +343,13 @@ static bool dsa_port_setup_routing_table(struct dsa_port *dp)
 			return false;
 		}
 
-		dl = dsa_link_touch(dp, link_dp);
+		dl = dsa_link_touch(dp, link_dp, adjacent);
 		if (!dl) {
 			of_node_put(it.node);
 			return false;
 		}
+
+		adjacent = false;
 	}
 
 	return true;
-- 
2.34.1

[PATCH net-next 4/4] net: dsa: sja1105: implement management routes for cascaded switches

[Vladimir Oltean]

The SJA1105 management route concept was previously explained in commits
227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through
standalone ports") and 0a51826c6e05 ("net: dsa: sja1105: Always send
through management routes in slot 0").

In a daisy chained topology with at least 2 switches, sending link-local
frames belonging to the downstream switch should program 2 management
routes: one on the upstream switch and one on the leaf switch. In the
general case, each switch along the TX path of the packet, starting from
the CPU, need a one-shot management route installed over SPI.

The driver currently does not handle this, but instead limits link-local
traffic support to a single switch, due to 2 major blockers:

1. There was no way up until now to calculate the path (the management
   route itself) between the CPU and a leaf user port. Sure, we can start
   with dp->cpu_dp and use dsa_routing_port() to figure out the cascade
   port that targets the next switch. But we cannot make the jump from
   one switch to the next. The dst->rtable is fundamentally flawed by
   construction. It contains not only directly-connected link_dp entries,
   but links to _all_ other cascade ports in the tree. For trees with 3
   or more switches, this means that we don't know, by following
   dst->rtable, if the link_dp that we pick is really one hop away, or
   more than one hop away. So we might skip programming some switches
   along the packet's path.

2. The current priv->mgmt_lock does not serialize enough code to work in
   a cross-chip scenario. When sending a packet across a tree, we want
   to block updates to the management route tables for all switches
   along that path, not just for the leaf port (because link-local
   traffic might be transmitted concurrently towards other ports).
   Keeping this lock where it is (in struct sja1105_private, which is
   per switch) will not work, because sja1105_port_deferred_xmit() would
   have to acquire and then release N locks, and that's simply
   impossible to do without risking AB/BA deadlocks.

To solve 1, recent changes have introduced struct dsa_port :: link_dp in
the DSA core, to make the hop-by-hop traversal of the DSA tree possible.
Using that information, we statically compute management routes for each
user port at switch setup time.

To solve 2, we go for the much more complex scheme of allocating a
tree-wide structure for managing the management routes, which holds a
single lock.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105.h      |  43 ++++-
 drivers/net/dsa/sja1105/sja1105_main.c | 253 ++++++++++++++++++++++---
 2 files changed, 263 insertions(+), 33 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 8c66d3bf61f0..7753b4d62bc6 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -245,6 +245,43 @@ struct sja1105_flow_block {
 	int num_virtual_links;
 };
 
+/**
+ * sja1105_mgmt_route_port: Representation of one port in a management route
+ * @dp: DSA user or cascade port
+ * @list: List node element for the mgmt_route->ports list membership
+ */
+struct sja1105_mgmt_route_port {
+	struct dsa_port *dp;
+	struct list_head list;
+};
+
+/**
+ * sja1105_mgmt_route: Structure to represent a SJA1105 management route
+ * @ports: List of ports on which the management route needs to be installed,
+ *	   starting with the downstream-facing cascade port of the switch which
+ *	   has the CPU connection, and ending with the user port of the leaf
+ *	   switch.
+ * @list: List node element for the mgmt_tree->routes list membership.
+ */
+struct sja1105_mgmt_route {
+	struct list_head ports;
+	struct list_head list;
+};
+
+/**
+ * sja1105_mgmt_tree: DSA switch tree-level structure for management routes
+ * @lock: Serializes transmission of management frames across the tree, so that
+ *	  the switches don't confuse them with one another.
+ * @routes: List of sja1105_mgmt_route structures, one for each user port in
+ *	    the tree.
+ * @refcount: Reference count.
+ */
+struct sja1105_mgmt_tree {
+	struct mutex lock;
+	struct list_head routes;
+	refcount_t refcount;
+};
+
 struct sja1105_private {
 	struct sja1105_static_config static_config;
 	int rgmii_rx_delay_ps[SJA1105_MAX_NUM_PORTS];
@@ -259,13 +296,11 @@ struct sja1105_private {
 	size_t max_xfer_len;
 	struct spi_device *spidev;
 	struct dsa_switch *ds;
+	struct sja1105_mgmt_tree *mgmt_tree;
+	struct sja1105_mgmt_route **mgmt_routes;
 	u16 bridge_pvid[SJA1105_MAX_NUM_PORTS];
 	u16 tag_8021q_pvid[SJA1105_MAX_NUM_PORTS];
 	struct sja1105_flow_block flow_block;
-	/* Serializes transmission of management frames so that
-	 * the switch doesn't confuse them with one another.
-	 */
-	struct mutex mgmt_lock;
 	/* Serializes accesses to the FDB */
 	struct mutex fdb_lock;
 	/* PTP two-step TX timestamp ID, and its serialization lock */
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index bc7e50dcb57c..81e380ff8a56 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2302,8 +2302,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 	int rc, i;
 	s64 now;
 
+	mutex_lock(&priv->mgmt_tree->lock);
 	mutex_lock(&priv->fdb_lock);
-	mutex_lock(&priv->mgmt_lock);
 
 	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
 
@@ -2414,8 +2414,8 @@ int sja1105_static_config_reload(struct sja1105_private *priv,
 	if (rc < 0)
 		goto out;
 out:
-	mutex_unlock(&priv->mgmt_lock);
 	mutex_unlock(&priv->fdb_lock);
+	mutex_unlock(&priv->mgmt_tree->lock);
 
 	return rc;
 }
@@ -2668,39 +2668,41 @@ static int sja1105_prechangeupper(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
-			     struct sk_buff *skb, bool takets)
+static int sja1105_mgmt_route_install(struct dsa_port *dp, const u8 *addr,
+				      bool takets, int slot)
 {
-	struct sja1105_mgmt_entry mgmt_route = {0};
-	struct sja1105_private *priv = ds->priv;
-	struct ethhdr *hdr;
-	int timeout = 10;
-	int rc;
-
-	hdr = eth_hdr(skb);
+	struct sja1105_mgmt_entry mgmt_route = {};
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
 
-	mgmt_route.macaddr = ether_addr_to_u64(hdr->h_dest);
+	mgmt_route.macaddr = ether_addr_to_u64(addr);
 	mgmt_route.destports = BIT(port);
 	mgmt_route.enfport = 1;
 	mgmt_route.tsreg = 0;
-	mgmt_route.takets = takets;
+	/* Only the leaf port takes the TX timestamp, the cascade ports just
+	 * route the packet towards the leaf switch
+	 */
+	mgmt_route.takets = dsa_port_is_user(dp) ? takets : false;
 
-	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MGMT_ROUTE,
-					  slot, &mgmt_route, true);
-	if (rc < 0) {
-		kfree_skb(skb);
-		return rc;
-	}
+	return sja1105_dynamic_config_write(ds->priv, BLK_IDX_MGMT_ROUTE,
+					    slot, &mgmt_route, true);
+}
 
-	/* Transfer skb to the host port. */
-	dsa_enqueue_skb(skb, dsa_to_port(ds, port)->user);
+static void sja1105_mgmt_route_poll(struct dsa_port *dp, int slot)
+{
+	struct sja1105_mgmt_entry mgmt_route = {};
+	struct dsa_switch *ds = dp->ds;
+	struct sja1105_private *priv;
+	int timeout = 10;
+	int rc;
+
+	priv = ds->priv;
 
-	/* Wait until the switch has processed the frame */
 	do {
 		rc = sja1105_dynamic_config_read(priv, BLK_IDX_MGMT_ROUTE,
 						 slot, &mgmt_route);
 		if (rc < 0) {
-			dev_err_ratelimited(priv->ds->dev,
+			dev_err_ratelimited(ds->dev,
 					    "failed to poll for mgmt route\n");
 			continue;
 		}
@@ -2720,8 +2722,36 @@ static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
 		 */
 		sja1105_dynamic_config_write(priv, BLK_IDX_MGMT_ROUTE,
 					     slot, &mgmt_route, false);
-		dev_err_ratelimited(priv->ds->dev, "xmit timed out\n");
+		dev_err_ratelimited(ds->dev, "xmit timed out\n");
 	}
+}
+
+static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
+			     struct sk_buff *skb, bool takets)
+{
+	struct sja1105_mgmt_route_port *route_port;
+	struct sja1105_private *priv = ds->priv;
+	struct ethhdr *hdr = eth_hdr(skb);
+	struct sja1105_mgmt_route *route;
+	int rc;
+
+	route = priv->mgmt_routes[port];
+
+	list_for_each_entry(route_port, &route->ports, list) {
+		rc = sja1105_mgmt_route_install(route_port->dp, hdr->h_dest,
+						takets, slot);
+		if (rc) {
+			kfree_skb(skb);
+			return rc;
+		}
+	}
+
+	/* Transfer skb to the host port. */
+	dsa_enqueue_skb(skb, dsa_to_port(ds, port)->user);
+
+	/* Wait until the switches have processed the frame */
+	list_for_each_entry(route_port, &route->ports, list)
+		sja1105_mgmt_route_poll(route_port->dp, slot);
 
 	return NETDEV_TX_OK;
 }
@@ -2743,7 +2773,7 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
 
 	clone = SJA1105_SKB_CB(skb)->clone;
 
-	mutex_lock(&priv->mgmt_lock);
+	mutex_lock(&priv->mgmt_tree->lock);
 
 	sja1105_mgmt_xmit(ds, port, 0, skb, !!clone);
 
@@ -2751,7 +2781,7 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
 	if (clone)
 		sja1105_ptp_txtstamp_skb(ds, port, clone);
 
-	mutex_unlock(&priv->mgmt_lock);
+	mutex_unlock(&priv->mgmt_tree->lock);
 
 	kfree(xmit_work);
 }
@@ -3078,6 +3108,165 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static struct sja1105_mgmt_tree *sja1105_mgmt_tree_find(struct dsa_switch *ds)
+{
+	struct dsa_switch_tree *dst = ds->dst;
+	struct sja1105_private *priv;
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list) {
+		priv = dp->ds->priv;
+		if (priv->mgmt_tree)
+			return priv->mgmt_tree;
+	}
+
+	return NULL;
+}
+
+static struct sja1105_mgmt_tree *sja1105_mgmt_tree_get(struct dsa_switch *ds)
+{
+	struct sja1105_mgmt_tree *mgmt_tree = sja1105_mgmt_tree_find(ds);
+
+	if (mgmt_tree) {
+		refcount_inc(&mgmt_tree->refcount);
+		return mgmt_tree;
+	}
+
+	mgmt_tree = kzalloc(sizeof(*mgmt_tree), GFP_KERNEL);
+	if (!mgmt_tree)
+		return NULL;
+
+	INIT_LIST_HEAD(&mgmt_tree->routes);
+	refcount_set(&mgmt_tree->refcount, 1);
+	mutex_init(&mgmt_tree->lock);
+
+	return mgmt_tree;
+}
+
+static void sja1105_mgmt_tree_put(struct sja1105_mgmt_tree *mgmt_tree)
+{
+	if (!refcount_dec_and_test(&mgmt_tree->refcount))
+		return;
+
+	WARN_ON(!list_empty(&mgmt_tree->routes));
+	kfree(mgmt_tree);
+}
+
+static void sja1105_mgmt_route_destroy(struct sja1105_mgmt_route *mgmt_route)
+{
+	struct sja1105_mgmt_route_port *mgmt_route_port, *next;
+
+	list_for_each_entry_safe(mgmt_route_port, next, &mgmt_route->ports,
+				 list) {
+		list_del(&mgmt_route_port->list);
+		kfree(mgmt_route_port);
+	}
+
+	kfree(mgmt_route);
+}
+
+static int sja1105_mgmt_route_create(struct dsa_port *dp)
+{
+	struct sja1105_mgmt_route_port *mgmt_route_port;
+	struct sja1105_mgmt_route *mgmt_route;
+	struct dsa_switch *ds = dp->ds;
+	struct sja1105_private *priv;
+	struct dsa_port *upstream_dp;
+	int upstream, rc;
+
+	priv = ds->priv;
+
+	mgmt_route = kzalloc(sizeof(*mgmt_route), GFP_KERNEL);
+	if (!mgmt_route)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&mgmt_route->ports);
+
+	priv->mgmt_routes[dp->index] = mgmt_route;
+
+	while (dp) {
+		mgmt_route_port = kzalloc(sizeof(*mgmt_route_port), GFP_KERNEL);
+		if (!mgmt_route_port) {
+			rc = -ENOMEM;
+			goto err_free_route;
+		}
+
+		mgmt_route_port->dp = dp;
+		list_add(&mgmt_route_port->list, &mgmt_route->ports);
+
+		upstream = dsa_upstream_port(dp->ds, dp->index);
+		upstream_dp = dsa_to_port(dp->ds, upstream);
+		if (dsa_port_is_cpu(upstream_dp))
+			break;
+
+		/* upstream_dp is a cascade port. Jump hop by hop towards the
+		 * CPU port using the dp->link_dp adjacency information.
+		 */
+		dp = upstream_dp->link_dp;
+	}
+
+	list_add_tail(&mgmt_route->list, &priv->mgmt_tree->routes);
+
+	return 0;
+
+err_free_route:
+	sja1105_mgmt_route_destroy(mgmt_route);
+
+	return rc;
+}
+
+static int sja1105_mgmt_setup(struct dsa_switch *ds)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct dsa_port *dp;
+	int rc;
+
+	if (priv->info->tag_proto != DSA_TAG_PROTO_SJA1105)
+		return 0;
+
+	priv->mgmt_tree = sja1105_mgmt_tree_get(ds);
+	if (!priv->mgmt_tree)
+		return -ENOMEM;
+
+	priv->mgmt_routes = kcalloc(ds->num_ports, sizeof(*priv->mgmt_routes),
+				    GFP_KERNEL);
+	if (!priv->mgmt_routes) {
+		rc = -ENOMEM;
+		goto err_put_tree;
+	}
+
+	dsa_switch_for_each_user_port(dp, ds) {
+		rc = sja1105_mgmt_route_create(dp);
+		if (rc)
+			goto err_destroy_routes;
+	}
+
+	return 0;
+
+err_destroy_routes:
+	dsa_switch_for_each_user_port_continue_reverse(dp, ds)
+		sja1105_mgmt_route_destroy(priv->mgmt_routes[dp->index]);
+err_put_tree:
+	sja1105_mgmt_tree_put(priv->mgmt_tree);
+
+	return rc;
+}
+
+static void sja1105_mgmt_teardown(struct dsa_switch *ds)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct dsa_port *dp;
+
+	if (priv->info->tag_proto != DSA_TAG_PROTO_SJA1105)
+		return;
+
+	dsa_switch_for_each_user_port(dp, ds)
+		sja1105_mgmt_route_destroy(priv->mgmt_routes[dp->index]);
+
+	kfree(priv->mgmt_routes);
+	sja1105_mgmt_tree_put(priv->mgmt_tree);
+}
+
 /* The programming model for the SJA1105 switch is "all-at-once" via static
  * configuration tables. Some of these can be dynamically modified at runtime,
  * but not the xMII mode parameters table.
@@ -3095,13 +3284,17 @@ static int sja1105_setup(struct dsa_switch *ds)
 	struct sja1105_private *priv = ds->priv;
 	int rc;
 
+	rc = sja1105_mgmt_setup(ds);
+	if (rc)
+		return rc;
+
 	if (priv->info->disable_microcontroller) {
 		rc = priv->info->disable_microcontroller(priv);
 		if (rc < 0) {
 			dev_err(ds->dev,
 				"Failed to disable microcontroller: %pe\n",
 				ERR_PTR(rc));
-			return rc;
+			goto out_mgmt_teardown;
 		}
 	}
 
@@ -3109,7 +3302,7 @@ static int sja1105_setup(struct dsa_switch *ds)
 	rc = sja1105_static_config_load(priv);
 	if (rc < 0) {
 		dev_err(ds->dev, "Failed to load static config: %d\n", rc);
-		return rc;
+		goto out_mgmt_teardown;
 	}
 
 	/* Configure the CGU (PHY link modes and speeds) */
@@ -3181,6 +3374,8 @@ static int sja1105_setup(struct dsa_switch *ds)
 	sja1105_tas_teardown(ds);
 out_static_config_free:
 	sja1105_static_config_free(&priv->static_config);
+out_mgmt_teardown:
+	sja1105_mgmt_teardown(ds);
 
 	return rc;
 }
@@ -3199,6 +3394,7 @@ static void sja1105_teardown(struct dsa_switch *ds)
 	sja1105_flower_teardown(ds);
 	sja1105_tas_teardown(ds);
 	sja1105_static_config_free(&priv->static_config);
+	sja1105_mgmt_teardown(ds);
 }
 
 static const struct phylink_mac_ops sja1105_phylink_mac_ops = {
@@ -3388,7 +3584,6 @@ static int sja1105_probe(struct spi_device *spi)
 
 	mutex_init(&priv->ptp_data.lock);
 	mutex_init(&priv->dynamic_config_lock);
-	mutex_init(&priv->mgmt_lock);
 	mutex_init(&priv->fdb_lock);
 	spin_lock_init(&priv->ts_id_lock);
 
-- 
2.34.1

Re: [PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA port must appear first in "link" property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> If we don't add something along these lines, it is absolutely impossible
> to know, for trees with 3 or more switches, which links represent direct
> connections and which don't.
> 
> I've studied existing mainline device trees, and it seems that the rule
> has been respected thus far. I've actually tested such a 3-switch setup
> with the Turris MOX.

What about out of tree (so in u-boot or the likes)? Are there other
users that we need to care about?

This doesn't really seem like an ABI change, if this is the established
convention, but feels like a fixes tag and backports to stable etc are
in order to me.

> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> index 480120469953..307c61aadcbc 100644
> --- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
> @@ -31,10 +31,11 @@ properties:
>  
>    link:
>      description:
> -      Should be a list of phandles to other switch's DSA port. This
> -      port is used as the outgoing port towards the phandle ports. The
> -      full routing information must be given, not just the one hop
> -      routes to neighbouring switches
> +      Should be a list of phandles to other switch's DSA port. This port is
> +      used as the outgoing port towards the phandle ports. In case of trees
> +      with more than 2 switches, the full routing information must be given.
> +      The first element of the list must be the directly connected DSA port
> +      of the adjacent switch.
>      $ref: /schemas/types.yaml#/definitions/phandle-array
>      items:
>        maxItems: 1
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA port must appear first in "link" property

[Andrew Lunn]

On Fri, Sep 13, 2024 at 06:04:17PM +0100, Conor Dooley wrote:
> On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> > If we don't add something along these lines, it is absolutely impossible
> > to know, for trees with 3 or more switches, which links represent direct
> > connections and which don't.
> > 
> > I've studied existing mainline device trees, and it seems that the rule
> > has been respected thus far. I've actually tested such a 3-switch setup
> > with the Turris MOX.
> 
> What about out of tree (so in u-boot or the likes)? Are there other
> users that we need to care about?
> 
> This doesn't really seem like an ABI change, if this is the established
> convention, but feels like a fixes tag and backports to stable etc are
> in order to me.

Looking at the next patch, it does not appear to change the behaviour
of existing drivers, it just adds additional information a driver may
choose to use.

As Vladimir says, all existing instances already appear to be in this
order, but that is just happen stance, because it does not matter. So
i would be reluctant to say there is an established convention.

The mv88e6xxx driver does not care about the order of the list, and
this patch series does not appear to change that. So i'm O.K. with the
code changes.

> > -      Should be a list of phandles to other switch's DSA port. This
> > -      port is used as the outgoing port towards the phandle ports. The
> > -      full routing information must be given, not just the one hop
> > -      routes to neighbouring switches
> > +      Should be a list of phandles to other switch's DSA port. This port is
> > +      used as the outgoing port towards the phandle ports. In case of trees
> > +      with more than 2 switches, the full routing information must be given.
> > +      The first element of the list must be the directly connected DSA port
> > +      of the adjacent switch.

I would prefer to weaken this, just to avoid future backwards
compatibility issues. `must` is too strong, because mv88e6xxx does not
care, and there could be DT blobs out there which do not conform to
this. Maybe 'For the SJA1105, the first element ...".

What i don't want is some future developer reading this, assume it is
actually true when it maybe is not, and making use of it in the
mv88e6xxx or the core, breaking backwards compatibility.

	Andrew

Re: [PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA port must appear first in "link" property

[Vladimir Oltean]

Hi Conor, Andrew,

Thanks for taking a look at the patch.

On Fri, Sep 13, 2024 at 07:26:49PM +0200, Andrew Lunn wrote:
> On Fri, Sep 13, 2024 at 06:04:17PM +0100, Conor Dooley wrote:
> > On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> > > If we don't add something along these lines, it is absolutely impossible
> > > to know, for trees with 3 or more switches, which links represent direct
> > > connections and which don't.
> > > 
> > > I've studied existing mainline device trees, and it seems that the rule
> > > has been respected thus far. I've actually tested such a 3-switch setup
> > > with the Turris MOX.
> > 
> > What about out of tree (so in u-boot or the likes)? Are there other
> > users that we need to care about?

In U-Boot there is only armada-3720-turris-mox.dts which is in sync with
Linux as far as I'm aware. Also, we don't really support cascade ports
in U-Boot DM_DSA yet. So all device trees in U-Boot should be coming
from Linux. I'm not aware of other device tree users, sadly.

> > This doesn't really seem like an ABI change, if this is the established
> > convention, but feels like a fixes tag and backports to stable etc are
> > in order to me.
> 
> Looking at the next patch, it does not appear to change the behaviour
> of existing drivers, it just adds additional information a driver may
> choose to use.
> 
> As Vladimir says, all existing instances already appear to be in this
> order, but that is just happen stance, because it does not matter. So
> i would be reluctant to say there is an established convention.

Yes, indeed, it's not a convention yet. However, it is a convention I
would really like to establish, based on the practice I have seen.

> The mv88e6xxx driver does not care about the order of the list, and
> this patch series does not appear to change that. So i'm O.K. with the
> code changes.
> 
> > > -      Should be a list of phandles to other switch's DSA port. This
> > > -      port is used as the outgoing port towards the phandle ports. The
> > > -      full routing information must be given, not just the one hop
> > > -      routes to neighbouring switches
> > > +      Should be a list of phandles to other switch's DSA port. This port is
> > > +      used as the outgoing port towards the phandle ports. In case of trees
> > > +      with more than 2 switches, the full routing information must be given.
> > > +      The first element of the list must be the directly connected DSA port
> > > +      of the adjacent switch.
> 
> I would prefer to weaken this, just to avoid future backwards
> compatibility issues. `must` is too strong, because mv88e6xxx does not
> care, and there could be DT blobs out there which do not conform to
> this. Maybe 'For the SJA1105, the first element ...".
> 
> What i don't want is some future developer reading this, assume it is
> actually true when it maybe is not, and making use of it in the
> mv88e6xxx or the core, breaking backwards compatibility.

Backward compatibility issues aside, the way dp->link_dp is populated
can _only_ be done based on the assumption that this is true.

I'm afraid that any verb less strong than "must" would be insufficient
for my patch 3/4.

I have unpublished downstream patches which even make all the rest of
the "link = <...>" elements optional. Bottom line, only the direct
connection between ports (first element) represents hardware description.
The other reachable ports (the routing table, practically speaking) can be*
computed based on breadth-first search at DSA probe time. They are
listed in the device tree for "convenience".

*and IMO, also should be. For a 3-switch daisy chain, there are 8 links
to describe, and for a 4-switch daisy chain, there are 22 links, if my
math is correct. I think it's unreasonable to expect that board DT
writers won't make mistakes in listing this amount of elements, rather
than just concentrating on the physically relevant info - the direct
connection.

Baking the assumption proposed here into the binding now makes the BFS
algorithm perfectly implementable, and the binding much more scalable.

Do you have reasonable concerns that there exist device trees which are
written differently than "first 'link' element is the direct connection"?

Re: [PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA port must appear first in "link" property

[Andrew Lunn]

> I have unpublished downstream patches which even make all the rest of
> the "link = <...>" elements optional. Bottom line, only the direct
> connection between ports (first element) represents hardware description.
> The other reachable ports (the routing table, practically speaking) can be*
> computed based on breadth-first search at DSA probe time. They are
> listed in the device tree for "convenience".

If you have this code, then i would actually go for a new property,
maybe 'direct-link = <...>;', which only lists the direct
relationship. Keep the current property with its current meaning, an
unordered list. If the new property is present, use it to compute the
table. If both sets of properties are present, ensure they result in
the same table, otherwise -EINVAL.

    Andrew

Re: [PATCH net-next 4/4] net: dsa: sja1105: implement management routes for cascaded switches

[Simon Horman]

On Fri, Sep 13, 2024 at 04:15:07PM +0300, Vladimir Oltean wrote:
> The SJA1105 management route concept was previously explained in commits
> 227d07a07ef1 ("net: dsa: sja1105: Add support for traffic through
> standalone ports") and 0a51826c6e05 ("net: dsa: sja1105: Always send
> through management routes in slot 0").
> 
> In a daisy chained topology with at least 2 switches, sending link-local
> frames belonging to the downstream switch should program 2 management
> routes: one on the upstream switch and one on the leaf switch. In the
> general case, each switch along the TX path of the packet, starting from
> the CPU, need a one-shot management route installed over SPI.
> 
> The driver currently does not handle this, but instead limits link-local
> traffic support to a single switch, due to 2 major blockers:
> 
> 1. There was no way up until now to calculate the path (the management
>    route itself) between the CPU and a leaf user port. Sure, we can start
>    with dp->cpu_dp and use dsa_routing_port() to figure out the cascade
>    port that targets the next switch. But we cannot make the jump from
>    one switch to the next. The dst->rtable is fundamentally flawed by
>    construction. It contains not only directly-connected link_dp entries,
>    but links to _all_ other cascade ports in the tree. For trees with 3
>    or more switches, this means that we don't know, by following
>    dst->rtable, if the link_dp that we pick is really one hop away, or
>    more than one hop away. So we might skip programming some switches
>    along the packet's path.
> 
> 2. The current priv->mgmt_lock does not serialize enough code to work in
>    a cross-chip scenario. When sending a packet across a tree, we want
>    to block updates to the management route tables for all switches
>    along that path, not just for the leaf port (because link-local
>    traffic might be transmitted concurrently towards other ports).
>    Keeping this lock where it is (in struct sja1105_private, which is
>    per switch) will not work, because sja1105_port_deferred_xmit() would
>    have to acquire and then release N locks, and that's simply
>    impossible to do without risking AB/BA deadlocks.
> 
> To solve 1, recent changes have introduced struct dsa_port :: link_dp in
> the DSA core, to make the hop-by-hop traversal of the DSA tree possible.
> Using that information, we statically compute management routes for each
> user port at switch setup time.
> 
> To solve 2, we go for the much more complex scheme of allocating a
> tree-wide structure for managing the management routes, which holds a
> single lock.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/sja1105/sja1105.h      |  43 ++++-
>  drivers/net/dsa/sja1105/sja1105_main.c | 253 ++++++++++++++++++++++---
>  2 files changed, 263 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
> index 8c66d3bf61f0..7753b4d62bc6 100644
> --- a/drivers/net/dsa/sja1105/sja1105.h
> +++ b/drivers/net/dsa/sja1105/sja1105.h
> @@ -245,6 +245,43 @@ struct sja1105_flow_block {
>  	int num_virtual_links;
>  };
>  
> +/**
> + * sja1105_mgmt_route_port: Representation of one port in a management route

Hi Vladimir,

As this series has been deferred, a minor nit from my side.

Tooling seems to want the keyword struct at the beginning of the
short description. So I suggest something like this:

 * struct sja1105_mgmt_route_port: One port in a management route

Likewise for the two Kernel docs for structures below.

Flagged by ./scripts/kernel-doc -none

> + * @dp: DSA user or cascade port
> + * @list: List node element for the mgmt_route->ports list membership
> + */
> +struct sja1105_mgmt_route_port {
> +	struct dsa_port *dp;
> +	struct list_head list;
> +};
> +
> +/**
> + * sja1105_mgmt_route: Structure to represent a SJA1105 management route
> + * @ports: List of ports on which the management route needs to be installed,
> + *	   starting with the downstream-facing cascade port of the switch which
> + *	   has the CPU connection, and ending with the user port of the leaf
> + *	   switch.
> + * @list: List node element for the mgmt_tree->routes list membership.
> + */
> +struct sja1105_mgmt_route {
> +	struct list_head ports;
> +	struct list_head list;
> +};
> +
> +/**
> + * sja1105_mgmt_tree: DSA switch tree-level structure for management routes
> + * @lock: Serializes transmission of management frames across the tree, so that
> + *	  the switches don't confuse them with one another.
> + * @routes: List of sja1105_mgmt_route structures, one for each user port in
> + *	    the tree.
> + * @refcount: Reference count.
> + */
> +struct sja1105_mgmt_tree {
> +	struct mutex lock;
> +	struct list_head routes;
> +	refcount_t refcount;
> +};
> +
>  struct sja1105_private {
>  	struct sja1105_static_config static_config;
>  	int rgmii_rx_delay_ps[SJA1105_MAX_NUM_PORTS];

...

Re: [PATCH net-next 3/4] net: dsa: populate dp->link_dp for cascade ports

[Simon Horman]

On Fri, Sep 13, 2024 at 04:15:06PM +0300, Vladimir Oltean wrote:
> Drivers may need to walk the tree hop by hop, activity which is
> currently impossible. This is because dst->rtable offers no guarantee as
> to whether we are looking at a dsa_link that represents a direct
> connection or not.
> 
> Partially address the long-standing TODO that we have, and do introduce
> a link_dp member in struct dsa_port. This will actually represent the
> adjacent cascade port.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

...

> @@ -307,9 +308,23 @@ static struct dsa_link *dsa_link_touch(struct dsa_port *dp,
>  	INIT_LIST_HEAD(&dl->list);
>  	list_add_tail(&dl->list, &dst->rtable);
>  
> +	if (adjacent)
> +		dp->link_dp = link_dp;
> +
>  	return dl;
>  }
>  
> +/**
> + * dsa_port_setup_routing_table(): Set up tree routing table based on
> + *	information from this cascade port
> + * @dp: cascade port
> + *
> + * Parse the device tree node for the "link" array of phandles to other cascade
> + * ports, creating routing table elements from this source to each destination
> + * list element found. One assumption is being made, which is backed by the
> + * device tree bindings: that the first "link" element is the directly
> + * connected cascade port.
> + */

Hi Vladimir,

Another minor nit from my side (I think this is the last one).

Please consider documenting the return value of functions that return
a value using a "Return:" or "Returns:" section.

Flagged by ./scripts/kernel-doc -none -Wall

>  static bool dsa_port_setup_routing_table(struct dsa_port *dp)
>  {
>  	struct dsa_switch *ds = dp->ds;
> @@ -317,6 +332,7 @@ static bool dsa_port_setup_routing_table(struct dsa_port *dp)
>  	struct device_node *dn = dp->dn;
>  	struct of_phandle_iterator it;
>  	struct dsa_port *link_dp;
> +	bool adjacent = true;
>  	struct dsa_link *dl;
>  	int err;
>  

...