[PATCH net-next 0/7] VLAN improvements for Ocelot switch

[PATCH net-next 0/7] VLAN improvements for Ocelot switch

[Vladimir Oltean]

The main reason why I started this work is that deleting the bridge mdb
entries fails when the bridge is deleted, as described here:
https://lore.kernel.org/netdev/20201015173355.564934-1-vladimir.oltean@nxp.com/

In short, that happens because the bridge mdb entries are added with a
vid of 1, but deletion is attempted with a vid of 0. So the deletion
code fails to find the mdb entries.

The solution is to make ocelot use a pvid of 0 when it is under a bridge
with vlan_filtering 0. When vlan_filtering is 1, the pvid of the bridge
is what is programmed into the hardware.

The patch series also uncovers more bugs and does some more cleanup, but
the above is the main idea behind it.

Vladimir Oltean (7):
  net: mscc: ocelot: use the pvid of zero when bridged with
    vlan_filtering=0
  net: mscc: ocelot: don't reset the pvid to 0 when deleting it
  net: mscc: ocelot: transform the pvid and native vlan values into a
    structure
  net: mscc: ocelot: add a "valid" boolean to struct ocelot_vlan
  net: mscc: ocelot: move the logic to drop 802.1p traffic to the pvid
    deletion
  net: mscc: ocelot: deny changing the native VLAN from the prepare
    phase
  net: dsa: felix: improve the workaround for multiple native VLANs on
    NPI port

 drivers/net/dsa/ocelot/felix.c         |  27 ++++-
 drivers/net/ethernet/mscc/ocelot.c     | 147 +++++++++++++------------
 drivers/net/ethernet/mscc/ocelot_net.c |  38 +++++--
 include/soc/mscc/ocelot.h              |  17 ++-
 4 files changed, 138 insertions(+), 91 deletions(-)

-- 
2.25.1

[PATCH net-next 1/7] net: mscc: ocelot: use the pvid of zero when bridged with vlan_filtering=0

[Vladimir Oltean]

Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and
inherit the pvid from the bridge when one is present.

When the bridge has vlan_filtering=0, the software semantics are that
packets should be received regardless of whether there's a pvid
configured on the ingress port or not. However, ocelot does not observe
those semantics today.

Moreover, changing the PVID is also a problem with vlan_filtering=0.
We are privately remapping the VID of FDB, MDB entries to the port's
PVID when those are VLAN-unaware (i.e. when the VID of these entries
comes to us as 0). But we have no logic of adjusting that remapping when
the user changes the pvid and vlan_filtering is 0. So stale entries
would be left behind, and untagged traffic will stop matching on them.

And even if we were to solve that, there's an even bigger problem. If
swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0
bridge, they should be able to forward traffic between one another.
However, with ocelot they wouldn't do that.

The simplest way of fixing this is to never configure the pvid based on
what the bridge is asking for, when vlan_filtering is 0. Only if there
was a VLAN that the bridge couldn't mangle, that we could use as pvid....
So, turns out, there's 0 just for that. And for a reason: IEEE
802.1Q-2018, page 247, Table 9-2-Reserved VID values says:

	The null VID. Indicates that the tag header contains only
	priority information; no VID is present in the frame.
	This VID value shall not be configured as a PVID or a member
	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	of a VID Set, or configured in any FDB entry, or used in any
	Management operation.

So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but
what we're doing here is just driver-level bookkeeping, all for the
better. The fact that we're using a pvid of 0 is not observable behavior
from the outside world: the network stack does not see the classified
VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more
consistent with the standalone mode now.

And now that we use the pvid of 0 in this mode, there's another advantage:
we don't need to perform any VID remapping for FDB and MDB entries either,
we can just use the VID of 0 that the bridge is passing to us.

The only gotcha is that every time we change the vlan_filtering setting,
we need to reapply the pvid (either to 0, or to the value from the bridge).
A small side-effect visible in the patch is that ocelot_port_set_pvid
needs to be moved above ocelot_port_vlan_filtering, so that it can be
called from there without forward-declarations.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 53 ++++++++++--------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 323dbd30661a..bc5b15d7bce7 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -199,6 +199,22 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 	return 0;
 }
 
+/* Default vlan to clasify for untagged frames (may be zero) */
+static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	ocelot_port->pvid = pvid;
+
+	if (!ocelot_port->vlan_aware)
+		pvid = 0;
+
+	ocelot_rmw_gix(ocelot,
+		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
+		       ANA_PORT_VLAN_CFG_VLAN_VID_M,
+		       ANA_PORT_VLAN_CFG, port);
+}
+
 int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
 			       bool vlan_aware, struct switchdev_trans *trans)
 {
@@ -233,25 +249,13 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
 		       ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
 		       ANA_PORT_VLAN_CFG, port);
 
+	ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid);
 	ocelot_port_set_native_vlan(ocelot, port, ocelot_port->vid);
 
 	return 0;
 }
 EXPORT_SYMBOL(ocelot_port_vlan_filtering);
 
