[PATCH v2 net-next 0/8] Replay and offload host VLAN entries in DSA

[PATCH v2 net-next 0/8] Replay and offload host VLAN entries in DSA

[Vladimir Oltean]

v1->v2:
- prune switchdev VLAN additions with no actual change differently
- no longer need to revert struct net_bridge_vlan changes on error from
  switchdev
- no longer need to first delete a changed VLAN before readding it
- pass 'bool changed' and 'u16 old_flags' through switchdev_obj_port_vlan
  so that DSA can do some additional post-processing with the
  BRIDGE_VLAN_INFO_BRENTRY flag
- support VLANs on foreign interfaces
- fix the same -EOPNOTSUPP error in mv88e6xxx, this time on removal, due
  to VLAN deletion getting replayed earlier than FDB deletion

The motivation behind these patches is that Rafael reported the
following error with mv88e6xxx when the first switch port joins a
bridge:

mv88e6085 0x0000000008b96000:00: port 0 failed to add a6:ef:77:c8:5f:3d vid 1 to fdb: -95 (-EOPNOTSUPP)

The FDB entry that's added is the MAC address of the bridge, in VID 1
(the default_pvid), being replayed as part of br_add_if() -> ... ->
nbp_switchdev_sync_objs().

-EOPNOTSUPP is the mv88e6xxx driver's way of saying that VID 1 doesn't
exist in the VTU, so it can't program the ATU with a FID, something
which it needs.

It appears to be a race, but it isn't, since we only end up installing
VID 1 in the VTU by coincidence. DSA's approximation of programming
VLANs on the CPU port together with the user ports breaks down with
host FDB entries on mv88e6xxx, since that strictly requires the VTU to
contain the VID. But the user may freely add VLANs pointing just towards
the bridge, and FDB entries in those VLANs, and DSA will not be aware of
them, because it only listens for VLANs on user ports.

To create a solution that scales properly to cross-chip setups and
doesn't leak entries behind, some changes in the bridge driver are
required. I believe that these are for the better overall, but I may be
wrong. Namely, the same refcounting procedure that DSA has in place for
host FDB and MDB entries can be replicated for VLANs, except that it's
garbage in, garbage out: the VLAN addition and removal notifications
from switchdev aren't balanced. So the first 2 patches attempt to deal
with that.

This patch set has been superficially tested on a board with 3 mv88e6xxx
switches in a daisy chain and appears to produce the primary desired
effect - the driver no longer returns -EOPNOTSUPP when the first port
joins a bridge, and is successful in performing local termination under
a VLAN-aware bridge.
As an additional side effect, it silences the annoying "p%d: already a
member of VLAN %d\n" warning messages that the mv88e6xxx driver produces
when coupled with systemd-networkd, and a few VLANs are configured.
Furthermore, it advances Florian's idea from a few years back, which
never got merged:
https://lore.kernel.org/lkml/20180624153339.13572-1-f.fainelli@gmail.com/
v2 has also been tested on the NXP LS1028A felix switch.

Some testing:

root@debian:~# bridge vlan add dev br0 vid 101 pvid self
[  100.709220] mv88e6085 d0032004.mdio-mii:10: mv88e6xxx_port_vlan_add: port 9 vlan 101
[  100.873426] mv88e6085 d0032004.mdio-mii:10: mv88e6xxx_port_vlan_add: port 10 vlan 101
[  100.892314] mv88e6085 d0032004.mdio-mii:11: mv88e6xxx_port_vlan_add: port 9 vlan 101
[  101.053392] mv88e6085 d0032004.mdio-mii:11: mv88e6xxx_port_vlan_add: port 10 vlan 101
[  101.076994] mv88e6085 d0032004.mdio-mii:12: mv88e6xxx_port_vlan_add: port 9 vlan 101
root@debian:~# bridge vlan add dev br0 vid 101 pvid self
root@debian:~# bridge vlan add dev br0 vid 101 pvid self
root@debian:~# bridge vlan
port              vlan-id
eth0              1 PVID Egress Untagged
lan9              1 PVID Egress Untagged
lan10             1 PVID Egress Untagged
lan11             1 PVID Egress Untagged
lan12             1 PVID Egress Untagged
lan13             1 PVID Egress Untagged
lan14             1 PVID Egress Untagged
lan15             1 PVID Egress Untagged
lan16             1 PVID Egress Untagged
lan17             1 PVID Egress Untagged
lan18             1 PVID Egress Untagged
lan19             1 PVID Egress Untagged
lan20             1 PVID Egress Untagged
lan21             1 PVID Egress Untagged
lan22             1 PVID Egress Untagged
lan23             1 PVID Egress Untagged
lan24             1 PVID Egress Untagged
sfp               1 PVID Egress Untagged
lan1              1 PVID Egress Untagged
lan2              1 PVID Egress Untagged
lan3              1 PVID Egress Untagged
lan4              1 PVID Egress Untagged
lan5              1 PVID Egress Untagged
lan6              1 PVID Egress Untagged
lan7              1 PVID Egress Untagged
lan8              1 PVID Egress Untagged
br0               1 Egress Untagged
                  101 PVID
root@debian:~# bridge vlan del dev br0 vid 101 pvid self
[  108.340487] mv88e6085 d0032004.mdio-mii:11: mv88e6xxx_port_vlan_del: port 9 vlan 101
[  108.379167] mv88e6085 d0032004.mdio-mii:11: mv88e6xxx_port_vlan_del: port 10 vlan 101
[  108.402319] mv88e6085 d0032004.mdio-mii:12: mv88e6xxx_port_vlan_del: port 9 vlan 101
[  108.425866] mv88e6085 d0032004.mdio-mii:10: mv88e6xxx_port_vlan_del: port 9 vlan 101
[  108.452280] mv88e6085 d0032004.mdio-mii:10: mv88e6xxx_port_vlan_del: port 10 vlan 101
root@debian:~# bridge vlan del dev br0 vid 101 pvid self
root@debian:~# bridge vlan del dev br0 vid 101 pvid self
root@debian:~# bridge vlan
port              vlan-id
eth0              1 PVID Egress Untagged
lan9              1 PVID Egress Untagged
lan10             1 PVID Egress Untagged
lan11             1 PVID Egress Untagged
lan12             1 PVID Egress Untagged
lan13             1 PVID Egress Untagged
lan14             1 PVID Egress Untagged
lan15             1 PVID Egress Untagged
lan16             1 PVID Egress Untagged
lan17             1 PVID Egress Untagged
lan18             1 PVID Egress Untagged
lan19             1 PVID Egress Untagged
lan20             1 PVID Egress Untagged
lan21             1 PVID Egress Untagged
lan22             1 PVID Egress Untagged
lan23             1 PVID Egress Untagged
lan24             1 PVID Egress Untagged
sfp               1 PVID Egress Untagged
lan1              1 PVID Egress Untagged
lan2              1 PVID Egress Untagged
lan3              1 PVID Egress Untagged
lan4              1 PVID Egress Untagged
lan5              1 PVID Egress Untagged
lan6              1 PVID Egress Untagged
lan7              1 PVID Egress Untagged
lan8              1 PVID Egress Untagged
br0               1 Egress Untagged
root@debian:~# bridge vlan del dev br0 vid 101 pvid self

Vladimir Oltean (8):
  net: bridge: vlan: notify switchdev only when something changed
  net: bridge: switchdev: differentiate new VLANs from changed ones
  net: bridge: make nbp_switchdev_unsync_objs() follow reverse order of
    sync()
  net: bridge: switchdev: replay all VLAN groups
  net: switchdev: rename switchdev_lower_dev_find to
    switchdev_lower_dev_find_rcu
  net: switchdev: introduce switchdev_handle_port_obj_{add,del} for
    foreign interfaces
  net: dsa: add explicit support for host bridge VLANs
  net: dsa: offload bridge port VLANs on foreign interfaces

 include/net/dsa.h         |  10 ++
 include/net/switchdev.h   |  45 +++++++++
 net/bridge/br_private.h   |   6 +-
 net/bridge/br_switchdev.c |  95 ++++++++++---------
 net/bridge/br_vlan.c      |  77 ++++++++++------
 net/dsa/dsa2.c            |   8 ++
 net/dsa/dsa_priv.h        |   7 ++
 net/dsa/port.c            |  42 +++++++++
 net/dsa/slave.c           | 164 ++++++++++++++++++++++++---------
 net/dsa/switch.c          | 187 ++++++++++++++++++++++++++++++++++++--
 net/switchdev/switchdev.c | 152 ++++++++++++++++++++++++++++---
 11 files changed, 656 insertions(+), 137 deletions(-)

-- 
2.25.1

[PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

Currently, when a VLAN entry is added multiple times in a row to a
bridge port, nbp_vlan_add() calls br_switchdev_port_vlan_add() each
time, even if the VLAN already exists and nothing about it has changed:

bridge vlan add dev lan12 vid 100 master static

Similarly, when a VLAN is added multiple times in a row to a bridge,
br_vlan_add_existing() doesn't filter at all the calls to
br_switchdev_port_vlan_add():

bridge vlan add dev br0 vid 100 self

This behavior makes driver-level accounting of VLANs impossible, since
it is enough for a single deletion event to remove a VLAN, but the
addition event can be emitted an unlimited number of times.

The cause for this can be identified as follows: we rely on
__vlan_add_flags() to retroactively tell us whether it has changed
anything about the VLAN flags or VLAN group pvid. So we'd first have to
call __vlan_add_flags() before calling br_switchdev_port_vlan_add(), in
order to have access to the "bool *changed" information. But we don't
want to change the event ordering, because we'd have to revert the
struct net_bridge_vlan changes we've made if switchdev returns an error.

So to solve this, we need another function that tells us whether any
change is going to occur in the VLAN or VLAN group, _prior_ to calling
__vlan_add_flags(). In fact, we even make __vlan_add_flags() return void.

With this lookahead function in place, we can avoid notifying switchdev
if nothing changed for the VLAN and VLAN group.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: drop the br_vlan_restore_existing() approach, just figure out
        ahead of time what will change.

 net/bridge/br_vlan.c | 71 ++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..c5355695c976 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,36 +34,29 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
 	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
 }
 
-static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
+static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 			    const struct net_bridge_vlan *v)
 {
 	if (vg->pvid == v->vid)
-		return false;
+		return;
 
 	smp_wmb();
 	br_vlan_set_pvid_state(vg, v->state);
 	vg->pvid = v->vid;
-
-	return true;
 }
 
-static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 {
 	if (vg->pvid != vid)
-		return false;
+		return;
 
 	smp_wmb();
 	vg->pvid = 0;
-
-	return true;
 }
 
-/* return true if anything changed, false otherwise */
-static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
+static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 {
 	struct net_bridge_vlan_group *vg;
-	u16 old_flags = v->flags;
-	bool ret;
 
 	if (br_vlan_is_master(v))
 		vg = br_vlan_group(v->br);
@@ -71,16 +64,36 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 		vg = nbp_vlan_group(v->port);
 
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		ret = __vlan_add_pvid(vg, v);
+		__vlan_add_pvid(vg, v);
 	else
-		ret = __vlan_delete_pvid(vg, v->vid);
+		__vlan_delete_pvid(vg, v->vid);
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 	else
 		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
+}
+
+/* return true if anything will change as a result of __vlan_add_flags,
+ * false otherwise
+ */
+static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
+{
+	struct net_bridge_vlan_group *vg;
+	u16 old_flags = v->flags;
+	bool pvid_changed;
 
-	return ret || !!(old_flags ^ v->flags);
+	if (br_vlan_is_master(v))
+		vg = br_vlan_group(v->br);
+	else
+		vg = nbp_vlan_group(v->port);
+
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		pvid_changed = (vg->pvid == v->vid);
+	else
+		pvid_changed = (vg->pvid != v->vid);
+
+	return pvid_changed || !!(old_flags ^ v->flags);
 }
 
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
@@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
 {
 	int err;
 
-	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	*changed = __vlan_flags_would_change(vlan, flags);
+	if (*changed) {
+		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
+						 extack);
+		if (err && err != -EOPNOTSUPP)
+			return err;
+	}
 
 	if (!br_vlan_is_brentry(vlan)) {
 		/* Trying to change flags of non-existent bridge vlan */
@@ -696,8 +713,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
 		br_multicast_toggle_one_vlan(vlan, true);
 	}
 
-	if (__vlan_add_flags(vlan, flags))
-		*changed = true;
+	__vlan_add_flags(vlan, flags);
 
 	return 0;
 
@@ -1247,11 +1263,16 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 	*changed = false;
 	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
-		/* Pass the flags to the hardware bridge */
-		ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
-		if (ret && ret != -EOPNOTSUPP)
-			return ret;
-		*changed = __vlan_add_flags(vlan, flags);
+		*changed = __vlan_flags_would_change(vlan, flags);
+		if (*changed) {
+			/* Pass the flags to the hardware bridge */
+			ret = br_switchdev_port_vlan_add(port->dev, vid,
+							 flags, extack);
+			if (ret && ret != -EOPNOTSUPP)
+				return ret;
+		}
+
+		__vlan_add_flags(vlan, flags);
 
 		return 0;
 	}
-- 
2.25.1

[PATCH v2 net-next 2/8] net: bridge: switchdev: differentiate new VLANs from changed ones

