[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230221002713.qdsabxy7y74jpbm4@skbuf>
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