-/* Default vlan to clasify for untagged frames (may be zero) */
-static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
-{
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
-
-	ocelot_rmw_gix(ocelot,
-		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
-		       ANA_PORT_VLAN_CFG_VLAN_VID_M,
-		       ANA_PORT_VLAN_CFG, port);
-
-	ocelot_port->pvid = pvid;
-}
-
 int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		    bool untagged)
 {
@@ -542,26 +546,11 @@ EXPORT_SYMBOL(ocelot_get_txtstamp);
 int ocelot_fdb_add(struct ocelot *ocelot, int port,
 		   const unsigned char *addr, u16 vid)
 {
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	int pgid = port;
 
 	if (port == ocelot->npi)
 		pgid = PGID_CPU;
 
-	if (!vid) {
-		if (!ocelot_port->vlan_aware)
-			/* If the bridge is not VLAN aware and no VID was
-			 * provided, set it to pvid to ensure the MAC entry
-			 * matches incoming untagged packets
-			 */
-			vid = ocelot_port->pvid;
-		else
-			/* If the bridge is VLAN aware a VID must be provided as
-			 * otherwise the learnt entry wouldn't match any frame.
-			 */
-			return -EINVAL;
-	}
-
 	return ocelot_mact_learn(ocelot, pgid, addr, vid, ENTRYTYPE_LOCKED);
 }
 EXPORT_SYMBOL(ocelot_fdb_add);
@@ -1048,7 +1037,6 @@ static void ocelot_encode_ports_to_mdb(unsigned char *addr,
 int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb)
 {
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	unsigned char addr[ETH_ALEN];
 	struct ocelot_multicast *mc;
 	struct ocelot_pgid *pgid;
@@ -1057,9 +1045,6 @@ int ocelot_port_mdb_add(struct ocelot *ocelot, int port,
 	if (port == ocelot->npi)
 		port = ocelot->num_phys_ports;
 
-	if (!vid)
-		vid = ocelot_port->pvid;
-
 	mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
 	if (!mc) {
 		/* New entry */
@@ -1108,7 +1093,6 @@ EXPORT_SYMBOL(ocelot_port_mdb_add);
 int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 			const struct switchdev_obj_port_mdb *mdb)
 {
-	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	unsigned char addr[ETH_ALEN];
 	struct ocelot_multicast *mc;
 	struct ocelot_pgid *pgid;
@@ -1117,9 +1101,6 @@ int ocelot_port_mdb_del(struct ocelot *ocelot, int port,
 	if (port == ocelot->npi)
 		port = ocelot->num_phys_ports;
 
-	if (!vid)
-		vid = ocelot_port->pvid;
-
 	mc = ocelot_multicast_get(ocelot, mdb->addr, vid);
 	if (!mc)
 		return -ENOENT;
-- 
2.25.1

Re: [PATCH net-next 1/7] net: mscc: ocelot: use the pvid of zero when bridged with vlan_filtering=0

[Alexandre Belloni]

Hello,

On 31/10/2020 12:29:10+0200, Vladimir Oltean wrote:
> Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and
> inherit the pvid from the bridge when one is present.
> 
> When the bridge has vlan_filtering=0, the software semantics are that
> packets should be received regardless of whether there's a pvid
> configured on the ingress port or not. However, ocelot does not observe
> those semantics today.
> 
> Moreover, changing the PVID is also a problem with vlan_filtering=0.
> We are privately remapping the VID of FDB, MDB entries to the port's
> PVID when those are VLAN-unaware (i.e. when the VID of these entries
> comes to us as 0). But we have no logic of adjusting that remapping when
> the user changes the pvid and vlan_filtering is 0. So stale entries
> would be left behind, and untagged traffic will stop matching on them.
> 
> And even if we were to solve that, there's an even bigger problem. If
> swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0
> bridge, they should be able to forward traffic between one another.
> However, with ocelot they wouldn't do that.
> 
> The simplest way of fixing this is to never configure the pvid based on
> what the bridge is asking for, when vlan_filtering is 0. Only if there
> was a VLAN that the bridge couldn't mangle, that we could use as pvid....
> So, turns out, there's 0 just for that. And for a reason: IEEE
> 802.1Q-2018, page 247, Table 9-2-Reserved VID values says:
> 
> 	The null VID. Indicates that the tag header contains only
> 	priority information; no VID is present in the frame.
> 	This VID value shall not be configured as a PVID or a member
> 	~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 	of a VID Set, or configured in any FDB entry, or used in any
> 	Management operation.
> 
> So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but
> what we're doing here is just driver-level bookkeeping, all for the
> better. The fact that we're using a pvid of 0 is not observable behavior
> from the outside world: the network stack does not see the classified
> VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more
> consistent with the standalone mode now.
> 

IIRC, we are using pvid 1 because else bridging breaks when
CONFIG_VLAN_8021Q is not enabled. Did you test that configuration?



-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/7] net: mscc: ocelot: use the pvid of zero when bridged with vlan_filtering=0

[Vladimir Oltean]

On Mon, Nov 02, 2020 at 09:47:20AM +0100, Alexandre Belloni wrote:
> IIRC, we are using pvid 1 because else bridging breaks when
> CONFIG_VLAN_8021Q is not enabled. Did you test that configuration?

Pertinent question.
I hadn't tested that, but I did now.

[root@LS1028ARDB ~] # zcat /proc/config.gz | grep 8021Q
# CONFIG_VLAN_8021Q is not set
[root@LS1028ARDB ~] # ip addr flush swp0
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev swp0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.717 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.442 ms
^C
--- 192.168.1.1 ping statistics ---
2 packets transmitted, 2 packets received, 0% packet loss
round-trip min/avg/max = 0.442/0.579/0.717 ms
[root@LS1028ARDB ~] # ip addr flush swp0
[root@LS1028ARDB ~] # ip link add br0 type bridge
[root@LS1028ARDB ~] # ip link set swp0 master br0
[  409.576303] br0: port 1(swp0) entered blocking state
[  409.581380] br0: port 1(swp0) entered disabled state
[  409.586738] device swp0 entered promiscuous mode
[  409.591866] br0: port 1(swp0) entered blocking state
[  409.596852] br0: port 1(swp0) entered forwarding state
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev br0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.768 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.657 ms
64 bytes from 192.168.1.1: seq=2 ttl=64 time=0.509 ms
64 bytes from 192.168.1.1: seq=3 ttl=64 time=0.513 ms
64 bytes from 192.168.1.1: seq=4 ttl=64 time=0.496 ms
^C
--- 192.168.1.1 ping statistics ---
5 packets transmitted, 5 packets received, 0% packet loss
round-trip min/avg/max = 0.496/0.588/0.768 ms
[root@LS1028ARDB ~] # ip link del br0
[  135.526825] device swp0 left promiscuous mode
[  135.531729] br0: port 1(swp0) entered disabled state
[root@LS1028ARDB ~] # ip addr add 192.168.1.2/24 dev swp0
[root@LS1028ARDB ~] # ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: seq=0 ttl=64 time=0.783 ms
64 bytes from 192.168.1.1: seq=1 ttl=64 time=0.289 ms
64 bytes from 192.168.1.1: seq=2 ttl=64 time=0.412 ms
64 bytes from 192.168.1.1: seq=3 ttl=64 time=0.399 ms
64 bytes from 192.168.1.1: seq=4 ttl=64 time=0.396 ms
64 bytes from 192.168.1.1: seq=5 ttl=64 time=0.390 ms
^C
--- 192.168.1.1 ping statistics ---
6 packets transmitted, 6 packets received, 0% packet loss
round-trip min/avg/max = 0.289/0.444/0.783 ms

There's no logical reason why it wouldn't work. Thanks to your code
which ensures VLAN 0 is in the VLAN table. Nobody is removing VLAN 0
right now.

	/* Because VLAN filtering is enabled, we need VID 0 to get untagged
	 * traffic.  It is added automatically if 8021q module is loaded, but
	 * we can't rely on it since module may be not loaded.
	 */
	ocelot->vlan_mask[0] = GENMASK(ocelot->num_phys_ports - 1, 0);
	ocelot_vlant_set_mask(ocelot, 0, ocelot->vlan_mask[0]);

I cannot test the mscc_ocelot driver, I am only testing felix DSA, and
for that reason I can't even go very far down the history. Remember that
when CONFIG_VLAN_8021Q is disabled, CONFIG_BRIDGE_VLAN_FILTERING also
needs to be disabled. So logically speaking, nobody calls any VLAN
function when CONFIG_VLAN_8021Q is disabled. The standalone
configuration should work in this mode too, shouldn't it? I am not sure
if there's any relevant difference for mscc_ocelot.

[PATCH net-next 2/7] net: mscc: ocelot: don't reset the pvid to 0 when deleting it

[Vladimir Oltean]

I have no idea why this code is here, but I have 2 hypotheses:

1.
A desperate attempt to keep untagged traffic working when the bridge
deletes the pvid on a port.

There was a fairly okay discussion here:
https://lore.kernel.org/netdev/CA+h21hrRMrLH-RjBGhEJSTZd6_QPRSd3RkVRQF-wNKkrgKcRSA@mail.gmail.com/#t
which established that in vlan_filtering=1 mode, the absence of a pvid
should denote that the ingress port should drop untagged and priority
tagged traffic. While in vlan_filtering=0 mode, nothing should change.

So in vlan_filtering=1 mode, we should simply let things happen, and not
attempt to save the day. And in vlan_filtering=0 mode, the pvid is 0
anyway, no need to do anything.

2.
The driver encodes the native VLAN (ocelot_port->vid) value of 0 as
special, meaning "not valid". There are checks based on that. But there
are no such checks for the ocelot_port->pvid value of 0. In fact, that's
a perfectly valid value, which is used in standalone mode. Maybe there
was some confusion and the author thought that 0 means "invalid" here as
well.

In conclusion, delete the code*.

*in fact we'll add it back later, in a slightly different form, but for
an entirely different reason than the one for which this exists now.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index bc5b15d7bce7..ae25a79bf907 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -293,10 +293,6 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
 	if (ret)
 		return ret;
 
-	/* Ingress */
-	if (ocelot_port->pvid == vid)
-		ocelot_port_set_pvid(ocelot, port, 0);
-
 	/* Egress */
 	if (ocelot_port->vid == vid)
 		ocelot_port_set_native_vlan(ocelot, port, 0);
-- 
2.25.1

[PATCH net-next 3/7] net: mscc: ocelot: transform the pvid and native vlan values into a structure

[Vladimir Oltean]

This is a mechanical patch only.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c     | 55 ++++++++++++++++----------
 drivers/net/ethernet/mscc/ocelot_net.c | 16 ++++----
 include/soc/mscc/ocelot.h              | 14 ++++---
 3 files changed, 50 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index ae25a79bf907..a7e724ae01f7 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -148,27 +148,27 @@ static int ocelot_vlant_set_mask(struct ocelot *ocelot, u16 vid, u32 mask)
 }
 
 static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
-				       u16 vid)
+				       struct ocelot_vlan native_vlan)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 val = 0;
 
-	if (ocelot_port->vid != vid) {
+	if (ocelot_port->native_vlan.vid != native_vlan.vid) {
 		/* Always permit deleting the native VLAN (vid = 0) */
-		if (ocelot_port->vid && vid) {
+		if (ocelot_port->native_vlan.vid && native_vlan.vid) {
 			dev_err(ocelot->dev,
 				"Port already has a native VLAN: %d\n",
-				ocelot_port->vid);
+				ocelot_port->native_vlan.vid);
 			return -EBUSY;
 		}
-		ocelot_port->vid = vid;
+		ocelot_port->native_vlan = native_vlan;
 	}
 
-	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(vid),
+	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
 		       REW_PORT_VLAN_CFG_PORT_VID_M,
 		       REW_PORT_VLAN_CFG, port);
 
-	if (ocelot_port->vlan_aware && !ocelot_port->vid)
+	if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.vid)
 		/* If port is vlan-aware and tagged, drop untagged and priority
 		 * tagged frames.
 		 */