[Vladimir Oltean]

br_switchdev_port_vlan_add() currently emits a SWITCHDEV_PORT_OBJ_ADD
event with a SWITCHDEV_OBJ_ID_PORT_VLAN for 2 distinct cases:

- a struct net_bridge_vlan got created
- an existing struct net_bridge_vlan was modified

This makes it impossible for switchdev drivers to properly balance
PORT_OBJ_ADD with PORT_OBJ_DEL events, so if we want to allow that to
happen, we must provide a way for drivers to distinguish between a
VLAN with changed flags and a new one.

Annotate struct switchdev_obj_port_vlan with a "bool changed" that
distinguishes the 2 cases above. If the VLAN is changed, also provide
the old flags such that the driver can determine which flags were
actually changed.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new, logically replaces the need for "net: bridge:
        vlan: notify a switchdev deletion when modifying flags of
        existing VLAN"

 include/net/switchdev.h   |  6 ++++++
 net/bridge/br_private.h   |  6 ++++--
 net/bridge/br_switchdev.c |  3 +++
 net/bridge/br_vlan.c      | 12 +++++++-----
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index d353793dfeb5..24ec1f82a521 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -79,8 +79,14 @@ struct switchdev_obj {
 /* SWITCHDEV_OBJ_ID_PORT_VLAN */
 struct switchdev_obj_port_vlan {
 	struct switchdev_obj obj;
+	/* Valid only if @changed is set */
+	u16 old_flags;
 	u16 flags;
 	u16 vid;
+	/* If set, the notifier signifies a change of flags for
+	 * a VLAN that already exists.
+	 */
+	bool changed;
 };
 
 #define SWITCHDEV_OBJ_PORT_VLAN(OBJ) \
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2661dda1a92b..633cc048c590 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1985,6 +1985,7 @@ void br_switchdev_mdb_notify(struct net_device *dev,
 			     struct net_bridge_port_group *pg,
 			     int type);
 int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags,
+			       bool changed, u16 old_flags,
 			       struct netlink_ext_ack *extack);
 int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid);
 void br_switchdev_init(struct net_bridge *br);
@@ -2052,8 +2053,9 @@ static inline int br_switchdev_set_port_flag(struct net_bridge_port *p,
 	return 0;
 }
 
-static inline int br_switchdev_port_vlan_add(struct net_device *dev,
-					     u16 vid, u16 flags,
+static inline int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid,
+					     u16 flags, bool changed,
+					     u16 old_flags,
 					     struct netlink_ext_ack *extack)
 {
 	return -EOPNOTSUPP;
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index f8fbaaa7c501..f36f60766478 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -160,6 +160,7 @@ br_switchdev_fdb_notify(struct net_bridge *br,
 }
 
 int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags,
+			       bool changed, u16 old_flags,
 			       struct netlink_ext_ack *extack)
 {
 	struct switchdev_obj_port_vlan v = {
@@ -167,6 +168,8 @@ int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags,
 		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
 		.flags = flags,
 		.vid = vid,
+		.changed = changed,
+		.old_flags = old_flags,
 	};
 
 	return switchdev_port_obj_add(dev, &v.obj, extack);
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index c5355695c976..6f3ee4d8fea8 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -105,7 +105,7 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 	/* Try switchdev op first. In case it is not supported, fallback to
 	 * 8021q add.
 	 */
-	err = br_switchdev_port_vlan_add(dev, v->vid, flags, extack);
+	err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, 0, extack);
 	if (err == -EOPNOTSUPP)
 		return vlan_vid_add(dev, br->vlan_proto, v->vid);
 	v->priv_flags |= BR_VLFLAG_ADDED_BY_SWITCHDEV;
@@ -297,7 +297,8 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 		}
 		br_multicast_port_ctx_init(p, v, &v->port_mcast_ctx);
 	} else {
-		err = br_switchdev_port_vlan_add(dev, v->vid, flags, extack);
+		err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, 0,
+						 extack);
 		if (err && err != -EOPNOTSUPP)
 			goto out;
 		br_multicast_ctx_init(br, v, &v->br_mcast_ctx);
@@ -688,7 +689,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
 	*changed = __vlan_flags_would_change(vlan, flags);
 	if (*changed) {
 		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
-						 extack);
+						 true, vlan->flags, extack);
 		if (err && err != -EOPNOTSUPP)
 			return err;
 	}
@@ -1266,8 +1267,9 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 		*changed = __vlan_flags_would_change(vlan, flags);
 		if (*changed) {
 			/* Pass the flags to the hardware bridge */
-			ret = br_switchdev_port_vlan_add(port->dev, vid,
-							 flags, extack);
+			ret = br_switchdev_port_vlan_add(port->dev, vid, flags,
+							 true, vlan->flags,
+							 extack);
 			if (ret && ret != -EOPNOTSUPP)
 				return ret;
 		}
-- 
2.25.1

[PATCH v2 net-next 3/8] net: bridge: make nbp_switchdev_unsync_objs() follow reverse order of sync()

[Vladimir Oltean]

There may be switchdev drivers that can add/remove a FDB or MDB entry
only as long as the VLAN it's in has been notified and offloaded first.
The nbp_switchdev_sync_objs() method satisfies this requirement on
addition, but nbp_switchdev_unsync_objs() first deletes VLANs, then
deletes MDBs and FDBs. Reverse the order of the function calls to cater
to this requirement.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/bridge/br_switchdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index f36f60766478..a8e201e73a34 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -709,11 +709,11 @@ static void nbp_switchdev_unsync_objs(struct net_bridge_port *p,
 	struct net_device *br_dev = p->br->dev;
 	struct net_device *dev = p->dev;
 
-	br_switchdev_vlan_replay(br_dev, dev, ctx, false, blocking_nb, NULL);
+	br_switchdev_fdb_replay(br_dev, ctx, false, atomic_nb);
 
 	br_switchdev_mdb_replay(br_dev, dev, ctx, false, blocking_nb, NULL);
 
-	br_switchdev_fdb_replay(br_dev, ctx, false, atomic_nb);
+	br_switchdev_vlan_replay(br_dev, dev, ctx, false, blocking_nb, NULL);
 }
 
 /* Let the bridge know that this port is offloaded, so that it can assign a
-- 
2.25.1

[PATCH v2 net-next 4/8] net: bridge: switchdev: replay all VLAN groups

[Vladimir Oltean]

The major user of replayed switchdev objects is DSA, and so far it
hasn't needed information about anything other than bridge port VLANs,
so this is all that br_switchdev_vlan_replay() knows to handle.

DSA has managed to get by through replicating every VLAN addition on a
user port such that the same VLAN is also added on all DSA and CPU
ports, but there is a corner case where this does not work.

The mv88e6xxx DSA driver currently prints this error message as soon as
the first port of a switch joins a bridge:

mv88e6085 0x0000000008b96000:00: port 0 failed to add a6:ef:77:c8:5f:3d vid 1 to fdb: -95

where a6:ef:77:c8:5f:3d vid 1 is a local FDB entry corresponding to the
bridge MAC address in the default_pvid.

The -EOPNOTSUPP is returned by mv88e6xxx_port_db_load_purge() because it
tries to map VID 1 to a FID (the ATU is indexed by FID not VID), but
fails to do so. This is because ->port_fdb_add() is called before
->port_vlan_add() for VID 1.

The abridged timeline of the calls is:

br_add_if
-> netdev_master_upper_dev_link
   -> dsa_port_bridge_join
      -> switchdev_bridge_port_offload
         -> br_switchdev_vlan_replay (*)
         -> br_switchdev_fdb_replay
            -> mv88e6xxx_port_fdb_add
-> nbp_vlan_init
   -> nbp_vlan_add
      -> mv88e6xxx_port_vlan_add

and the issue is that at the time of (*), the bridge port isn't in VID 1
(nbp_vlan_init hasn't been called), therefore br_switchdev_vlan_replay()
won't have anything to replay, therefore VID 1 won't be in the VTU by
the time mv88e6xxx_port_fdb_add() is called.

This happens only when the first port of a switch joins. For further
ports, the initial mv88e6xxx_port_vlan_add() is sufficient for VID 1 to
be loaded in the VTU (which is switch-wide, not per port).

The problem is somewhat unique to mv88e6xxx by chance, because most
other drivers offload an FDB entry by VID, so FDBs and VLANs can be
added asynchronously with respect to each other, but addressing the
issue at the bridge layer makes sense, since what mv88e6xxx requires
isn't absurd.

To fix this problem, we need to recognize that it isn't the VLAN group
of the port that we're interested in, but the VLAN group of the bridge
itself (so it isn't a timing issue, but rather insufficient information
being passed from switchdev to drivers).

As mentioned, currently nbp_switchdev_sync_objs() only calls
br_switchdev_vlan_replay() for VLANs corresponding to the port, but the
VLANs corresponding to the bridge itself, for local termination, also
need to be replayed. In this case, VID 1 is not (yet) present in the
port's VLAN group but is present in the bridge's VLAN group.

So to fix this bug, DSA is now obligated to explicitly handle VLANs
pointing towards the bridge in order to "close this race" (which isn't
really a race). As Tobias Waldekranz notices, this also implies that it
must explicitly handle port VLANs on foreign interfaces, something that
worked implicitly before:
https://patchwork.kernel.org/project/netdevbpf/patch/20220209213044.2353153-6-vladimir.oltean@nxp.com/#24735260

So in the end, br_switchdev_vlan_replay() must replay all VLANs from all
VLAN groups: all the ports, and the bridge itself.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new, logically replaces the need for "net: bridge:
        switchdev: replay VLANs present on the bridge device itself"

 net/bridge/br_switchdev.c | 90 +++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index a8e201e73a34..f4b3160127bc 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -354,51 +354,19 @@ br_switchdev_vlan_replay_one(struct notifier_block *nb,
 	return notifier_to_errno(err);
 }
 
-static int br_switchdev_vlan_replay(struct net_device *br_dev,
-				    struct net_device *dev,
-				    const void *ctx, bool adding,
-				    struct notifier_block *nb,
-				    struct netlink_ext_ack *extack)
+static int br_switchdev_vlan_replay_group(struct notifier_block *nb,
+					  struct net_device *dev,
+					  struct net_bridge_vlan_group *vg,
+					  const void *ctx, unsigned long action,
+					  struct netlink_ext_ack *extack)
 {
-	struct net_bridge_vlan_group *vg;
 	struct net_bridge_vlan *v;
-	struct net_bridge_port *p;
-	struct net_bridge *br;
-	unsigned long action;
 	int err = 0;
 	u16 pvid;
 
-	ASSERT_RTNL();
-
-	if (!nb)
-		return 0;
-
-	if (!netif_is_bridge_master(br_dev))
-		return -EINVAL;
-
-	if (!netif_is_bridge_master(dev) && !netif_is_bridge_port(dev))
-		return -EINVAL;
-
-	if (netif_is_bridge_master(dev)) {
-		br = netdev_priv(dev);
-		vg = br_vlan_group(br);
-		p = NULL;
-	} else {
-		p = br_port_get_rtnl(dev);
-		if (WARN_ON(!p))
-			return -EINVAL;
-		vg = nbp_vlan_group(p);
-		br = p->br;
-	}
-
 	if (!vg)
 		return 0;
 
-	if (adding)
-		action = SWITCHDEV_PORT_OBJ_ADD;
-	else
-		action = SWITCHDEV_PORT_OBJ_DEL;
-
 	pvid = br_get_pvid(vg);
 
 	list_for_each_entry(v, &vg->vlan_list, vlist) {
@@ -418,7 +386,48 @@ static int br_switchdev_vlan_replay(struct net_device *br_dev,
 			return err;
 	}
 
-	return err;
+	return 0;
+}
+
+static int br_switchdev_vlan_replay(struct net_device *br_dev,
+				    const void *ctx, bool adding,
+				    struct notifier_block *nb,
+				    struct netlink_ext_ack *extack)
+{
+	struct net_bridge *br = netdev_priv(br_dev);
+	struct net_bridge_port *p;
+	unsigned long action;
+	int err;
+
+	ASSERT_RTNL();
+
+	if (!nb)
+		return 0;
+
+	if (!netif_is_bridge_master(br_dev))
+		return -EINVAL;
+
+	if (adding)
+		action = SWITCHDEV_PORT_OBJ_ADD;
+	else
+		action = SWITCHDEV_PORT_OBJ_DEL;
+
+	err = br_switchdev_vlan_replay_group(nb, br_dev, br_vlan_group(br),
+					     ctx, action, extack);
+	if (err)
+		return err;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		struct net_device *dev = p->dev;
+
+		err = br_switchdev_vlan_replay_group(nb, dev,
+						     nbp_vlan_group(p),
+						     ctx, action, extack);
+		if (err)
+			return err;
+	}
+
+	return 0;
 }
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
@@ -684,8 +693,7 @@ static int nbp_switchdev_sync_objs(struct net_bridge_port *p, const void *ctx,
 	struct net_device *dev = p->dev;
 	int err;
 
-	err = br_switchdev_vlan_replay(br_dev, dev, ctx, true, blocking_nb,
-				       extack);
+	err = br_switchdev_vlan_replay(br_dev, ctx, true, blocking_nb, extack);
 	if (err && err != -EOPNOTSUPP)
 		return err;
 
@@ -713,7 +721,7 @@ static void nbp_switchdev_unsync_objs(struct net_bridge_port *p,
 
 	br_switchdev_mdb_replay(br_dev, dev, ctx, false, blocking_nb, NULL);
 
-	br_switchdev_vlan_replay(br_dev, dev, ctx, false, blocking_nb, NULL);
+	br_switchdev_vlan_replay(br_dev, ctx, false, blocking_nb, NULL);
 }
 
 /* Let the bridge know that this port is offloaded, so that it can assign a
-- 
2.25.1

[PATCH v2 net-next 5/8] net: switchdev: rename switchdev_lower_dev_find to switchdev_lower_dev_find_rcu

[Vladimir Oltean]

switchdev_lower_dev_find() assumes RCU read-side critical section
calling context, since it uses netdev_walk_all_lower_dev_rcu().

Rename it appropriately, in preparation of adding a similar iterator
that assumes writer-side rtnl_mutex protection.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/switchdev/switchdev.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 12e6b4146bfb..d53f364870a5 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -409,10 +409,10 @@ static int switchdev_lower_dev_walk(struct net_device *lower_dev,
 }
 
 static struct net_device *
-switchdev_lower_dev_find(struct net_device *dev,
-			 bool (*check_cb)(const struct net_device *dev),
-			 bool (*foreign_dev_check_cb)(const struct net_device *dev,
-						      const struct net_device *foreign_dev))
+switchdev_lower_dev_find_rcu(struct net_device *dev,
+			     bool (*check_cb)(const struct net_device *dev),
+			     bool (*foreign_dev_check_cb)(const struct net_device *dev,
+							  const struct net_device *foreign_dev))
 {
 	struct switchdev_nested_priv switchdev_priv = {
 		.check_cb = check_cb,
@@ -451,7 +451,7 @@ static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
 		return mod_cb(dev, orig_dev, event, info->ctx, fdb_info);
 
 	if (netif_is_lag_master(dev)) {
-		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))
+		if (!switchdev_lower_dev_find_rcu(dev, check_cb, foreign_dev_check_cb))
 			goto maybe_bridged_with_us;
 
 		/* This is a LAG interface that we offload */
