Message-ID: <20220718162434.72fqamkv4v274tny@skbuf>
Date:   Mon, 18 Jul 2022 16:24:35 +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 Mon, Jul 18, 2022 at 02:34:33PM +0000, Arun.Ramadoss@...rochip.com wrote:
> Hi Vladimir,
> There was a mistake in our testing on the latest code base of net-next.
> Today we tried in the latest net-next and following are the
> observation.
>
> Scenario 1: Before applying the patch
> ------------------------------
> ip link set dev br0 type bridge vlan_filtering 0
>
> bridge vlan add vid 10 dev lan1 pvid untagged
> bridge vlan add vid 10 dev lan2 pvid untagged

"bridge vlan add" on lan2 was never part of the test instructions.

>
> We got warning skipping configuration of VLAN and ksz_port_vlan_add()
> is not called.
>
> Packet is received in Host2 when transmitted from Host1. So there is no
> breakage in the forwarding.

Nonetheless it's good to know that the control scenario behaves as
expected, i.e. the VLANs are skipped while VLAN-unaware (and therefore,
the port hardware PVID remains what it was).

> Scenario 2: After applying the patch
> ----------------------------
> ip link set dev br0 type bridge vlan_filtering 0
>
> bridge vlan add vid 10 dev lan1 pvid untagged
>
> --> Packet is not received in the Host2

The problem is exactly here. The driver should pass packets at this stage.
This is because the bridge is still VLAN-unaware, despite its VLAN-aware
pvid VLAN having been changed from 1 (vlan_default_pvid) to 10.

> bridge vlan add vid 10 dev lan2 pvid untagged
>
> --> packet is received in the Host2

This only goes to prove that the VLAN in which the switch processes
traffic while VLAN-unaware is the PVID of the port. So when the PVID on
the ingress and egress ports matches, forwarding is naturally restored.

> bridge vlan del vid 10 dev lan1
>
> --> packet is received in the Host2
>
> bridge vlan del vid 10 dev lan2
>
> --> packet is received in the Host2
>
>  * Let us know, do we need to test anything further on this.

Yes, now you need to go fix the driver :) Please read the comments from
this patch and the series in general (including cover letter), they
point to similar issues in other drivers and to commits which have
solved them. I need the ksz driver to work properly before I can delete
the configure_vlan_while_not_filtering workaround. Generally speaking,
the PVID needs to be committed to hardware based on a smarter logic, see
this for example (and all the places from which it is called):

static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, int port)
{
	struct dsa_port *dp = dsa_to_port(chip->ds, port);
	struct net_device *br = dsa_port_bridge_dev_get(dp);
	struct mv88e6xxx_port *p = &chip->ports[port];
	u16 pvid = MV88E6XXX_VID_STANDALONE; // Dedicated PVID for standalone mode
	bool drop_untagged = false;
	int err;

	if (br) {
		if (br_vlan_enabled(br)) {
			pvid = p->bridge_pvid.vid; // PVID is inherited from bridge only if the bridge is *currently* VLAN-aware
			drop_untagged = !p->bridge_pvid.valid;
		} else {
			pvid = MV88E6XXX_VID_BRIDGED; // Dedicated PVID for VLAN-unaware bridging
		}
	}

	err = mv88e6xxx_port_set_pvid(chip, port, pvid);
	if (err)
		return err;

	return mv88e6xxx_port_drop_untagged(chip, port, drop_untagged);
}

Additionally, DaveM has just merged some DSA documentation updates which
I think are very relevant to this discussion. See the new "Address databases"
chapter for a review of how things are supposed to actually work when
done carefully:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst#n730

Thanks!

Powered by blists - more mailing lists