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

Hi,

> Gesendet: Sonntag, 19. Februar 2023 um 08:35 Uhr
> Von: "Arınç ÜNAL" <arinc.unal@...nc9.com>
> On 18.02.2023 23:52, Vladimir Oltean wrote:

> > Before making any change, I believe that the first step is to be in agreement
> > as to what the problem is.

the problem is that the first cpu-port is not always ideal. in our case driver supports only port6 over
years till we have changed this to get r2pro working which uses only port 5 (mt7531).

so port6 is tested over long time and port 5 now comes into the view as spare cpu-port for
getting additional bandwidth.

> > The current DSA device tree binding has always chosen the numerically first
> > CPU port as the default CPU port. This was also the case at the time when the
> > mt7530 driver was accepted upstream. Clearly there are cases where this choice
> > is not the optimal choice (from the perspective of an outside observer).
> > For example when another CPU port has a higher link speed. But it's not
> > exactly obvious that higher link speed is always better. If you have a CPU
> > port at a higher link speed than the user ports, but it doesn't have flow
> > control, then the choice is practically worse than the CPU port which operates
> > at the same link speed as user ports.
> > 
> > So the choice between RGMII and TRGMII is not immediately obvious to some code
> > which should take this decision automatically. And this complicates things
> > a lot. If there is no downside to having the kernel take a decision automatically,
> > generally I don't have a problem taking it. But here, I would like to hear
> > some strong arguments why port 6 is preferable over port 5.
> 
> 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...looks starnge to me, but maybe i 
oversee a detail.

======================================================
static struct dsa_port *dsa_tree_find_first_cpu(struct dsa_switch_tree *dst)
{
	list_for_each_entry(dp, &dst->ports, list)
		if (dsa_port_is_cpu(dp))
			return dp;
...

static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
{
	cpu_dp = dsa_tree_find_first_cpu(dst);
...

static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
...
	list_for_each_entry(cpu_dp, &dst->ports, list) {
		if (!dsa_port_is_cpu(cpu_dp))
			continue;
...
return dsa_tree_setup_default_cpu(dst);
}

===============================================================================
so i would change dsa_tree_setup_cpu_ports to something like this (not ready):

struct dsa_switch_ops {
...
	struct dsa_port	*(*get_default_cpu_port)(struct dsa_switch *ds);


static struct dsa_port *dsa_tree_get_default_cpu(struct dsa_switch *ds)
{
	struct dsa_port *cpu_dp;
	struct dsa_switch_tree *dst = ds->dst;

	//first let driver choose a cpu_port
	if (ds->ops->get_default_cpu_port)
		cpu_dp = ds->ops->get_default_cpu_port(ds);

	//if driver callback not implemented or no result fall back to dsa core default
	if (!cpu_dp)
		cpu_dp = dsa_tree_find_first_cpu(dst);

	return cpu_dp;
}


static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
	struct dsa_port *cpu_dp, *dp;

	cpu_dp = dsa_tree_get_default_cpu(ds); //how to get ds from dst??
	
	//list_for_each_entry(cpu_dp, &dst->ports, list) {
	//	if (!dsa_port_is_cpu(cpu_dp))
	//		continue;

		/* Prefer a local CPU port */
		dsa_switch_for_each_port(dp, cpu_dp->ds) {
			/* Prefer the first local CPU port found */
			if (dp->cpu_dp)
				continue;

			if (dsa_port_is_user(dp) || dsa_port_is_dsa(dp))
			{
				dp->cpu_dp = cpu_dp;
				printk(KERN_ALERT "DEBUG: Passed %s %d cpu:%d set to port %s (%d)\n",__FUNCTION__,__LINE__,cpu_dp->index,dp->name,dp->index);
			}
		}
	//}

	return dsa_tree_setup_default_cpu(dst);
}

and in mt7530.c (can use other values for selecting the better one)

static struct dsa_port *mt753x_get_default_cpu(struct dsa_switch *ds)
{
	struct dsa_port *cpu_dp;
	unsigned int port = 0;

	if (dsa_is_cpu_port(ds, 6))
		port=6;
	else if (dsa_is_cpu_port(ds, 5))
		port =5;
	if (port)
		cpu_dp = dsa_to_port(ds, port);
	
	return cpu_dp;
}

static const struct dsa_switch_ops mt7530_switch_ops = {
...
	.get_default_cpu_port	= mt753x_get_default_cpu,

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

> > 
> > If there are strong reasons to consider port 6 a primary CPU port and
> > port 5 a secondary one, then there is also a very valid concern of forward
> > compatibility between the mt7530 driver and device trees from the future
> > (with multiple CPU ports defined). The authors of the mt7530 driver must have
> > been aware of the DSA binding implementation's choice of selecting the
> > first CPU port as the default, but they decided to hide their head in
> > the sand and pretend like this issue will never crop up. The driver has
> > not been coded up to accept port 5 as a valid CPU port until very recently.
> > What should have been done (*assuming strong arguments*) is that
> > dsa_tree_setup_default_cpu() should have been modified from day one of
> > mt7530 driver introduction, such that the driver has a way of specifying
> > a preferred default CPU port.
> > 
> > In other words, the fact that the CPU port becomes port 5 when booting
> > on a new device tree is equally a problem for current kernels as it is
> > for past LTS kernels. I would like this to be treated seriously, and
> > current + stable kernels should behave in a reasonable manner with
> > device trees from the future, before support for multiple CPU ports is
> > added to mt7530. Forcing users to change device trees in lockstep with
> > the kernel is not something that I want to maintain and support, if user
> > questions and issues do crop up.
> > 
> > Since this wasn't done, the only thing we're left with is to retroactively
> > add this functionality to pick a preferred default CPU port, as patches
> > to "net" which get backported to stable kernels. Given enough forethought
> > in the mt7530 driver development, this should not have been necessary,
> > but here we are.
> > 
> > Now that I expressed this point of view, let me comment on why your
> > proposal, Arınç, solves exactly nothing.
> > 
> > If you add a device tree property for the preferred CPU port, you
> > haven't solved any of the compatibility problems:
> > 
> > - old kernels + new device trees will not have the logic to interpret
> >    that device tree property
> > - old device trees + new kernels will not have that device tree property
> > 
> > so... yeah.
> 
> Makes perfect sense. I always make the assumption that once the DTs on 
> the kernel source code is updated, it will be used everywhere, which is 
> just not the case.

imho there is no way to ensure both ways backwards compatible..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).

regards Frank

Powered by blists - more mailing lists