@@ -465,7 +465,7 @@ static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
 	 * towards a bridge device.
 	 */
 	if (netif_is_bridge_master(dev)) {
-		if (!switchdev_lower_dev_find(dev, check_cb, foreign_dev_check_cb))
+		if (!switchdev_lower_dev_find_rcu(dev, check_cb, foreign_dev_check_cb))
 			return 0;
 
 		/* This is a bridge interface that we offload */
@@ -478,8 +478,8 @@ static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
 			 * that we offload.
 			 */
 			if (!check_cb(lower_dev) &&
-			    !switchdev_lower_dev_find(lower_dev, check_cb,
-						      foreign_dev_check_cb))
+			    !switchdev_lower_dev_find_rcu(lower_dev, check_cb,
+							  foreign_dev_check_cb))
 				continue;
 
 			err = __switchdev_handle_fdb_event_to_device(lower_dev, orig_dev,
@@ -501,7 +501,7 @@ static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
 	if (!br || !netif_is_bridge_master(br))
 		return 0;
 
-	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))
+	if (!switchdev_lower_dev_find_rcu(br, check_cb, foreign_dev_check_cb))
 		return 0;
 
 	return __switchdev_handle_fdb_event_to_device(br, orig_dev, event, fdb_info,
-- 
2.25.1

[PATCH v2 net-next 6/8] net: switchdev: introduce switchdev_handle_port_obj_{add,del} for foreign interfaces

[Vladimir Oltean]

The switchdev_handle_port_obj_add() helper is good for replicating a
port object on the lower interfaces of @dev, if that object was emitted
on a bridge, or on a bridge port that is a LAG.

However, drivers that use this helper limit themselves to a box from
which they can no longer intercept port objects notified on neighbor
ports ("foreign interfaces").

One such driver is DSA, where software bridging with foreign interfaces
such as standalone NICs or Wi-Fi APs is an important use case. There, a
VLAN installed on a neighbor bridge port roughly corresponds to a
forwarding VLAN installed on the DSA switch's CPU port.

To support this use case while also making use of the benefits of the
switchdev_handle_* replication helper for port objects, introduce a new
variant of these functions that crawls through the neighbor ports of
@dev, in search of potentially compatible switchdev ports that are
interested in the event.

The strategy is identical to switchdev_handle_fdb_event_to_device():
if @dev wasn't a switchdev interface, then go one step upper, and
recursively call this function on the bridge that this port belongs to.
At the next recursion step, __switchdev_handle_port_obj_add() will
iterate through the bridge's lower interfaces. Among those, some will be
switchdev interfaces, and one will be the original @dev that we came
from. To prevent infinite recursion, we must suppress reentry into the
original @dev, and just call the @add_cb for the switchdev_interfaces.

It looks like this:

                br0
               / | \
              /  |  \
             /   |   \
           swp0 swp1 eth0

1. __switchdev_handle_port_obj_add(eth0)
   -> check_cb(eth0) returns false
   -> eth0 has no lower interfaces
   -> eth0's bridge is br0
   -> switchdev_lower_dev_find(br0, check_cb, foreign_dev_check_cb))
      finds br0

2. __switchdev_handle_port_obj_add(br0)
   -> check_cb(br0) returns false
   -> netdev_for_each_lower_dev
      -> check_cb(swp0) returns true, so we don't skip this interface

3. __switchdev_handle_port_obj_add(swp0)
   -> check_cb(swp0) returns true, so we call add_cb(swp0)

(back to netdev_for_each_lower_dev from 2)
      -> check_cb(swp1) returns true, so we don't skip this interface

4. __switchdev_handle_port_obj_add(swp1)
   -> check_cb(swp1) returns true, so we call add_cb(swp1)

(back to netdev_for_each_lower_dev from 2)
      -> check_cb(eth0) returns false, so we skip this interface to
         avoid infinite recursion

Note: eth0 could have been a LAG, and we don't want to suppress the
recursion through its lowers if those exist, so when check_cb() returns
false, we still call switchdev_lower_dev_find() to estimate whether
there's anything worth a recursion beneath that LAG. Using check_cb()
and foreign_dev_check_cb(), switchdev_lower_dev_find() not only figures
out whether the lowers of the LAG are switchdev, but also whether they
actively offload the LAG or not (whether the LAG is "foreign" to the
switchdev interface or not).

The port_obj_info->orig_dev is preserved across recursive calls, so
switchdev drivers still know on which device was this notification
originally emitted.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 include/net/switchdev.h   |  39 +++++++++++
 net/switchdev/switchdev.c | 140 +++++++++++++++++++++++++++++++++++---
 2 files changed, 171 insertions(+), 8 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 24ec1f82a521..2ceede8e2aad 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -323,11 +323,26 @@ int switchdev_handle_port_obj_add(struct net_device *dev,
 			int (*add_cb)(struct net_device *dev, const void *ctx,
 				      const struct switchdev_obj *obj,
 				      struct netlink_ext_ack *extack));
+int switchdev_handle_port_obj_add_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*add_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj,
+				      struct netlink_ext_ack *extack));
 int switchdev_handle_port_obj_del(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
 			bool (*check_cb)(const struct net_device *dev),
 			int (*del_cb)(struct net_device *dev, const void *ctx,
 				      const struct switchdev_obj *obj));
+int switchdev_handle_port_obj_del_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*del_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj));
 
 int switchdev_handle_port_attr_set(struct net_device *dev,
 			struct switchdev_notifier_port_attr_info *port_attr_info,
@@ -446,6 +461,18 @@ switchdev_handle_port_obj_add(struct net_device *dev,
 	return 0;
 }
 
+static inline int switchdev_handle_port_obj_add_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*add_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj,
+				      struct netlink_ext_ack *extack))
+{
+	return 0;
+}
+
 static inline int
 switchdev_handle_port_obj_del(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
@@ -456,6 +483,18 @@ switchdev_handle_port_obj_del(struct net_device *dev,
 	return 0;
 }
 
+static inline int
+switchdev_handle_port_obj_del_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*del_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj))
+{
+	return 0;
+}
+
 static inline int
 switchdev_handle_port_attr_set(struct net_device *dev,
 			struct switchdev_notifier_port_attr_info *port_attr_info,
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index d53f364870a5..6a00c390547b 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -429,6 +429,27 @@ switchdev_lower_dev_find_rcu(struct net_device *dev,
 	return switchdev_priv.lower_dev;
 }
 
+static struct net_device *
+switchdev_lower_dev_find(struct net_device *dev,
+			 bool (*check_cb)(const struct net_device *dev),
+			 bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						      const struct net_device *foreign_dev))
+{
+	struct switchdev_nested_priv switchdev_priv = {
+		.check_cb = check_cb,
+		.foreign_dev_check_cb = foreign_dev_check_cb,
+		.dev = dev,
+		.lower_dev = NULL,
+	};
+	struct netdev_nested_priv priv = {
+		.data = &switchdev_priv,
+	};
+
+	netdev_walk_all_lower_dev(dev, switchdev_lower_dev_walk, &priv);
+
+	return switchdev_priv.lower_dev;
+}
+
 static int __switchdev_handle_fdb_event_to_device(struct net_device *dev,
 		struct net_device *orig_dev, unsigned long event,
 		const struct switchdev_notifier_fdb_info *fdb_info,
@@ -536,13 +557,15 @@ EXPORT_SYMBOL_GPL(switchdev_handle_fdb_event_to_device);
 static int __switchdev_handle_port_obj_add(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
 			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
 			int (*add_cb)(struct net_device *dev, const void *ctx,
 				      const struct switchdev_obj *obj,
 				      struct netlink_ext_ack *extack))
 {
 	struct switchdev_notifier_info *info = &port_obj_info->info;
+	struct net_device *br, *lower_dev;
 	struct netlink_ext_ack *extack;
-	struct net_device *lower_dev;
 	struct list_head *iter;
 	int err = -EOPNOTSUPP;
 
@@ -566,15 +589,42 @@ static int __switchdev_handle_port_obj_add(struct net_device *dev,
 		if (netif_is_bridge_master(lower_dev))
 			continue;
 
+		/* When searching for switchdev interfaces that are neighbors
+		 * of foreign ones, and @dev is a bridge, do not recurse on the
+		 * foreign interface again, it was already visited.
+		 */
+		if (foreign_dev_check_cb && !check_cb(lower_dev) &&
+		    !switchdev_lower_dev_find(lower_dev, check_cb, foreign_dev_check_cb))
+			continue;
+
 		err = __switchdev_handle_port_obj_add(lower_dev, port_obj_info,
-						      check_cb, add_cb);
+						      check_cb, foreign_dev_check_cb,
+						      add_cb);
 		if (err && err != -EOPNOTSUPP)
 			return err;
 	}
 
-	return err;
+	/* Event is neither on a bridge nor a LAG. Check whether it is on an
+	 * interface that is in a bridge with us.
+	 */
+	if (!foreign_dev_check_cb)
+		return err;
+
+	br = netdev_master_upper_dev_get(dev);
+	if (!br || !netif_is_bridge_master(br))
+		return err;
+
+	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))
+		return err;
+
+	return __switchdev_handle_port_obj_add(br, port_obj_info, check_cb,
+					       foreign_dev_check_cb, add_cb);
 }
 
+/* Pass through a port object addition, if @dev passes @check_cb, or replicate
+ * it towards all lower interfaces of @dev that pass @check_cb, if @dev is a
+ * bridge or a LAG.
+ */
 int switchdev_handle_port_obj_add(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
 			bool (*check_cb)(const struct net_device *dev),
@@ -585,21 +635,46 @@ int switchdev_handle_port_obj_add(struct net_device *dev,
 	int err;
 
 	err = __switchdev_handle_port_obj_add(dev, port_obj_info, check_cb,
-					      add_cb);
+					      NULL, add_cb);
 	if (err == -EOPNOTSUPP)
 		err = 0;
 	return err;
 }
 EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_add);
 
+/* Same as switchdev_handle_port_obj_add(), except if object is notified on a
+ * @dev that passes @foreign_dev_check_cb, it is replicated towards all devices
+ * that pass @check_cb and are in the same bridge as @dev.
+ */
+int switchdev_handle_port_obj_add_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*add_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj,
+				      struct netlink_ext_ack *extack))
+{
+	int err;
+
+	err = __switchdev_handle_port_obj_add(dev, port_obj_info, check_cb,
+					      foreign_dev_check_cb, add_cb);
+	if (err == -EOPNOTSUPP)
+		err = 0;
+	return err;
+}
+EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_add_foreign);
+
 static int __switchdev_handle_port_obj_del(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
 			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
 			int (*del_cb)(struct net_device *dev, const void *ctx,
 				      const struct switchdev_obj *obj))
 {
 	struct switchdev_notifier_info *info = &port_obj_info->info;
-	struct net_device *lower_dev;
+	struct net_device *br, *lower_dev;
 	struct list_head *iter;
 	int err = -EOPNOTSUPP;
 
@@ -621,15 +696,42 @@ static int __switchdev_handle_port_obj_del(struct net_device *dev,
 		if (netif_is_bridge_master(lower_dev))
 			continue;
 
+		/* When searching for switchdev interfaces that are neighbors
+		 * of foreign ones, and @dev is a bridge, do not recurse on the
+		 * foreign interface again, it was already visited.
+		 */
+		if (foreign_dev_check_cb && !check_cb(lower_dev) &&
+		    !switchdev_lower_dev_find(lower_dev, check_cb, foreign_dev_check_cb))
+			continue;
+
 		err = __switchdev_handle_port_obj_del(lower_dev, port_obj_info,
-						      check_cb, del_cb);
+						      check_cb, foreign_dev_check_cb,
+						      del_cb);
 		if (err && err != -EOPNOTSUPP)
 			return err;
 	}
 