@@ -182,7 +182,7 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 		       ANA_PORT_DROP_CFG, port);
 
 	if (ocelot_port->vlan_aware) {
-		if (ocelot_port->vid)
+		if (ocelot_port->native_vlan.vid)
 			/* Tag all frames except when VID == DEFAULT_VLAN */
 			val = REW_TAG_CFG_TAG_CFG(1);
 		else
@@ -200,17 +200,18 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 }
 
 /* Default vlan to clasify for untagged frames (may be zero) */
-static void ocelot_port_set_pvid(struct ocelot *ocelot, int port, u16 pvid)
+static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
+				 struct ocelot_vlan pvid_vlan)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 
-	ocelot_port->pvid = pvid;
+	ocelot_port->pvid_vlan = pvid_vlan;
 
 	if (!ocelot_port->vlan_aware)
-		pvid = 0;
+		pvid_vlan.vid = 0;
 
 	ocelot_rmw_gix(ocelot,
-		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid),
+		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid_vlan.vid),
 		       ANA_PORT_VLAN_CFG_VLAN_VID_M,
 		       ANA_PORT_VLAN_CFG, port);
 }
@@ -249,8 +250,8 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
 		       ANA_PORT_VLAN_CFG_VLAN_POP_CNT_M,
 		       ANA_PORT_VLAN_CFG, port);
 
