Re: [RFC PATCH net-next 3/3] net: dsa: never skip VLAN configuration

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Hi Arun,

On Mon, Jul 18, 2022 at 02:34:33PM +0000, Arun.Ramadoss@microchip.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!