-	return err;
+	/* Event is neither on a bridge nor a LAG. Check whether it is on an
+	 * interface that is in a bridge with us.
+	 */
+	if (!foreign_dev_check_cb)
+		return err;
+
+	br = netdev_master_upper_dev_get(dev);
+	if (!br || !netif_is_bridge_master(br))
+		return err;
+
+	if (!switchdev_lower_dev_find(br, check_cb, foreign_dev_check_cb))
+		return err;
+
+	return __switchdev_handle_port_obj_del(br, port_obj_info, check_cb,
+					       foreign_dev_check_cb, del_cb);
 }
 
+/* Pass through a port object deletion, if @dev passes @check_cb, or replicate
+ * it towards all lower interfaces of @dev that pass @check_cb, if @dev is a
+ * bridge or a LAG.
+ */
 int switchdev_handle_port_obj_del(struct net_device *dev,
 			struct switchdev_notifier_port_obj_info *port_obj_info,
 			bool (*check_cb)(const struct net_device *dev),
@@ -639,13 +741,35 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
 	int err;
 
 	err = __switchdev_handle_port_obj_del(dev, port_obj_info, check_cb,
-					      del_cb);
+					      NULL, del_cb);
 	if (err == -EOPNOTSUPP)
 		err = 0;
 	return err;
 }
 EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_del);
 
+/* Same as switchdev_handle_port_obj_del(), except if object is notified on a
+ * @dev that passes @foreign_dev_check_cb, it is replicated towards all devices
+ * that pass @check_cb and are in the same bridge as @dev.
+ */
+int switchdev_handle_port_obj_del_foreign(struct net_device *dev,
+			struct switchdev_notifier_port_obj_info *port_obj_info,
+			bool (*check_cb)(const struct net_device *dev),
+			bool (*foreign_dev_check_cb)(const struct net_device *dev,
+						     const struct net_device *foreign_dev),
+			int (*del_cb)(struct net_device *dev, const void *ctx,
+				      const struct switchdev_obj *obj))
+{
+	int err;
+
+	err = __switchdev_handle_port_obj_del(dev, port_obj_info, check_cb,
+					      foreign_dev_check_cb, del_cb);
+	if (err == -EOPNOTSUPP)
+		err = 0;
+	return err;
+}
+EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_del_foreign);
+
 static int __switchdev_handle_port_attr_set(struct net_device *dev,
 			struct switchdev_notifier_port_attr_info *port_attr_info,
 			bool (*check_cb)(const struct net_device *dev),
-- 
2.25.1

[PATCH v2 net-next 7/8] net: dsa: add explicit support for host bridge VLANs

[Vladimir Oltean]

Currently, DSA programs VLANs on shared (DSA and CPU) ports each time it
does so on user ports. This is good for basic functionality but has
several limitations:

- the VLAN group which must reach the CPU may be radically different
  from the VLAN group that must be autonomously forwarded by the switch.
  In other words, the admin may want to isolate noisy stations and avoid
  traffic from them going to the control processor of the switch, where
  it would just waste useless cycles. The bridge already supports
  independent control of VLAN groups on bridge ports and on the bridge
  itself, and when VLAN-aware, it will drop packets in software anyway
  if their VID isn't added as a 'self' entry towards the bridge device.

- Replaying host FDB entries may depend, for some drivers like mv88e6xxx,
  on replaying the host VLANs as well. The 2 VLAN groups are
  approximately the same in most regular cases, but there are corner
  cases when timing matters, and DSA's approximation of replicating
  VLANs on shared ports simply does not work.

- If a user makes the bridge (implicitly the CPU port) join a VLAN by
  accident, there is no way for the CPU port to isolate itself from that
  noisy VLAN except by rebooting the system. This is because for each
  VLAN added on a user port, DSA will add it on shared ports too, but
  for each VLAN deletion on a user port, it will remain installed on
  shared ports, since DSA has no good indication of whether the VLAN is
  still in use or not.

Now that the bridge driver emits well-balanced SWITCHDEV_OBJ_ID_PORT_VLAN
addition and removal events, DSA has a simple and straightforward task
of separating the bridge port VLANs (these have an orig_dev which is a
DSA slave interface, or a LAG interface) from the host VLANs (these have
an orig_dev which is a bridge interface), and to keep a simple reference
count of each VID on each shared port.

Forwarding VLANs must be installed on the bridge ports and on all DSA
ports interconnecting them. We don't have a good view of the exact
topology, so we simply install forwarding VLANs on all DSA ports, which
is what has been done until now.

Host VLANs must be installed primarily on the dedicated CPU port of each
bridge port. More subtly, they must also be installed on upstream-facing
and downstream-facing DSA ports that are connecting the bridge ports and
the CPU. This ensures that the mv88e6xxx's problem (VID of host FDB
entry may be absent from VTU) is still addressed even if that switch is
in a cross-chip setup, and it has no local CPU port.

Therefore:
- user ports contain only bridge port (forwarding) VLANs, and no
  refcounting is necessary
- DSA ports contain both forwarding and host VLANs. Refcounting is
  necessary among these 2 types.
- CPU ports contain only host VLANs. Refcounting is also necessary.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: figure out locally within DSA whether to bump the refcount or
        not for a VLAN, based on the new "changed" and "old_flags"
        properties.

 include/net/dsa.h  |  10 +++
 net/dsa/dsa2.c     |   2 +
 net/dsa/dsa_priv.h |   7 ++
 net/dsa/port.c     |  42 ++++++++++
 net/dsa/slave.c    | 112 +++++++++++++++++----------
 net/dsa/switch.c   | 187 +++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 313 insertions(+), 47 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 3f683773aaf6..1456313a1faa 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -312,6 +312,10 @@ struct dsa_port {
 	struct mutex		addr_lists_lock;
 	struct list_head	fdbs;
 	struct list_head	mdbs;
+
+	/* List of VLANs that CPU and DSA ports are members of. */
+	struct mutex		vlans_lock;
+	struct list_head	vlans;
 };
 
 /* TODO: ideally DSA ports would have a single dp->link_dp member,
@@ -332,6 +336,12 @@ struct dsa_mac_addr {
 	struct list_head list;
 };
 
+struct dsa_vlan {
+	u16 vid;
+	refcount_t refcount;
+	struct list_head list;
+};
+
 struct dsa_switch {
 	struct device *dev;
 
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e498c927c3d0..1df8c2356463 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -453,8 +453,10 @@ static int dsa_port_setup(struct dsa_port *dp)
 		return 0;
 
 	mutex_init(&dp->addr_lists_lock);
+	mutex_init(&dp->vlans_lock);
 	INIT_LIST_HEAD(&dp->fdbs);
 	INIT_LIST_HEAD(&dp->mdbs);
+	INIT_LIST_HEAD(&dp->vlans);
 
 	if (ds->ops->port_setup) {
 		err = ds->ops->port_setup(ds, dp->index);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 6076d695a917..a37f0883676a 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -34,6 +34,8 @@ enum {
 	DSA_NOTIFIER_HOST_MDB_DEL,
 	DSA_NOTIFIER_VLAN_ADD,
 	DSA_NOTIFIER_VLAN_DEL,
+	DSA_NOTIFIER_HOST_VLAN_ADD,
+	DSA_NOTIFIER_HOST_VLAN_DEL,
 	DSA_NOTIFIER_MTU,
 	DSA_NOTIFIER_TAG_PROTO,
 	DSA_NOTIFIER_TAG_PROTO_CONNECT,
@@ -233,6 +235,11 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct netlink_ext_ack *extack);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
+int dsa_port_host_vlan_add(struct dsa_port *dp,
+			   const struct switchdev_obj_port_vlan *vlan,
+			   struct netlink_ext_ack *extack);
+int dsa_port_host_vlan_del(struct dsa_port *dp,
+			   const struct switchdev_obj_port_vlan *vlan);
 int dsa_port_mrp_add(const struct dsa_port *dp,
 		     const struct switchdev_obj_mrp *mrp);
 int dsa_port_mrp_del(const struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index bd78192e0e47..cca5cf686f74 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -904,6 +904,48 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
+int dsa_port_host_vlan_add(struct dsa_port *dp,
+			   const struct switchdev_obj_port_vlan *vlan,
+			   struct netlink_ext_ack *extack)
+{
+	struct dsa_notifier_vlan_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.vlan = vlan,
+		.extack = extack,
+	};
+	struct dsa_port *cpu_dp = dp->cpu_dp;
+	int err;
+
+	err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_ADD, &info);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	vlan_vid_add(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid);
+
+	return err;
+}
+
+int dsa_port_host_vlan_del(struct dsa_port *dp,
+			   const struct switchdev_obj_port_vlan *vlan)
+{
+	struct dsa_notifier_vlan_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.vlan = vlan,
+	};
+	struct dsa_port *cpu_dp = dp->cpu_dp;
+	int err;
+
+	err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_DEL, &info);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
+	vlan_vid_del(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid);
+
+	return err;
+}
+
 int dsa_port_mrp_add(const struct dsa_port *dp,
 		     const struct switchdev_obj_mrp *mrp)
 {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2f6caf5d037e..314628c34084 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -348,9 +348,8 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 			      const struct switchdev_obj *obj,
 			      struct netlink_ext_ack *extack)
 {
-	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-	struct switchdev_obj_port_vlan vlan;
+	struct switchdev_obj_port_vlan *vlan;
 	int err;
 
 	if (dsa_port_skip_vlan_configuration(dp)) {
@@ -358,14 +357,14 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 		return 0;
 	}
 
-	vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj);
+	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
 	/* Deny adding a bridge VLAN when there is already an 802.1Q upper with
 	 * the same VID.
 	 */
 	if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
 		rcu_read_lock();
-		err = dsa_slave_vlan_check_for_8021q_uppers(dev, &vlan);
+		err = dsa_slave_vlan_check_for_8021q_uppers(dev, vlan);
 		rcu_read_unlock();
 		if (err) {
 			NL_SET_ERR_MSG_MOD(extack,
@@ -374,21 +373,44 @@ static int dsa_slave_vlan_add(struct net_device *dev,
 		}
 	}
 
-	err = dsa_port_vlan_add(dp, &vlan, extack);
-	if (err)
-		return err;
+	return dsa_port_vlan_add(dp, vlan, extack);
+}
+
+static int dsa_slave_host_vlan_add(struct net_device *dev,
+				   const struct switchdev_obj *obj,
+				   struct netlink_ext_ack *extack)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan;
+
+	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);
+
+	/* Skip case 3 VLANs from __vlan_add() from the bridge driver
+	 * (master VLANs with no brentry created yet, just for context).
+	 * I've no idea why the bridge even notifies these.
+	 */
+	if (!(vlan.flags & BRIDGE_VLAN_INFO_BRENTRY))
+		return 0;
 
-	/* We need the dedicated CPU port to be a member of the VLAN as well.
-	 * Even though drivers often handle CPU membership in special ways,
+	/* Because we've skipped master VLANs while they weren't yet
+	 * brentries, we need to treat the flag change towards brentry
+	 * as a new VLAN, and refcount it in dsa_port_do_vlan_add().
+	 */
+	if (vlan.changed &&
+	    (vlan.flags ^ vlan.old_flags) & BRIDGE_VLAN_INFO_BRENTRY)
+		vlan.changed = false;
+
+	/* Even though drivers often handle CPU membership in special ways,
 	 * it doesn't make sense to program a PVID, so clear this flag.
 	 */
 	vlan.flags &= ~BRIDGE_VLAN_INFO_PVID;
 
-	err = dsa_port_vlan_add(dp->cpu_dp, &vlan, extack);
-	if (err)
-		return err;
-
-	return vlan_vid_add(master, htons(ETH_P_8021Q), vlan.vid);
+	return dsa_port_host_vlan_add(dp, &vlan, extack);
 }
 
 static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
@@ -415,10 +437,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
 		err = dsa_port_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
-			return -EOPNOTSUPP;
+		if (netif_is_bridge_master(obj->orig_dev)) {
+			if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
+				return -EOPNOTSUPP;
 
-		err = dsa_slave_vlan_add(dev, obj, extack);
+			err = dsa_slave_host_vlan_add(dev, obj, extack);
+		} else {
+			if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
+				return -EOPNOTSUPP;
+
+			err = dsa_slave_vlan_add(dev, obj, extack);
+		}
 		break;
 	case SWITCHDEV_OBJ_ID_MRP:
 		if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