-	ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid);
-	ocelot_port_set_native_vlan(ocelot, port, ocelot_port->vid);
+	ocelot_port_set_pvid(ocelot, port, ocelot_port->pvid_vlan);
+	ocelot_port_set_native_vlan(ocelot, port, ocelot_port->native_vlan);
 
 	return 0;
 }
@@ -268,12 +269,19 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		return ret;
 
 	/* Default ingress vlan classification */
-	if (pvid)
-		ocelot_port_set_pvid(ocelot, port, vid);
+	if (pvid) {
+		struct ocelot_vlan pvid_vlan;
+
+		pvid_vlan.vid = vid;
+		ocelot_port_set_pvid(ocelot, port, pvid_vlan);
+	}
 
 	/* Untagged egress vlan clasification */
 	if (untagged) {
-		ret = ocelot_port_set_native_vlan(ocelot, port, vid);
+		struct ocelot_vlan native_vlan;
+
+		native_vlan.vid = vid;
+		ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 		if (ret)
 			return ret;
 	}
@@ -294,8 +302,12 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
 		return ret;
 
 	/* Egress */
-	if (ocelot_port->vid == vid)
-		ocelot_port_set_native_vlan(ocelot, port, 0);
+	if (ocelot_port->native_vlan.vid == vid) {
+		struct ocelot_vlan native_vlan;
+
+		native_vlan.vid = 0;
+		ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+	}
 
 	return 0;
 }
@@ -1151,6 +1163,7 @@ EXPORT_SYMBOL(ocelot_port_bridge_join);
 int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 			     struct net_device *bridge)
 {
+	struct ocelot_vlan pvid = {0}, native_vlan = {0};
 	struct switchdev_trans trans;
 	int ret;
 
@@ -1169,8 +1182,8 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 	if (ret)
 		return ret;
 
-	ocelot_port_set_pvid(ocelot, port, 0);
-	return ocelot_port_set_native_vlan(ocelot, port, 0);
+	ocelot_port_set_pvid(ocelot, port, pvid);
+	return ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 }
 EXPORT_SYMBOL(ocelot_port_bridge_leave);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index b34da11acf65..cf5c2a0ddfc0 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -409,7 +409,7 @@ static int ocelot_mc_unsync(struct net_device *dev, const unsigned char *addr)
 	struct ocelot_port *ocelot_port = &priv->port;
 	struct ocelot *ocelot = ocelot_port->ocelot;
 
