Date:   Tue, 26 Jul 2022 17:21:28 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     "Arun.Ramadoss@...rochip.com" <Arun.Ramadoss@...rochip.com>
CC:     Claudiu Manoil <claudiu.manoil@....com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "idosch@...dia.com" <idosch@...dia.com>,
        "linux@...pel-privat.de" <linux@...pel-privat.de>,
        "petrm@...dia.com" <petrm@...dia.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "hauke@...ke-m.de" <hauke@...ke-m.de>,
        "martin.blumenstingl@...glemail.com" 
        <martin.blumenstingl@...glemail.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Woojung.Huh@...rochip.com" <Woojung.Huh@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [RFC PATCH net-next 3/3] net: dsa: never skip VLAN configuration

Hi Arun,

On Tue, Jul 26, 2022 at 03:10:24PM +0000, Arun.Ramadoss@...rochip.com wrote:
> I tried to update the ksz code and tested after applying this patch
> series. Following are the observation,
> 
(...)
> In summary, only for pvid 1 below patch is working. Initially I tried
> with pvid 0, 21, 4095, it were not working, only for pvid 1 it is
> working. Kindly suggest whether any changes to be done in patch or
> testing methodology.

What are you saying exactly that you tried with pvid 0, 21, 4095?
Do you mean
(a) you changed the vlan_default_pvid of the bridge to these values, or
(b) you edited "u16 pvid = 1" in ksz_commit_pvid() to "u16 pvid = 0" (or 21, 4095 etc)?

Either way, the fundamental reason why neither was going to work is the
same, although the explanation is going to be slightly different.

What vlan_default_pvid means is what the bridge layer uses as a pvid
value for VLAN-aware ports. The value of 0 is special and it means
"don't add a PVID at all". It's the same as if you compiled your kernel
with CONFIG_BRIDGE_VLAN_FILTERING=n.

The problem is that you're not making a difference between the bridge
PVID and the hardware PVID.

See, things don't work due to the line highlighted below:

static int ksz_commit_pvid(struct dsa_switch *ds, int port)
{
	struct dsa_port *dp = dsa_to_port(ds, port);
	struct net_device *br = dsa_port_bridge_dev_get(dp);
	struct ksz_device *dev = ds->priv;
	bool drop_untagged = false;
	struct ksz_port *p;
	u16 pvid = 1;       /* bridge vlan unaware pvid */   <--- this line

	p = &dev->ports[port];

	if (br && br_vlan_enabled(br)) {
		pvid = p->bridge_pvid.vid;
		drop_untagged = !p->bridge_pvid.valid;
	}

	ksz_set_pvid(dev, port, pvid);

	if (dev->dev_ops->drop_untagged)
		dev->dev_ops->drop_untagged(dev, port, drop_untagged);

	return 0;
}

You can't just gingerly say that the PVID is 1, then rely on some other
code (the bridge layer) to actually _add_ VLAN 1 to your VLAN database,
and expect things to work (as you've noticed, they don't, when the
bridge doesn't add this VLAN).

If I'm following along properly, and *there is some guesswork involved*,
VLAN-unaware bridging only works with the KSZ driver if you have
CONFIG_BRIDGE_VLAN_FILTERING enabled. If you don't, nobody adds any VLAN
to your VLAN table, and (this is the guessing part) the VLAN table of
these switches is by default empty.

You need to pick a VLAN that you use for the address database of a VLAN
unaware bridge, and manage it by yourself. That means, add logic to the
driver to add said VLAN ID to the VLAN table, and deny other layers
(DSA's port_vlan_add) from touching it.

That's why some other drivers use VLAN 4095 and/or 0 for that purpose,
because they don't need to deny these VIDs from the bridge layer,
because 'bridge vlan add' already fails for VID 0 and 4095 (IEEE 802.1Q
defines them as having reserved values).

This is done for example in mt7530_setup_vlan0() in mt7530, managed by
tag_8021q for sja1105, or ocelot_port_bridge_join() -> ocelot_add_vlan_unaware_pvid()
-> ocelot_vlan_unaware_pvid() in the case of the felix driver.
mt7530 has nothing to reject, while sja1105 rejects private VLANs in
sja1105_bridge_vlan_add() and felix in ocelot_vlan_prepare() - see
OCELOT_RSV_VLAN_RANGE_START.

Now don't take anything of what I've said too literal, because I'm a
complete non-expert for KSZ switches, and the way in which you enforce
isolation of address databases is completely hardware-specific.

Of course what I've told you above relies on cropping some VLANs from
what the bridge layer can use (and then you need to make sure that
somebody from the outside can't 'attack' you by sending a packet with
VLAN 4095 on a port from a VLAN-aware bridge, and your switch thinks
that it should process it in the forwarding domain of a VLAN-unaware
bridge that *actually* uses VID 4095 as part of your private driver
implementation). So the approach of cropping VLANs to use privately is
not optimal, and does need care to implement properly.

I advise you to read back starting from this thread with Qingfang about
how address database isolation was implemented for mt7530.
There, we ended up using transparent mode for standalone ports (VLAN
table is not looked up at all, packets default to one FID), and fallback
mode for VLAN-unaware bridge ports (packets default to another FID).
So that can be extended to allocate a FID for each VLAN unaware bridge,
without cropping more and more VLANs, just mapping the ports of each
VLAN unaware bridge to a different FID.
https://lore.kernel.org/all/20210730175139.518992-1-dqfext@gmail.com/

Some experimentation needs to be done to find the optimum configuration
for this hardware. At the very least, what I care to see is, in this order:
(1) a driver that makes sure the private VLANs it uses are present in
    the VLAN table
(2) a driver that only changes the hardware PVID when it should, i.e.
    not when it's VLAN-unaware and the bridge changes the VLAN-aware PVID
(3) a driver that separates the address databases of standalone ports
    from that of VLAN-unaware bridge ports
(4) a driver which separates the address databases of *each* VLAN-unaware
    bridge from each other

For the purposes of my change, I only need (1) and (2), but I strongly
suggest you to look into (3) and (4) as well. Some selftests (which I'd
very much like for the KSZ driver to pass!) rely on (3) working properly.

Sorry for not diving into KSZ hardware documentation, I wrote this email
in a bit of a hurry.

Powered by blists - more mailing lists