@@ -444,26 +473,29 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
 static int dsa_slave_vlan_del(struct net_device *dev,
 			      const struct switchdev_obj *obj)
 {
-	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct switchdev_obj_port_vlan *vlan;
-	int err;
 
 	if (dsa_port_skip_vlan_configuration(dp))
 		return 0;
 
 	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
 
-	/* Do not deprogram the CPU port as it may be shared with other user
-	 * ports which can be members of this VLAN as well.
-	 */
-	err = dsa_port_vlan_del(dp, vlan);
-	if (err)
-		return err;
+	return dsa_port_vlan_del(dp, vlan);
+}
 
-	vlan_vid_del(master, htons(ETH_P_8021Q), vlan->vid);
+static int dsa_slave_host_vlan_del(struct net_device *dev,
+				   const struct switchdev_obj *obj)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan *vlan;
 
-	return 0;
+	if (dsa_port_skip_vlan_configuration(dp))
+		return 0;
+
+	vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
+
+	return dsa_port_host_vlan_del(dp, vlan);
 }
 
 static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,
@@ -489,10 +521,17 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,
 		err = dsa_port_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
-			return -EOPNOTSUPP;
+		if (netif_is_bridge_master(obj->orig_dev)) {
+			if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
+				return -EOPNOTSUPP;
+
+			err = dsa_slave_host_vlan_del(dev, obj);
+		} else {
+			if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
+				return -EOPNOTSUPP;
 
-		err = dsa_slave_vlan_del(dev, obj);
+			err = dsa_slave_vlan_del(dev, obj);
+		}
 		break;
 	case SWITCHDEV_OBJ_ID_MRP:
 		if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
@@ -1347,7 +1386,6 @@ static int dsa_slave_get_ts_info(struct net_device *dev,
 static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 				     u16 vid)
 {
-	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct switchdev_obj_port_vlan vlan = {
 		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
@@ -1367,7 +1405,7 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 	}
 
 	/* And CPU port... */
-	ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &extack);
+	ret = dsa_port_host_vlan_add(dp, &vlan, &extack);
 	if (ret) {
 		if (extack._msg)
 			netdev_err(dev, "CPU port %d: %s\n", dp->cpu_dp->index,
@@ -1375,13 +1413,12 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 		return ret;
 	}
 
-	return vlan_vid_add(master, proto, vid);
+	return 0;
 }
 
 static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 				      u16 vid)
 {
-	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	struct switchdev_obj_port_vlan vlan = {
 		.vid = vid,
@@ -1390,16 +1427,11 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	};
 	int err;
 
-	/* Do not deprogram the CPU port as it may be shared with other user
-	 * ports which can be members of this VLAN as well.
-	 */
 	err = dsa_port_vlan_del(dp, &vlan);
 	if (err)
 		return err;
 
-	vlan_vid_del(master, proto, vid);
-
-	return 0;
+	return dsa_port_host_vlan_del(dp, &vlan);
 }
 
 static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg)
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 4866b58649e4..0bb3987bd4e6 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -558,6 +558,7 @@ static int dsa_switch_host_mdb_del(struct dsa_switch *ds,
 	return err;
 }
 
+/* Port VLANs match on the targeted port and on all DSA ports */
 static bool dsa_port_vlan_match(struct dsa_port *dp,
 				struct dsa_notifier_vlan_info *info)
 {
@@ -570,6 +571,126 @@ static bool dsa_port_vlan_match(struct dsa_port *dp,
 	return false;
 }
 
+/* Host VLANs match on the targeted port's CPU port, and on all DSA ports
+ * (upstream and downstream) of that switch and its upstream switches.
+ */
+static bool dsa_port_host_vlan_match(struct dsa_port *dp,
+				     struct dsa_notifier_vlan_info *info)
+{
+	struct dsa_port *targeted_dp, *cpu_dp;
+	struct dsa_switch *targeted_ds;
+
+	targeted_ds = dsa_switch_find(dp->ds->dst->index, info->sw_index);
+	targeted_dp = dsa_to_port(targeted_ds, info->port);
+	cpu_dp = targeted_dp->cpu_dp;
+
+	if (dsa_switch_is_upstream_of(dp->ds, targeted_ds))
+		return dsa_port_is_dsa(dp) || dp == cpu_dp;
+
+	return false;
+}
+
+static struct dsa_vlan *dsa_vlan_find(struct list_head *vlan_list,
+				      const struct switchdev_obj_port_vlan *vlan)
+{
+	struct dsa_vlan *v;
+
+	list_for_each_entry(v, vlan_list, list)
+		if (v->vid == vlan->vid)
+			return v;
+
+	return NULL;
+}
+
+static int dsa_port_do_vlan_add(struct dsa_port *dp,
+				const struct switchdev_obj_port_vlan *vlan,
+				struct netlink_ext_ack *extack)
+{
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
+	struct dsa_vlan *v;
+	int err = 0;
+
+	/* No need to bother with refcounting for user ports. */
+	if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
+		return ds->ops->port_vlan_add(ds, port, vlan, extack);
+
+	/* No need to propagate on shared ports the existing VLANs that were
+	 * re-notified after just the flags have changed. This would cause a
+	 * refcount bump which we need to avoid, since it unbalances the
+	 * additions with the deletions.
+	 */
+	if (vlan->changed)
+		return 0;
+
+	mutex_lock(&dp->vlans_lock);
+
+	v = dsa_vlan_find(&dp->vlans, vlan);
+	if (v) {
+		refcount_inc(&v->refcount);
+		goto out;
+	}
+
+	v = kzalloc(sizeof(*v), GFP_KERNEL);
+	if (!v) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = ds->ops->port_vlan_add(ds, port, vlan, extack);
+	if (err) {
+		kfree(v);
+		goto out;
+	}
+
+	v->vid = vlan->vid;
+	refcount_set(&v->refcount, 1);
+	list_add_tail(&v->list, &dp->vlans);
+
+out:
+	mutex_unlock(&dp->vlans_lock);
+
+	return err;
+}
+
+static int dsa_port_do_vlan_del(struct dsa_port *dp,
+				const struct switchdev_obj_port_vlan *vlan)
+{
+	struct dsa_switch *ds = dp->ds;
+	int port = dp->index;
+	struct dsa_vlan *v;
+	int err = 0;
+
+	/* No need to bother with refcounting for user ports */
+	if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
+		return ds->ops->port_vlan_del(ds, port, vlan);
+
+	mutex_lock(&dp->vlans_lock);
+
+	v = dsa_vlan_find(&dp->vlans, vlan);
+	if (!v) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	if (!refcount_dec_and_test(&v->refcount))
+		goto out;
+
+	err = ds->ops->port_vlan_del(ds, port, vlan);
+	if (err) {
+		refcount_set(&v->refcount, 1);
+		goto out;
+	}
+
+	list_del(&v->list);
+	kfree(v);
+
+out:
+	mutex_unlock(&dp->vlans_lock);
+
+	return err;
+}
+
 static int dsa_switch_vlan_add(struct dsa_switch *ds,
 			       struct dsa_notifier_vlan_info *info)
 {
@@ -581,8 +702,8 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 
 	dsa_switch_for_each_port(dp, ds) {
 		if (dsa_port_vlan_match(dp, info)) {
-			err = ds->ops->port_vlan_add(ds, dp->index, info->vlan,
-						     info->extack);
+			err = dsa_port_do_vlan_add(dp, info->vlan,
+						   info->extack);
 			if (err)
 				return err;
 		}
@@ -594,15 +715,61 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 static int dsa_switch_vlan_del(struct dsa_switch *ds,
 			       struct dsa_notifier_vlan_info *info)
 {
+	struct dsa_port *dp;
+	int err;
+
 	if (!ds->ops->port_vlan_del)
 		return -EOPNOTSUPP;
 
-	if (ds->index == info->sw_index)
-		return ds->ops->port_vlan_del(ds, info->port, info->vlan);
+	dsa_switch_for_each_port(dp, ds) {
+		if (dsa_port_vlan_match(dp, info)) {
+			err = dsa_port_do_vlan_del(dp, info->vlan);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+static int dsa_switch_host_vlan_add(struct dsa_switch *ds,
+				    struct dsa_notifier_vlan_info *info)
+{
+	struct dsa_port *dp;
+	int err;
+
+	if (!ds->ops->port_vlan_add)
+		return -EOPNOTSUPP;
+
+	dsa_switch_for_each_port(dp, ds) {
+		if (dsa_port_host_vlan_match(dp, info)) {
+			err = dsa_port_do_vlan_add(dp, info->vlan,
+						   info->extack);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
+static int dsa_switch_host_vlan_del(struct dsa_switch *ds,
+				    struct dsa_notifier_vlan_info *info)
+{
+	struct dsa_port *dp;
+	int err;
+
+	if (!ds->ops->port_vlan_del)
+		return -EOPNOTSUPP;
+
+	dsa_switch_for_each_port(dp, ds) {
+		if (dsa_port_host_vlan_match(dp, info)) {
+			err = dsa_port_do_vlan_del(dp, info->vlan);
+			if (err)
+				return err;
+		}
+	}
 
-	/* Do not deprogram the DSA links as they may be used as conduit
-	 * for other VLAN members in the fabric.
-	 */
 	return 0;
 }
 
@@ -764,6 +931,12 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_VLAN_DEL:
 		err = dsa_switch_vlan_del(ds, info);
 		break;
+	case DSA_NOTIFIER_HOST_VLAN_ADD:
+		err = dsa_switch_host_vlan_add(ds, info);
+		break;
+	case DSA_NOTIFIER_HOST_VLAN_DEL:
+		err = dsa_switch_host_vlan_del(ds, info);
+		break;
 	case DSA_NOTIFIER_MTU:
 		err = dsa_switch_mtu(ds, info);
 		break;
-- 
2.25.1

[PATCH v2 net-next 8/8] net: dsa: offload bridge port VLANs on foreign interfaces

[Vladimir Oltean]

DSA now explicitly handles VLANs installed with the 'self' flag on the
bridge as host VLANs, instead of just replicating every bridge port VLAN
also on the CPU port and never deleting it, which is what it did before.

However, this leaves a corner case uncovered, as explained by
Tobias Waldekranz:
https://patchwork.kernel.org/project/netdevbpf/patch/20220209213044.2353153-6-vladimir.oltean@nxp.com/#24735260

Forwarding towards a bridge port VLAN installed on a bridge port foreign
to DSA (separate NIC, Wi-Fi AP) used to work by virtue of the fact that
DSA itself needed to have at least one port in that VLAN (therefore, it
also had the CPU port in said VLAN). However, now that the CPU port may
not be member of all VLANs that user ports are members of, we need to
ensure this isn't the case if software forwarding to a foreign interface
is required.

The solution is to treat bridge port VLANs on standalone interfaces in
the exact same way as host VLANs. From DSA's perspective, there is no
difference between local termination and software forwarding; packets in
that VLAN must reach the CPU in both cases.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: patch is new

 net/dsa/dsa2.c  |  6 +++++
 net/dsa/slave.c | 70 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 1df8c2356463..408b79a28cd4 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -565,6 +565,7 @@ static void dsa_port_teardown(struct dsa_port *dp)
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a, *tmp;
 	struct net_device *slave;
+	struct dsa_vlan *v, *n;
 
 	if (!dp->setup)
 		return;
@@ -605,6 +606,11 @@ static void dsa_port_teardown(struct dsa_port *dp)
 		kfree(a);
 	}
 
+	list_for_each_entry_safe(v, n, &dp->vlans, list) {
+		list_del(&v->list);
+		kfree(v);
+	}
+
 	dp->setup = false;
 }
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 314628c34084..9ca38654b61b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -413,6 +413,31 @@ static int dsa_slave_host_vlan_add(struct net_device *dev,
 	return dsa_port_host_vlan_add(dp, &vlan, extack);
 }
 
+/* For DSA ports that offload a bridge port, also offload bridge port VLANs on
+ * foreign interfaces the same way as host VLANs.
+ */
+static int dsa_slave_foreign_vlan_add(struct net_device *dev,
+				      const struct switchdev_obj *obj,
+				      struct netlink_ext_ack *extack)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan;
+
+	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);
+
+	vlan.flags &= ~BRIDGE_VLAN_INFO_PVID;
+
+	return dsa_port_host_vlan_add(dp, &vlan, extack);
+}
+
 static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
 				  const struct switchdev_obj *obj,
 				  struct netlink_ext_ack *extack)
@@ -442,11 +467,10 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
 				return -EOPNOTSUPP;
 
 			err = dsa_slave_host_vlan_add(dev, obj, extack);
-		} else {
-			if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
-				return -EOPNOTSUPP;
-
+		} else if (dsa_port_offloads_bridge_port(dp, obj->orig_dev)) {
 			err = dsa_slave_vlan_add(dev, obj, extack);
+		} else {
+			err = dsa_slave_foreign_vlan_add(dev, obj, extack);
 		}
 		break;
 	case SWITCHDEV_OBJ_ID_MRP:
@@ -498,6 +522,23 @@ static int dsa_slave_host_vlan_del(struct net_device *dev,
 	return dsa_port_host_vlan_del(dp, vlan);
 }
 
+static int dsa_slave_foreign_vlan_del(struct net_device *dev,
+				      const struct switchdev_obj *obj)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan *vlan;
+
+	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);
+}
+
 static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,
 				  const struct switchdev_obj *obj)
 {
@@ -526,11 +567,10 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,
 				return -EOPNOTSUPP;
 
 			err = dsa_slave_host_vlan_del(dev, obj);
-		} else {
-			if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev))
-				return -EOPNOTSUPP;
-
+		} else if (dsa_port_offloads_bridge_port(dp, obj->orig_dev)) {
 			err = dsa_slave_vlan_del(dev, obj);
+		} else {
+			err = dsa_slave_foreign_vlan_del(dev, obj);
 		}
 		break;
 	case SWITCHDEV_OBJ_ID_MRP:
