netdev - Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu

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]

Date:   Wed, 07 Jun 2017 19:24:00 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        John Crispin <john@...ozen.org>,
        David Miller <davem@...emloft.net>,
        Sean Wang <sean.wang@...iatek.com>
Subject: Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

Hi Florian,

Sorry to bother you again, I don't want to be annoying but I might not
get things right still.

Florian Fainelli <f.fainelli@...il.com> writes:

>>>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>>>> correctly assigned at setup time, isn't better (especially for future
>>>> multi-CPU support) to provide an helper which returns the CPU port for a
>>>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>>>
>>>> Or is there something blocking? I might be wrong.
>>>
>>> mt7530.c needs access to the CPU port at ops->setup() time which is
>>> why this is still here.
>> 
>> Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
>> what I meant was, shouldn't we have this instead:
>> 
>>     struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
>>     {
>>         return ds->ports[port].cpu_dp;
>>     }
>
> We don't actually have a CPU port point to itself:
>
> +
> +		for (i = 0; i < ds->num_ports; i++) {
> +			p = &ds->ports[i];
> +			if (!dsa_port_is_valid(p) ||
> +			    i == index) <=============
> +				continue;
> +
> +			p->cpu_dp = port;
> +		}
>  	}
>
>> 
>> And:
>> 
>> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
>> +       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
>> +       dn = cpu_dp->netdev->dev.of_node->parent;
>
> If we are giving the port number to get its cpu_dp pointer back, that
> seems a bit pointless.

What a driver may want is the master interface (e.g. eth0), actually
soldered to the dedicated switch CPU port, that will be used to
send/receive frames.

Also I do not think that it is a good thing that a DSA driver play much
in dsa_port structures (they are ideally DSA core only specific data).
They only seem to need the master interface, so what I see is:

    static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
    {
            struct dsa_port *dp = &ds->ports[port];

            if (!dsa_is_cpu_port(ds, port))
                    dp = dp->cpu_dp;

            return dp->netdev;
    }

> I still think the helper with fls(ds->cpu_port_mask) - 1 is better in
> that it will return what you have configured from Device Tree/platform
> data. MT7530 does allow the CPU port being arbitrary, and it would
> disable MTK tags in that case.

If MT7530 allows several CPU ports, what is MT7530_CPU_PORT then?
(Maybe Sean can give me some details here?)

Now let's think that you can have several CPU ports (as with mv88e6xxx).
I think it is the driver responsibility to iterate over CPU capable
ports and inspect the master devices if they need to, instead of having
DSA core return an arbitrary one (which might be the wrong one.)

This is a static data (describing to which SoC interface a switch CPU
port is wired) that should've been parsed by DSA core before setup.

Thanks,

        Vivien