[RFC PATCH net-next 0/3] Delete ds->configure_vlan_while_not_filtering

[RFC PATCH net-next 0/3] Delete ds->configure_vlan_while_not_filtering

[Vladimir Oltean]

Even though this isn't documented explicitly, we have told driver
writers on different occasions (mainly during review) that
ds->configure_vlan_while_not_filtering is an option which ideally should
not exist, is opt-in, that we should work towards deleting it, and new
drivers should not opt into it.

However, what seems to be happening is that new drivers still seem to be
able to slip through the cracks and get introduced with the VLAN
skipping legacy behavior.

Such is the case of the Microchip LAN937x, which was merged in v15,
after being taken over by Arun Ramadoss from Prasanna Vengateshan.
https://patchwork.kernel.org/project/netdevbpf/cover/20220701144652.10526-1-arun.ramadoss@microchip.com/

I had asked Prasanna to remove the deprecated option from existing KSZ
drivers:
https://patchwork.kernel.org/project/netdevbpf/patch/20210723173108.459770-11-prasanna.vengateshan@microchip.com/#24351125
and yet somehow, how we are in the situation that after Arun's KSZ
driver refactoring to use more common code, the quirks are common too,
including ds->configure_vlan_while_not_filtering being inherited by the
new LAN937x driver.

Maybe the problem was that I wasn't specific enough about what should be
done to move forward, so this patch set attempts to be a more concrete
step. I've created a selftest that captures what I believe to be the
essence of the workaround, and I'd like to ask maintainers with access
to KSZ and to GSWIP hardware to test it and to propose fixes.

Vladimir Oltean (3):
  selftests: forwarding: add a vlan_deletion test to bridge_vlan_unaware
  net: dsa: ar9331: remove ds->configure_vlan_while_not_filtering
  net: dsa: never skip VLAN configuration

 drivers/net/dsa/lantiq_gswip.c                |  2 --
 drivers/net/dsa/microchip/ksz_common.c        |  2 --
 drivers/net/dsa/qca/ar9331.c                  |  2 --
 include/net/dsa.h                             |  7 ------
 net/dsa/dsa2.c                                |  2 --
 net/dsa/dsa_priv.h                            |  1 -
 net/dsa/port.c                                | 14 -----------
 net/dsa/slave.c                               | 22 +---------------
 .../net/forwarding/bridge_vlan_unaware.sh     | 25 ++++++++++++++++---
 9 files changed, 23 insertions(+), 54 deletions(-)

-- 
2.25.1

[RFC PATCH net-next 1/3] selftests: forwarding: add a vlan_deletion test to bridge_vlan_unaware

[Vladimir Oltean]

Historically, DSA drivers have seen problems with the model in which
bridge VLANs work, particularly with them being offloaded to switchdev
asynchronously relative to when they become active (vlan_filtering=1).

This switchdev API peculiarity was papered over by commit 2ea7a679ca2a
("net: dsa: Don't add vlans when vlan filtering is disabled"), which
introduced other problems, fixed by commit 54a0ed0df496 ("net: dsa:
provide an option for drivers to always receive bridge VLANs") through
an opt-in ds->configure_vlan_while_not_filtering bool (which later
became an opt-out).

The point is that some DSA drivers still skip VLAN configuration while
VLAN-unaware, and there is a desire to get rid of that behavior.

It's hard to deduce from the wording "at least one corner case" what
Andrew saw, but my best guess is that there is a discrepancy of meaning
between bridge pvid and hardware port pvid which caused breakage.

On one side, the Linux bridge with vlan_filtering=0 is completely
VLAN-unaware, and will accept and process a packet the same way
irrespective of the VLAN groups on the ports or the bridge itself
(there may not even be a pvid, and this makes no difference).

On the other hand, DSA switches still do VLAN processing internally,
even with vlan_filtering disabled, but they are expected to classify all
packets to the port pvid. That pvid shouldn't be confused with the
bridge pvid, and there lies the problem.

When a switch port is under a VLAN-unaware bridge, the hardware pvid
must be explicitly managed by the driver to classify all received
packets to it, regardless of bridge VLAN groups. When under a VLAN-aware
bridge, the hardware pvid must be synchronized to the bridge port pvid.
To do this correctly, the pattern is unfortunately a bit complicated,
and involves hooking the pvid change logic into quite a few places
(the ones that change the input variables which determine the value to
use as hardware pvid for a port). See mv88e6xxx_port_commit_pvid(),
sja1105_commit_pvid(), ocelot_port_set_pvid() etc.

The point is that not all drivers used to do that, especially in older
kernels. If a driver is to blindly program a bridge pvid VLAN received
from switchdev while it's VLAN-unaware, this might in turn change the
hardware pvid used by a VLAN-unaware bridge port, which might result in
packet loss depending which other ports have that pvid too (in that same
note, it might also go unnoticed).

To capture that condition, it is sufficient to take a VLAN-unaware
bridge and change the [VLAN-aware] bridge pvid on a single port, to a
VID that isn't present on any other port. This shouldn't have absolutely
any effect on packet classification or forwarding. However, broken
drivers will take the bait, and change their PVID to 3, causing packet
loss.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
In case you see some apparently unrelated failures reported by
bridge_vlan_unaware.sh, it's good to be aware of some fixes sent earlier
this week to "net", which are hence absent from net-next currently:
https://patchwork.kernel.org/project/netdevbpf/cover/20220703073626.937785-1-vladimir.oltean@nxp.com/

 .../net/forwarding/bridge_vlan_unaware.sh     | 25 ++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
index 1c8a26046589..2b5700b61ffa 100755
--- a/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_vlan_unaware.sh
@@ -1,7 +1,7 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="ping_ipv4 ping_ipv6 learning flooding"
+ALL_TESTS="ping_ipv4 ping_ipv6 learning flooding pvid_change"
 NUM_NETIFS=4
 source lib.sh
 
@@ -77,12 +77,16 @@ cleanup()
 
 ping_ipv4()
 {
-	ping_test $h1 192.0.2.2
+	local msg=$1
+
+	ping_test $h1 192.0.2.2 "$msg"
 }
 
 ping_ipv6()
 {
-	ping6_test $h1 2001:db8:1::2
+	local msg=$1
+
+	ping6_test $h1 2001:db8:1::2 "$msg"
 }
 
 learning()
@@ -95,6 +99,21 @@ flooding()
 	flood_test $swp2 $h1 $h2
 }
 
+pvid_change()
+{
+	# Test that the changing of the VLAN-aware PVID does not affect
+	# VLAN-unaware forwarding
+	bridge vlan add vid 3 dev $swp1 pvid untagged
+
+	ping_ipv4 " with bridge port $swp1 PVID changed"
+	ping_ipv6 " with bridge port $swp1 PVID changed"
+
+	bridge vlan del vid 3 dev $swp1
+
+	ping_ipv4 " with bridge port $swp1 PVID deleted"
+	ping_ipv6 " with bridge port $swp1 PVID deleted"
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.25.1

Re: [RFC PATCH net-next 1/3] selftests: forwarding: add a vlan_deletion test to bridge_vlan_unaware

[Ido Schimmel]

On Tue, Jul 05, 2022 at 08:31:12PM +0300, Vladimir Oltean wrote:
> Historically, DSA drivers have seen problems with the model in which
> bridge VLANs work, particularly with them being offloaded to switchdev
> asynchronously relative to when they become active (vlan_filtering=1).
> 
> This switchdev API peculiarity was papered over by commit 2ea7a679ca2a
> ("net: dsa: Don't add vlans when vlan filtering is disabled"), which
> introduced other problems, fixed by commit 54a0ed0df496 ("net: dsa:
> provide an option for drivers to always receive bridge VLANs") through
> an opt-in ds->configure_vlan_while_not_filtering bool (which later
> became an opt-out).
> 
> The point is that some DSA drivers still skip VLAN configuration while
> VLAN-unaware, and there is a desire to get rid of that behavior.
> 
> It's hard to deduce from the wording "at least one corner case" what
> Andrew saw, but my best guess is that there is a discrepancy of meaning
> between bridge pvid and hardware port pvid which caused breakage.
> 
> On one side, the Linux bridge with vlan_filtering=0 is completely
> VLAN-unaware, and will accept and process a packet the same way
> irrespective of the VLAN groups on the ports or the bridge itself
> (there may not even be a pvid, and this makes no difference).
> 
> On the other hand, DSA switches still do VLAN processing internally,
> even with vlan_filtering disabled, but they are expected to classify all
> packets to the port pvid. That pvid shouldn't be confused with the
> bridge pvid, and there lies the problem.
> 
> When a switch port is under a VLAN-unaware bridge, the hardware pvid
> must be explicitly managed by the driver to classify all received
> packets to it, regardless of bridge VLAN groups. When under a VLAN-aware
> bridge, the hardware pvid must be synchronized to the bridge port pvid.
> To do this correctly, the pattern is unfortunately a bit complicated,
> and involves hooking the pvid change logic into quite a few places
> (the ones that change the input variables which determine the value to
> use as hardware pvid for a port). See mv88e6xxx_port_commit_pvid(),
> sja1105_commit_pvid(), ocelot_port_set_pvid() etc.
> 
> The point is that not all drivers used to do that, especially in older
> kernels. If a driver is to blindly program a bridge pvid VLAN received
> from switchdev while it's VLAN-unaware, this might in turn change the
> hardware pvid used by a VLAN-unaware bridge port, which might result in
> packet loss depending which other ports have that pvid too (in that same
> note, it might also go unnoticed).
> 
> To capture that condition, it is sufficient to take a VLAN-unaware
> bridge and change the [VLAN-aware] bridge pvid on a single port, to a
> VID that isn't present on any other port. This shouldn't have absolutely
> any effect on packet classification or forwarding. However, broken
> drivers will take the bait, and change their PVID to 3, causing packet
> loss.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Tested-by: Ido Schimmel <idosch@nvidia.com>

Re: [RFC PATCH net-next 1/3] selftests: forwarding: add a vlan_deletion test to bridge_vlan_unaware

[Martin Blumenstingl]

Hi Vladimir,

On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
[...]
>  .../net/forwarding/bridge_vlan_unaware.sh     | 25 ++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
While working on an OpenWrt package for these selftests I found that
this should be added to the Makefile (in the same directory as the
test).
That way it can be installed by a distribution using:
  make -C tools/testing/selftests/net/forwarding/ \
      INSTALL_PATH="some/install/path" \
      install

If you agree then I can also send patches for adding no_forwarding.sh
and local_termination.sh to that Makefile (which are the only two
files which are missing currently).


Best regards,
Martin

Re: [RFC PATCH net-next 1/3] selftests: forwarding: add a vlan_deletion test to bridge_vlan_unaware

[Vladimir Oltean]

On Thu, Jul 07, 2022 at 03:34:07PM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> [...]
> >  .../net/forwarding/bridge_vlan_unaware.sh     | 25 ++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> While working on an OpenWrt package for these selftests I found that
> this should be added to the Makefile (in the same directory as the
> test).
> That way it can be installed by a distribution using:
>   make -C tools/testing/selftests/net/forwarding/ \
>       INSTALL_PATH="some/install/path" \
>       install
> 
> If you agree then I can also send patches for adding no_forwarding.sh
> and local_termination.sh to that Makefile (which are the only two
> files which are missing currently).

Yes, please do that. I noticed that was the correct way of doing things,
but rsync was enough for my caveman testing so far...
Thanks again for doing this, I haven't forgotten about your other email
and will respond to it later today.

[RFC PATCH net-next 2/3] net: dsa: ar9331: remove ds->configure_vlan_while_not_filtering

[Vladimir Oltean]

This driver does not need the DSA legacy opt-in, because it offloads
neither port_bridge_join() nor port_vlan_add(), so all bridging happens
in software.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/qca/ar9331.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 0796b7cf8cae..049127758eaf 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -474,8 +474,6 @@ static int ar9331_sw_setup(struct dsa_switch *ds)
 			goto error;
 	}
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 error:
 	dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
-- 
2.25.1

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

[Vladimir Oltean]

Stop protecting DSA drivers from switchdev VLAN notifications emitted
while the bridge has vlan_filtering 0, by deleting the deprecated bool
ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see
all notifications and should save the bridge PVID until they need to
commit it to hardware.

The 2 remaining unconverted drivers are the gswip and the Microchip KSZ
family. They are both probably broken and would need changing as far as
I can see:

- For lantiq_gswip, after the initial call path
  -> gswip_port_bridge_join
     -> gswip_vlan_add_unaware
        -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
  nobody seems to prevent a future call path
  -> gswip_port_vlan_add
     -> gswip_vlan_add_aware
        -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));