@@ -2562,14 +2602,16 @@ static int dsa_slave_switchdev_blocking_event(struct notifier_block *unused,
 
 	switch (event) {
 	case SWITCHDEV_PORT_OBJ_ADD:
-		err = switchdev_handle_port_obj_add(dev, ptr,
-						    dsa_slave_dev_check,
-						    dsa_slave_port_obj_add);
+		err = switchdev_handle_port_obj_add_foreign(dev, ptr,
+							    dsa_slave_dev_check,
+							    dsa_foreign_dev_check,
+							    dsa_slave_port_obj_add);
 		return notifier_from_errno(err);
 	case SWITCHDEV_PORT_OBJ_DEL:
-		err = switchdev_handle_port_obj_del(dev, ptr,
-						    dsa_slave_dev_check,
-						    dsa_slave_port_obj_del);
+		err = switchdev_handle_port_obj_del_foreign(dev, ptr,
+							    dsa_slave_dev_check,
+							    dsa_foreign_dev_check,
+							    dsa_slave_port_obj_del);
 		return notifier_from_errno(err);
 	case SWITCHDEV_PORT_ATTR_SET:
 		err = switchdev_handle_port_attr_set(dev, ptr,
-- 
2.25.1

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 01:31:04AM +0200, Vladimir Oltean wrote:
> +/* return true if anything will change as a result of __vlan_add_flags,
> + * false otherwise
> + */
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> +	struct net_bridge_vlan_group *vg;
> +	u16 old_flags = v->flags;
> +	bool pvid_changed;
>  
> -	return ret || !!(old_flags ^ v->flags);
> +	if (br_vlan_is_master(v))
> +		vg = br_vlan_group(v->br);
> +	else
> +		vg = nbp_vlan_group(v->port);
> +
> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> +		pvid_changed = (vg->pvid == v->vid);
> +	else
> +		pvid_changed = (vg->pvid != v->vid);

Yikes, I was planning to fix this but I forgot. The conditions are in
reverse, it should be:

	if (flags & BRIDGE_VLAN_INFO_PVID)
		pvid_changed = (vg->pvid != v->vid);
	else
		pvid_changed = (vg->pvid == v->vid);

> +
> +	return pvid_changed || !!(old_flags ^ v->flags);
>  }

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 01:31, Vladimir Oltean wrote:
> Currently, when a VLAN entry is added multiple times in a row to a
> bridge port, nbp_vlan_add() calls br_switchdev_port_vlan_add() each
> time, even if the VLAN already exists and nothing about it has changed:
> 
> bridge vlan add dev lan12 vid 100 master static
> 
> Similarly, when a VLAN is added multiple times in a row to a bridge,
> br_vlan_add_existing() doesn't filter at all the calls to
> br_switchdev_port_vlan_add():
> 
> bridge vlan add dev br0 vid 100 self
> 
> This behavior makes driver-level accounting of VLANs impossible, since
> it is enough for a single deletion event to remove a VLAN, but the
> addition event can be emitted an unlimited number of times.
> 
> The cause for this can be identified as follows: we rely on
> __vlan_add_flags() to retroactively tell us whether it has changed
> anything about the VLAN flags or VLAN group pvid. So we'd first have to
> call __vlan_add_flags() before calling br_switchdev_port_vlan_add(), in
> order to have access to the "bool *changed" information. But we don't
> want to change the event ordering, because we'd have to revert the
> struct net_bridge_vlan changes we've made if switchdev returns an error.
> 
> So to solve this, we need another function that tells us whether any
> change is going to occur in the VLAN or VLAN group, _prior_ to calling
> __vlan_add_flags(). In fact, we even make __vlan_add_flags() return void.
> 
> With this lookahead function in place, we can avoid notifying switchdev
> if nothing changed for the VLAN and VLAN group.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: drop the br_vlan_restore_existing() approach, just figure out
>         ahead of time what will change.
> 
>  net/bridge/br_vlan.c | 71 ++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 25 deletions(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1402d5ca242d..c5355695c976 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -34,36 +34,29 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
>  	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
>  }
>  
> -static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
> +static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>  			    const struct net_bridge_vlan *v)
>  {
>  	if (vg->pvid == v->vid)
> -		return false;
> +		return;
>  
>  	smp_wmb();
>  	br_vlan_set_pvid_state(vg, v->state);
>  	vg->pvid = v->vid;
> -
> -	return true;
>  }
>  
> -static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
> +static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>  {
>  	if (vg->pvid != vid)
> -		return false;
> +		return;
>  
>  	smp_wmb();
>  	vg->pvid = 0;
> -
> -	return true;
>  }
>  
> -/* return true if anything changed, false otherwise */
> -static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> +static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>  {
>  	struct net_bridge_vlan_group *vg;
> -	u16 old_flags = v->flags;
> -	bool ret;
>  
>  	if (br_vlan_is_master(v))
>  		vg = br_vlan_group(v->br);
> @@ -71,16 +64,36 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>  		vg = nbp_vlan_group(v->port);
>  
>  	if (flags & BRIDGE_VLAN_INFO_PVID)
> -		ret = __vlan_add_pvid(vg, v);
> +		__vlan_add_pvid(vg, v);
>  	else
> -		ret = __vlan_delete_pvid(vg, v->vid);
> +		__vlan_delete_pvid(vg, v->vid);
>  
>  	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>  		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>  	else
>  		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
> +}
> +
> +/* return true if anything will change as a result of __vlan_add_flags,
> + * false otherwise
> + */
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> +	struct net_bridge_vlan_group *vg;
> +	u16 old_flags = v->flags;
> +	bool pvid_changed;
>  
> -	return ret || !!(old_flags ^ v->flags);
> +	if (br_vlan_is_master(v))
> +		vg = br_vlan_group(v->br);
> +	else
> +		vg = nbp_vlan_group(v->port);
> +
> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> +		pvid_changed = (vg->pvid == v->vid);
> +	else
> +		pvid_changed = (vg->pvid != v->vid);
> +
> +	return pvid_changed || !!(old_flags ^ v->flags);
>  }

These two have to depend on each other, otherwise it's error-prone and
surely in the future someone will forget to update both.
How about add a "commit" argument to __vlan_add_flags and possibly rename
it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
with commit == false and __vlan_update_flags_commit with commit == true.
Or some other naming, the point is to always use the same flow and checks
when updating the flags to make sure people don't forget.

>  
>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> @@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
>  {
>  	int err;
>  
> -	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
> -	if (err && err != -EOPNOTSUPP)
> -		return err;
> +	*changed = __vlan_flags_would_change(vlan, flags);
> +	if (*changed) {
> +		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
> +						 extack);
> +		if (err && err != -EOPNOTSUPP)
> +			return err;
> +	}

You should revert *changed to false in the error path below,
otherwise we will emit a notification without anything changed
if the br_vlan_is_brentry(vlan)) { } block errors out.

>  
>  	if (!br_vlan_is_brentry(vlan)) {
>  		/* Trying to change flags of non-existent bridge vlan */
> @@ -696,8 +713,7 @@ static int br_vlan_add_existing(struct net_bridge *br,
>  		br_multicast_toggle_one_vlan(vlan, true);
>  	}
>  
> -	if (__vlan_add_flags(vlan, flags))
> -		*changed = true;
> +	__vlan_add_flags(vlan, flags);
>  
>  	return 0;
>  
> @@ -1247,11 +1263,16 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
>  	*changed = false;
>  	vlan = br_vlan_find(nbp_vlan_group(port), vid);
>  	if (vlan) {
> -		/* Pass the flags to the hardware bridge */
> -		ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
> -		if (ret && ret != -EOPNOTSUPP)
> -			return ret;
> -		*changed = __vlan_add_flags(vlan, flags);
> +		*changed = __vlan_flags_would_change(vlan, flags);
> +		if (*changed) {
> +			/* Pass the flags to the hardware bridge */
> +			ret = br_switchdev_port_vlan_add(port->dev, vid,
> +							 flags, extack);
> +			if (ret && ret != -EOPNOTSUPP)
> +				return ret;
> +		}
> +
> +		__vlan_add_flags(vlan, flags);
>  
>  		return 0;
>  	}

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> > +/* return true if anything will change as a result of __vlan_add_flags,
> > + * false otherwise
> > + */
> > +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> > +{
> > +	struct net_bridge_vlan_group *vg;
> > +	u16 old_flags = v->flags;
> > +	bool pvid_changed;
> >  
> > -	return ret || !!(old_flags ^ v->flags);
> > +	if (br_vlan_is_master(v))
> > +		vg = br_vlan_group(v->br);
> > +	else
> > +		vg = nbp_vlan_group(v->port);
> > +
> > +	if (flags & BRIDGE_VLAN_INFO_PVID)
> > +		pvid_changed = (vg->pvid == v->vid);
> > +	else
> > +		pvid_changed = (vg->pvid != v->vid);
> > +
> > +	return pvid_changed || !!(old_flags ^ v->flags);
> >  }
> 
> These two have to depend on each other, otherwise it's error-prone and
> surely in the future someone will forget to update both.
> How about add a "commit" argument to __vlan_add_flags and possibly rename
> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
> with commit == false and __vlan_update_flags_commit with commit == true.
> Or some other naming, the point is to always use the same flow and checks
> when updating the flags to make sure people don't forget.

You want to squash __vlan_flags_would_change() and __vlan_add_flags()
into a single function? But "would_change" returns bool, and "add"
returns void.

> >  
> >  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
> > @@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
> >  {
> >  	int err;
> >  
> > -	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
> > -	if (err && err != -EOPNOTSUPP)
> > -		return err;
> > +	*changed = __vlan_flags_would_change(vlan, flags);
> > +	if (*changed) {
> > +		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
> > +						 extack);
> > +		if (err && err != -EOPNOTSUPP)
> > +			return err;
> > +	}
> 
> You should revert *changed to false in the error path below,
> otherwise we will emit a notification without anything changed
> if the br_vlan_is_brentry(vlan)) { } block errors out.

Ok.

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 11:54:05AM +0200, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> > > +/* return true if anything will change as a result of __vlan_add_flags,
> > > + * false otherwise
> > > + */
> > > +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> > > +{
> > > +	struct net_bridge_vlan_group *vg;
> > > +	u16 old_flags = v->flags;
> > > +	bool pvid_changed;
> > >  
> > > -	return ret || !!(old_flags ^ v->flags);
> > > +	if (br_vlan_is_master(v))
> > > +		vg = br_vlan_group(v->br);
> > > +	else
> > > +		vg = nbp_vlan_group(v->port);
> > > +
> > > +	if (flags & BRIDGE_VLAN_INFO_PVID)
> > > +		pvid_changed = (vg->pvid == v->vid);
> > > +	else
> > > +		pvid_changed = (vg->pvid != v->vid);
> > > +
> > > +	return pvid_changed || !!(old_flags ^ v->flags);
> > >  }
> > 
> > These two have to depend on each other, otherwise it's error-prone and
> > surely in the future someone will forget to update both.
> > How about add a "commit" argument to __vlan_add_flags and possibly rename
> > it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
> > with commit == false and __vlan_update_flags_commit with commit == true.
> > Or some other naming, the point is to always use the same flow and checks
> > when updating the flags to make sure people don't forget.
> 
> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> into a single function? But "would_change" returns bool, and "add"
> returns void.

Plus, we have call paths that would bypass the "prepare" stage and jump
to commit, and for good reason. Do we want to change those as well, or
what would be the benefit?

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 11:54, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
>>> +/* return true if anything will change as a result of __vlan_add_flags,
>>> + * false otherwise
>>> + */
>>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
>>> +{
>>> +	struct net_bridge_vlan_group *vg;
>>> +	u16 old_flags = v->flags;
>>> +	bool pvid_changed;
>>>  
>>> -	return ret || !!(old_flags ^ v->flags);
>>> +	if (br_vlan_is_master(v))
>>> +		vg = br_vlan_group(v->br);
>>> +	else
>>> +		vg = nbp_vlan_group(v->port);
>>> +
>>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
>>> +		pvid_changed = (vg->pvid == v->vid);
>>> +	else
>>> +		pvid_changed = (vg->pvid != v->vid);
>>> +
>>> +	return pvid_changed || !!(old_flags ^ v->flags);
>>>  }
>>
>> These two have to depend on each other, otherwise it's error-prone and
>> surely in the future someone will forget to update both.
>> How about add a "commit" argument to __vlan_add_flags and possibly rename
>> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
>> with commit == false and __vlan_update_flags_commit with commit == true.
>> Or some other naming, the point is to always use the same flow and checks
>> when updating the flags to make sure people don't forget.
> 
> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> into a single function? But "would_change" returns bool, and "add"
> returns void.
> 