-	return ocelot_mact_forget(ocelot, addr, ocelot_port->pvid);
+	return ocelot_mact_forget(ocelot, addr, ocelot_port->pvid_vlan.vid);
 }
 
 static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr)
@@ -418,8 +418,8 @@ static int ocelot_mc_sync(struct net_device *dev, const unsigned char *addr)
 	struct ocelot_port *ocelot_port = &priv->port;
 	struct ocelot *ocelot = ocelot_port->ocelot;
 
-	return ocelot_mact_learn(ocelot, PGID_CPU, addr, ocelot_port->pvid,
-				 ENTRYTYPE_LOCKED);
+	return ocelot_mact_learn(ocelot, PGID_CPU, addr,
+				 ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
 }
 
 static void ocelot_set_rx_mode(struct net_device *dev)
@@ -462,10 +462,10 @@ static int ocelot_port_set_mac_address(struct net_device *dev, void *p)
 	const struct sockaddr *addr = p;
 
 	/* Learn the new net device MAC address in the mac table. */
-	ocelot_mact_learn(ocelot, PGID_CPU, addr->sa_data, ocelot_port->pvid,
-			  ENTRYTYPE_LOCKED);
+	ocelot_mact_learn(ocelot, PGID_CPU, addr->sa_data,
+			  ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
 	/* Then forget the previous one. */
-	ocelot_mact_forget(ocelot, dev->dev_addr, ocelot_port->pvid);
+	ocelot_mact_forget(ocelot, dev->dev_addr, ocelot_port->pvid_vlan.vid);
 
 	ether_addr_copy(dev->dev_addr, addr->sa_data);
 	return 0;
@@ -1074,8 +1074,8 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target,
 
 	memcpy(dev->dev_addr, ocelot->base_mac, ETH_ALEN);
 	dev->dev_addr[ETH_ALEN - 1] += port;
-	ocelot_mact_learn(ocelot, PGID_CPU, dev->dev_addr, ocelot_port->pvid,
-			  ENTRYTYPE_LOCKED);
+	ocelot_mact_learn(ocelot, PGID_CPU, dev->dev_addr,
+			  ocelot_port->pvid_vlan.vid, ENTRYTYPE_LOCKED);
 
 	ocelot_init_port(ocelot, port);
 
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index cc126d1796be..baf6a498f7d1 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -571,18 +571,20 @@ struct ocelot_vcap_block {
 	int pol_lpr;
 };
 
+struct ocelot_vlan {
+	u16 vid;
+};
+
 struct ocelot_port {
 	struct ocelot			*ocelot;
 
 	struct regmap			*target;
 
 	bool				vlan_aware;
-
-	/* Ingress default VLAN (pvid) */
-	u16				pvid;
-
-	/* Egress default VLAN (vid) */
-	u16				vid;
+	/* VLAN that untagged frames are classified to, on ingress */
+	struct ocelot_vlan		pvid_vlan;
+	/* The VLAN ID that will be transmitted as untagged, on egress */
+	struct ocelot_vlan		native_vlan;
 
 	u8				ptp_cmd;
 	struct sk_buff_head		tx_skbs;
-- 
2.25.1

[PATCH net-next 4/7] net: mscc: ocelot: add a "valid" boolean to struct ocelot_vlan

[Vladimir Oltean]

Currently we are checking in some places whether the port has a native
VLAN on egress or not, by comparing the ocelot_port->vid value with zero.

That works, because VID 0 can never be a native VLAN configured by the
bridge, but now we want to make similar checks for the pvid. That won't
work, because there are cases when we do have the pvid set to 0 (not by
the bridge, by ourselves, but still.. it's confusing). And we can't
encode a negative value into an u16, so add a bool to the structure.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 27 ++++++++++++++-------------
 include/soc/mscc/ocelot.h          |  1 +
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index a7e724ae01f7..d49e34430e23 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -153,22 +153,22 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 val = 0;
 
-	if (ocelot_port->native_vlan.vid != native_vlan.vid) {
-		/* Always permit deleting the native VLAN (vid = 0) */
-		if (ocelot_port->native_vlan.vid && native_vlan.vid) {
-			dev_err(ocelot->dev,
-				"Port already has a native VLAN: %d\n",
-				ocelot_port->native_vlan.vid);
-			return -EBUSY;
-		}
-		ocelot_port->native_vlan = native_vlan;
+	/* Deny changing the native VLAN, but always permit deleting it */
+	if (ocelot_port->native_vlan.vid != native_vlan.vid &&
+	    ocelot_port->native_vlan.valid && native_vlan.valid) {
+		dev_err(ocelot->dev,
+			"Port already has a native VLAN: %d\n",
+			ocelot_port->native_vlan.vid);
+		return -EBUSY;
 	}
 
+	ocelot_port->native_vlan = native_vlan;
+
 	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
 		       REW_PORT_VLAN_CFG_PORT_VID_M,
 		       REW_PORT_VLAN_CFG, port);
 
-	if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.vid)
+	if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.valid)
 		/* If port is vlan-aware and tagged, drop untagged and priority
 		 * tagged frames.
 		 */
@@ -182,7 +182,7 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 		       ANA_PORT_DROP_CFG, port);
 
 	if (ocelot_port->vlan_aware) {
-		if (ocelot_port->native_vlan.vid)
+		if (native_vlan.valid)
 			/* Tag all frames except when VID == DEFAULT_VLAN */
 			val = REW_TAG_CFG_TAG_CFG(1);
 		else
@@ -273,6 +273,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		struct ocelot_vlan pvid_vlan;
 
 		pvid_vlan.vid = vid;
+		pvid_vlan.valid = true;
 		ocelot_port_set_pvid(ocelot, port, pvid_vlan);
 	}
 