- For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the
  bridge VLAN add/del handlers, so there is no logic to update it on
  VLAN awareness change. I don't know exactly how the switch behaves
  after ksz9477_port_vlan_filtering() is called with "false".
  If packets are classified to the REG_PORT_DEFAULT_VID, then it is
  broken. Similar thing can be said for KSZ8.

In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the
pvid at 0 when VLAN-unaware") for an example of how to deal with the
problem, and test pvid_change() in
tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for how
to check whether the driver behaves correctly. I don't have the hardware
to test any changes.

Cc: Arun Ramadoss <arun.ramadoss@microchip.com>
Cc: Hauke Mehrtens <hauke@hauke-m.de>
Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/lantiq_gswip.c         |  2 --
 drivers/net/dsa/microchip/ksz_common.c |  2 --
 include/net/dsa.h                      |  7 -------
 net/dsa/dsa2.c                         |  2 --
 net/dsa/dsa_priv.h                     |  1 -
 net/dsa/port.c                         | 14 --------------
 net/dsa/slave.c                        | 22 +---------------------
 7 files changed, 1 insertion(+), 49 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index e531b93f3cb2..86acf8a4e53c 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -893,8 +893,6 @@ static int gswip_setup(struct dsa_switch *ds)
 
 	gswip_port_enable(ds, cpu_port, NULL);
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 28d7cb2ce98f..561a515c7cb8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -927,8 +927,6 @@ static int ksz_setup(struct dsa_switch *ds)
 
 	ksz_init_mib_timer(dev);
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	if (dev->dev_ops->setup) {
 		ret = dev->dev_ops->setup(ds);
 		if (ret)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index b902b31bebce..2ed1c2db4037 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -400,13 +400,6 @@ struct dsa_switch {
 	/* Keep VLAN filtering enabled on ports not offloading any upper */
 	u32			needs_standalone_vlan_filtering:1;
 
-	/* Pass .port_vlan_add and .port_vlan_del to drivers even for bridges
-	 * that have vlan_filtering=0. All drivers should ideally set this (and
-	 * then the option would get removed), but it is unknown whether this
-	 * would break things or not.
-	 */
-	u32			configure_vlan_while_not_filtering:1;
-
 	/* If the switch driver always programs the CPU port as egress tagged
 	 * despite the VLAN configuration indicating otherwise, then setting
 	 * @untag_bridge_pvid will force the DSA receive path to pop the
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cac48a741f27..47a2e60b6080 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -890,8 +890,6 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (err)
 		goto unregister_devlink_ports;
 
-	ds->configure_vlan_while_not_filtering = true;
-
 	err = ds->ops->setup(ds);
 	if (err < 0)
 		goto unregister_notifier;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index d9722e49864b..63191db45d02 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -224,7 +224,6 @@ void dsa_port_pre_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
 void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev);
 int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct netlink_ext_ack *extack);
-bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
 int dsa_port_mst_enable(struct dsa_port *dp, bool on,
 			struct netlink_ext_ack *extack);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 3738f2d40a0b..79853f673b65 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -834,20 +834,6 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 	return err;
 }
 
-/* This enforces legacy behavior for switch drivers which assume they can't
- * receive VLAN configuration when enslaved to a bridge with vlan_filtering=0
- */
-bool dsa_port_skip_vlan_configuration(struct dsa_port *dp)
-{
-	struct net_device *br = dsa_port_bridge_dev_get(dp);
-	struct dsa_switch *ds = dp->ds;
-
-	if (!br)
-		return false;
-
-	return !ds->configure_vlan_while_not_filtering && !br_vlan_enabled(br);
-}
-
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock)
 {
 	unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ad6a6663feeb..d1284f8684d8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -533,11 +533,6 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 	struct switchdev_obj_port_vlan *vlan;
 	int err;
 
-	if (dsa_port_skip_vlan_configuration(dp)) {
-		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
-		return 0;
-	}
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	/* Deny adding a bridge VLAN when there is already an 802.1Q upper with
@@ -571,11 +566,6 @@ static int dsa_slave_host_vlan_add(struct net_device *dev,
 	if (!dp->bridge)
 		return -EOPNOTSUPP;
 
-	if (dsa_port_skip_vlan_configuration(dp)) {
-		NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN");
-		return 0;
-	}
-
 	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	/* Even though drivers often handle CPU membership in special ways,
@@ -642,9 +632,6 @@ static int dsa_slave_vlan_del(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct switchdev_obj_port_vlan *vlan;
 
-	if (dsa_port_skip_vlan_configuration(dp))
-		return 0;
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	return dsa_port_vlan_del(dp, vlan);
@@ -660,9 +647,6 @@ static int dsa_slave_host_vlan_del(struct net_device *dev,
 	if (!dp->bridge)
 		return -EOPNOTSUPP;
 
-	if (dsa_port_skip_vlan_configuration(dp))
-		return 0;
-
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	return dsa_port_host_vlan_del(dp, vlan);
@@ -1655,11 +1639,7 @@ static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg)
  *         - no VLAN (any 8021q upper is a software VLAN)
  *
  * - If under a vlan_filtering=0 bridge which it offload:
- *     - if ds->configure_vlan_while_not_filtering = true (default):
- *         - the bridge VLANs. These VLANs are committed to hardware but inactive.
- *     - else (deprecated):
- *         - no VLAN. The bridge VLANs are not restored when VLAN awareness is
- *           enabled, so this behavior is broken and discouraged.
+ *     - the bridge VLANs. These VLANs are committed to hardware but inactive.
  *
  * - If under a vlan_filtering=1 bridge which it offload:
  *     - the bridge VLANs
-- 
2.25.1

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

[Arun.Ramadoss]

Hi Vladimir,

On Tue, 2022-07-05 at 20:31 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Stop protecting DSA drivers from switchdev VLAN notifications emitted
> while the bridge has vlan_filtering 0, by deleting the deprecated
> bool
> ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers
> see
> all notifications and should save the bridge PVID until they need to
> commit it to hardware.
> 
> The 2 remaining unconverted drivers are the gswip and the Microchip
> KSZ
> family. They are both probably broken and would need changing as far
> as
> I can see:
> 
> - For lantiq_gswip, after the initial call path
>   -> gswip_port_bridge_join
>      -> gswip_vlan_add_unaware
>         -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
>   nobody seems to prevent a future call path
>   -> gswip_port_vlan_add
>      -> gswip_vlan_add_aware
>         -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));
> 
> - For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the
>   bridge VLAN add/del handlers, so there is no logic to update it on
>   VLAN awareness change. I don't know exactly how the switch behaves
>   after ksz9477_port_vlan_filtering() is called with "false".
>   If packets are classified to the REG_PORT_DEFAULT_VID, then it is
>   broken. Similar thing can be said for KSZ8.

When ksz9477_port_vlan_filtering is set to false, then it clears the
802.1Q vlan enable bit in the switch. So the packets are not classified
based on vlan tag. Only if the vlan bit is set, packets are classified
based on pvid.

> 
> In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the
> pvid at 0 when VLAN-unaware") for an example of how to deal with the
> problem, and test pvid_change() in
> tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for
> how
> to check whether the driver behaves correctly. I don't have the
> hardware
> to test any changes.

I can test it and report the observation. But I haven't run the
selftests before. I looked in the scripts today, but couldn't find out
how to compile it as part of kernel and program it to the target &
test. I infer that, this scripts should be run on target (I have
SAMA5D3 + KSZ9477) with 4 switch ports. Can you guide me on testing
this.

> 
> Cc: Arun Ramadoss <arun.ramadoss@microchip.com>
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/lantiq_gswip.c         |  2 --
>  drivers/net/dsa/microchip/ksz_common.c |  2 --
>  include/net/dsa.h                      |  7 -------
>  net/dsa/dsa2.c                         |  2 --
>  net/dsa/dsa_priv.h                     |  1 -
>  net/dsa/port.c                         | 14 --------------
>  net/dsa/slave.c                        | 22 +---------------------
>  7 files changed, 1 insertion(+), 49 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c
> b/drivers/net/dsa/lantiq_gswip.c
> index e531b93f3cb2..86acf8a4e53c 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -893,8 +893,6 @@ static int gswip_setup(struct dsa_switch *ds)
> 
>         gswip_port_enable(ds, cpu_port, NULL);
> 
> -       ds->configure_vlan_while_not_filtering = false;
> -
>         return 0;
>  }
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 28d7cb2ce98f..561a515c7cb8 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -927,8 +927,6 @@ static int ksz_setup(struct dsa_switch *ds)
> 
>         ksz_init_mib_timer(dev);
> 
> -       ds->configure_vlan_while_not_filtering = false;
> -
>         if (dev->dev_ops->setup) {
>                 ret = dev->dev_ops->setup(ds);
>                 if (ret)
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index b902b31bebce..2ed1c2db4037 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -400,13 +400,6 @@ struct dsa_switch {
>         /* Keep VLAN filtering enabled on ports not offloading any
> upper */
>         u32                     needs_standalone_vlan_filtering:1;
> 
> -       /* Pass .port_vlan_add and .port_vlan_del to drivers even for
> bridges
> -        * that have vlan_filtering=0. All drivers should ideally set
> this (and
> -        * then the option would get removed), but it is unknown
> whether this
> -        * would break things or not.
> -        */
> -       u32                     configure_vlan_while_not_filtering:1;
> -
>         /* If the switch driver always programs the CPU port as
> egress tagged
>          * despite the VLAN configuration indicating otherwise, then
> setting
>          * @untag_bridge_pvid will force the DSA receive path to pop
> the
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index cac48a741f27..47a2e60b6080 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -890,8 +890,6 @@ static int dsa_switch_setup(struct dsa_switch
> *ds)
>         if (err)
>                 goto unregister_devlink_ports;
> 
> -       ds->configure_vlan_while_not_filtering = true;
> -
>         err = ds->ops->setup(ds);
>         if (err < 0)
>                 goto unregister_notifier;
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index d9722e49864b..63191db45d02 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -224,7 +224,6 @@ void dsa_port_pre_lag_leave(struct dsa_port *dp,
> struct net_device *lag_dev);
>  void dsa_port_lag_leave(struct dsa_port *dp, struct net_device
> *lag_dev);
>  int dsa_port_vlan_filtering(struct dsa_port *dp, bool
> vlan_filtering,
>                             struct netlink_ext_ack *extack);
> -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp);
>  int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock);
>  int dsa_port_mst_enable(struct dsa_port *dp, bool on,
>                         struct netlink_ext_ack *extack);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 3738f2d40a0b..79853f673b65 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -834,20 +834,6 @@ int dsa_port_vlan_filtering(struct dsa_port *dp,
> bool vlan_filtering,
>         return err;
>  }
> 
> -/* This enforces legacy behavior for switch drivers which assume
> they can't
> - * receive VLAN configuration when enslaved to a bridge with
> vlan_filtering=0
> - */
> -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp)
> -{
> -       struct net_device *br = dsa_port_bridge_dev_get(dp);
> -       struct dsa_switch *ds = dp->ds;
> -
> -       if (!br)
> -               return false;
> -
> -       return !ds->configure_vlan_while_not_filtering &&
> !br_vlan_enabled(br);
> -}
> -
>  int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock)
>  {
>         unsigned long ageing_jiffies =
> clock_t_to_jiffies(ageing_clock);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index ad6a6663feeb..d1284f8684d8 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -533,11 +533,6 @@ static int dsa_slave_vlan_add(struct net_device
> *dev,
>         struct switchdev_obj_port_vlan *vlan;
>         int err;
> 
> -       if (dsa_port_skip_vlan_configuration(dp)) {
> -               NL_SET_ERR_MSG_MOD(extack, "skipping configuration of
> VLAN");
> -               return 0;
> -       }
> -
>         vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> 
>         /* Deny adding a bridge VLAN when there is already an 802.1Q
> upper with
> @@ -571,11 +566,6 @@ static int dsa_slave_host_vlan_add(struct
> net_device *dev,
>         if (!dp->bridge)
>                 return -EOPNOTSUPP;
> 
> -       if (dsa_port_skip_vlan_configuration(dp)) {
> -               NL_SET_ERR_MSG_MOD(extack, "skipping configuration of
> VLAN");
> -               return 0;
> -       }
> -
>         vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
> 
>         /* Even though drivers often handle CPU membership in special
> ways,
> @@ -642,9 +632,6 @@ static int dsa_slave_vlan_del(struct net_device
> *dev,
>         struct dsa_port *dp = dsa_slave_to_port(dev);
>         struct switchdev_obj_port_vlan *vlan;
> 
> -       if (dsa_port_skip_vlan_configuration(dp))
> -               return 0;
> -
>         vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> 
>         return dsa_port_vlan_del(dp, vlan);
> @@ -660,9 +647,6 @@ static int dsa_slave_host_vlan_del(struct
> net_device *dev,
>         if (!dp->bridge)
>                 return -EOPNOTSUPP;
> 
> -       if (dsa_port_skip_vlan_configuration(dp))
> -               return 0;
> -
>         vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
> 
>         return dsa_port_host_vlan_del(dp, vlan);
> @@ -1655,11 +1639,7 @@ static int dsa_slave_clear_vlan(struct
> net_device *vdev, int vid, void *arg)
>   *         - no VLAN (any 8021q upper is a software VLAN)
>   *
>   * - If under a vlan_filtering=0 bridge which it offload:
> - *     - if ds->configure_vlan_while_not_filtering = true (default):
> - *         - the bridge VLANs. These VLANs are committed to hardware
> but inactive.
> - *     - else (deprecated):
> - *         - no VLAN. The bridge VLANs are not restored when VLAN
> awareness is
> - *           enabled, so this behavior is broken and discouraged.
> + *     - the bridge VLANs. These VLANs are committed to hardware but
> inactive.
>   *
>   * - If under a vlan_filtering=1 bridge which it offload:
>   *     - the bridge VLANs
> --
> 2.25.1
> 

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

[Vladimir Oltean]

Hi Arun,

On Wed, Jul 06, 2022 at 10:51:34AM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Vladimir,
> 
> On Tue, 2022-07-05 at 20:31 +0300, Vladimir Oltean wrote:
> > - For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the
> >   bridge VLAN add/del handlers, so there is no logic to update it on
> >   VLAN awareness change. I don't know exactly how the switch behaves
> >   after ksz9477_port_vlan_filtering() is called with "false".
> >   If packets are classified to the REG_PORT_DEFAULT_VID, then it is
> >   broken. Similar thing can be said for KSZ8.
> 
> When ksz9477_port_vlan_filtering is set to false, then it clears the
> 802.1Q vlan enable bit in the switch. So the packets are not classified
> based on vlan tag. Only if the vlan bit is set, packets are classified
> based on pvid.

So bit 7 of the global Switch Lookup Engine Control 0 Register says:

802.1Q VLAN Enable
This is the master enable for VLAN forwarding and filtering. Note that the
VLAN Table must be set up before VLAN mode is enabled.
1 = VLAN mode enabled
0 = VLAN mode disabled

I'm not completely clear, personally, from the chosen wording.

More interesting is the definition for the Port Control 1 Register,
where it says

Port VLAN Membership
Each bit corresponds to a device port. This feature does not utilize VLAN
tags or the VLAN Table, and is unrelated to tag-based VLAN functions. Also
refer to bit 1 in the Queue Management Control 0 Register.
Bit 0 is for port 1
Bit 1 is for port 2, etc.
1 = Frames may be forwarded to the corresponding port
0 = Frames are blocked from being forwarded to corresponding port

So you may be right, if the VLAN mode is disabled, the packet is
forwarded within the "port VLAN". It would be good to check,
nonetheless.

> > In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the
> > pvid at 0 when VLAN-unaware") for an example of how to deal with the
> > problem, and test pvid_change() in
> > tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for
> > how
> > to check whether the driver behaves correctly. I don't have the
> > hardware
> > to test any changes.
> 
> I can test it and report the observation. But I haven't run the
> selftests before. I looked in the scripts today, but couldn't find out
> how to compile it as part of kernel and program it to the target &
> test. I infer that, this scripts should be run on target (I have
> SAMA5D3 + KSZ9477) with 4 switch ports. Can you guide me on testing
> this.

To be honest I've no idea how to run the kselftests "correctly" either,
I just rsync -avr tools/testing/selftests/ root@<board-ip>:selftests/,
then install the packages I need (running the selftest usually shows
what those are), then run the test itself:

$ cd selftests/drivers/net/dsa/ocelot
$ ./bridge_vlan_unaware.sh lan1 lan2 lan3 lan4

The topology for most of these selftests is pretty standard. There are 2
host interfaces $h1 and $h2 (the first and the last one), and 2 switch
interfaces $swp1 and $swp2 (the middle 2). There is supposed to be a
loopback cable between $h1 and $swp1, and a cable between $swp2 and $h2.
The $h1 and $h2 can be any physical Ethernet interfaces, $swp1 and $swp2
need to be switchdev.

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

[Martin Blumenstingl]

Hi Vladimir,

On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> Stop protecting DSA drivers from switchdev VLAN notifications emitted
> while the bridge has vlan_filtering 0, by deleting the deprecated bool
> ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see
> all notifications and should save the bridge PVID until they need to
> commit it to hardware.
>
> The 2 remaining unconverted drivers are the gswip and the Microchip KSZ
> family. They are both probably broken and would need changing as far as
> I can see:
>
> - For lantiq_gswip, after the initial call path
>   -> gswip_port_bridge_join
>      -> gswip_vlan_add_unaware
>         -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
>   nobody seems to prevent a future call path
>   -> gswip_port_vlan_add
>      -> gswip_vlan_add_aware
>         -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));
Thanks for bringing this to my attention!

I tried to reproduce this issue with the selftest script you provided
(patch #1 in this series).
Unfortunately not even the ping_ipv4 and ping_ipv6 tests from
bridge_vlan_unaware.sh are working for me, nor are the tests from
router_bridge.sh.
I suspect that this is an issue with OpenWrt: I already enabled bash,
jq and the full ip package, vrf support in the kernel. OpenWrt's ping
command doesn't like a ping interval of 0.1s so I replaced that with
an 1s interval.

I will try to get the selftests to work here but I think that
shouldn't block this patch.


Best regards,
Martin

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

[Vladimir Oltean]

Hi Martin,

On Wed, Jul 06, 2022 at 06:33:18PM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> >
> > Stop protecting DSA drivers from switchdev VLAN notifications emitted
> > while the bridge has vlan_filtering 0, by deleting the deprecated bool
> > ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see
> > all notifications and should save the bridge PVID until they need to
> > commit it to hardware.
> >
> > The 2 remaining unconverted drivers are the gswip and the Microchip KSZ
> > family. They are both probably broken and would need changing as far as
> > I can see:
> >
> > - For lantiq_gswip, after the initial call path
> >   -> gswip_port_bridge_join
> >      -> gswip_vlan_add_unaware
> >         -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
> >   nobody seems to prevent a future call path
> >   -> gswip_port_vlan_add
> >      -> gswip_vlan_add_aware
> >         -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));
> Thanks for bringing this to my attention!
> 
> I tried to reproduce this issue with the selftest script you provided
> (patch #1 in this series).
> Unfortunately not even the ping_ipv4 and ping_ipv6 tests from
> bridge_vlan_unaware.sh are working for me, nor are the tests from
> router_bridge.sh.
> I suspect that this is an issue with OpenWrt: I already enabled bash,
> jq and the full ip package, vrf support in the kernel. OpenWrt's ping
> command doesn't like a ping interval of 0.1s so I replaced that with
> an 1s interval.
> 
> I will try to get the selftests to work here but I think that
> shouldn't block this patch.

Thanks for the willingness to test!

Somehow we should do something to make sure that the OpenWRT devices are
able to run the selftests, because there's a large number of DSA switches
intended for that segment and we should all be onboard (easily).

I wonder, would it be possible to set up a debian chroot?

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

[Martin Blumenstingl]

Hi Vladimir,

On Wed, Jul 6, 2022 at 6:45 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
[...]
> Somehow we should do something to make sure that the OpenWRT devices are
> able to run the selftests, because there's a large number of DSA switches
> intended for that segment and we should all be onboard (easily).
I could work on this on the OpenWrt side.
But this would require me to get any test working at all...

I am struggling with not seeing any ping responses.
Instead of adding VLANs and others to the mix I started with seemingly
simple commands while connecting an Ethernet cable between lan1 and
lan2 and another cable between lan3 and lan4:
1) give each port a unique MAC address, which is not the default on my
device under test
ip link set dev lan1 address 6a:88:f1:99:e1:81
ip link set dev lan2 address 6a:88:f1:99:e1:82
ip link set dev lan3 address 6a:88:f1:99:e1:83
ip link set dev lan4 address 6a:88:f1:99:e1:84

2) set up IP addresses on LAN1 and LAN2
ip addr add 192.168.2.1/24 brd + dev lan1
ip addr add 192.168.2.2/24 brd + dev lan2

3) bring up the interfaces
ip link set up dev lan1
ip link set up dev lan2

4) start tcpdump on LAN1
tcpdump -i lan1 &

5) start ping from LAN2 to LAN1
ping -I lan2 192.168.2.1

result:
PING 192.168.2.1 (192.168.2.1): 56 data bytes
20:02:01.522977 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
20:02:02.569234 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
20:02:03.609132 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
20:02:05.524200 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
20:02:06.569226 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
...repeats...

So LAN1 can see the ARP request from the ping on LAN2.
But I am not seeing Linux trying to send a reply.

I already verified that nftables doesn't have any rules active (if it
was I think it would rather result in tcpdump not seeing the who-has
request):
# nft list tables
#

# grep "" /proc/sys/net/ipv4/icmp_echo_ignore_*
/proc/sys/net/ipv4/icmp_echo_ignore_all:0
/proc/sys/net/ipv4/icmp_echo_ignore_broadcasts:1

# ps | grep netif
3014 root      1308 S    grep netif
#

To make things worse: if I let OpenWrt's netifd configure LAN1 and
LAN2 as single ports with above IP addresses ping works.
Something is odd and I am not sure how to find out what's wrong.

> I wonder, would it be possible to set up a debian chroot?
I'm thinking of packaging the selftests as OpenWrt package and also
providing all needed dependencies as OpenWrt packages.
I think (or I hope, not sure yet) the ping interval is just a matter
of a busybox config option.


Best regards,
Martin

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

[Vladimir Oltean]

Hi Martin,

On Wed, Jul 06, 2022 at 09:57:40PM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Wed, Jul 6, 2022 at 6:45 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> [...]
> > Somehow we should do something to make sure that the OpenWRT devices are
> > able to run the selftests, because there's a large number of DSA switches
> > intended for that segment and we should all be onboard (easily).
> 
> I could work on this on the OpenWrt side.
> But this would require me to get any test working at all...
> 
> I am struggling with not seeing any ping responses.
> Instead of adding VLANs and others to the mix I started with seemingly
> simple commands while connecting an Ethernet cable between lan1 and
> lan2 and another cable between lan3 and lan4:
> 1) give each port a unique MAC address, which is not the default on my
> device under test
> ip link set dev lan1 address 6a:88:f1:99:e1:81
> ip link set dev lan2 address 6a:88:f1:99:e1:82
> ip link set dev lan3 address 6a:88:f1:99:e1:83
> ip link set dev lan4 address 6a:88:f1:99:e1:84
> 
> 2) set up IP addresses on LAN1 and LAN2
> ip addr add 192.168.2.1/24 brd + dev lan1
> ip addr add 192.168.2.2/24 brd + dev lan2
> 
> 3) bring up the interfaces
> ip link set up dev lan1
> ip link set up dev lan2
> 
> 4) start tcpdump on LAN1
> tcpdump -i lan1 &
> 
> 5) start ping from LAN2 to LAN1
> ping -I lan2 192.168.2.1
> 
> result:
> PING 192.168.2.1 (192.168.2.1): 56 data bytes
> 20:02:01.522977 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
> 20:02:02.569234 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
> 20:02:03.609132 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
> 20:02:05.524200 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
> 20:02:06.569226 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46
> ...repeats...
> 
> So LAN1 can see the ARP request from the ping on LAN2.
> But I am not seeing Linux trying to send a reply.

It won't reply, you either need a network namespace or a VRF to do
loopback IP networking. A VRF is a bit more complicated to do, here's a
netns setup:

ip netns add ns0
ip link set lan2 netns ns0
ip -n ns0 link set lan2 up
ip -n ns0 addr add 192.168.2.2/24 dev lan2
ip netns exec ns0 tcpdump -i lan2 -e -n
ping 192.168.2.2

> I already verified that nftables doesn't have any rules active (if it
> was I think it would rather result in tcpdump not seeing the who-has
> request):
> # nft list tables
> #
> 
> # grep "" /proc/sys/net/ipv4/icmp_echo_ignore_*
> /proc/sys/net/ipv4/icmp_echo_ignore_all:0
> /proc/sys/net/ipv4/icmp_echo_ignore_broadcasts:1
> 
> # ps | grep netif
> 3014 root      1308 S    grep netif
> #
> 
> To make things worse: if I let OpenWrt's netifd configure LAN1 and
> LAN2 as single ports with above IP addresses ping works.
> Something is odd and I am not sure how to find out what's wrong.

I'm not familiar with OpenWrt, sorry, I don't know what netifd does.
Also, it's curious that this works, are you sure that the ARP responses
and ICMP replies actually exit through the Ethernet port? ethtool -S
should show if the physical counters increment.

> > I wonder, would it be possible to set up a debian chroot?
> 
> I'm thinking of packaging the selftests as OpenWrt package and also
> providing all needed dependencies as OpenWrt packages.
> I think (or I hope, not sure yet) the ping interval is just a matter
> of a busybox config option.

I think it depends on busybox version. At least the latest one
https://github.com/mirror/busybox/blob/master/networking/ping.c#L970
seems to support fractions of a second as intervals, I didn't see any
restriction to sub-second values. In any case, the iputils version
certainly does work.

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

[Martin Blumenstingl]

Hi Vladimir,

On Fri, Jul 8, 2022 at 12:31 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
[...]
> > So LAN1 can see the ARP request from the ping on LAN2.
> > But I am not seeing Linux trying to send a reply.
>
> It won't reply, you either need a network namespace or a VRF to do
> loopback IP networking. A VRF is a bit more complicated to do, here's a
> netns setup:
>
> ip netns add ns0
> ip link set lan2 netns ns0
> ip -n ns0 link set lan2 up
> ip -n ns0 addr add 192.168.2.2/24 dev lan2
> ip netns exec ns0 tcpdump -i lan2 -e -n
> ping 192.168.2.2
This does indeed work!
That made me look at another selftest and indeed: most of the
local_termination.sh tests are passing (albeit after having to make
some changes to the selftest scripts, I'll provide patches for these
soon)

None (zero) of the tests from bridge_vlan_unaware.sh and only a single
test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing
& roaming") are passing for me on GSWIP.
Also most of the ethtool.sh tests are failing (ping always reports
"Destination Host Unreachable").
I guess most (or at least more) of these are supposed to pass? Do you
want me to open another thread for this or is it fine to reply here?

[...]
> I'm not familiar with OpenWrt, sorry, I don't know what netifd does.
netifd is the network configuration daemon, it takes the network
configuration (from OpenWrts configuration files/format) and sets up
the corresponding interfaces and manages things like pppoe.

> Also, it's curious that this works, are you sure that the ARP responses
> and ICMP replies actually exit through the Ethernet port? ethtool -S
> should show if the physical counters increment.
Since it works with your example and I got the first selftests to pass
I'll skip further investigation here

> > > I wonder, would it be possible to set up a debian chroot?
> >
> > I'm thinking of packaging the selftests as OpenWrt package and also
> > providing all needed dependencies as OpenWrt packages.
> > I think (or I hope, not sure yet) the ping interval is just a matter
> > of a busybox config option.
>
> I think it depends on busybox version. At least the latest one
> https://github.com/mirror/busybox/blob/master/networking/ping.c#L970
> seems to support fractions of a second as intervals, I didn't see any
> restriction to sub-second values. In any case, the iputils version
> certainly does work.
Yes, there's a duration library inside busybox which by default only
takes integer values (it can be configured to use floats though).
I pushed my work in progress OpenWrt package to a branch, making use
of the iputils version: [0]
Compressed initramfs size is below 10M and uncompressed at 22M. My
device under test has 128M of RAM and that seems to be enough to run
mausezahn as well as any other tool that was run so far. So I am not
particularly concerned about storage size (anything with 32M flash or
more will do - 16M could be a bit tight in the end but will still work
I guess).


Best regards,
Martin


[0] https://github.com/xdarklight/openwrt-packages/commits/wip-kernel-selftests-net-forwarding-20220707

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

[Vladimir Oltean]

On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl wrote:
> That made me look at another selftest and indeed: most of the
> local_termination.sh tests are passing (albeit after having to make
> some changes to the selftest scripts, I'll provide patches for these
> soon)
> 
> None (zero) of the tests from bridge_vlan_unaware.sh and only a single
> test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing
> & roaming") are passing for me on GSWIP.
> Also most of the ethtool.sh tests are failing (ping always reports
> "Destination Host Unreachable").

I don't recall having run ethtool.sh, so I don't know what's the status
there.

> I guess most (or at least more) of these are supposed to pass? Do you
> want me to open another thread for this or is it fine to reply here?

So yes, the intention is for the selftests to pass, but I wouldn't be
surprised if they don't. They didn't when I started this effort for the
ocelot/felix DSA driver either, it's most likely that individual drivers
will need changes, that's kind of the whole point of having selftests,
to have implementations that are uniform in terms of behavior.
For the ocelot driver, the tests symlinked in the DSA folder do pass
(with the exception of the locked port test, which isn't implemented,
and the bridge local_termination.sh tests, but that's a bridge problem
and not a driver problem). I should have a remote setup and I should be
able to repeat some tests if necessary.

I think it would be a good idea to create a new email thread with the
relevant DSA maintainers for selftest status on GSWIP. You'll need to
gather some information on what exactly fails when things fail.
The way I prefer to do this is to run the test itself with "bash -x
./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where
things get stuck, then edit the script, put a "bash" command after the
failing portion, and run the selftest again; this gives me a subshell
with all the VRFs configured from which I have more control and can
re-run the commands that just failed (I copy them from right above,
they're visible when run with bash -x). Then I try to manually check
counters, tcpdump, things like that.

> > I'm not familiar with OpenWrt, sorry, I don't know what netifd does.
> netifd is the network configuration daemon, it takes the network
> configuration (from OpenWrts configuration files/format) and sets up
> the corresponding interfaces and manages things like pppoe.

Thanks for the summary, I meant that I don't know what netifd does to
make loopback IP networking work in this case. Anyway it's not the
center point of the topic, as long as the interfaces used by the
kselftests are not managed by netifd or another network management
program.

> > > I'm thinking of packaging the selftests as OpenWrt package and also
> > > providing all needed dependencies as OpenWrt packages.
> > > I think (or I hope, not sure yet) the ping interval is just a matter
> > > of a busybox config option.
> >
> > I think it depends on busybox version. At least the latest one
> > https://github.com/mirror/busybox/blob/master/networking/ping.c#L970
> > seems to support fractions of a second as intervals, I didn't see any
> > restriction to sub-second values. In any case, the iputils version
> > certainly does work.
> Yes, there's a duration library inside busybox which by default only
> takes integer values (it can be configured to use floats though).
> I pushed my work in progress OpenWrt package to a branch, making use
> of the iputils version: [0]
> Compressed initramfs size is below 10M and uncompressed at 22M. My
> device under test has 128M of RAM and that seems to be enough to run
> mausezahn as well as any other tool that was run so far. So I am not
> particularly concerned about storage size (anything with 32M flash or
> more will do - 16M could be a bit tight in the end but will still work
> I guess).
> 
> 
> Best regards,
> Martin
> 
> 
> [0] https://github.com/xdarklight/openwrt-packages/commits/wip-kernel-selftests-net-forwarding-20220707

Sounds good then.

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

[Martin Blumenstingl]

Hi Vladimir,

On Fri, Jul 8, 2022 at 2:09 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl wrote:
> > That made me look at another selftest and indeed: most of the
> > local_termination.sh tests are passing (albeit after having to make
> > some changes to the selftest scripts, I'll provide patches for these
> > soon)
> >
> > None (zero) of the tests from bridge_vlan_unaware.sh and only a single
> > test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing
> > & roaming") are passing for me on GSWIP.
> > Also most of the ethtool.sh tests are failing (ping always reports
> > "Destination Host Unreachable").
>
> I don't recall having run ethtool.sh, so I don't know what's the status
> there.
OK, no worries there

> > I guess most (or at least more) of these are supposed to pass? Do you
> > want me to open another thread for this or is it fine to reply here?
>
> So yes, the intention is for the selftests to pass, but I wouldn't be
> surprised if they don't. They didn't when I started this effort for the
> ocelot/felix DSA driver either, it's most likely that individual drivers
> will need changes, that's kind of the whole point of having selftests,
> to have implementations that are uniform in terms of behavior.
> For the ocelot driver, the tests symlinked in the DSA folder do pass
> (with the exception of the locked port test, which isn't implemented,
> and the bridge local_termination.sh tests, but that's a bridge problem
> and not a driver problem). I should have a remote setup and I should be
> able to repeat some tests if necessary.
>
> I think it would be a good idea to create a new email thread with the
> relevant DSA maintainers for selftest status on GSWIP. You'll need to
> gather some information on what exactly fails when things fail.
> The way I prefer to do this is to run the test itself with "bash -x
> ./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where
> things get stuck, then edit the script, put a "bash" command after the
> failing portion, and run the selftest again; this gives me a subshell
> with all the VRFs configured from which I have more control and can
> re-run the commands that just failed (I copy them from right above,
> they're visible when run with bash -x). Then I try to manually check
> counters, tcpdump, things like that.
I already found "bash -x" and used a similar trick (launching tcpdump
before the failing portion). But it's good to have it written down!
Thanks a lot again for all your detailed explanations and the time
you've taken to help me out!
I'll start a new thread on this in the next few days.


Best regards,
Martin

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

[Arun.Ramadoss]

Hi Vladimir,
We couldn't able to setup the selftests and failed during installation
of packages. In the mean time, We tried the following things

Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from
Host1 and received by Host2.

Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid
ip link set dev br0 type bridge vlan_filtering 1
bridge vlan add dev lan2 vid 10 pvid untagged
bridge vlan add dev lan1 vid 10 pvid untagged

Packet transmitted from Host1 with vid 10 is received by the Host2.
Packet transmitted from Host1 with vid 5 is not received by the Host2.

Scenario-2: Vlan unaware system 
ip link set dev br0 type bridge vlan_filtering 0

Now, irrespective of the vid, the packets are received by Host2
Packet transmitted from Host1 with vid 10 is received by the Host2.
Packet transmitted from Host1 with vid 5 is  received by the Host2.

Whether the above approach is correct or do we need to test anything
further.

Thanks
Arun 
On Sat, 2022-07-09 at 00:27 +0200, Martin Blumenstingl wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Vladimir,
> 
> On Fri, Jul 8, 2022 at 2:09 PM Vladimir Oltean <
> vladimir.oltean@nxp.com> wrote:
> > 
> > On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl
> > wrote:
> > > That made me look at another selftest and indeed: most of the
> > > local_termination.sh tests are passing (albeit after having to
> > > make
> > > some changes to the selftest scripts, I'll provide patches for
> > > these
> > > soon)
> > > 
> > > None (zero) of the tests from bridge_vlan_unaware.sh and only a
> > > single
> > > test from bridge_vlan_aware.sh ("Externally learned FDB entry -
> > > ageing
> > > & roaming") are passing for me on GSWIP.
> > > Also most of the ethtool.sh tests are failing (ping always
> > > reports
> > > "Destination Host Unreachable").
> > 
> > I don't recall having run ethtool.sh, so I don't know what's the
> > status
> > there.
> 
> OK, no worries there
> 
> > > I guess most (or at least more) of these are supposed to pass? Do
> > > you
> > > want me to open another thread for this or is it fine to reply
> > > here?
> > 
> > So yes, the intention is for the selftests to pass, but I wouldn't
> > be
> > surprised if they don't. They didn't when I started this effort for
> > the
> > ocelot/felix DSA driver either, it's most likely that individual
> > drivers
> > will need changes, that's kind of the whole point of having
> > selftests,
> > to have implementations that are uniform in terms of behavior.
> > For the ocelot driver, the tests symlinked in the DSA folder do
> > pass
> > (with the exception of the locked port test, which isn't
> > implemented,
> > and the bridge local_termination.sh tests, but that's a bridge
> > problem
> > and not a driver problem). I should have a remote setup and I
> > should be
> > able to repeat some tests if necessary.
> > 
> > I think it would be a good idea to create a new email thread with
> > the
> > relevant DSA maintainers for selftest status on GSWIP. You'll need
> > to
> > gather some information on what exactly fails when things fail.
> > The way I prefer to do this is to run the test itself with "bash -x
> > ./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where
> > things get stuck, then edit the script, put a "bash" command after
> > the
> > failing portion, and run the selftest again; this gives me a
> > subshell
> > with all the VRFs configured from which I have more control and can
> > re-run the commands that just failed (I copy them from right above,
> > they're visible when run with bash -x). Then I try to manually
> > check
> > counters, tcpdump, things like that.
> 
> I already found "bash -x" and used a similar trick (launching tcpdump
> before the failing portion). But it's good to have it written down!
> Thanks a lot again for all your detailed explanations and the time
> you've taken to help me out!
> I'll start a new thread on this in the next few days.
> 
> 
> Best regards,
> Martin

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

[Vladimir Oltean]

Hi Arun,

On Thu, Jul 14, 2022 at 10:46:02AM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Vladimir,
> We couldn't able to setup the selftests and failed during installation
> of packages. In the mean time, We tried the following things
> 
> Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from
> Host1 and received by Host2.
> 
> Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid
> ip link set dev br0 type bridge vlan_filtering 1
> bridge vlan add dev lan2 vid 10 pvid untagged
> bridge vlan add dev lan1 vid 10 pvid untagged
> 
> Packet transmitted from Host1 with vid 10 is received by the Host2.
> Packet transmitted from Host1 with vid 5 is not received by the Host2.
> 
> Scenario-2: Vlan unaware system 
> ip link set dev br0 type bridge vlan_filtering 0
> 
> Now, irrespective of the vid, the packets are received by Host2
> Packet transmitted from Host1 with vid 10 is received by the Host2.
> Packet transmitted from Host1 with vid 5 is  received by the Host2.
> 
> Whether the above approach is correct or do we need to test anything
> further.
> 
> Thanks
> Arun 

The above is correct to the extent that it is a valid configuration,
but isn't what my pvid_change() selftest was intended to capture.

The pvid_change() selftest from patch 1/3
https://patchwork.kernel.org/project/netdevbpf/patch/20220705173114.2004386-2-vladimir.oltean@nxp.com/
checks that VLAN-unaware forwarding still takes place after this array
of operations:

ip link add br0 type bridge vlan_filtering 0 # notice the 0 instead of 1
ip link set $swp1 master br0
ip link set $swp2 master br0
bridge vlan add vid 3 dev $swp1 pvid untagged # notice how VID 3 is absent on $swp2

If you let me know if this works, I can continue and resend this patch
set while you figure out the kselftest setup issues.

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

[Arun.Ramadoss]

Hi Vladimir,

On Thu, 2022-07-14 at 15:12 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Arun,
> 
> On Thu, Jul 14, 2022 at 10:46:02AM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > Hi Vladimir,
> > We couldn't able to setup the selftests and failed during
> > installation
> > of packages. In the mean time, We tried the following things
> > 
> > Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from
> > Host1 and received by Host2.
> > 
> > Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid
> > ip link set dev br0 type bridge vlan_filtering 1
> > bridge vlan add dev lan2 vid 10 pvid untagged
> > bridge vlan add dev lan1 vid 10 pvid untagged
> > 
> > Packet transmitted from Host1 with vid 10 is received by the Host2.
> > Packet transmitted from Host1 with vid 5 is not received by the
> > Host2.
> > 
> > Scenario-2: Vlan unaware system
> > ip link set dev br0 type bridge vlan_filtering 0
> > 
> > Now, irrespective of the vid, the packets are received by Host2
> > Packet transmitted from Host1 with vid 10 is received by the Host2.
> > Packet transmitted from Host1 with vid 5 is  received by the Host2.
> > 
> > Whether the above approach is correct or do we need to test
> > anything
> > further.
> > 
> > Thanks
> > Arun
> 
> The above is correct to the extent that it is a valid configuration,
> but isn't what my pvid_change() selftest was intended to capture.
> 
> The pvid_change() selftest from patch 1/3
> 
https://patchwork.kernel.org/project/netdevbpf/patch/20220705173114.2004386-2-vladimir.oltean@nxp.com/
> checks that VLAN-unaware forwarding still takes place after this
> array
> of operations:
> 
> ip link add br0 type bridge vlan_filtering 0 # notice the 0 instead
> of 1
> ip link set $swp1 master br0
> ip link set $swp2 master br0
> bridge vlan add vid 3 dev $swp1 pvid untagged # notice how VID 3 is
> absent on $swp2
> 
> If you let me know if this works, I can continue and resend this
> patch
> set while you figure out the kselftest setup issues.
We tried the following test

ip link set dev br0 type bridge vlan_filtering 0

ip link set lan1 master br0
ip link set lan2 master br0

bridge vlan add vid 10 dev lan1 pvid untagged

==>
Packet transmitted from Host1 with vid 5 is not received by the Host2 
Packet transmitted from Host1 with vid 10 is not received by the Host2
==> 

bridge vlan add vid 10 dev lan2 pvid untagged

==>
Packet transmitted from Host1 with vid 5 is received by the Host2 
Pa
cket transmitted from Host1 with vid 10 is received by the Host2
==> 

bridge vlan del vid 10 dev lan2

==>
Packet transmitted from Host1 with vid 5 is not received by the Host2 
Packet transmitted from Host1 with vid 10 is not received by the Host2
==> 

Tried this test before and after applying this patch series. And got
the same result.

In summary, packets are dropped when pvid is added to vlan unaware
bridge. Let me know if anything need to performed on this.

Thanks
Arun 

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

[Vladimir Oltean]

On Fri, Jul 15, 2022 at 09:23:19AM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Vladimir,
> 
> We tried the following test
> 
> ip link set dev br0 type bridge vlan_filtering 0
> 
> ip link set lan1 master br0
> ip link set lan2 master br0
> 
> bridge vlan add vid 10 dev lan1 pvid untagged
> 
> ==>
> Packet transmitted from Host1 with vid 5 is not received by the Host2 
> Packet transmitted from Host1 with vid 10 is not received by the Host2
> ==> 
> 
> bridge vlan add vid 10 dev lan2 pvid untagged
> 
> ==>
> Packet transmitted from Host1 with vid 5 is received by the Host2 
> Pa
> cket transmitted from Host1 with vid 10 is received by the Host2
> ==> 
> 
> bridge vlan del vid 10 dev lan2
> 
> ==>
> Packet transmitted from Host1 with vid 5 is not received by the Host2 
> Packet transmitted from Host1 with vid 10 is not received by the Host2
> ==> 
> 
> Tried this test before and after applying this patch series. And got
> the same result.
> 
> In summary, packets are dropped when pvid is added to vlan unaware
> bridge. Let me know if anything need to performed on this.

I'm not surprised that forwarding is broken after removing
"ds->configure_vlan_while_not_filtering = false", but I'm surprised that
it's broken even without the change. That suggests that either the flag
wasn't effective in the first place, or that the breakage is caused by
other code paths (not sure which).

Do you get the "skipping configuration of VLAN" warning extack when you
run the "bridge vlan add" command without the patches here? Does
ksz_port_vlan_add() get called at all with VID 10?

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

[Arun.Ramadoss]

Hi Vladimir,

On Fri, 2022-07-15 at 15:26 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jul 15, 2022 at 09:23:19AM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > Hi Vladimir,
> > 
> > We tried the following test
> > 
> > ip link set dev br0 type bridge vlan_filtering 0
> > 
> > ip link set lan1 master br0
> > ip link set lan2 master br0
> > 
> > bridge vlan add vid 10 dev lan1 pvid untagged
> > 
> > ==>
> > Packet transmitted from Host1 with vid 5 is not received by the
> > Host2
> > Packet transmitted from Host1 with vid 10 is not received by the
> > Host2
> > ==>
> > 
> > bridge vlan add vid 10 dev lan2 pvid untagged
> > 
> > ==>
> > Packet transmitted from Host1 with vid 5 is received by the Host2
> > Pa
> > cket transmitted from Host1 with vid 10 is received by the Host2
> > ==>
> > 
> > bridge vlan del vid 10 dev lan2
> > 
> > ==>
> > Packet transmitted from Host1 with vid 5 is not received by the
> > Host2
> > Packet transmitted from Host1 with vid 10 is not received by the
> > Host2
> > ==>
> > 
> > Tried this test before and after applying this patch series. And
> > got
> > the same result.
> > 
> > In summary, packets are dropped when pvid is added to vlan unaware
> > bridge. Let me know if anything need to performed on this.
> 
> I'm not surprised that forwarding is broken after removing
> "ds->configure_vlan_while_not_filtering = false", but I'm surprised
> that
> it's broken even without the change. That suggests that either the
> flag
> wasn't effective in the first place, or that the breakage is caused
> by
> other code paths (not sure which).
> 
> Do you get the "skipping configuration of VLAN" warning extack when
> you
> run the "bridge vlan add" command without the patches here? Does
> ksz_port_vlan_add() get called at all with VID 10?

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

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.

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

bridge vlan add vid 10 dev lan2 pvid untagged

--> packet is received in the Host2

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.

Thanks
Arun 

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

[Vladimir Oltean]

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!

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

[Arun.Ramadoss]

Hi Vladimir,
On Mon, 2022-07-18 at 16:24 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 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!
I tried to update the ksz code and tested after applying this patch
series. Following are the observation,

Scenario 1: bridge_vlan_unaware default pvid = 1
---------------------------------
ip link set dev br0 type bridge vlan_filtering 0

--> packet is received in the Host 2                       

bridge vlan add vid 10 dev lan1 pvid untagged

--> packet is received in the Host 2

bridge vlan add vid 10 dev lan2 pvid untagged

--> packet is received in the Host 2

bridge vlan del vid 10 dev lan1

--> packet is received in the Host 2

bridge vlan del vid 10 dev lan2

--> packet is received in the Host2

Scenario 2: bridge_vlan_unaware default pvid other than 1
---------------------------------
ip link set dev br0 type bridge vlan_filtering 0

--> packet is not received in the Host 2. Only after enabling the
vlan_filtering and bridge vlan add packets are receiving.

ip link set dev br0 type bridge vlan_filtering 1
bridge vlan add vid 10 dev lan2 pvid untagged   

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.

Updated code patch
------------------
diff --git a/drivers/net/dsa/microchip/ksz8.h
b/drivers/net/dsa/microchip/ksz8.h
index 42c50cc4d853..e8e1590a78c0 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -43,6 +43,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int
port,
 		       struct netlink_ext_ack *extack);
 int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 		       const struct switchdev_obj_port_vlan *vlan);
+void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool
state);
 int ksz8_port_mirror_add(struct ksz_device *dev, int port,
 			 struct dsa_mall_mirror_tc_entry *mirror,
 			 bool ingress, struct netlink_ext_ack *extack);
diff --git a/drivers/net/dsa/microchip/ksz8795.c
b/drivers/net/dsa/microchip/ksz8795.c
index c79a5128235f..4f4e26001957 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -952,7 +952,7 @@ int ksz8_port_vlan_filtering(struct ksz_device
*dev, int port, bool flag,
 	return 0;
 }
 
-static void ksz8_port_enable_pvid(struct ksz_device *dev, int port,
bool state)
+void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool
state)
 {
 	if (ksz_is_ksz88x3(dev)) {
 		ksz_cfg(dev, REG_SW_INSERT_SRC_PVID,
@@ -968,8 +968,8 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int
port,
 {
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	struct ksz_port *p = &dev->ports[port];
-	u16 data, new_pvid = 0;
 	u8 fid, member, valid;
+	u16 data;
 
 	if (ksz_is_ksz88x3(dev))
 		return -ENOTSUPP;
@@ -1016,36 +1016,18 @@ int ksz8_port_vlan_add(struct ksz_device *dev,
int port,
 	ksz8_to_vlan(dev, fid, member, valid, &data);
 	ksz8_w_vlan_table(dev, vlan->vid, data);
 
-	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		new_pvid = vlan->vid;
-
-	if (new_pvid) {
-		u16 vid;
-
-		ksz_pread16(dev, port, REG_PORT_CTRL_VID, &vid);
-		vid &= ~VLAN_VID_MASK;
-		vid |= new_pvid;
-		ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid);
-
-		ksz8_port_enable_pvid(dev, port, true);
-	}
-
 	return 0;
 }
 
 int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 		       const struct switchdev_obj_port_vlan *vlan)
 {
-	u16 data, pvid;
+	u16 data;
 	u8 fid, member, valid;
 
 	if (ksz_is_ksz88x3(dev))
 		return -ENOTSUPP;
 
-	ksz_pread16(dev, port, REG_PORT_CTRL_VID, &pvid);
-	pvid = pvid & 0xFFF;
-
 	ksz8_r_vlan_table(dev, vlan->vid, &data);
 	ksz8_from_vlan(dev, data, &fid, &member, &valid);
 
@@ -1060,9 +1042,6 @@ int ksz8_port_vlan_del(struct ksz_device *dev,
int port,
 	ksz8_to_vlan(dev, fid, member, valid, &data);
 	ksz8_w_vlan_table(dev, vlan->vid, data);
 
-	if (pvid == vlan->vid)
-		ksz8_port_enable_pvid(dev, port, false);
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c
b/drivers/net/dsa/microchip/ksz9477.c
index 4b14d80d27ed..6a39d6893142 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -354,6 +354,12 @@ void ksz9477_flush_dyn_mac_table(struct ksz_device
*dev, int port)
 	}
 }
 
+void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool
state)
+{
+	ksz_port_cfg(dev, port, REG_PORT_MRI_MAC_CTRL,
PORT_DROP_NON_VLAN,
+		     state);
+}
+
 int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port,
 				bool flag, struct netlink_ext_ack
*extack)
 {
@@ -399,10 +405,6 @@ int ksz9477_port_vlan_add(struct ksz_device *dev,
int port,
 		return err;
 	}
 
-	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan-
>vid);
-
 	return 0;
 }
 
@@ -411,10 +413,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev,
int port,
 {
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	u32 vlan_table[3];
-	u16 pvid;
-
-	ksz_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
-	pvid = pvid & 0xFFF;
 
 	if (ksz9477_get_vlan_table(dev, vlan->vid, vlan_table)) {
 		dev_dbg(dev->dev, "Failed to get vlan table\n");
@@ -423,9 +421,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev,
int port,
 
 	vlan_table[2] &= ~BIT(port);
 
-	if (pvid == vlan->vid)
-		pvid = 1;
-
 	if (untagged)
 		vlan_table[1] &= ~BIT(port);
 
@@ -434,8 +429,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev,
int port,
 		return -ETIMEDOUT;
 	}
 
-	ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.h
b/drivers/net/dsa/microchip/ksz9477.h
index cd278b307b3c..005c510ee6c2 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -30,6 +30,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int
port,
 			  struct netlink_ext_ack *extack);
 int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 			  const struct switchdev_obj_port_vlan *vlan);
+void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool
state);
 int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
 			    struct dsa_mall_mirror_tc_entry *mirror,
 			    bool ingress, struct netlink_ext_ack
*extack);
diff --git a/drivers/net/dsa/microchip/ksz_common.c
b/drivers/net/dsa/microchip/ksz_common.c
index ed7d137cba99..8711be6155e6 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -14,6 +14,7 @@
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
 #include <linux/of_device.h>
 #include <linux/of_net.h>
 #include <linux/micrel_phy.h>
@@ -160,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.vlan_filtering = ksz8_port_vlan_filtering,
 	.vlan_add = ksz8_port_vlan_add,
 	.vlan_del = ksz8_port_vlan_del,
+	.drop_untagged = ksz8_port_enable_pvid,
 	.mirror_add = ksz8_port_mirror_add,
 	.mirror_del = ksz8_port_mirror_del,
 	.get_caps = ksz8_get_caps,
@@ -186,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.vlan_filtering = ksz9477_port_vlan_filtering,
 	.vlan_add = ksz9477_port_vlan_add,
 	.vlan_del = ksz9477_port_vlan_del,
+	.drop_untagged = ksz9477_port_drop_untagged,
 	.mirror_add = ksz9477_port_mirror_add,
 	.mirror_del = ksz9477_port_mirror_del,
 	.get_caps = ksz9477_get_caps,
@@ -219,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
 	.vlan_filtering = ksz9477_port_vlan_filtering,
 	.vlan_add = ksz9477_port_vlan_add,
 	.vlan_del = ksz9477_port_vlan_del,
+	.drop_untagged = ksz9477_port_drop_untagged,
 	.mirror_add = ksz9477_port_mirror_add,
 	.mirror_del = ksz9477_port_mirror_del,
 	.get_caps = lan937x_phylink_get_caps,
@@ -258,6 +262,7 @@ static const u16 ksz8795_regs[] = {
 	[S_MULTICAST_CTRL]		= 0x04,
 	[P_XMII_CTRL_0]			= 0x06,
 	[P_XMII_CTRL_1]			= 0x56,
+	[P_DEFAULT_PVID]		= 0x03,
 };
 
 static const u32 ksz8795_masks[] = {
@@ -330,6 +335,7 @@ static const u16 ksz8863_regs[] = {
 	[S_START_CTRL]			= 0x01,
 	[S_BROADCAST_CTRL]		= 0x06,
 	[S_MULTICAST_CTRL]		= 0x04,
+	[P_DEFAULT_PVID]		= 0x03,
 };
 
 static const u32 ksz8863_masks[] = {
@@ -372,6 +378,7 @@ static const u16 ksz9477_regs[] = {
 	[S_MULTICAST_CTRL]		= 0x0331,
 	[P_XMII_CTRL_0]			= 0x0300,
 	[P_XMII_CTRL_1]			= 0x0301,
+	[P_DEFAULT_PVID]		= 0x0000,
 };
 
 static const u32 ksz9477_masks[] = {
@@ -1333,38 +1340,120 @@ static enum dsa_tag_protocol
ksz_get_tag_protocol(struct dsa_switch *ds,
 	return proto;
 }
 
+static void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid)
+{
+	const u16 *regs = dev->info->regs;
+	u16 val;
+
+	ksz_pread16(dev, port, regs[P_DEFAULT_PVID], &val);
+
+	*pvid = val & VLAN_VID_MASK;
+}
+
+static void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid)
+{
+	const u16 *regs = dev->info->regs;
+
+	ksz_prmw16(dev, port, regs[P_DEFAULT_PVID], VLAN_VID_MASK,
pvid);
+}
+
+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 */
+
+	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;
+}
+
 static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,
 				   bool flag, struct netlink_ext_ack
*extack)
 {
 	struct ksz_device *dev = ds->priv;
+	int ret;
 
 	if (!dev->dev_ops->vlan_filtering)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_filtering(dev, port, flag, extack);
+	ret = dev->dev_ops->vlan_filtering(dev, port, flag, extack);
+	if (ret)
+		return ret;
+
+	return ksz_commit_pvid(ds, port);
 }
 
 static int ksz_port_vlan_add(struct dsa_switch *ds, int port,
 			     const struct switchdev_obj_port_vlan
*vlan,
 			     struct netlink_ext_ack *extack)
 {
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
 	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	int ret;
+
+	p = &dev->ports[port];
 
 	if (!dev->dev_ops->vlan_add)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_add(dev, port, vlan, extack);
+	ret = dev->dev_ops->vlan_add(dev, port, vlan, extack);
+	if (ret)
+		return ret;
+
+	if (pvid) {
+		p->bridge_pvid.vid = vlan->vid;
+		p->bridge_pvid.valid = true;
+	} else if (vlan->vid && p->bridge_pvid.vid == vlan->vid) {
+		/* The old pvid was reinstalled as a non-pvid VLAN */
+		p->bridge_pvid.valid = false;
+	}
+
+	return ksz_commit_pvid(ds, port);
 }
 
 static int ksz_port_vlan_del(struct dsa_switch *ds, int port,
 			     const struct switchdev_obj_port_vlan
*vlan)
 {
 	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	u16 pvid;
+	int ret;
+
+	p = &dev->ports[port];
 
 	if (!dev->dev_ops->vlan_del)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_del(dev, port, vlan);
+	ksz_get_pvid(dev, port, &pvid);
+
+	ret = dev->dev_ops->vlan_del(dev, port, vlan);
+	if (ret)
+		return ret;
+
+	if (vlan->vid == pvid) {
+		p->bridge_pvid.valid = false;
+
+		ret = ksz_commit_pvid(ds, port);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int ksz_port_mirror_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.h
b/drivers/net/dsa/microchip/ksz_common.h
index 764ada3a0f42..c583cda6cc3d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -63,6 +63,11 @@ struct ksz_chip_data {
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 };
 
+struct ksz_vlan {
+	u16	vid;
+	bool	valid;
+};
+
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for
ksz8795 only */
 	int stp_state;
@@ -81,6 +86,7 @@ struct ksz_port {
 	u16 max_frame;
 	u32 rgmii_tx_val;
 	u32 rgmii_rx_val;
+	struct ksz_vlan bridge_pvid;
 };
 
 struct ksz_device {
@@ -175,6 +181,7 @@ enum ksz_regs {
 	S_MULTICAST_CTRL,
 	P_XMII_CTRL_0,
 	P_XMII_CTRL_1,
+	P_DEFAULT_PVID,
 };
 
 enum ksz_masks {
@@ -272,6 +279,7 @@ struct ksz_dev_ops {
 			 struct netlink_ext_ack *extack);
 	int  (*vlan_del)(struct ksz_device *dev, int port,
 			 const struct switchdev_obj_port_vlan *vlan);
+	void (*drop_untagged)(struct ksz_device *dev, int port, bool
untagged);
 	int (*mirror_add)(struct ksz_device *dev, int port,
 			  struct dsa_mall_mirror_tc_entry *mirror,
 			  bool ingress, struct netlink_ext_ack
*extack);
@@ -434,6 +442,14 @@ static inline void ksz_prmw8(struct ksz_device
*dev, int port, int offset,
 			   mask, val);
 }
 
+static inline void ksz_prmw16(struct ksz_device *dev, int port, int
offset,
+			      u16 mask, u16 val)
+{
+	regmap_update_bits(dev->regmap[1],
+			   dev->dev_ops->get_port_addr(port, offset),
+			   mask, val);
+}
+
 static inline void ksz_regmap_lock(void *__mtx)
 {
 	struct mutex *mtx = __mtx;
-- 
2.36.1
                                

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

[Vladimir Oltean]

Hi Arun,

On Tue, Jul 26, 2022 at 03:10:24PM +0000, Arun.Ramadoss@microchip.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.

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

[Arun.Ramadoss]

On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
Hi Vladimir,
I am trying to bringup the kselftest for bridge_vlan_aware.sh, in that
I am facing problem during the ping test and all the tests are failing.

#ip vrf exec vlan1 ping 192.0.2.2
Cannot open network namespace: No such file or directory
Failed to get name of network namespace: No such file or directory

Is there any configurations need to be enabled in the linux kernel, can
you suggest/help me out in resolving it.

--
Arun


> 
> Hi Arun,
> 
> On Tue, Jul 26, 2022 at 03:10:24PM +0000, Arun.Ramadoss@microchip.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;
> }
> 
> 

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

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 03:30:18PM +0000, Arun.Ramadoss@microchip.com wrote:
> On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> Hi Vladimir,
> I am trying to bringup the kselftest for bridge_vlan_aware.sh, in that
> I am facing problem during the ping test and all the tests are failing.
> 
> #ip vrf exec vlan1 ping 192.0.2.2
> Cannot open network namespace: No such file or directory
> Failed to get name of network namespace: No such file or directory
> 
> Is there any configurations need to be enabled in the linux kernel, can
> you suggest/help me out in resolving it.
> 
> --
> Arun

Yes, quite a few, in fact.
Note that I'm not quite sure why it says "cannot open network namespace"
when the command accesses a VRF instead.

Try these:

CONFIG_IP_ADVANCED_ROUTER=y
CONFIG_IP_MULTIPLE_TABLES=y
CONFIG_NET_L3_MASTER_DEV=y
CONFIG_IPV6_MULTIPLE_TABLES=y
CONFIG_NET_VRF=y
CONFIG_BPF_SYSCALL=y
CONFIG_CGROUP_BPF=y

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

[Arun.Ramadoss]

Hi Vladimir,
Thanks for reply.
On Mon, 2022-09-12 at 15:42 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Mon, Sep 12, 2022 at 03:30:18PM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > 
> > Hi Vladimir,
> > I am trying to bringup the kselftest for bridge_vlan_aware.sh, in
> > that
> > I am facing problem during the ping test and all the tests are
> > failing.
> > 
> > #ip vrf exec vlan1 ping 192.0.2.2
> > Cannot open network namespace: No such file or directory
> > Failed to get name of network namespace: No such file or directory
> > 
> > Is there any configurations need to be enabled in the linux kernel,
> > can
> > you suggest/help me out in resolving it.
> > 
> > --
> > Arun
> 
> Yes, quite a few, in fact.
> Note that I'm not quite sure why it says "cannot open network
> namespace"
> when the command accesses a VRF instead.
> 
> Try these:
> 
> CONFIG_IP_ADVANCED_ROUTER=y
> CONFIG_IP_MULTIPLE_TABLES=y
> CONFIG_NET_L3_MASTER_DEV=y
> CONFIG_IPV6_MULTIPLE_TABLES=y
> CONFIG_NET_VRF=y
> CONFIG_BPF_SYSCALL=y
> CONFIG_CGROUP_BPF=y

In addition to above config, I had set CONFIG_NAMESPACES=y, then the
above error message disappered.
But the ping is not successful. 

If I change the setup like
Linux laptop 1 --> DUT1 (Lan2) --> DUT1 (Lan3) --> Linux laptop 2
then ping is successful.

If I use the standard kselftest setup 
lan1 --> lan2 --> lan3 --> lan4, ping is not success.

I went through the comments given in this thread to bring up ping for
openwrt, is that applicable to kselftest also. 

ip netns add ns0
ip link set lan2 netns ns0
ip -n ns0 link set lan2 up
ip -n ns0 addr add 192.168.2.2/24 dev lan2 
ip netns exec ns0 tcpdump -i lan2 -e -n ping 192.168.2.2

I am struck with the ping test bringup. It would be helpful, if you can
give some suggestion to bring it up.

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

[Vladimir Oltean]

On Tue, Sep 13, 2022 at 10:57:44AM +0000, Arun.Ramadoss@microchip.com wrote:
> In addition to above config, I had set CONFIG_NAMESPACES=y, then the
> above error message disappered.
> But the ping is not successful. 
> 
> If I change the setup like
> Linux laptop 1 --> DUT1 (Lan2) --> DUT1 (Lan3) --> Linux laptop 2
> then ping is successful.
> 
> If I use the standard kselftest setup 
> lan1 --> lan2 --> lan3 --> lan4, ping is not success.
> 
> I went through the comments given in this thread to bring up ping for
> openwrt, is that applicable to kselftest also. 
> 
> ip netns add ns0
> ip link set lan2 netns ns0
> ip -n ns0 link set lan2 up
> ip -n ns0 addr add 192.168.2.2/24 dev lan2 
> ip netns exec ns0 tcpdump -i lan2 -e -n
> ping 192.168.2.2

You meant to put lan4 in a namespace and ping it and not lan2, since
lan2 will be a bridge port, right?

> I am struck with the ping test bringup. It would be helpful, if you can
> give some suggestion to bring it up.

Well, do you have unique MAC addresses for lan1, lan2, lan3, lan4? The
kselftests will ensure you do, via the STABLE_MAC_ADDRS variable, but
otherwise you may not.

Then, can you tell us what packets you do see reaching lan4 in the ping
setup? If none, can you show us the output of ethtool -S lan2 | grep -v ': 0'
to figure out what is the drop reason (supposing the packets are dropped
at the ingress of lan2)? Or even tcpdump -i lan2, maybe the packets are
forwarded to the termination plane of the bridge, instead of being
autonomously forwarded to lan3.

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

[Hauke Mehrtens]

On 7/6/22 18:45, Vladimir Oltean wrote:
> Hi Martin,
> 
> On Wed, Jul 06, 2022 at 06:33:18PM +0200, Martin Blumenstingl wrote:
>> Hi Vladimir,
>>
>> On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>>>
>>> Stop protecting DSA drivers from switchdev VLAN notifications emitted
>>> while the bridge has vlan_filtering 0, by deleting the deprecated bool
>>> ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see
>>> all notifications and should save the bridge PVID until they need to
>>> commit it to hardware.
>>>
>>> The 2 remaining unconverted drivers are the gswip and the Microchip KSZ
>>> family. They are both probably broken and would need changing as far as
>>> I can see:
>>>
>>> - For lantiq_gswip, after the initial call path
>>>    -> gswip_port_bridge_join
>>>       -> gswip_vlan_add_unaware
>>>          -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port));
>>>    nobody seems to prevent a future call path
>>>    -> gswip_port_vlan_add
>>>       -> gswip_vlan_add_aware
>>>          -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port));
>> Thanks for bringing this to my attention!
>>
>> I tried to reproduce this issue with the selftest script you provided
>> (patch #1 in this series).
>> Unfortunately not even the ping_ipv4 and ping_ipv6 tests from
>> bridge_vlan_unaware.sh are working for me, nor are the tests from
>> router_bridge.sh.
>> I suspect that this is an issue with OpenWrt: I already enabled bash,
>> jq and the full ip package, vrf support in the kernel. OpenWrt's ping
>> command doesn't like a ping interval of 0.1s so I replaced that with
>> an 1s interval.
>>
>> I will try to get the selftests to work here but I think that
>> shouldn't block this patch.
> 
> Thanks for the willingness to test!
> 
> Somehow we should do something to make sure that the OpenWRT devices are
> able to run the selftests, because there's a large number of DSA switches
> intended for that segment and we should all be onboard (easily).
> 
> I wonder, would it be possible to set up a debian chroot?

OpenWrt takes many packages like ping from busybox, but OpenWrt can also 
install the full versions. Adding a package which packs the self tests 
from the kernel and has the needed applications as dependencies would be 
nice. It would be nice to have such a thing in the OpenWrt package feed 
then we can easily test switches.

A debian chroot should be possible, but Debian supports MIPS32 BE only 
till Debian 10, I do not know if this recent enough. The GSWIP driver 
only works on SoCs with a MIPS32 BE CPU, I think this is similar for 
some other switches too. There are some old manuals in the Internet on 
how to run Debian on such systems.

Hauke

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

[Vladimir Oltean]

Hi Hauke,

On Wed, Jul 06, 2022 at 10:04:19PM +0200, Hauke Mehrtens wrote:
> OpenWrt takes many packages like ping from busybox, but OpenWrt can also
> install the full versions. Adding a package which packs the self tests from
> the kernel and has the needed applications as dependencies would be nice. It
> would be nice to have such a thing in the OpenWrt package feed then we can
> easily test switches.

I didn't want to insist on that because I know that OpenWrt devices are
generally strapped for storage. Also, I never managed to build an OpenWrt
by myself, plus my only OpenWrt device is helping me send this email, so
I can't be of very much help on that front, I'm afraid. But I'm happy to
see Martin take the initiative, and the encouragement from you.

> A debian chroot should be possible, but Debian supports MIPS32 BE only till
> Debian 10, I do not know if this recent enough. The GSWIP driver only works
> on SoCs with a MIPS32 BE CPU, I think this is similar for some other
> switches too. There are some old manuals in the Internet on how to run
> Debian on such systems.

I was definitely suggesting this as the easy way out, as you can put
Debian on an ext4 formatted USB stick. Debian 10 should definitely be
enough, as these selftests don't use the latest and greatest features
added to iproute2 IIRC, but it's concerning on the other hand that
maintainership for the debian-mips port was dropped. Are you closely
involved with the topic, do you know what's blocking the upgrade?