Hence the wrappers for commit == false and commit == true. You could name the precommit
one __vlan_flags_would_change or something more appropriate. The point is to make
sure we always update both when flags are changed.

>>>  
>>>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>>> @@ -672,9 +685,13 @@ static int br_vlan_add_existing(struct net_bridge *br,
>>>  {
>>>  	int err;
>>>  
>>> -	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
>>> -	if (err && err != -EOPNOTSUPP)
>>> -		return err;
>>> +	*changed = __vlan_flags_would_change(vlan, flags);
>>> +	if (*changed) {
>>> +		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
>>> +						 extack);
>>> +		if (err && err != -EOPNOTSUPP)
>>> +			return err;
>>> +	}
>>
>> You should revert *changed to false in the error path below,
>> otherwise we will emit a notification without anything changed
>> if the br_vlan_is_brentry(vlan)) { } block errors out.
> 
> Ok.

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 12:10, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 11:54:05AM +0200, Vladimir Oltean wrote:
>> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
>>>> +/* return true if anything will change as a result of __vlan_add_flags,
>>>> + * false otherwise
>>>> + */
>>>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
>>>> +{
>>>> +	struct net_bridge_vlan_group *vg;
>>>> +	u16 old_flags = v->flags;
>>>> +	bool pvid_changed;
>>>>  
>>>> -	return ret || !!(old_flags ^ v->flags);
>>>> +	if (br_vlan_is_master(v))
>>>> +		vg = br_vlan_group(v->br);
>>>> +	else
>>>> +		vg = nbp_vlan_group(v->port);
>>>> +
>>>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
>>>> +		pvid_changed = (vg->pvid == v->vid);
>>>> +	else
>>>> +		pvid_changed = (vg->pvid != v->vid);
>>>> +
>>>> +	return pvid_changed || !!(old_flags ^ v->flags);
>>>>  }
>>>
>>> These two have to depend on each other, otherwise it's error-prone and
>>> surely in the future someone will forget to update both.
>>> How about add a "commit" argument to __vlan_add_flags and possibly rename
>>> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
>>> with commit == false and __vlan_update_flags_commit with commit == true.
>>> Or some other naming, the point is to always use the same flow and checks
>>> when updating the flags to make sure people don't forget.
>>
>> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
>> into a single function? But "would_change" returns bool, and "add"
>> returns void.
> 
> Plus, we have call paths that would bypass the "prepare" stage and jump
> to commit, and for good reason. Do we want to change those as well, or
> what would be the benefit?

It's not really prepare (doesn't have any effect), it's a precommit check.
You can keep the would change name, just make sure both wrappers use the
same function so people will be changing just 1 function and won't forget
to update anything. Would be nice to add a comment about it.

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
> On 15/02/2022 11:54, Vladimir Oltean wrote:
> > On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> >>> +/* return true if anything will change as a result of __vlan_add_flags,
> >>> + * false otherwise
> >>> + */
> >>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> >>> +{
> >>> +	struct net_bridge_vlan_group *vg;
> >>> +	u16 old_flags = v->flags;
> >>> +	bool pvid_changed;
> >>>  
> >>> -	return ret || !!(old_flags ^ v->flags);
> >>> +	if (br_vlan_is_master(v))
> >>> +		vg = br_vlan_group(v->br);
> >>> +	else
> >>> +		vg = nbp_vlan_group(v->port);
> >>> +
> >>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> >>> +		pvid_changed = (vg->pvid == v->vid);
> >>> +	else
> >>> +		pvid_changed = (vg->pvid != v->vid);
> >>> +
> >>> +	return pvid_changed || !!(old_flags ^ v->flags);
> >>>  }
> >>
> >> These two have to depend on each other, otherwise it's error-prone and
> >> surely in the future someone will forget to update both.
> >> How about add a "commit" argument to __vlan_add_flags and possibly rename
> >> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
> >> with commit == false and __vlan_update_flags_commit with commit == true.
> >> Or some other naming, the point is to always use the same flow and checks
> >> when updating the flags to make sure people don't forget.
> > 
> > You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> > into a single function? But "would_change" returns bool, and "add"
> > returns void.
> > 
> 
> Hence the wrappers for commit == false and commit == true. You could name the precommit
> one __vlan_flags_would_change or something more appropriate. The point is to make
> sure we always update both when flags are changed.

I still have a little doubt that I understood you properly.
Do you mean like this?

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..0883c11897d6 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,36 +34,32 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
 	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
 }
 
-static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
+static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 			    const struct net_bridge_vlan *v)
 {
 	if (vg->pvid == v->vid)
-		return false;
+		return;
 
 	smp_wmb();
 	br_vlan_set_pvid_state(vg, v->state);
 	vg->pvid = v->vid;
-
-	return true;
 }
 
-static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 {
 	if (vg->pvid != vid)
-		return false;
+		return;
 
 	smp_wmb();
 	vg->pvid = 0;
-
-	return true;
 }
 
-/* return true if anything changed, false otherwise */
-static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
+/* Commit a change of flags to @v. Do not use directly, instead go through
+ * __vlan_update_flags().
+ */
+static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 {
 	struct net_bridge_vlan_group *vg;
-	u16 old_flags = v->flags;
-	bool ret;
 
 	if (br_vlan_is_master(v))
 		vg = br_vlan_group(v->br);
@@ -71,16 +67,48 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 		vg = nbp_vlan_group(v->port);
 
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		ret = __vlan_add_pvid(vg, v);
+		__vlan_add_pvid(vg, v);
 	else
-		ret = __vlan_delete_pvid(vg, v->vid);
+		__vlan_delete_pvid(vg, v->vid);
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 	else
 		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
+}
+
+/* Return true if anything will change as a result of __vlan_add_flags(),
+ * false otherwise. Do not use directly, instead go through
+ * __vlan_update_flags().
+ */
+static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
+{
+	struct net_bridge_vlan_group *vg;
+	u16 old_flags = v->flags;
+	bool pvid_changed;
 
-	return ret || !!(old_flags ^ v->flags);
+	if (br_vlan_is_master(v))
+		vg = br_vlan_group(v->br);
+	else
+		vg = nbp_vlan_group(v->port);
+
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		pvid_changed = (vg->pvid != v->vid);
+	else
+		pvid_changed = (vg->pvid == v->vid);
+
+	return pvid_changed || !!(old_flags ^ v->flags);
+}
+
+static bool __vlan_update_flags(struct net_bridge_vlan *v, u16 flags,
+				bool commit)
+{
+	if (!commit)
+		return __vlan_flags_would_change(v, flags);
+
+	__vlan_add_flags(v, flags);
+
+	return false;
 }
 
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
@@ -310,7 +338,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 		goto out_fdb_insert;
 
 	__vlan_add_list(v);
-	__vlan_add_flags(v, flags);
+	__vlan_update_flags(v, flags, true);
 	br_multicast_toggle_one_vlan(v, true);
 
 	if (p)
@@ -670,11 +698,15 @@ static int br_vlan_add_existing(struct net_bridge *br,
 				u16 flags, bool *changed,
 				struct netlink_ext_ack *extack)
 {
+	bool would_change = __vlan_update_flags(vlan, flags, false);
 	int err;
 
-	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (would_change) {
+		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
+						 extack);
+		if (err && err != -EOPNOTSUPP)
+			return err;
+	}
 
 	if (!br_vlan_is_brentry(vlan)) {
 		/* Trying to change flags of non-existent bridge vlan */
@@ -696,7 +728,8 @@ static int br_vlan_add_existing(struct net_bridge *br,
 		br_multicast_toggle_one_vlan(vlan, true);
 	}
 
-	if (__vlan_add_flags(vlan, flags))
+	__vlan_update_flags(vlan, flags, true);
+	if (would_change)
 		*changed = true;
 
 	return 0;
@@ -1247,11 +1280,18 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 	*changed = false;
 	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
-		/* Pass the flags to the hardware bridge */
-		ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
-		if (ret && ret != -EOPNOTSUPP)
-			return ret;
-		*changed = __vlan_add_flags(vlan, flags);
+		bool would_change = __vlan_update_flags(vlan, flags, false);
+
+		if (would_change) {
+			/* Pass the flags to the hardware bridge */
+			ret = br_switchdev_port_vlan_add(port->dev, vid,
+							 flags, extack);
+			if (ret && ret != -EOPNOTSUPP)
+				return ret;
+		}
+
+		__vlan_update_flags(vlan, flags, true);
+		*changed = would_change;
 
 		return 0;
 	}

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 12:30, Vladimir Oltean wrote:
> On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
>> On 15/02/2022 11:54, Vladimir Oltean wrote:
>>> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
>>>>> +/* return true if anything will change as a result of __vlan_add_flags,
>>>>> + * false otherwise
>>>>> + */
>>>>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
>>>>> +{
>>>>> +	struct net_bridge_vlan_group *vg;
>>>>> +	u16 old_flags = v->flags;
>>>>> +	bool pvid_changed;
>>>>>  
>>>>> -	return ret || !!(old_flags ^ v->flags);
>>>>> +	if (br_vlan_is_master(v))
>>>>> +		vg = br_vlan_group(v->br);
>>>>> +	else
>>>>> +		vg = nbp_vlan_group(v->port);
>>>>> +
>>>>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
>>>>> +		pvid_changed = (vg->pvid == v->vid);
>>>>> +	else
>>>>> +		pvid_changed = (vg->pvid != v->vid);
>>>>> +
>>>>> +	return pvid_changed || !!(old_flags ^ v->flags);
>>>>>  }
>>>>
>>>> These two have to depend on each other, otherwise it's error-prone and
>>>> surely in the future someone will forget to update both.
>>>> How about add a "commit" argument to __vlan_add_flags and possibly rename
>>>> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
>>>> with commit == false and __vlan_update_flags_commit with commit == true.
>>>> Or some other naming, the point is to always use the same flow and checks
>>>> when updating the flags to make sure people don't forget.
>>>
>>> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
>>> into a single function? But "would_change" returns bool, and "add"
>>> returns void.
>>>
>>
>> Hence the wrappers for commit == false and commit == true. You could name the precommit
>> one __vlan_flags_would_change or something more appropriate. The point is to make
>> sure we always update both when flags are changed.
> 
> I still have a little doubt that I understood you properly.
> Do you mean like this?
> 

By the way I just noticed that __vlan_flags_would_change has another bug, it's testing
vlan's flags against themselves without any change (old_flags == v->flags).

I meant something similar to this (quickly hacked, untested, add flags probably
could be renamed to something more appropriate):

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..1de69090d3cb 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,53 +34,66 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
        return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
 }
 
-static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
+static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
                            const struct net_bridge_vlan *v)
 {
        if (vg->pvid == v->vid)
-               return false;
+               return;
 
        smp_wmb();
        br_vlan_set_pvid_state(vg, v->state);
        vg->pvid = v->vid;
-
-       return true;
 }
 
-static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 {
        if (vg->pvid != vid)
-               return false;
+               return;
 
        smp_wmb();
        vg->pvid = 0;
-
-       return true;
 }
 
 /* return true if anything changed, false otherwise */
-static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
+static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags, bool commit)
 {
        struct net_bridge_vlan_group *vg;
-       u16 old_flags = v->flags;
-       bool ret;
+       bool change;
 
        if (br_vlan_is_master(v))
                vg = br_vlan_group(v->br);
        else
                vg = nbp_vlan_group(v->port);
 
+       /* check if anything would be changed on commit */
+       change = (!!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
+                 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED));
+
+       if (!commit)
+               goto out;
+
        if (flags & BRIDGE_VLAN_INFO_PVID)
-               ret = __vlan_add_pvid(vg, v);
+               __vlan_add_pvid(vg, v);
        else
-               ret = __vlan_delete_pvid(vg, v->vid);
+               __vlan_delete_pvid(vg, v->vid);
 
        if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
                v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
        else
                v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
 
