netdev - Re: [PATCH net-next 1/2] net: dsa: Add ability to elect CPU port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <21f28a95-9eea-19b4-fee9-c9f436bf2416@gmail.com>
Date:   Tue, 29 Oct 2019 10:23:26 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, vivien.didelot@...il.com,
        davem@...emloft.net, Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net-next 1/2] net: dsa: Add ability to elect CPU port

Hi Andrew,

On 10/28/19 6:39 PM, Andrew Lunn wrote:
> On Mon, Oct 28, 2019 at 03:32:35PM -0700, Florian Fainelli wrote:
>> In a configuration where multiple CPU ports are declared within the
>> platform configuration, it may be desirable to make sure that a
>> particular CPU port gets used. This is particularly true for Broadcom
>> switch that are fairly flexible to some extent in which port can be the
>> CPU port, yet will be more featureful if port 8 is elected.
> 
>> -static struct dsa_port *dsa_tree_find_first_cpu(struct dsa_switch_tree *dst)
>> +static struct dsa_port *dsa_tree_find_cpu(struct dsa_switch_tree *dst)
>>  {
>> +	struct dsa_switch *ds;
>>  	struct dsa_port *dp;
>> +	int err;
>>  
>> -	list_for_each_entry(dp, &dst->ports, list)
>> -		if (dsa_port_is_cpu(dp))
>> +	list_for_each_entry(dp, &dst->ports, list) {
>> +		ds = dp->ds;
>> +		if (!dsa_port_is_cpu(dp))
>> +			continue;
>> +
>> +		if (!ds->ops->elect_cpu_port)
>>  			return dp;
>>  
>> +		err = ds->ops->elect_cpu_port(ds, dp->index);
>> +		if (err == 0)
>> +			return dp;
>> +	}
>> +
>>  	return NULL;
>>  }
> 
> Hi Florian
> 
> I think is_preferred_cpu_port() would be a better name, and maybe a
> bool?
> 
> Also, i don't think we should be returning NULL at the end like
> this. If the device tree does not have the preferred port as CPU port,
> we should use dsa_tree_find_cpu() to pick one of the actually offered
> CPU ports in DT? It sounds like your hardware will still work if any
> port is used as the CPU port.

It would indeed work, although most likely in a degraded mode, so not a
great fit usually.

> 
> And maybe we need a is_valid_cpu_port()? Some of the chipsets only
> have a subset which can be CPU ports, the hardware is not as flexible.
> The core can then validate the CPU port really is valid, rather than
> the driver, e.g. qca8k, validating the CPU port in setup() and
> returning an error.

Initially I had added a port_validate() function and had it be called
from dsa_switch_parse_ports_of() and dsa_switch_parse_ports() such that
the driver could check whether the port type (DSA, USER, CPU) was valid
and return 0, an error or -EAGAIN if the CPU port was not the preferred one.

Later on, I did introduce is_preferred_cpu_port() returning a bool, but
thought this was too long of a name and went for elect_cpu_port which is
not really an election process.... Now you know my thought process :)

Let me sleep on it a bit, maybe coming back with the port_validate()
approach is the most flexible/capable solution.
-- 
Florian