@@ -281,6 +282,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		struct ocelot_vlan native_vlan;
 
 		native_vlan.vid = vid;
+		native_vlan.valid = true;
 		ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 		if (ret)
 			return ret;
@@ -303,9 +305,8 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
 
 	/* Egress */
 	if (ocelot_port->native_vlan.vid == vid) {
-		struct ocelot_vlan native_vlan;
+		struct ocelot_vlan native_vlan = {0};
 
-		native_vlan.vid = 0;
 		ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 	}
 
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index baf6a498f7d1..67c2af1c4c5c 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -572,6 +572,7 @@ struct ocelot_vcap_block {
 };
 
 struct ocelot_vlan {
+	bool valid;
 	u16 vid;
 };
 
-- 
2.25.1

[PATCH net-next 5/7] net: mscc: ocelot: move the logic to drop 802.1p traffic to the pvid deletion

[Vladimir Oltean]

Currently, the ocelot_port_set_native_vlan() function starts dropping
untagged and prio-tagged traffic when the native VLAN is removed?

What is the native VLAN? It is the only egress-untagged VLAN that ocelot
supports on a port. If the port is a trunk with 100 VLANs, one of those
VLANs can be transmitted as egress-untagged, and that's the native VLAN.

Is it wrong to drop untagged and prio-tagged traffic if there's no
native VLAN? Yes and no.

In this case, which is more typical, it's ok to apply that drop
configuration:
$ bridge vlan add dev swp0 vid 1 pvid untagged <- this is the native VLAN
$ bridge vlan add dev swp0 vid 100
$ bridge vlan add dev swp0 vid 101
$ bridge vlan del dev swp0 vid 1 <- delete the native VLAN
But only because the pvid and the native VLAN have the same ID.

In this case, it isn't:
$ bridge vlan add dev swp0 vid 1 pvid
$ bridge vlan add dev swp0 vid 100 untagged <- this is the native VLAN
$ bridge vlan del dev swp0 vid 101
$ bridge vlan del dev swp0 vid 100 <- delete the native VLAN

It's wrong, because the switch will drop untagged and prio-tagged
traffic now, despite having a valid pvid of 1.

The confusion seems to stem from the fact that the native VLAN is an
egress setting, while the PVID is an ingress setting. It would be
correct to drop untagged and prio-tagged traffic only if there was no
pvid on the port. So let's do just that.

Background:
https://lore.kernel.org/netdev/CA+h21hrRMrLH-RjBGhEJSTZd6_QPRSd3RkVRQF-wNKkrgKcRSA@mail.gmail.com/#t

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 35 +++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index d49e34430e23..60186fc99280 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -168,19 +168,6 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 		       REW_PORT_VLAN_CFG_PORT_VID_M,
 		       REW_PORT_VLAN_CFG, port);
 
-	if (ocelot_port->vlan_aware && !ocelot_port->native_vlan.valid)
-		/* If port is vlan-aware and tagged, drop untagged and priority
-		 * tagged frames.
-		 */
-		val = ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
-		      ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
-		      ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
-	ocelot_rmw_gix(ocelot, val,
-		       ANA_PORT_DROP_CFG_DROP_UNTAGGED_ENA |
-		       ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
-		       ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA,
-		       ANA_PORT_DROP_CFG, port);
-
 	if (ocelot_port->vlan_aware) {
 		if (native_vlan.valid)
 			/* Tag all frames except when VID == DEFAULT_VLAN */
@@ -204,6 +191,7 @@ static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
 				 struct ocelot_vlan pvid_vlan)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
+	u32 val = 0;
 
 	ocelot_port->pvid_vlan = pvid_vlan;
 
@@ -214,6 +202,20 @@ static void ocelot_port_set_pvid(struct ocelot *ocelot, int port,
 		       ANA_PORT_VLAN_CFG_VLAN_VID(pvid_vlan.vid),
 		       ANA_PORT_VLAN_CFG_VLAN_VID_M,
 		       ANA_PORT_VLAN_CFG, port);
+
+	/* If there's no pvid, we should drop not only untagged traffic (which
+	 * happens automatically), but also 802.1p traffic which gets
+	 * classified to VLAN 0, but that is always in our RX filter, so it
+	 * would get accepted were it not for this setting.
+	 */
+	if (!pvid_vlan.valid && ocelot_port->vlan_aware)
+		val = ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
+		      ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA;
+
+	ocelot_rmw_gix(ocelot, val,
+		       ANA_PORT_DROP_CFG_DROP_PRIO_S_TAGGED_ENA |
+		       ANA_PORT_DROP_CFG_DROP_PRIO_C_TAGGED_ENA,
+		       ANA_PORT_DROP_CFG, port);
 }
 
 int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