-       return ret || !!(old_flags ^ v->flags);
+out:
+       return change;
+}
+
+static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
+{
+       return __vlan_add_flags(v, flags, false);
+}
+
+static bool __vlan_flags_commit(struct net_bridge_vlan *v, u16 flags)
+{
+       return __vlan_add_flags(v, flags, true);
 }
 
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 13:08, Nikolay Aleksandrov wrote:
> On 15/02/2022 12:30, Vladimir Oltean wrote:
>> On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
>>> On 15/02/2022 11:54, Vladimir Oltean wrote:
>>>> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
>>>>>> +/* return true if anything will change as a result of __vlan_add_flags,
>>>>>> + * false otherwise
>>>>>> + */
>>>>>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
>>>>>> +{
>>>>>> +	struct net_bridge_vlan_group *vg;
>>>>>> +	u16 old_flags = v->flags;
>>>>>> +	bool pvid_changed;
>>>>>>  
>>>>>> -	return ret || !!(old_flags ^ v->flags);
>>>>>> +	if (br_vlan_is_master(v))
>>>>>> +		vg = br_vlan_group(v->br);
>>>>>> +	else
>>>>>> +		vg = nbp_vlan_group(v->port);
>>>>>> +
>>>>>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
>>>>>> +		pvid_changed = (vg->pvid == v->vid);
>>>>>> +	else
>>>>>> +		pvid_changed = (vg->pvid != v->vid);
>>>>>> +
>>>>>> +	return pvid_changed || !!(old_flags ^ v->flags);
>>>>>>  }
>>>>>
>>>>> These two have to depend on each other, otherwise it's error-prone and
>>>>> surely in the future someone will forget to update both.
>>>>> How about add a "commit" argument to __vlan_add_flags and possibly rename
>>>>> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
>>>>> with commit == false and __vlan_update_flags_commit with commit == true.
>>>>> Or some other naming, the point is to always use the same flow and checks
>>>>> when updating the flags to make sure people don't forget.
>>>>
>>>> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
>>>> into a single function? But "would_change" returns bool, and "add"
>>>> returns void.
>>>>
>>>
>>> Hence the wrappers for commit == false and commit == true. You could name the precommit
>>> one __vlan_flags_would_change or something more appropriate. The point is to make
>>> sure we always update both when flags are changed.
>>
>> I still have a little doubt that I understood you properly.
>> Do you mean like this?
>>
> 
> By the way I just noticed that __vlan_flags_would_change has another bug, it's testing
> vlan's flags against themselves without any change (old_flags == v->flags).
> 
> I meant something similar to this (quickly hacked, untested, add flags probably
> could be renamed to something more appropriate):
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1402d5ca242d..1de69090d3cb 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
[snip]
> +}
> +
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> +       return __vlan_add_flags(v, flags, false);
> +}
> +
> +static bool __vlan_flags_commit(struct net_bridge_vlan *v, u16 flags)
> +{

blah, obviously should be void here and ignore the return value

> +       return __vlan_add_flags(v, flags, true);
>  }
>  
>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 01:08:13PM +0200, Nikolay Aleksandrov wrote:
> On 15/02/2022 12:30, Vladimir Oltean wrote:
> > On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
> >> On 15/02/2022 11:54, Vladimir Oltean wrote:
> >>> On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> >>>>> +/* return true if anything will change as a result of __vlan_add_flags,
> >>>>> + * false otherwise
> >>>>> + */
> >>>>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> >>>>> +{
> >>>>> +	struct net_bridge_vlan_group *vg;
> >>>>> +	u16 old_flags = v->flags;
> >>>>> +	bool pvid_changed;
> >>>>>  
> >>>>> -	return ret || !!(old_flags ^ v->flags);
> >>>>> +	if (br_vlan_is_master(v))
> >>>>> +		vg = br_vlan_group(v->br);
> >>>>> +	else
> >>>>> +		vg = nbp_vlan_group(v->port);
> >>>>> +
> >>>>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> >>>>> +		pvid_changed = (vg->pvid == v->vid);
> >>>>> +	else
> >>>>> +		pvid_changed = (vg->pvid != v->vid);
> >>>>> +
> >>>>> +	return pvid_changed || !!(old_flags ^ v->flags);
> >>>>>  }
> >>>>
> >>>> These two have to depend on each other, otherwise it's error-prone and
> >>>> surely in the future someone will forget to update both.
> >>>> How about add a "commit" argument to __vlan_add_flags and possibly rename
> >>>> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
> >>>> with commit == false and __vlan_update_flags_commit with commit == true.
> >>>> Or some other naming, the point is to always use the same flow and checks
> >>>> when updating the flags to make sure people don't forget.
> >>>
> >>> You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> >>> into a single function? But "would_change" returns bool, and "add"
> >>> returns void.
> >>>
> >>
> >> Hence the wrappers for commit == false and commit == true. You could name the precommit
> >> one __vlan_flags_would_change or something more appropriate. The point is to make
> >> sure we always update both when flags are changed.
> > 
> > I still have a little doubt that I understood you properly.
> > Do you mean like this?
> > 
> 
> By the way I just noticed that __vlan_flags_would_change has another bug, it's testing
> vlan's flags against themselves without any change (old_flags == v->flags).

Yes, I think I noticed that too when I put some debugging prints, I
wasn't sure where it came from though.

> I meant something similar to this (quickly hacked, untested, add flags probably
> could be renamed to something more appropriate):
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1402d5ca242d..1de69090d3cb 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -34,53 +34,66 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
>         return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
>  }
>  
> -static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
> +static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>                             const struct net_bridge_vlan *v)
>  {
>         if (vg->pvid == v->vid)
> -               return false;
> +               return;
>  
>         smp_wmb();
>         br_vlan_set_pvid_state(vg, v->state);
>         vg->pvid = v->vid;
> -
> -       return true;
>  }
>  
> -static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
> +static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>  {
>         if (vg->pvid != vid)
> -               return false;
> +               return;
>  
>         smp_wmb();
>         vg->pvid = 0;
> -
> -       return true;
>  }
>  
>  /* return true if anything changed, false otherwise */
> -static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
> +static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags, bool commit)
>  {
>         struct net_bridge_vlan_group *vg;
> -       u16 old_flags = v->flags;
> -       bool ret;
> +       bool change;
>  
>         if (br_vlan_is_master(v))
>                 vg = br_vlan_group(v->br);
>         else
>                 vg = nbp_vlan_group(v->port);
>  
> +       /* check if anything would be changed on commit */
> +       change = (!!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
> +                 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED));
> +
> +       if (!commit)
> +               goto out;
> +
>         if (flags & BRIDGE_VLAN_INFO_PVID)
> -               ret = __vlan_add_pvid(vg, v);
> +               __vlan_add_pvid(vg, v);
>         else
> -               ret = __vlan_delete_pvid(vg, v->vid);
> +               __vlan_delete_pvid(vg, v->vid);
>  
>         if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>                 v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>         else
>                 v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>  
> -       return ret || !!(old_flags ^ v->flags);
> +out:
> +       return change;
> +}
> +
> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> +{
> +       return __vlan_add_flags(v, flags, false);
> +}
> +
> +static bool __vlan_flags_commit(struct net_bridge_vlan *v, u16 flags)
> +{
> +       return __vlan_add_flags(v, flags, true);
>  }
>  
>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,

Ah, ok, now I understand what you mean. I'll integrate this, retest and
prepare a v3 if there are no other objections. Thanks!

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

Hi Nikolay,

On Tue, Feb 15, 2022 at 01:08:13PM +0200, Nikolay Aleksandrov wrote:
> +static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags, bool commit)
>  {
>         struct net_bridge_vlan_group *vg;
> -       u16 old_flags = v->flags;
> -       bool ret;
> +       bool change;
>  
>         if (br_vlan_is_master(v))
>                 vg = br_vlan_group(v->br);
>         else
>                 vg = nbp_vlan_group(v->port);
>  
> +       /* check if anything would be changed on commit */
> +       change = (!!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
> +                 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED));
> +
> +       if (!commit)
> +               goto out;
> +
>         if (flags & BRIDGE_VLAN_INFO_PVID)
> -               ret = __vlan_add_pvid(vg, v);
> +               __vlan_add_pvid(vg, v);
>         else
> -               ret = __vlan_delete_pvid(vg, v->vid);
> +               __vlan_delete_pvid(vg, v->vid);
>  
>         if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>                 v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>         else
>                 v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>  
> -       return ret || !!(old_flags ^ v->flags);
> +out:
> +       return change;
> +}

I'm really sorry that I keep insisting, but could you please clarify
something for me.

Here you change the behavior of __vlan_add_flags(): yes, it only changes
BRIDGE_VLAN_INFO_UNTAGGED and BRIDGE_VLAN_INFO_PVID, but it used to
return true if other flags changed as well, like BRIDGE_VLAN_INFO_BRENTRY.
Now it does not, since you've explicitly made it so, and for good
reason: make the function do what it says on the box.

However, this forces me to add extra logic in br_vlan_add_existing()
such that a transition of master vlan flags from !BRENTRY -> BRENTRY is
still notified to switchdev.

Which begs the question, why exactly do we even bother to notify to
switchdev master VLANs that aren't brentries?! All drivers that I could
find just ignore VLANs that aren't brentries.

I can suppress the switchdev notification of these private bridge data
structures like this, and nothing else seems to be affected:

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 6fc2e366f13a..4607689c4e6a 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -300,10 +300,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 		}
 		br_multicast_port_ctx_init(p, v, &v->port_mcast_ctx);
 	} else {
-		err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, 0,
-						 extack);
-		if (err && err != -EOPNOTSUPP)
-			goto out;
+		if (br_vlan_should_use(v)) {
+			err = br_switchdev_port_vlan_add(dev, v->vid, flags,
+							 false, 0, extack);
+			if (err && err != -EOPNOTSUPP)
+				goto out;
+		}
 		br_multicast_ctx_init(br, v, &v->br_mcast_ctx);
 		v->priv_flags |= BR_VLFLAG_GLOBAL_MCAST_ENABLED;
 	}

Would you mind if I added an extra patch that also does this (it would
also remove the need for this check in drivers, plus it would change the
definition of "changed" to something IMO more reasonable, i.e. a master
VLAN that isn't brentry doesn't really exist, so even though the
!BRENTRY->BRENTRY is a flag change, it isn't a change of a switchdev
VLAN object that anybody offloads), or is there some reason I'm missing?

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Nikolay Aleksandrov]

On 15/02/2022 17:38, Vladimir Oltean wrote:
> Hi Nikolay,
> 
> On Tue, Feb 15, 2022 at 01:08:13PM +0200, Nikolay Aleksandrov wrote:
>> +static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags, bool commit)
>>  {
>>         struct net_bridge_vlan_group *vg;
>> -       u16 old_flags = v->flags;
>> -       bool ret;
>> +       bool change;
>>  
>>         if (br_vlan_is_master(v))
>>                 vg = br_vlan_group(v->br);
>>         else
>>                 vg = nbp_vlan_group(v->port);
>>  
>> +       /* check if anything would be changed on commit */
>> +       change = (!!(flags & BRIDGE_VLAN_INFO_PVID) == !!(vg->pvid != v->vid) ||
>> +                 ((flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED));
>> +
>> +       if (!commit)
>> +               goto out;
>> +
>>         if (flags & BRIDGE_VLAN_INFO_PVID)
>> -               ret = __vlan_add_pvid(vg, v);
>> +               __vlan_add_pvid(vg, v);
>>         else
>> -               ret = __vlan_delete_pvid(vg, v->vid);
>> +               __vlan_delete_pvid(vg, v->vid);
>>  
>>         if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>>                 v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>>         else
>>                 v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
>>  
>> -       return ret || !!(old_flags ^ v->flags);
>> +out:
>> +       return change;
>> +}
> 
> I'm really sorry that I keep insisting, but could you please clarify
> something for me.
> 
> Here you change the behavior of __vlan_add_flags(): yes, it only changes
> BRIDGE_VLAN_INFO_UNTAGGED and BRIDGE_VLAN_INFO_PVID, but it used to
> return true if other flags changed as well, like BRIDGE_VLAN_INFO_BRENTRY.
> Now it does not, since you've explicitly made it so, and for good
> reason: make the function do what it says on the box.
> 
> However, this forces me to add extra logic in br_vlan_add_existing()
> such that a transition of master vlan flags from !BRENTRY -> BRENTRY is
> still notified to switchdev.
> 
> Which begs the question, why exactly do we even bother to notify to
> switchdev master VLANs that aren't brentries?! All drivers that I could
> find just ignore VLANs that aren't brentries.
> 
> I can suppress the switchdev notification of these private bridge data
> structures like this, and nothing else seems to be affected:
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 6fc2e366f13a..4607689c4e6a 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -300,10 +300,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>  		}
>  		br_multicast_port_ctx_init(p, v, &v->port_mcast_ctx);
>  	} else {
> -		err = br_switchdev_port_vlan_add(dev, v->vid, flags, false, 0,
> -						 extack);
> -		if (err && err != -EOPNOTSUPP)
> -			goto out;
> +		if (br_vlan_should_use(v)) {
> +			err = br_switchdev_port_vlan_add(dev, v->vid, flags,
> +							 false, 0, extack);
> +			if (err && err != -EOPNOTSUPP)
> +				goto out;
> +		}
>  		br_multicast_ctx_init(br, v, &v->br_mcast_ctx);
>  		v->priv_flags |= BR_VLFLAG_GLOBAL_MCAST_ENABLED;
>  	}
> 
> Would you mind if I added an extra patch that also does this (it would
> also remove the need for this check in drivers, plus it would change the
> definition of "changed" to something IMO more reasonable, i.e. a master
> VLAN that isn't brentry doesn't really exist, so even though the
> !BRENTRY->BRENTRY is a flag change, it isn't a change of a switchdev
> VLAN object that anybody offloads), or is there some reason I'm missing?

I agree, patch looks good to me. I see that everyone already filters the
!BRENTRY case anyway.

Thanks,
 Nik

Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean]

On Tue, Feb 15, 2022 at 05:44:52PM +0200, Nikolay Aleksandrov wrote:
> > Would you mind if I added an extra patch that also does this (it would
> > also remove the need for this check in drivers, plus it would change the
> > definition of "changed" to something IMO more reasonable, i.e. a master
> > VLAN that isn't brentry doesn't really exist, so even though the
> > !BRENTRY->BRENTRY is a flag change, it isn't a change of a switchdev
> > VLAN object that anybody offloads), or is there some reason I'm missing?
> 
> I agree, patch looks good to me. I see that everyone already filters the
> !BRENTRY case anyway.

Thanks, this simplifies things quite a bit.