Date:   Tue, 21 Feb 2023 02:27:13 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Frank Wunderlich <frank-w@...lic-files.de>
Cc:     Arınç ÜNAL <arinc.unal@...nc9.com>,
        netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: Choose a default DSA CPU port

On Sun, Feb 19, 2023 at 10:49:24AM +0100, Frank Wunderlich wrote:
> > I'm leaving this to Frank to explain.
> 
> yes only looking at phy speed may not be the right way, but one way...i would like the hw driver
> choose the right port which is used as default.
> 
> currently i try to figure out why dsa_tree_setup_cpu_ports does not use dsa_tree_find_first_cpu.
> the loops looks same, first returns the first one, second skips all further which should be same
> and at the end it calls dsa_tree_find_first_cpu for all ports not yet having a cpu-port assigned...

Because by the time dsa_tree_find_first_cpu() has been called, all user
and cascade ports got their dp->cpu_dp pointers resolved by the earlier
logic, which prefers a CPU port local to that switch before assigning
CPU ports which are potentially on remote switches.

> looks starnge to me, but maybe i oversee a detail.

Yeah, at least one.

> does not yet compile because i do not know how to get dsa_switch from dsa_switch_tree,
> but basicly shows how i would do it (select the right cpu at driver level without dts properties).

Basically what you want is something like this:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..c4771a848319 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,20 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
 }
 
+/* If port 6 is available as a CPU port, always prefer that as the default,
+ * otherwise don't care.
+ */
+static struct dsa_port *
+mt7530_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+	struct dsa_port *cpu_dp = dsa_to_port(ds, 6);
+
+	if (dsa_port_is_cpu(cpu_dp))
+		return cpu_dp;
+
+	return NULL;
+}
+
 /* Setup TX circuit including relevant PAD and driving */
 static int
 mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -3152,6 +3166,7 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt753x_setup,
+	.preferred_default_local_cpu_port = mt7530_preferred_default_local_cpu_port,
 	.get_strings		= mt7530_get_strings,
 	.get_ethtool_stats	= mt7530_get_ethtool_stats,
 	.get_sset_count		= mt7530_get_sset_count,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a15f17a38eca..5db43be2a464 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -973,6 +973,14 @@ struct dsa_switch_ops {
 			       struct phy_device *phy);
 	void	(*port_disable)(struct dsa_switch *ds, int port);
 
+	/*
+	 * Compatibility between device trees defining multiple CPU ports and
+	 * drivers which are not ok to use by default the numerically first CPU
+	 * port of a switch for its local ports. This can return NULL, meaning
+	 * "don't know/don't care".
+	 */
+	struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds);
+
 	/*
 	 * Port's MAC EEE settings
 	 */
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..6cd8607a3928 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -402,6 +402,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
 	return 0;
 }
 
+static struct dsa_port *
+dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+	struct dsa_port *cpu_dp;
+
+	if (!ds->ops->preferred_default_local_cpu_port)
+		return NULL;
+
+	cpu_dp = ds->ops->preferred_default_local_cpu_port(ds);
+	if (!cpu_dp)
+		return NULL;
+
+	if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds))
+		return NULL;
+
+	return cpu_dp;
+}
+
 /* Perform initial assignment of CPU ports to user ports and DSA links in the
  * fabric, giving preference to CPU ports local to each switch. Default to
  * using the first CPU port in the switch tree if the port does not have a CPU
@@ -409,12 +427,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
  */
 static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
 {
-	struct dsa_port *cpu_dp, *dp;
+	struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp;
 
 	list_for_each_entry(cpu_dp, &dst->ports, list) {
 		if (!dsa_port_is_cpu(cpu_dp))
 			continue;
 
+		preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds);
+		if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp)
+			continue;
+
 		/* Prefer a local CPU port */
 		dsa_switch_for_each_port(dp, cpu_dp->ds) {
 			/* Prefer the first local CPU port found */

> imho there is no way to ensure both ways backwards compatible..

if you say so... can't argue with that

> in the moment you add port5 it will be the default cpu-port which is
> what we try to "fix" here. either driver should select the better one
> (drivercode not backported if no real fix) or it needs a setting in
> dts (which is not read in older driver/core).

If changes to driver code to resolve a device tree compatibility issue
are not treated as stable-worthy bug fixes, then the implication is that
the whole thing with device tree as stable ABI is just a moronic and
pointless effort.

Have you ever tried fixing some kind of issue similar to this and gotten
adverse feedback?

There are 2 ways of addressing the compatibility issue for stable
kernels. Either port 6 always gets preferred over port 5, or the patches
which make port 5 work as a CPU port are resubmitted as stable material.
I was hoping you'd be able to bring an argument why preferring port 6
would unconditionally be a better choice than working with the first
port that's given: $insert_reason_here.

Powered by blists - more mailing lists