netdev - Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 19 Feb 2019 10:42:42 -0500
From:   Vivien Didelot <vivien.didelot@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags

Hi Russell,

On Mon, 18 Feb 2019 11:23:55 +0000, Russell King - ARM Linux admin <linux@...linux.org.uk> wrote:
> On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> > From what I can see, the port_vlan_add()/port_vlan_del() implementation
> > is far from ideal, just like "always enabling flooding for CPU/DSA
> > ports" is not ideal.
> 
> There also seems to be a discrepency between what net/dsa wants to do
> and some of the implementations in drivers/net/dsa:
> 
> dsa_switch_vlan_add() does this:
> 
>         bitmap_zero(ds->bitmap, ds->num_ports);
>         if (ds->index == info->sw_index)
>                 set_bit(info->port, ds->bitmap);
>         for (port = 0; port < ds->num_ports; port++)
>                 if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
>                         set_bit(port, ds->bitmap);
> 
> We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
> which will include DSA and CPU ports on every switch in the tree.
> 
> For rtl8366, this calls into rtl8366_vlan_add(), which does:
> 
>         if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
>                 dev_err(smi->dev, "port is DSA or CPU port\n");
> 
> which is surely a guaranteed error message if we have any CPU or DSA
> ports specified on a rtl8366.  The example in the DT documentation
> for this driver does suggest that it is capable of having CPU ports.

Correct, at the very beginning, most of the logic was implemented by the DSA
drivers themselves, like programming DSA and CPU ports. But this makes them
more complicated for no value, so the logic is slowly moved up to the core,
which now handles more of the generic logic.

Ideally I would like DSA drivers to be dumb, implementing simple switch-wide
ops reflecting their specs, e.g. switch-wide vlan_add ops taking a bitmap
of ports for a given VID.

This is a work in progress and we are slowly getting there. In the meantime
there are indeed discrepencies between DSA core and drivers as you pointed out.


Thanks,

	Vivien