@@ -303,6 +305,13 @@ int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid)
 	if (ret)
 		return ret;
 
+	/* Ingress */
+	if (ocelot_port->pvid_vlan.vid == vid) {
+		struct ocelot_vlan pvid_vlan = {0};
+
+		ocelot_port_set_pvid(ocelot, port, pvid_vlan);
+	}
+
 	/* Egress */
 	if (ocelot_port->native_vlan.vid == vid) {
 		struct ocelot_vlan native_vlan = {0};
-- 
2.25.1

[PATCH net-next 6/7] net: mscc: ocelot: deny changing the native VLAN from the prepare phase

[Vladimir Oltean]

Put the preparation phase of switchdev VLAN objects to some good use,
and move the check we already had, for preventing the existence of more
than one egress-untagged VLAN per port, to the preparation phase of the
addition.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c         | 13 +++++++-
 drivers/net/ethernet/mscc/ocelot.c     | 41 +++++++++++++++-----------
 drivers/net/ethernet/mscc/ocelot_net.c | 22 ++++++++++++--
 include/soc/mscc/ocelot.h              |  2 ++
 4 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f791860d495f..3848f6bc922b 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -112,10 +112,21 @@ static void felix_bridge_leave(struct dsa_switch *ds, int port,
 	ocelot_port_bridge_leave(ocelot, port, br);
 }
 
-/* This callback needs to be present */
 static int felix_vlan_prepare(struct dsa_switch *ds, int port,
 			      const struct switchdev_obj_port_vlan *vlan)
 {
+	struct ocelot *ocelot = ds->priv;
+	u16 vid, flags = vlan->flags;
+	int err;
+
+	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
+		err = ocelot_vlan_prepare(ocelot, port, vid,
+					  flags & BRIDGE_VLAN_INFO_PVID,
+					  flags & BRIDGE_VLAN_INFO_UNTAGGED);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 60186fc99280..2632fe2d2448 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -147,21 +147,12 @@ static int ocelot_vlant_set_mask(struct ocelot *ocelot, u16 vid, u32 mask)
 	return ocelot_vlant_wait_for_completion(ocelot);
 }
 
-static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
-				       struct ocelot_vlan native_vlan)
+static void ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
+					struct ocelot_vlan native_vlan)
 {
 	struct ocelot_port *ocelot_port = ocelot->ports[port];
 	u32 val = 0;
 
-	/* Deny changing the native VLAN, but always permit deleting it */
-	if (ocelot_port->native_vlan.vid != native_vlan.vid &&
-	    ocelot_port->native_vlan.valid && native_vlan.valid) {
-		dev_err(ocelot->dev,
-			"Port already has a native VLAN: %d\n",
-			ocelot_port->native_vlan.vid);
-		return -EBUSY;
-	}
-
 	ocelot_port->native_vlan = native_vlan;
 
 	ocelot_rmw_gix(ocelot, REW_PORT_VLAN_CFG_PORT_VID(native_vlan.vid),
@@ -182,8 +173,6 @@ static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port,
 	ocelot_rmw_gix(ocelot, val,
 		       REW_TAG_CFG_TAG_CFG_M,
 		       REW_TAG_CFG, port);
-
-	return 0;
 }
 
 /* Default vlan to clasify for untagged frames (may be zero) */
@@ -259,6 +248,24 @@ int ocelot_port_vlan_filtering(struct ocelot *ocelot, int port,
 }
 EXPORT_SYMBOL(ocelot_port_vlan_filtering);
 
+int ocelot_vlan_prepare(struct ocelot *ocelot, int port, u16 vid, bool pvid,
+			bool untagged)
+{
+	struct ocelot_port *ocelot_port = ocelot->ports[port];
+
+	/* Deny changing the native VLAN, but always permit deleting it */
+	if (untagged && ocelot_port->native_vlan.vid != vid &&
+	    ocelot_port->native_vlan.valid) {
+		dev_err(ocelot->dev,
+			"Port already has a native VLAN: %d\n",
+			ocelot_port->native_vlan.vid);
+		return -EBUSY;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(ocelot_vlan_prepare);
+
 int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		    bool untagged)
 {
@@ -285,9 +292,7 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 
 		native_vlan.vid = vid;
 		native_vlan.valid = true;
-		ret = ocelot_port_set_native_vlan(ocelot, port, native_vlan);
-		if (ret)
-			return ret;
+		ocelot_port_set_native_vlan(ocelot, port, native_vlan);
 	}
 
 	return 0;
@@ -1193,7 +1198,9 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
 		return ret;
 
 	ocelot_port_set_pvid(ocelot, port, pvid);
-	return ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+	ocelot_port_set_native_vlan(ocelot, port, native_vlan);
+
+	return 0;
 }
 EXPORT_SYMBOL(ocelot_port_bridge_leave);
 
diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
index cf5c2a0ddfc0..c65ae6f75a16 100644
--- a/drivers/net/ethernet/mscc/ocelot_net.c
+++ b/drivers/net/ethernet/mscc/ocelot_net.c
@@ -206,6 +206,17 @@ static void ocelot_port_adjust_link(struct net_device *dev)
 	ocelot_adjust_link(ocelot, port, dev->phydev);
 }
 
+static int ocelot_vlan_vid_prepare(struct net_device *dev, u16 vid, bool pvid,
+				   bool untagged)
+{
+	struct ocelot_port_private *priv = netdev_priv(dev);
+	struct ocelot_port *ocelot_port = &priv->port;
+	struct ocelot *ocelot = ocelot_port->ocelot;
+	int port = priv->chip_port;
+
+	return ocelot_vlan_prepare(ocelot, port, vid, pvid, untagged);
+}
+
 static int ocelot_vlan_vid_add(struct net_device *dev, u16 vid, bool pvid,
 			       bool untagged)
 {
@@ -812,9 +823,14 @@ static int ocelot_port_obj_add_vlan(struct net_device *dev,
 	u16 vid;
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		ret = ocelot_vlan_vid_add(dev, vid,
-					  vlan->flags & BRIDGE_VLAN_INFO_PVID,
-					  vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
+		bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+		bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+
+		if (switchdev_trans_ph_prepare(trans))
+			ret = ocelot_vlan_vid_prepare(dev, vid, pvid,
+						      untagged);
+		else
+			ret = ocelot_vlan_vid_add(dev, vid, pvid, untagged);
 		if (ret)
 			return ret;
 	}
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 67c2af1c4c5c..ea1de185f2e4 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -747,6 +747,8 @@ int ocelot_fdb_add(struct ocelot *ocelot, int port,
 		   const unsigned char *addr, u16 vid);
 int ocelot_fdb_del(struct ocelot *ocelot, int port,
 		   const unsigned char *addr, u16 vid);
+int ocelot_vlan_prepare(struct ocelot *ocelot, int port, u16 vid, bool pvid,
+			bool untagged);
 int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
 		    bool untagged);
 int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);
-- 
2.25.1

[PATCH net-next 7/7] net: dsa: felix: improve the workaround for multiple native VLANs on NPI port

[Vladimir Oltean]

After the good discussion with Florian from here:
https://lore.kernel.org/netdev/20200911000337.htwr366ng3nc3a7d@skbuf/

I realized that the VLAN settings on the NPI port (the hardware "CPU port",
in DSA parlance) don't actually make any difference, because that port
is hardcoded in hardware to use what mv88e6xxx would call "unmodified"
egress policy for VLANs.

So earlier patch 183be6f967fe ("net: dsa: felix: send VLANs on CPU port
as egress-tagged") was incorrect in the sense that it didn't actually
make the VLANs be sent on the NPI port as egress-tagged. It only made
ocelot_port_set_native_vlan shut up.

Now that we have moved the check from ocelot_port_set_native_vlan to
ocelot_vlan_prepare, we can simply shunt ocelot_vlan_prepare from DSA,
and avoid calling it. This is the correct way to deal with things,
because the NPI port configuration is DSA-specific, so the ocelot switch
library should not have the check for multiple native VLANs refined in
any way, it is correct the way it is.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 3848f6bc922b..ada75fa15861 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -119,6 +119,17 @@ static int felix_vlan_prepare(struct dsa_switch *ds, int port,
 	u16 vid, flags = vlan->flags;
 	int err;
 
+	/* Ocelot switches copy frames as-is to the CPU, so the flags:
+	 * egress-untagged or not, pvid or not, make no difference. This
+	 * behavior is already better than what DSA just tries to approximate
+	 * when it installs the VLAN with the same flags on the CPU port.
+	 * Just accept any configuration, and don't let ocelot deny installing
+	 * multiple native VLANs on the NPI port, because the switch doesn't
+	 * look at the port tag settings towards the NPI interface anyway.
+	 */
+	if (port == ocelot->npi)
+		return 0;
+
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		err = ocelot_vlan_prepare(ocelot, port, vid,
 					  flags & BRIDGE_VLAN_INFO_PVID,
@@ -146,9 +157,6 @@ static void felix_vlan_add(struct dsa_switch *ds, int port,
 	u16 vid;
 	int err;
 
-	if (dsa_is_cpu_port(ds, port))
-		flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
-
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		err = ocelot_vlan_add(ocelot, port, vid,
 				      flags & BRIDGE_VLAN_INFO_PVID,
-- 
2.25.1

Re: [PATCH net-next 0/7] VLAN improvements for Ocelot switch

[Jakub Kicinski]

On Sat, 31 Oct 2020 12:29:09 +0200 Vladimir Oltean wrote:
> The main reason why I started this work is that deleting the bridge mdb
> entries fails when the bridge is deleted, as described here:
> https://lore.kernel.org/netdev/20201015173355.564934-1-vladimir.oltean@nxp.com/
> 
> In short, that happens because the bridge mdb entries are added with a
> vid of 1, but deletion is attempted with a vid of 0. So the deletion
> code fails to find the mdb entries.
> 
> The solution is to make ocelot use a pvid of 0 when it is under a bridge
> with vlan_filtering 0. When vlan_filtering is 1, the pvid of the bridge
> is what is programmed into the hardware.
> 
> The patch series also uncovers more bugs and does some more cleanup, but
> the above is the main idea behind it.

Applied, thanks!