[PATCH v2 net-next 0/6] ATU and FDB synchronization on locked ports

[PATCH v2 net-next 0/6] ATU and FDB synchronization on locked ports

[Hans J. Schultz]

This patch set makes it possible to have synchronized dynamic ATU and FDB
entries on locked ports. As locked ports are not able to automatically
learn, they depend on userspace added entries, where userspace can add
static or dynamic entries. The lifetime of static entries are completely
dependent on userspace intervention, and thus not of interest here. We
are only concerned with dynamic entries, which can be added with a
command like:

bridge fdb replace ADDR dev <DEV> master dynamic

We choose only to support this feature on locked ports, as it involves
utilizing the CPU to handle ATU related switchcore events (typically
interrupts) and thus can result in significant performance loss if
exposed to heavy traffic.

On locked ports it is important for userspace to know when an authorized
station has become silent, hence not breaking the communication of a
station that has been authorized based on the MAC-Authentication Bypass
(MAB) scheme. Thus if the station keeps being active after authorization,
it will continue to have an open port as long as it is active. Only after
a silent period will it have to be reauthorized. As the ageing process in
the ATU is dependent on incoming traffic to the switchcore port, it is
necessary for the ATU to signal that an entry has aged out, so that the
FDB can be updated at the correct time.

This patch set includes a solution for the Marvell mv88e6xxx driver, where
for this driver we use the Hold-At-One feature so that an age-out
violation interrupt occurs when a station has been silent for the
system-set age time. The age out violation interrupt allows the switchcore
driver to remove both the ATU and the FDB entry at the same time.

It is up to the maintainers of other switchcore drivers to implement the
feature for their specific driver.

LOG:
	V2:	Ensure the port is locked when using the feature as we
		must ensure that learning is enabled at all times for
		the interrupts to occur. This was missed in the previous
		version.

		Instead of ignoring unsupported flags, ensure that
		drivers are only called when supporting the feature.
		As 'dynamic' flag is legacy, all drivers support it at
		least by their previous handling.

Hans J. Schultz (6):
  net: bridge: add dynamic flag to switchdev notifier
  net: dsa: propagate flags down towards drivers
  drivers: net: dsa: add fdb entry flags incoming to switchcore drivers
  net: bridge: ensure FDB offloaded flag is handled as needed
  net: dsa: mv88e6xxx: implementation of dynamic ATU entries
  selftests: forwarding: add dynamic FDB test

 drivers/net/dsa/b53/b53_common.c              |  4 +-
 drivers/net/dsa/b53/b53_priv.h                |  4 +-
 drivers/net/dsa/hirschmann/hellcreek.c        |  4 +-
 drivers/net/dsa/lan9303-core.c                |  4 +-
 drivers/net/dsa/lantiq_gswip.c                |  4 +-
 drivers/net/dsa/microchip/ksz_common.c        |  6 +-
 drivers/net/dsa/mt7530.c                      |  4 +-
 drivers/net/dsa/mv88e6xxx/chip.c              | 20 ++++--
 drivers/net/dsa/mv88e6xxx/chip.h              |  9 ++-
 drivers/net/dsa/mv88e6xxx/global1_atu.c       | 21 +++++++
 drivers/net/dsa/mv88e6xxx/port.c              |  6 +-
 drivers/net/dsa/mv88e6xxx/switchdev.c         | 61 +++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h         |  5 ++
 drivers/net/dsa/mv88e6xxx/trace.h             |  5 ++
 drivers/net/dsa/ocelot/felix.c                |  4 +-
 drivers/net/dsa/qca/qca8k-common.c            |  4 +-
 drivers/net/dsa/qca/qca8k.h                   |  4 +-
 drivers/net/dsa/rzn1_a5psw.c                  |  4 +-
 drivers/net/dsa/sja1105/sja1105_main.c        | 11 ++--
 include/net/dsa.h                             |  9 ++-
 include/net/switchdev.h                       |  1 +
 net/bridge/br_fdb.c                           |  5 +-
 net/bridge/br_switchdev.c                     |  1 +
 net/dsa/dsa.c                                 |  6 ++
 net/dsa/port.c                                | 28 +++++----
 net/dsa/port.h                                |  8 +--
 net/dsa/slave.c                               | 20 ++++--
 net/dsa/switch.c                              | 26 +++++---
 net/dsa/switch.h                              |  1 +
 .../net/forwarding/bridge_locked_port.sh      | 36 +++++++++++
 30 files changed, 258 insertions(+), 67 deletions(-)

-- 
2.34.1

[PATCH v2 net-next 1/6] net: bridge: add dynamic flag to switchdev notifier

[Hans J. Schultz]

To be able to add dynamic FDB entries to drivers from userspace, the
dynamic flag must be added when sending RTM_NEWNEIGH events down.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 include/net/switchdev.h   | 1 +
 net/bridge/br_switchdev.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index ca0312b78294..aaf918d4ba67 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
 	u8 added_by_user:1,
 	   is_local:1,
 	   locked:1,
+	   is_dyn:1,
 	   offloaded:1;
 };
 
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index de18e9c1d7a7..9707d3fdb396 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -134,6 +134,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
 	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
 	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
 	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
+	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);
 	item->locked = false;
 	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
 	item->info.ctx = ctx;
-- 
2.34.1

[PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans J. Schultz]

Dynamic FDB flag needs to be propagated through the DSA layer to be
added to drivers.
Use a u16 for fdb flags for future use, so that other flags can also be
sent the same way without having to change function interfaces. If we
have unsupported flags in the new flags, we do not do unnecessary work
and so we return at once. This ensures that the drivers are not called
with unsupported flags, so that the drivers do not need to check the
new flags. As the dynamic flag is a legacy flag, all drivers support it
by default at least as they have done hitherto.
Ensure that the dynamic FDB flag is only set when added from userspace,
as the feature it supports is to be handled from userspace.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 include/net/dsa.h |  5 +++++
 net/dsa/dsa.c     |  6 ++++++
 net/dsa/port.c    | 28 ++++++++++++++++------------
 net/dsa/port.h    |  8 ++++----
 net/dsa/slave.c   | 20 ++++++++++++++++----
 net/dsa/switch.c  | 18 ++++++++++++------
 net/dsa/switch.h  |  1 +
 7 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index a15f17a38eca..9e98c4fe1520 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -437,6 +437,9 @@ struct dsa_switch {
 	 */
 	u32			fdb_isolation:1;
 
+	/* Supported fdb flags going from the bridge to drivers */
+	u16			supported_fdb_flags;
+
 	/* Listener for switch fabric events */
 	struct notifier_block	nb;
 
@@ -818,6 +821,8 @@ static inline bool dsa_port_tree_same(const struct dsa_port *a,
 	return a->ds->dst == b->ds->dst;
 }
 
+#define DSA_FDB_FLAG_DYNAMIC		BIT(0)
+
 typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
 			      bool is_static, void *data);
 struct dsa_switch_ops {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..c07a2e225ae5 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -626,6 +626,12 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 	ds->configure_vlan_while_not_filtering = true;
 
+	/* Since dynamic FDB entries are legacy, all switch drivers should
+	 * support the flag at least by just installing a static entry and
+	 * letting the bridge age it.
+	 */
+	ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC;
+
 	err = ds->ops->setup(ds);
 	if (err < 0)
 		goto unregister_notifier;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 67ad1adec2a2..9a7c1265546d 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -976,12 +976,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu)
 }
 
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid)
+		     u16 vid, u16 flags)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.flags = flags,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -999,12 +1000,13 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 }
 
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid)
+		     u16 vid, u16 flags)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.flags = flags,
 		.db = {
 			.type = DSA_DB_BRIDGE,
 			.bridge = *dp->bridge,
@@ -1019,12 +1021,13 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 
 static int dsa_port_host_fdb_add(struct dsa_port *dp,
 				 const unsigned char *addr, u16 vid,
-				 struct dsa_db db)
+				 u16 flags, struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.flags = flags,
 		.db = db,
 	};
 
@@ -1042,11 +1045,11 @@ int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 		.dp = dp,
 	};
 
-	return dsa_port_host_fdb_add(dp, addr, vid, db);
+	return dsa_port_host_fdb_add(dp, addr, vid, 0, db);
 }
 
-int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
+				 u16 vid, u16 flags)
 {
 	struct net_device *master = dsa_port_to_master(dp);
 	struct dsa_db db = {
@@ -1065,17 +1068,18 @@ int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
 			return err;
 	}
 
-	return dsa_port_host_fdb_add(dp, addr, vid, db);
+	return dsa_port_host_fdb_add(dp, addr, vid, flags, db);
 }
 
 static int dsa_port_host_fdb_del(struct dsa_port *dp,
 				 const unsigned char *addr, u16 vid,
-				 struct dsa_db db)
+				 u16 flags, struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.dp = dp,
 		.addr = addr,
 		.vid = vid,
+		.flags = flags,
 		.db = db,
 	};
 
@@ -1093,11 +1097,11 @@ int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 		.dp = dp,
 	};
 
-	return dsa_port_host_fdb_del(dp, addr, vid, db);
+	return dsa_port_host_fdb_del(dp, addr, vid, 0, db);
 }
 
-int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
-				 const unsigned char *addr, u16 vid)
+int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+				 u16 vid, u16 flags)
 {
 	struct net_device *master = dsa_port_to_master(dp);
 	struct dsa_db db = {
@@ -1112,7 +1116,7 @@ int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
 			return err;
 	}
 
-	return dsa_port_host_fdb_del(dp, addr, vid, db);
+	return dsa_port_host_fdb_del(dp, addr, vid, flags, db);
 }
 
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/port.h b/net/dsa/port.h
index 9c218660d223..88a9dfed3bce 100644
--- a/net/dsa/port.h
+++ b/net/dsa/port.h
@@ -47,17 +47,17 @@ int dsa_port_vlan_msti(struct dsa_port *dp,
 		       const struct switchdev_vlan_msti *msti);
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu);
 int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid);
+		     u16 vid, u16 flags);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-		     u16 vid);
+		     u16 vid, u16 flags);
 int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
 int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
 				     const unsigned char *addr, u16 vid);
 int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid);
+				 u16 vid, u16 flags);
 int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid);
+				 u16 vid, u16 flags);
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 			 u16 vid);
 int dsa_port_lag_fdb_del(struct dsa_port *dp, const unsigned char *addr,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6957971c2db2..20d2d1c000ea 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -39,6 +39,7 @@ struct dsa_switchdev_event_work {
 	 */
 	unsigned char addr[ETH_ALEN];
 	u16 vid;
+	u16 fdb_flags;
 	bool host_addr;
 };
 
@@ -3331,6 +3332,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 		container_of(work, struct dsa_switchdev_event_work, work);
 	const unsigned char *addr = switchdev_work->addr;
 	struct net_device *dev = switchdev_work->dev;
+	u16 flags = switchdev_work->fdb_flags;
 	u16 vid = switchdev_work->vid;
 	struct dsa_switch *ds;
 	struct dsa_port *dp;
@@ -3342,11 +3344,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 	switch (switchdev_work->event) {
 	case SWITCHDEV_FDB_ADD_TO_DEVICE:
 		if (switchdev_work->host_addr)
-			err = dsa_port_bridge_host_fdb_add(dp, addr, vid);
+			err = dsa_port_bridge_host_fdb_add(dp, addr,
+							   vid, flags);
 		else if (dp->lag)
 			err = dsa_port_lag_fdb_add(dp, addr, vid);
 		else
-			err = dsa_port_fdb_add(dp, addr, vid);
+			err = dsa_port_fdb_add(dp, addr, vid, flags);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to add %pM vid %d to fdb: %d\n",
@@ -3358,11 +3361,12 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 
 	case SWITCHDEV_FDB_DEL_TO_DEVICE:
 		if (switchdev_work->host_addr)
-			err = dsa_port_bridge_host_fdb_del(dp, addr, vid);
+			err = dsa_port_bridge_host_fdb_del(dp, addr,
+							   vid, flags);
 		else if (dp->lag)
 			err = dsa_port_lag_fdb_del(dp, addr, vid);
 		else
-			err = dsa_port_fdb_del(dp, addr, vid);
+			err = dsa_port_fdb_del(dp, addr, vid, flags);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to delete %pM vid %d from fdb: %d\n",
@@ -3400,6 +3404,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	bool host_addr = fdb_info->is_local;
 	struct dsa_switch *ds = dp->ds;
+	u16 flags = 0;
 
 	if (ctx && ctx != dp)
 		return 0;
@@ -3437,6 +3442,12 @@ static int dsa_slave_fdb_event(struct net_device *dev,
 			return -EOPNOTSUPP;
 	}
 
+	if (fdb_info->is_dyn && fdb_info->added_by_user)
+		flags |= DSA_FDB_FLAG_DYNAMIC;
+
+	if (flags & ~ds->supported_fdb_flags)
+		return 0;
+
 	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
 	if (!switchdev_work)
 		return -ENOMEM;
@@ -3454,6 +3465,7 @@ static int dsa_slave_fdb_event(struct net_device *dev,
 	ether_addr_copy(switchdev_work->addr, fdb_info->addr);
 	switchdev_work->vid = fdb_info->vid;
 	switchdev_work->host_addr = host_addr;
+	switchdev_work->fdb_flags = flags;
 
 	dsa_schedule_work(&switchdev_work->work);
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index d5bc4bb7310d..0f5626a425b6 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -239,7 +239,7 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp,
 }
 
 static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-			       u16 vid, struct dsa_db db)
+			       u16 vid, u16 flags, struct dsa_db db)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a;
@@ -283,7 +283,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 }
 
 static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-			       u16 vid, struct dsa_db db)
+			       u16 vid, u16 flags, struct dsa_db db)
 {
 	struct dsa_switch *ds = dp->ds;
 	struct dsa_mac_addr *a;
@@ -410,7 +410,9 @@ static int dsa_switch_host_fdb_add(struct dsa_switch *ds,
 								info->db);
 			} else {
 				err = dsa_port_do_fdb_add(dp, info->addr,
-							  info->vid, info->db);
+							  info->vid,
+							  info->flags,
+							  info->db);
 			}
 			if (err)
 				break;
@@ -438,7 +440,9 @@ static int dsa_switch_host_fdb_del(struct dsa_switch *ds,
 								info->db);
 			} else {
 				err = dsa_port_do_fdb_del(dp, info->addr,
-							  info->vid, info->db);
+							  info->vid,
+							  info->flags,
+							  info->db);
 			}
 			if (err)
 				break;
@@ -457,7 +461,8 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_add)
 		return -EOPNOTSUPP;
 
-	return dsa_port_do_fdb_add(dp, info->addr, info->vid, info->db);
+	return dsa_port_do_fdb_add(dp, info->addr, info->vid,
+				   info->flags, info->db);
 }
 
 static int dsa_switch_fdb_del(struct dsa_switch *ds,
@@ -469,7 +474,8 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_del)
 		return -EOPNOTSUPP;
 
-	return dsa_port_do_fdb_del(dp, info->addr, info->vid, info->db);
+	return dsa_port_do_fdb_del(dp, info->addr, info->vid,
+				   info->flags, info->db);
 }
 
 static int dsa_switch_lag_fdb_add(struct dsa_switch *ds,
diff --git a/net/dsa/switch.h b/net/dsa/switch.h
index 15e67b95eb6e..2c3bf4020158 100644
--- a/net/dsa/switch.h
+++ b/net/dsa/switch.h
@@ -55,6 +55,7 @@ struct dsa_notifier_fdb_info {
 	const struct dsa_port *dp;
 	const unsigned char *addr;
 	u16 vid;
+	u16 flags;
 	struct dsa_db db;
 };
 
-- 
2.34.1

[PATCH v2 net-next 3/6] drivers: net: dsa: add fdb entry flags incoming to switchcore drivers

[Hans J. Schultz]

Drivers are only called with supported fdb flags set.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/b53/b53_common.c       |  4 ++--
 drivers/net/dsa/b53/b53_priv.h         |  4 ++--
 drivers/net/dsa/hirschmann/hellcreek.c |  4 ++--
 drivers/net/dsa/lan9303-core.c         |  4 ++--
 drivers/net/dsa/lantiq_gswip.c         |  4 ++--
 drivers/net/dsa/microchip/ksz_common.c |  6 +++---
 drivers/net/dsa/mt7530.c               |  4 ++--
 drivers/net/dsa/mv88e6xxx/chip.c       |  4 ++--
 drivers/net/dsa/ocelot/felix.c         |  4 ++--
 drivers/net/dsa/qca/qca8k-common.c     |  4 ++--
 drivers/net/dsa/qca/qca8k.h            |  4 ++--
 drivers/net/dsa/rzn1_a5psw.c           |  4 ++--
 drivers/net/dsa/sja1105/sja1105_main.c | 11 ++++++-----
 include/net/dsa.h                      |  4 ++--
 net/dsa/switch.c                       |  8 ++++----
 15 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 59cdfc51ce06..f4bb0fed8913 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1684,7 +1684,7 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 
 int b53_fdb_add(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
-		struct dsa_db db)
+		u16 flags, struct dsa_db db)
 {
 	struct b53_device *priv = ds->priv;
 	int ret;
@@ -1705,7 +1705,7 @@ EXPORT_SYMBOL(b53_fdb_add);
 
 int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
-		struct dsa_db db)
+		u16 flags, struct dsa_db db)
 {
 	struct b53_device *priv = ds->priv;
 	int ret;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 795cbffd5c2b..5479340a0b00 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -362,10 +362,10 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan);
 int b53_fdb_add(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
-		struct dsa_db db);
+		u16 flags, struct dsa_db db);
 int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid,
-		struct dsa_db db);
+		u16 flags, struct dsa_db db);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
 		 dsa_fdb_dump_cb_t *cb, void *data);
 int b53_mdb_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index 595a548bb0a8..2b25203c5f58 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -839,7 +839,7 @@ static int hellcreek_fdb_get(struct hellcreek *hellcreek,
 
 static int hellcreek_fdb_add(struct dsa_switch *ds, int port,
 			     const unsigned char *addr, u16 vid,
-			     struct dsa_db db)
+			     u16 flags, struct dsa_db db)
 {
 	struct hellcreek_fdb_entry entry = { 0 };
 	struct hellcreek *hellcreek = ds->priv;
@@ -885,7 +885,7 @@ static int hellcreek_fdb_add(struct dsa_switch *ds, int port,
 
 static int hellcreek_fdb_del(struct dsa_switch *ds, int port,
 			     const unsigned char *addr, u16 vid,
-			     struct dsa_db db)
+			     u16 flags, struct dsa_db db)
 {
 	struct hellcreek_fdb_entry entry = { 0 };
 	struct hellcreek *hellcreek = ds->priv;
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index cbe831875347..e4f830a4f143 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1183,7 +1183,7 @@ static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
 
 static int lan9303_port_fdb_add(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
-				struct dsa_db db)
+				u16 flags, struct dsa_db db)
 {
 	struct lan9303 *chip = ds->priv;
 
@@ -1196,7 +1196,7 @@ static int lan9303_port_fdb_add(struct dsa_switch *ds, int port,
 
 static int lan9303_port_fdb_del(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
-				struct dsa_db db)
+				u16 flags, struct dsa_db db)
 {
 	struct lan9303 *chip = ds->priv;
 
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 05ecaa007ab1..65fcc57ad1b6 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1399,14 +1399,14 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 
 static int gswip_port_fdb_add(struct dsa_switch *ds, int port,
 			      const unsigned char *addr, u16 vid,
-			      struct dsa_db db)
+			      u16 flags, struct dsa_db db)
 {
 	return gswip_port_fdb(ds, port, addr, vid, true);
 }
 
 static int gswip_port_fdb_del(struct dsa_switch *ds, int port,
 			      const unsigned char *addr, u16 vid,
-			      struct dsa_db db)
+			      u16 flags, struct dsa_db db)
 {
 	return gswip_port_fdb(ds, port, addr, vid, false);
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 729b36eeb2c4..0bc6cc36e11f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2390,7 +2390,7 @@ static int ksz_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
 
 static int ksz_port_fdb_add(struct dsa_switch *ds, int port,
 			    const unsigned char *addr, u16 vid,
-			    struct dsa_db db)
+			    u16 flags, struct dsa_db db)
 {
 	struct ksz_device *dev = ds->priv;
 
@@ -2401,8 +2401,8 @@ static int ksz_port_fdb_add(struct dsa_switch *ds, int port,
 }
 
 static int ksz_port_fdb_del(struct dsa_switch *ds, int port,
-			    const unsigned char *addr,
-			    u16 vid, struct dsa_db db)
+			    const unsigned char *addr, u16 vid,
+			    u16 flags, struct dsa_db db)
 {
 	struct ksz_device *dev = ds->priv;
 
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..0b6eae2de0c1 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1374,7 +1374,7 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 static int
 mt7530_port_fdb_add(struct dsa_switch *ds, int port,
 		    const unsigned char *addr, u16 vid,
-		    struct dsa_db db)
+		    u16 flags, struct dsa_db db)
 {
 	struct mt7530_priv *priv = ds->priv;
 	int ret;
@@ -1391,7 +1391,7 @@ mt7530_port_fdb_add(struct dsa_switch *ds, int port,
 static int
 mt7530_port_fdb_del(struct dsa_switch *ds, int port,
 		    const unsigned char *addr, u16 vid,
-		    struct dsa_db db)
+		    u16 flags, struct dsa_db db)
 {
 	struct mt7530_priv *priv = ds->priv;
 	int ret;
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0a5d6c7bb128..6848fa0e5979 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2724,7 +2724,7 @@ static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds,
 
 static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 				  const unsigned char *addr, u16 vid,
-				  struct dsa_db db)
+				  u16 flags, struct dsa_db db)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
@@ -2739,7 +2739,7 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 
 static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 				  const unsigned char *addr, u16 vid,
-				  struct dsa_db db)
+				  u16 flags, struct dsa_db db)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index d4cc9e60f369..65cf02e3e515 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -782,7 +782,7 @@ static int felix_fdb_dump(struct dsa_switch *ds, int port,
 
 static int felix_fdb_add(struct dsa_switch *ds, int port,
 			 const unsigned char *addr, u16 vid,
-			 struct dsa_db db)
+			 u16 flags, struct dsa_db db)
 {
 	struct net_device *bridge_dev = felix_classify_db(db);
 	struct dsa_port *dp = dsa_to_port(ds, port);
@@ -803,7 +803,7 @@ static int felix_fdb_add(struct dsa_switch *ds, int port,
 
 static int felix_fdb_del(struct dsa_switch *ds, int port,
 			 const unsigned char *addr, u16 vid,
-			 struct dsa_db db)
+			 u16 flags, struct dsa_db db)
 {
 	struct net_device *bridge_dev = felix_classify_db(db);
 	struct dsa_port *dp = dsa_to_port(ds, port);
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 96773e432558..fd6e215a4054 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -758,7 +758,7 @@ int qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 
 int qca8k_port_fdb_add(struct dsa_switch *ds, int port,
 		       const unsigned char *addr, u16 vid,
-		       struct dsa_db db)
+		       u16 flags, struct dsa_db db)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
@@ -768,7 +768,7 @@ int qca8k_port_fdb_add(struct dsa_switch *ds, int port,
 
 int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 		       const unsigned char *addr, u16 vid,
-		       struct dsa_db db)
+		       u16 flags, struct dsa_db db)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 7996975d29d3..d175f15eb239 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -473,10 +473,10 @@ int qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 			  u16 port_mask, u16 vid);
 int qca8k_port_fdb_add(struct dsa_switch *ds, int port,
 		       const unsigned char *addr, u16 vid,
-		       struct dsa_db db);
+		       u16 flags, struct dsa_db db);
 int qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 		       const unsigned char *addr, u16 vid,
-		       struct dsa_db db);
+		       u16 flags, struct dsa_db db);
 int qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 			dsa_fdb_dump_cb_t *cb, void *data);
 
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 919027cf2012..5f0f6c08bd53 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -396,7 +396,7 @@ static int a5psw_lk_execute_lookup(struct a5psw *a5psw, union lk_data *lk_data,
 
 static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
 			      const unsigned char *addr, u16 vid,
-			      struct dsa_db db)
+			      u16 flags, struct dsa_db db)
 {
 	struct a5psw *a5psw = ds->priv;
 	union lk_data lk_data = {0};
@@ -447,7 +447,7 @@ static int a5psw_port_fdb_add(struct dsa_switch *ds, int port,
 
 static int a5psw_port_fdb_del(struct dsa_switch *ds, int port,
 			      const unsigned char *addr, u16 vid,
-			      struct dsa_db db)
+			      u16 flags, struct dsa_db db)
 {
 	struct a5psw *a5psw = ds->priv;
 	union lk_data lk_data = {0};
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b70dcf32a26d..e4592af21ba3 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1802,7 +1802,7 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
 
 static int sja1105_fdb_add(struct dsa_switch *ds, int port,
 			   const unsigned char *addr, u16 vid,
-			   struct dsa_db db)
+			   u16 flags, struct dsa_db db)
 {
 	struct sja1105_private *priv = ds->priv;
 
@@ -1824,7 +1824,7 @@ static int sja1105_fdb_add(struct dsa_switch *ds, int port,
 
 static int sja1105_fdb_del(struct dsa_switch *ds, int port,
 			   const unsigned char *addr, u16 vid,
-			   struct dsa_db db)
+			   u16 flags, struct dsa_db db)
 {
 	struct sja1105_private *priv = ds->priv;
 
@@ -1930,7 +1930,8 @@ static void sja1105_fast_age(struct dsa_switch *ds, int port)
 
 		u64_to_ether_addr(l2_lookup.macaddr, macaddr);
 
-		rc = sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid, db);
+		rc = sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid,
+				     0, db);
 		if (rc) {
 			dev_err(ds->dev,
 				"Failed to delete FDB entry %pM vid %lld: %pe\n",
@@ -1944,14 +1945,14 @@ static int sja1105_mdb_add(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_mdb *mdb,
 			   struct dsa_db db)
 {
-	return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
+	return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, 0, db);
 }
 
 static int sja1105_mdb_del(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_mdb *mdb,
 			   struct dsa_db db)
 {
-	return sja1105_fdb_del(ds, port, mdb->addr, mdb->vid, db);
+	return sja1105_fdb_del(ds, port, mdb->addr, mdb->vid, 0, db);
 }
 
 /* Common function for unicast and broadcast flood configuration.
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 9e98c4fe1520..3b2bd41c2af5 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1050,10 +1050,10 @@ struct dsa_switch_ops {
 	 */
 	int	(*port_fdb_add)(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
-				struct dsa_db db);
+				u16 flags, struct dsa_db db);
 	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid,
-				struct dsa_db db);
+				u16 flags, struct dsa_db db);
 	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
 				 dsa_fdb_dump_cb_t *cb, void *data);
 	int	(*lag_fdb_add)(struct dsa_switch *ds, struct dsa_lag lag,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 0f5626a425b6..1d7d01a1b2a3 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -248,7 +248,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 
 	/* No need to bother with refcounting for user ports */
 	if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
-		return ds->ops->port_fdb_add(ds, port, addr, vid, db);
+		return ds->ops->port_fdb_add(ds, port, addr, vid, flags, db);
 
 	mutex_lock(&dp->addr_lists_lock);
 
@@ -264,7 +264,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		goto out;
 	}
 
-	err = ds->ops->port_fdb_add(ds, port, addr, vid, db);
+	err = ds->ops->port_fdb_add(ds, port, addr, vid, flags, db);
 	if (err) {
 		kfree(a);
 		goto out;
@@ -292,7 +292,7 @@ static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 
 	/* No need to bother with refcounting for user ports */
 	if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
-		return ds->ops->port_fdb_del(ds, port, addr, vid, db);
+		return ds->ops->port_fdb_del(ds, port, addr, vid, flags, db);
 
 	mutex_lock(&dp->addr_lists_lock);
 
@@ -305,7 +305,7 @@ static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 	if (!refcount_dec_and_test(&a->refcount))
 		goto out;
 
-	err = ds->ops->port_fdb_del(ds, port, addr, vid, db);
+	err = ds->ops->port_fdb_del(ds, port, addr, vid, flags, db);
 	if (err) {
 		refcount_set(&a->refcount, 1);
 		goto out;
-- 
2.34.1

[PATCH v2 net-next 4/6] net: bridge: ensure FDB offloaded flag is handled as needed

[Hans J. Schultz]

Since user added entries in the bridge FDB will get the BR_FDB_OFFLOADED
flag set, we do not want the bridge to age those entries and we want the
entries to be deleted in the bridge upon an SWITCHDEV_FDB_DEL_TO_BRIDGE
event.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 net/bridge/br_fdb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e69a872bfc1d..b0c23a72bc76 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -537,6 +537,7 @@ void br_fdb_cleanup(struct work_struct *work)
 		unsigned long this_timer = f->updated + delay;
 
 		if (test_bit(BR_FDB_STATIC, &f->flags) ||
+		    test_bit(BR_FDB_OFFLOADED, &f->flags) ||
 		    test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
 			if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
 				if (time_after(this_timer, now))
@@ -1465,7 +1466,9 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 	spin_lock_bh(&br->hash_lock);
 
 	fdb = br_fdb_find(br, addr, vid);
-	if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
+	if (fdb &&
+	    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) ||
+	     test_bit(BR_FDB_OFFLOADED, &fdb->flags)))
 		fdb_delete(br, fdb, swdev_notify);
 	else
 		err = -ENOENT;
-- 
2.34.1

[PATCH v2 net-next 5/6] net: dsa: mv88e6xxx: implementation of dynamic ATU entries

[Hans J. Schultz]

For 802.1X or MAB security authed hosts we want to have these hosts authed
by adding dynamic FDB entries, so that if an authed host goes silent for
a time period it's FDB entry will be removed and it must reauth when
wanting to communicate again.
In the mv88e6xxx offloaded case, we can use the HoldAt1 feature, that
gives an age out interrupt when the FDB entry is about to age out, so
that userspace can be notified of the entry being deleted with the help
of an SWITCHDEV_FDB_DEL_TO_BRIDGE event.
When adding a dynamic entry the bridge must be informed that the driver
takes care of the ageing be sending an SWITCHDEV_FDB_OFFLOADED event,
telling the bridge that this added FDB entry will be handled by the
driver.
With this implementation, trace events for age out interrupts are also
added.

note: A special case arises with the age out interrupt, as the entry
state/spid (source port id) value read from the registers will be zero.
Thus we need to extract the source port from the port vector instead.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c        | 16 ++++++-
 drivers/net/dsa/mv88e6xxx/chip.h        |  9 +++-
 drivers/net/dsa/mv88e6xxx/global1_atu.c | 21 +++++++++
 drivers/net/dsa/mv88e6xxx/port.c        |  6 ++-
 drivers/net/dsa/mv88e6xxx/switchdev.c   | 61 +++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/switchdev.h   |  5 ++
 drivers/net/dsa/mv88e6xxx/trace.h       |  5 ++
 7 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 6848fa0e5979..843ed02da9a2 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -42,6 +42,7 @@
 #include "ptp.h"
 #include "serdes.h"
 #include "smi.h"
+#include "switchdev.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
 {
@@ -2726,14 +2727,23 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 				  const unsigned char *addr, u16 vid,
 				  u16 flags, struct dsa_db db)
 {
+	bool is_dynamic = !!(flags & DSA_FDB_FLAG_DYNAMIC);
 	struct mv88e6xxx_chip *chip = ds->priv;
+	u8 state;
 	int err;
 
+	is_dynamic &= chip->ports[port].locked;
+	state = MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC;
+	if (is_dynamic)
+		state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
+
 	mv88e6xxx_reg_lock(chip);
-	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
-					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
+	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, state);
 	mv88e6xxx_reg_unlock(chip);
 
+	if (is_dynamic && !err)
+		mv88e6xxx_set_fdb_offloaded(ds, port, addr, vid);
+
 	return err;
 }
 
@@ -6679,6 +6689,8 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 		err = mv88e6xxx_port_set_lock(chip, port, locked);
 		if (err)
 			goto out;
+
+		mv88e6xxx_port_set_locked(chip, port, locked);
 	}
 out:
 	mv88e6xxx_reg_unlock(chip);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index da6e1339f809..bf61eb54c091 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -281,8 +281,9 @@ struct mv88e6xxx_port {
 	char serdes_irq_name[64];
 	struct devlink_region *region;
 
-	/* MacAuth Bypass control flag */
+	/* Locked and MacAuth Bypass control flags */
 	bool mab;
+	bool locked;
 };
 
 enum mv88e6xxx_region_id {
@@ -795,6 +796,12 @@ static inline bool mv88e6xxx_is_invalid_port(struct mv88e6xxx_chip *chip, int po
 	return (chip->info->invalid_port_mask & BIT(port)) != 0;
 }
 
+static inline void mv88e6xxx_port_set_locked(struct mv88e6xxx_chip *chip,
+					     int port, bool locked)
+{
+	chip->ports[port].locked = locked;
+}
+
 static inline void mv88e6xxx_port_set_mab(struct mv88e6xxx_chip *chip,
 					  int port, bool mab)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index ce3b3690c3c0..c95f8cffba41 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -432,6 +432,27 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
 
 	spid = entry.state;
 
+	if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) {
+		unsigned long port = 0;
+		unsigned long portvec = entry.portvec;
+
+		port = find_first_bit(&portvec, MV88E6XXX_MAX_PVT_PORTS);
+		if (port >= MV88E6XXX_MAX_PVT_PORTS) {
+			dev_err(chip->dev,
+				"ATU err: mac: %pM. Port not in portvec: %x\n",
+				entry.mac, entry.portvec);
+			goto out;
+		}
+
+		spid = port;
+		trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid,
+						      entry.portvec, entry.mac,
+						      fid);
+
+		err = mv88e6xxx_handle_age_out_violation(chip, spid,
+							 &entry, fid);
+	}
+
 	if (val & MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION) {
 		trace_mv88e6xxx_atu_member_violation(chip->dev, spid,
 						     entry.portvec, entry.mac,
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index f79cf716c541..5225971b9a33 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1255,7 +1255,11 @@ int mv88e6xxx_port_set_lock(struct mv88e6xxx_chip *chip, int port,
 
 	reg &= ~MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
 	if (locked)
-		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT;
+		reg |= MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT |
+			MV88E6XXX_PORT_ASSOC_VECTOR_REFRESH_LOCKED |
+			MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG |
+			MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT |
+			MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
 
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR, reg);
 }
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.c b/drivers/net/dsa/mv88e6xxx/switchdev.c
index 4c346a884fb2..76f7f8cc1835 100644
--- a/drivers/net/dsa/mv88e6xxx/switchdev.c
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.c
@@ -12,6 +12,25 @@
 #include "global1.h"
 #include "switchdev.h"
 
+void mv88e6xxx_set_fdb_offloaded(struct dsa_switch *ds, int port,
+				 const unsigned char *addr, u16 vid)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = addr,
+		.vid = vid,
+		.offloaded = true,
+	};
+	struct net_device *brport;
+	struct dsa_port *dp;
+
+	dp = dsa_to_port(ds, port);
+	brport = dsa_port_to_bridge_port(dp);
+
+	if (brport)
+		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,
+					 brport, &info.info, NULL);
+}
+
 struct mv88e6xxx_fid_search_ctx {
 	u16 fid_search;
 	u16 vid_found;
@@ -81,3 +100,45 @@ int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,
 
 	return err;
 }
+
+int mv88e6xxx_handle_age_out_violation(struct mv88e6xxx_chip *chip, int port,
+				       struct mv88e6xxx_atu_entry *entry,
+				       u16 fid)
+{
+	struct switchdev_notifier_fdb_info info = {
+		.addr = entry->mac,
+	};
+	struct net_device *brport;
+	struct dsa_port *dp;
+	u16 vid;
+	int err;
+
+	err = mv88e6xxx_find_vid(chip, fid, &vid);
+	if (err)
+		return err;
+
+	info.vid = vid;
+	entry->portvec &= ~BIT(port);
+	entry->state = MV88E6XXX_G1_ATU_DATA_STATE_UC_UNUSED;
+	entry->trunk = false;
+
+	mv88e6xxx_reg_lock(chip);
+	err = mv88e6xxx_g1_atu_loadpurge(chip, fid, entry);
+	mv88e6xxx_reg_unlock(chip);
+	if (err)
+		return err;
+
+	dp = dsa_to_port(chip->ds, port);
+
+	rtnl_lock();
+	brport = dsa_port_to_bridge_port(dp);
+	if (!brport) {
+		rtnl_unlock();
+		return -ENODEV;
+	}
+	err = call_switchdev_notifiers(SWITCHDEV_FDB_DEL_TO_BRIDGE,
+				       brport, &info.info, NULL);
+	rtnl_unlock();
+
+	return err;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/switchdev.h b/drivers/net/dsa/mv88e6xxx/switchdev.h
index 62214f9d62b0..5af6ac6a490a 100644
--- a/drivers/net/dsa/mv88e6xxx/switchdev.h
+++ b/drivers/net/dsa/mv88e6xxx/switchdev.h
@@ -12,8 +12,13 @@
 
 #include "chip.h"
 
+void mv88e6xxx_set_fdb_offloaded(struct dsa_switch *ds, int port,
+				 const unsigned char *addr, u16 vid);
 int mv88e6xxx_handle_miss_violation(struct mv88e6xxx_chip *chip, int port,
 				    struct mv88e6xxx_atu_entry *entry,
 				    u16 fid);
+int mv88e6xxx_handle_age_out_violation(struct mv88e6xxx_chip *chip, int port,
+				       struct mv88e6xxx_atu_entry *entry,
+				       u16 fid);
 
 #endif /* _MV88E6XXX_SWITCHDEV_H_ */
diff --git a/drivers/net/dsa/mv88e6xxx/trace.h b/drivers/net/dsa/mv88e6xxx/trace.h
index f59ca04768e7..c6b32abf68a5 100644
--- a/drivers/net/dsa/mv88e6xxx/trace.h
+++ b/drivers/net/dsa/mv88e6xxx/trace.h
@@ -40,6 +40,11 @@ DECLARE_EVENT_CLASS(mv88e6xxx_atu_violation,
 		  __entry->addr, __entry->fid)
 );
 
+DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_age_out_violation,
+	     TP_PROTO(const struct device *dev, int spid, u16 portvec,
+		      const unsigned char *addr, u16 fid),
+	     TP_ARGS(dev, spid, portvec, addr, fid));
+
 DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_member_violation,
 	     TP_PROTO(const struct device *dev, int spid, u16 portvec,
 		      const unsigned char *addr, u16 fid),
-- 
2.34.1

[PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans J. Schultz]

Test FDB ageing of user entry created by

bridge fdb replace ADDR dev <DEV> master dynamic

Use LOW_AGEING_TIME variable in forwarding.config to set a low ageing time.
Beware, DSA might not accept the ageing time you want. Check the
age_time_coeff value for your driver.

Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>
---
 .../net/forwarding/bridge_locked_port.sh      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index dc92d32464f6..dbc7017fd45d 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -14,6 +14,7 @@ ALL_TESTS="
 NUM_NETIFS=4
 CHECK_TC="no"
 source lib.sh
+source tc_common.sh
 
 h1_create()
 {
@@ -319,6 +320,41 @@ locked_port_mab_flush()
 	log_test "Locked port MAB FDB flush"
 }
 
+# Test of dynamic FDB entries.
+locked_port_dyn_fdb()
+{
+	local mac=00:01:02:03:04:05
+	local ageing_time
+
+	RET=0
+	ageing_time=$(bridge_ageing_time_get br0)
+	tc qdisc add dev $swp2 clsact
+	ip link set dev br0 type bridge ageing_time $LOW_AGEING_TIME
+	bridge link set dev $swp1 learning on locked on
+
+	bridge fdb replace $mac dev $swp1 master dynamic
+	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
+		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
+
+	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
+	tc_check_packets "dev $swp2 egress" 1 1
+	check_err $? "Packet not seen on egress after adding dynamic FDB"
+
+	sleep $((LOW_AGEING_TIME / 100 + 10))
+
+	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
+	tc_check_packets "dev $swp2 egress" 1 1
+	check_fail $? "Dynamic FDB entry did not age out"
+
+	ip link set dev br0 type bridge ageing_time $ageing_time
+	bridge link set dev $swp1 learning off locked off
+	tc qdisc del dev $swp2 clsact
+
+	log_test "Locked port dyn FDB"
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.34.1

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Ido Schimmel]

On Sat, Mar 18, 2023 at 03:10:10PM +0100, Hans J. Schultz wrote:
> +# Test of dynamic FDB entries.
> +locked_port_dyn_fdb()
> +{
> +	local mac=00:01:02:03:04:05
> +	local ageing_time
> +
> +	RET=0
> +	ageing_time=$(bridge_ageing_time_get br0)
> +	tc qdisc add dev $swp2 clsact
> +	ip link set dev br0 type bridge ageing_time $LOW_AGEING_TIME
> +	bridge link set dev $swp1 learning on locked on
> +
> +	bridge fdb replace $mac dev $swp1 master dynamic
> +	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
> +		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
> +
> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q

Packets should be injected via $h1, not $swp1. See other test cases in
this file.

> +	tc_check_packets "dev $swp2 egress" 1 1
> +	check_err $? "Packet not seen on egress after adding dynamic FDB"

Does this actually work? The packet is transmitted via $swp1, I don't
understand how it can arrive at the egress of $swp2.

> +
> +	sleep $((LOW_AGEING_TIME / 100 + 10))
> +
> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> +	tc_check_packets "dev $swp2 egress" 1 1
> +	check_fail $? "Dynamic FDB entry did not age out"

Shouldn't this be check_err()? After the FDB entry was aged you want to
make sure that packets received via $swp1 with SMAC being $mac are no
longer forwarded by the bridge.

Also, I suggest executing 'bridge fdb get' to make sure the entry is no
longer present in the bridge FDB.

> +
> +	ip link set dev br0 type bridge ageing_time $ageing_time
> +	bridge link set dev $swp1 learning off locked off
> +	tc qdisc del dev $swp2 clsact
> +
> +	log_test "Locked port dyn FDB"
> +}
> +
>  trap cleanup EXIT
>  
>  setup_prepare
> -- 
> 2.34.1
> 

Re: [PATCH v2 net-next 1/6] net: bridge: add dynamic flag to switchdev notifier

[Ido Schimmel]

On Sat, Mar 18, 2023 at 03:10:05PM +0100, Hans J. Schultz wrote:
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index ca0312b78294..aaf918d4ba67 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -249,6 +249,7 @@ struct switchdev_notifier_fdb_info {
>  	u8 added_by_user:1,
>  	   is_local:1,
>  	   locked:1,
> +	   is_dyn:1,
>  	   offloaded:1;
>  };
>  
> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index de18e9c1d7a7..9707d3fdb396 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c
> @@ -134,6 +134,7 @@ static void br_switchdev_fdb_populate(struct net_bridge *br,
>  	item->added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
>  	item->offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
>  	item->is_local = test_bit(BR_FDB_LOCAL, &fdb->flags);
> +	item->is_dyn = !test_bit(BR_FDB_STATIC, &fdb->flags);

I was under the impression that the consensus was to rename this to
'is_static' so that it is consistent with other flags.

>  	item->locked = false;
>  	item->info.dev = (!p || item->is_local) ? br->dev : p->dev;
>  	item->info.ctx = ctx;
> -- 
> 2.34.1
> 

Re: [PATCH v2 net-next 1/6] net: bridge: add dynamic flag to switchdev notifier

[Hans Schultz]

On Mon, Mar 20, 2023 at 10:48, Ido Schimmel <idosch@nvidia.com> wrote:
>
> I was under the impression that the consensus was to rename this to
> 'is_static' so that it is consistent with other flags.
>

I think the consensus was that the bridge maintainers would decide if it
should be changed, this according to Oltean. I still think that
is_dyn is more secure codewise in the long run and it is logical as that
is what the feature the flag concerns.

When you say consistent with other flags, I don't understand the
inconsistency. Could you please explain.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> +	tc_check_packets "dev $swp2 egress" 1 1
>> +	check_fail $? "Dynamic FDB entry did not age out"
>
> Shouldn't this be check_err()? After the FDB entry was aged you want to
> make sure that packets received via $swp1 with SMAC being $mac are no
> longer forwarded by the bridge.

I was thinking that check_fail() will pass when tc_check_packets() does
not see any packets, thus the test passing here when no packets are forwarded?

>
> Also, I suggest executing 'bridge fdb get' to make sure the entry is no
> longer present in the bridge FDB.
>

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Sat, Mar 18, 2023 at 03:10:06PM +0100, Hans J. Schultz wrote:
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index e5f156940c67..c07a2e225ae5 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -626,6 +626,12 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  	ds->configure_vlan_while_not_filtering = true;
>  
> +	/* Since dynamic FDB entries are legacy, all switch drivers should
> +	 * support the flag at least by just installing a static entry and
> +	 * letting the bridge age it.
> +	 */
> +	ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC;

I believe that switchdev has a structural problem in the fact that FDB
entries with flags that aren't interpreted by drivers (so they don't
know if those flags are set or unset) are still passed to the switchdev
notifier chains by default.

I don't believe that anybody used 'bridge fdb add <mac> <dev> master dynamic"
while relying on a static FDB entry in the DSA offloaded data path.

Just like commit 6ab4c3117aec ("net: bridge: don't notify switchdev for
local FDB addresses"), we could deny that for stable kernels, and add
the correct interpretation of the flag in net-next.

Ido, Nikolay, Roopa, Jiri, thoughts?

> +
>  	err = ds->ops->setup(ds);
>  	if (err < 0)
>  		goto unregister_notifier;

By the way, there is a behavior change here.

Before:

$ ip link add br0 type bridge && ip link set br0 up
$ ip link set swp0 master br0 && ip link set swp0 up
$ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
[   70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0
[   70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1
.... 5 minutes later
[  371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1
[  371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0
$ bridge fdb | grep 00:01:02:03:04:05

After:

$ ip link add br0 type bridge && ip link set br0 up
$ ip link set swp0 master br0 && ip link set swp0 up
$ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
[  222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1
[  222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1
.... 5 minutes later
$ bridge fdb | grep 00:01:02:03:04:05
00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale
00:01:02:03:04:05 dev swp0 offload master br0 stale
00:01:02:03:04:05 dev swp0 vlan 1 self
00:01:02:03:04:05 dev swp0 self

As you can see, the behavior is not identical, and it made more sense
before.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> By the way, there is a behavior change here.
>
> Before:
>
> $ ip link add br0 type bridge && ip link set br0 up
> $ ip link set swp0 master br0 && ip link set swp0 up
> $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> [   70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0
> [   70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1
> .... 5 minutes later
> [  371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1
> [  371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0
> $ bridge fdb | grep 00:01:02:03:04:05
>
> After:
>
> $ ip link add br0 type bridge && ip link set br0 up
> $ ip link set swp0 master br0 && ip link set swp0 up
> $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> [  222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1
> [  222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1
> .... 5 minutes later
> $ bridge fdb | grep 00:01:02:03:04:05
> 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale
> 00:01:02:03:04:05 dev swp0 offload master br0 stale
> 00:01:02:03:04:05 dev swp0 vlan 1 self
> 00:01:02:03:04:05 dev swp0 self
>
> As you can see, the behavior is not identical, and it made more sense
> before.

I see this is Felix Ocelot and there is no changes in this patchset that
affects Felix Ocelot. Thus I am quite sure the results will be the same
without this patchset, ergo it must be because of another patch. All
that is done here in the DSA layer is to pass on an extra field and add
an extra check that will always pass in the case of this flag.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Mon, Mar 27, 2023 at 05:31:26PM +0200, Hans Schultz wrote:
> On Mon, Mar 27, 2023 at 14:52, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > By the way, there is a behavior change here.
> >
> > Before:
> >
> > $ ip link add br0 type bridge && ip link set br0 up
> > $ ip link set swp0 master br0 && ip link set swp0 up
> > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> > [   70.010181] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0
> > [   70.019105] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1
> > .... 5 minutes later
> > [  371.686935] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 1
> > [  371.695449] mscc_felix 0000:00:00.5: felix_fdb_del: port 0 addr 00:01:02:03:04:05 vid 0
> > $ bridge fdb | grep 00:01:02:03:04:05
> >
> > After:
> >
> > $ ip link add br0 type bridge && ip link set br0 up
> > $ ip link set swp0 master br0 && ip link set swp0 up
> > $ bridge fdb add dev swp0 00:01:02:03:04:05 master dynamic
> > [  222.071492] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 0 flags 0x1
> > [  222.081154] mscc_felix 0000:00:00.5: felix_fdb_add: port 0 addr 00:01:02:03:04:05 vid 1 flags 0x1
> > .... 5 minutes later
> > $ bridge fdb | grep 00:01:02:03:04:05
> > 00:01:02:03:04:05 dev swp0 vlan 1 offload master br0 stale
> > 00:01:02:03:04:05 dev swp0 offload master br0 stale
> > 00:01:02:03:04:05 dev swp0 vlan 1 self
> > 00:01:02:03:04:05 dev swp0 self
> >
> > As you can see, the behavior is not identical, and it made more sense
> > before.
> 
> I see this is Felix Ocelot and there is no changes in this patchset that
> affects Felix Ocelot. Thus I am quite sure the results will be the same
> without this patchset, ergo it must be because of another patch. All
> that is done here in the DSA layer is to pass on an extra field and add
> an extra check that will always pass in the case of this flag.

If mv88e6xxx is all you have, you can still sanity-check the equivalent
effect of your patch set to other drivers by simply not acting upon the
"flags" argument in mv88e6xxx_port_fdb_add()/mv88e6xxx_port_fdb_del(),
and disabling the logic to treat Age Out interrupts. Then you should be
able to notice exactly the behavior change I am talking about.

In your own commit message, it says:

Author: Hans J. Schultz <netdev@kapio-technology.com>

    net: bridge: ensure FDB offloaded flag is handled as needed

    Since user added entries in the bridge FDB will get the BR_FDB_OFFLOADED
                                                        ~~~~~~~~~~~~~~~~~~~~
    flag set, we do not want the bridge to age those entries and we want the
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    entries to be deleted in the bridge upon an SWITCHDEV_FDB_DEL_TO_BRIDGE
                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                        existing drivers do not emit this
    event.

    Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com>

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e69a872bfc1d..b0c23a72bc76 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -537,6 +537,7 @@ void br_fdb_cleanup(struct work_struct *work)
 		unsigned long this_timer = f->updated + delay;
 
 		if (test_bit(BR_FDB_STATIC, &f->flags) ||
+		    test_bit(BR_FDB_OFFLOADED, &f->flags) ||
 		    test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
 			if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
 				if (time_after(this_timer, now))
@@ -1465,7 +1466,9 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 	spin_lock_bh(&br->hash_lock);
 
 	fdb = br_fdb_find(br, addr, vid);
-	if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
+	if (fdb &&
+	    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) ||
+	     test_bit(BR_FDB_OFFLOADED, &fdb->flags)))
 		fdb_delete(br, fdb, swdev_notify);
 	else
 		err = -ENOENT;


A reasonable question you could ask yourself is: why do my BR_FDB_OFFLOADED
entries have this flag in the software bridge in the first place?
Did I add code for it? Is it because there is some difference between
mv88e6xxx and ocelot/felix, or is it because dsa_fdb_offload_notify()
gets called in both cases from generic code just the same?

And if dsa_fdb_offload_notify() gets called in both cases just the same,
but no other driver except for mv88e6xxx emits the SWITCHDEV_FDB_DEL_TO_BRIDGE
which you've patched the bridge to expect in this series, then what exactly
is surprising in the fact that offloaded and dynamic FDB entries now become
stale, but are not removed from the software bridge as they were before?

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Mon, Mar 27, 2023 at 19:00, Vladimir Oltean <olteanv@gmail.com> wrote:
> A reasonable question you could ask yourself is: why do my BR_FDB_OFFLOADED
> entries have this flag in the software bridge in the first place?
> Did I add code for it? Is it because there is some difference between
> mv88e6xxx and ocelot/felix, or is it because dsa_fdb_offload_notify()
> gets called in both cases from generic code just the same?
>
> And if dsa_fdb_offload_notify() gets called in both cases just the same,
> but no other driver except for mv88e6xxx emits the SWITCHDEV_FDB_DEL_TO_BRIDGE
> which you've patched the bridge to expect in this series, then what exactly
> is surprising in the fact that offloaded and dynamic FDB entries now become
> stale, but are not removed from the software bridge as they were before?

Yes, I see I have missed that the dsa layer already adds the offloaded
flag in dsa_slave_switchdev_event_work() in slave.c.

My first approach was to use the SWITCHDEV_FDB_ADD_TO_BRIDGE event
and not the SWITCHDEV_FDB_OFFLOADED event as the first would set the
external learned flag which is not aged out by the bridge.
I have at some point earlier asked why there would be two quite
equivalent flags and what the difference between them are, but I didn't
get a response.

Now I see the difference and that I cannot use the offloaded flag
without changing the behaviour of the system as I actually change the
behaviour of the offloaded flag in this version of the patch-set.

So if the idea of a 'synthetically' learned fdb entry from the driver
using the SWITCHDEV_FDB_ADD_TO_BRIDGE event from the driver towards the
bridge instead is accepted, I can go with that?
(thus removing all the changes in the patch-set regarding the offloaded
flag ofcourse)

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Mon, Mar 27, 2023 at 11:49:58PM +0200, Hans Schultz wrote:
> My first approach was to use the SWITCHDEV_FDB_ADD_TO_BRIDGE event
> and not the SWITCHDEV_FDB_OFFLOADED event as the first would set the
> external learned flag which is not aged out by the bridge.

Link to patch? I don't see any SWITCHDEV_FDB_ADD_TO_BRIDGE call in
either the v1:
https://lore.kernel.org/netdev/20230130173429.3577450-6-netdev@kapio-technology.com/
or the RFC:
https://lore.kernel.org/netdev/20230117185714.3058453-6-netdev@kapio-technology.com/
and the change log does not mention it either.

> I have at some point earlier asked why there would be two quite
> equivalent flags and what the difference between them are, but I didn't
> get a response.

Actually, the part which you are now posing as a question (what is the
difference?) was part of the premise of your earlier question (there is
no difference => why do we have both?).
https://lore.kernel.org/netdev/d972e76bed896b229d9df4da81ad8eb4@kapio-technology.com/

I believe that no one answered because the question was confused and it
wasn't really clear what you were asking.

> 
> Now I see the difference and that I cannot use the offloaded flag
> without changing the behaviour of the system as I actually change the
> behaviour of the offloaded flag in this version of the patch-set.
> 
> So if the idea of a 'synthetically' learned fdb entry from the driver
> using the SWITCHDEV_FDB_ADD_TO_BRIDGE event from the driver towards the
> bridge instead is accepted, I can go with that?
> (thus removing all the changes in the patch-set regarding the offloaded
> flag ofcourse)

which idea is that, again?

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> which idea is that, again?

So I cannot us the offloaded flag as it is added by DSA in the common
case when using 'bridge fdb replace ... dynamic'.

The idea is then to use the ext_learn flag instead, which is not aged by
the bridge. To do this the driver (mv88e6xxx) will send a
SWITCHDEV_FDB_ADD_TO_BRIDGE switchdev event when the new dynamic flag is
true. The function sending this event will then be named
mv88e6xxx_add_fdb_synth_learned() in
drivers/net/dsa/mv88e6xxx/switchdev.c, replacing the
mv88e6xxx_set_fdb_offloaded() function but in most part the same
content, just another event type.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Tue, Mar 28, 2023 at 01:04:23PM +0200, Hans Schultz wrote:
> On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > which idea is that, again?
> 
> So I cannot us the offloaded flag as it is added by DSA in the common
> case when using 'bridge fdb replace ... dynamic'.

Why not? I find it reasonable that the software bridge does not age out
a dynamic FDB entry that is offloaded to hardware... the hardware should
do that ("dynamic" being the key). At least, I find it more reasonable
than the current behavior, where the bridge notifies dynamic FDB entries
to switchdev, but doesn't say they're dynamic, and switchdev treats them
as static, so they don't roam from one bridge port to another until
software sees a packet with that MAC DA, and they have the potential of
blocking traffic because of that.

If for some reason you do think that behavior is useful and still want
to keep it (I'm not sure I would), I would consider extending struct
switchdev_notifier_fdb_info with a "bool pls_dont_age_out", and I would
make dsa_fdb_offload_notify() set this to true if the driver did
actually install the dynamic FDB entry as dynamic in the ATU.

> 
> The idea is then to use the ext_learn flag instead, which is not aged by
> the bridge. To do this the driver (mv88e6xxx) will send a
> SWITCHDEV_FDB_ADD_TO_BRIDGE switchdev event when the new dynamic flag is
> true. The function sending this event will then be named
> mv88e6xxx_add_fdb_synth_learned() in
> drivers/net/dsa/mv88e6xxx/switchdev.c, replacing the
> mv88e6xxx_set_fdb_offloaded() function but in most part the same
> content, just another event type.

Basically you're suggesting that the hardware driver, after receiving a
SWITCHDEV_FDB_ADD_TO_DEVICE and responding to it with SWITCHDEV_FDB_OFFLOADED,
emits a SWITCHDEV_FDB_ADD_TO_BRIDGE which takes over that software
bridge FDB entry, with the advantage that the new one already has the
semantics of not being aged out by the software bridge.

hmmm... I'd say that the flow should work even with a single notifier
emitted from the driver side, which would be SWITCHDEV_FDB_OFFLOADED,
perhaps annotated with some qualifiers that would inform the bridge a
certain behavior is required. Although, as mentioned, I think that in
principle, "pls_dont_age_out" should be unnecessary, because it just
papers over the issue that switchdev drivers treat static and dynamic
FDB entries just the same, and "pls_dont_age_out" would be the
differentiator for an issue that should have been solved elsewhere, as
it could lead to other problems of its own.

Basically we're designing around a workaround to a problem to which
we're turning a blind eye. These are my 2c.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Ido Schimmel]

On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> >> +	tc_check_packets "dev $swp2 egress" 1 1
> >> +	check_fail $? "Dynamic FDB entry did not age out"
> >
> > Shouldn't this be check_err()? After the FDB entry was aged you want to
> > make sure that packets received via $swp1 with SMAC being $mac are no
> > longer forwarded by the bridge.
> 
> I was thinking that check_fail() will pass when tc_check_packets() does
> not see any packets, thus the test passing here when no packets are forwarded?

What do you mean by "I was *thinking*"? How is it possible that you are
submitting a selftest that you didn't bother running?!

I see you trimmed my earlier question: "Does this actually work?"

I tried it and it passed:

# ./bridge_locked_port.sh                         
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]            
TEST: Locked port MAB                                               [ OK ]            
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]

And I couldn't understand how that's even possible. Then I realized that
the entire test is dead code because the patch is missing this
fundamental hunk:

```
diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index dbc7017fd45d..5bf6b2aa1098 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -9,6 +9,7 @@ ALL_TESTS="
        locked_port_mab_roam
        locked_port_mab_config
        locked_port_mab_flush
+       locked_port_dyn_fdb
 "
 
 NUM_NETIFS=4
```

Which tells me that you didn't even try running it once. Now the test
failed as I expected:

# ./bridge_locked_port.sh                         
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]            
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [FAIL]            
        Packet not seen on egress after adding dynamic FDB

Fixed by:

```
@@ -336,7 +337,7 @@ locked_port_dyn_fdb()
        tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
                dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
 
-       $MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
+       $MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
                -a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
        tc_check_packets "dev $swp2 egress" 1 1
        check_err $? "Packet not seen on egress after adding dynamic FDB"
```

Ran it again and it failed because of the second issue I pointed out:

# ./bridge_locked_port.sh 
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [FAIL]
        Dynamic FDB entry did not age out                                             

Fixed by:

```
@@ -346,7 +347,7 @@ locked_port_dyn_fdb()
        $MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
                -a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
        tc_check_packets "dev $swp2 egress" 1 1
-       check_fail $? "Dynamic FDB entry did not age out"
+       check_err $? "Dynamic FDB entry did not age out"
 
        ip link set dev br0 type bridge ageing_time $ageing_time
        bridge link set dev $swp1 learning off locked off
```

# ./bridge_locked_port.sh 
TEST: Locked port ipv4                                              [ OK ]
TEST: Locked port ipv6                                              [ OK ]
TEST: Locked port vlan                                              [ OK ]
TEST: Locked port MAB                                               [ OK ]
TEST: Locked port MAB roam                                          [ OK ]
TEST: Locked port MAB configuration                                 [ OK ]
TEST: Locked port MAB FDB flush                                     [ OK ]
TEST: Locked port dyn FDB                                           [ OK ]

Sigh

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
>> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> >> +	tc_check_packets "dev $swp2 egress" 1 1
>> >> +	check_fail $? "Dynamic FDB entry did not age out"
>> >
>> > Shouldn't this be check_err()? After the FDB entry was aged you want to
>> > make sure that packets received via $swp1 with SMAC being $mac are no
>> > longer forwarded by the bridge.
>> 
>> I was thinking that check_fail() will pass when tc_check_packets() does
>> not see any packets, thus the test passing here when no packets are forwarded?
>
> What do you mean by "I was *thinking*"? How is it possible that you are
> submitting a selftest that you didn't bother running?!
>

Sorry, but I have sent you several emails telling you about the problems
I have with running the selftests due to changes in the phy etc. Maybe
you have just not received all those emails?

Have you checked spamfilters?

With the kernels now, I cannot even test with the software bridge and
selftests as the compile fails - probably due to changes in uapi headers
compared to what the packages my system uses expects.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Tue, Mar 28, 2023 at 14:49, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Mar 28, 2023 at 01:04:23PM +0200, Hans Schultz wrote:
>> On Tue, Mar 28, 2023 at 01:59, Vladimir Oltean <olteanv@gmail.com> wrote:
>> >
>> > which idea is that, again?
>> 
>> So I cannot us the offloaded flag as it is added by DSA in the common
>> case when using 'bridge fdb replace ... dynamic'.
>
> Why not? I find it reasonable that the software bridge does not age out
> a dynamic FDB entry that is offloaded to hardware... the hardware should
> do that ("dynamic" being the key).

So the solution would be to not let the DSA layer send the
SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is
set?
Thus other drivers that don't support the flag yet will install a
static entry in HW and the bridge will age it out as there is no offloaded
flag on. For the mv88e6xxx it will set the offloaded flag and HW will
age it.

> At least, I find it more reasonable
> than the current behavior, where the bridge notifies dynamic FDB entries
> to switchdev, but doesn't say they're dynamic, and switchdev treats them
> as static, so they don't roam from one bridge port to another until
> software sees a packet with that MAC DA, and they have the potential of
> blocking traffic because of that.
>

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Ido Schimmel]

On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
> On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> > On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
> >> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
> >> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> >> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> >> >> +	tc_check_packets "dev $swp2 egress" 1 1
> >> >> +	check_fail $? "Dynamic FDB entry did not age out"
> >> >
> >> > Shouldn't this be check_err()? After the FDB entry was aged you want to
> >> > make sure that packets received via $swp1 with SMAC being $mac are no
> >> > longer forwarded by the bridge.
> >> 
> >> I was thinking that check_fail() will pass when tc_check_packets() does
> >> not see any packets, thus the test passing here when no packets are forwarded?
> >
> > What do you mean by "I was *thinking*"? How is it possible that you are
> > submitting a selftest that you didn't bother running?!
> >
> 
> Sorry, but I have sent you several emails telling you about the problems
> I have with running the selftests due to changes in the phy etc. Maybe
> you have just not received all those emails?
> 
> Have you checked spamfilters?
> 
> With the kernels now, I cannot even test with the software bridge and
> selftests as the compile fails - probably due to changes in uapi headers
> compared to what the packages my system uses expects.

My spam filters are fine. I saw your emails where you basically said
that you are too lazy to setup a VM to test your patches and that your
time is more valuable than mine, which is why I should be testing them.
Stop making your problems our problems. It's hardly the first time. If
you are unable to test your patches, then invest the time in fixing your
setup instead of submitting completely broken patches and making it our
problem to test and fix them. I refuse to invest time in reviewing /
testing / reworking your submissions as long as you insist on doing less
than the bare minimum.

Good luck

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Thu, Mar 30, 2023 at 09:37, Ido Schimmel <idosch@nvidia.com> wrote:
> On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
>> 
>> Sorry, but I have sent you several emails telling you about the problems
>> I have with running the selftests due to changes in the phy etc. Maybe
>> you have just not received all those emails?
>> 
>> Have you checked spamfilters?
>> 
>> With the kernels now, I cannot even test with the software bridge and
>> selftests as the compile fails - probably due to changes in uapi headers
>> compared to what the packages my system uses expects.
>
> My spam filters are fine. I saw your emails where you basically said
> that you are too lazy to setup a VM to test your patches and that your
> time is more valuable than mine, which is why I should be testing them.
> Stop making your problems our problems. It's hardly the first time. If
> you are unable to test your patches, then invest the time in fixing your
> setup instead of submitting completely broken patches and making it our
> problem to test and fix them. I refuse to invest time in reviewing /
> testing / reworking your submissions as long as you insist on doing less
> than the bare minimum.
>
> Good luck

I never said or indicated that my time is more valuable than yours. I
have a VM to run these things that some have spent countless hours to
develop with the right tools etc installed and set up. Fixing that
system will take quite many hours for me, so I am asking for some simple
assistance from someone who already has a system running supporting the
newest kernel.

Alternatively if there is an open sourced system available that would be
great.

As this patch-set is for the community and some companies that would
like to use it and not for myself, I am asking for some help from the
community with a task that when someone has the system in place should
not take more than 15-20 minutes, maybe even less.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Nikolay Aleksandrov]

On 30/03/2023 13:29, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 09:37, Ido Schimmel <idosch@nvidia.com> wrote:
>> On Tue, Mar 28, 2023 at 09:30:08PM +0200, Hans Schultz wrote:
>>>
>>> Sorry, but I have sent you several emails telling you about the problems
>>> I have with running the selftests due to changes in the phy etc. Maybe
>>> you have just not received all those emails?
>>>
>>> Have you checked spamfilters?
>>>
>>> With the kernels now, I cannot even test with the software bridge and
>>> selftests as the compile fails - probably due to changes in uapi headers
>>> compared to what the packages my system uses expects.
>>
>> My spam filters are fine. I saw your emails where you basically said
>> that you are too lazy to setup a VM to test your patches and that your
>> time is more valuable than mine, which is why I should be testing them.
>> Stop making your problems our problems. It's hardly the first time. If
>> you are unable to test your patches, then invest the time in fixing your
>> setup instead of submitting completely broken patches and making it our
>> problem to test and fix them. I refuse to invest time in reviewing /
>> testing / reworking your submissions as long as you insist on doing less
>> than the bare minimum.
>>
>> Good luck
> 
> I never said or indicated that my time is more valuable than yours. I
> have a VM to run these things that some have spent countless hours to
> develop with the right tools etc installed and set up. Fixing that
> system will take quite many hours for me, so I am asking for some simple
> assistance from someone who already has a system running supporting the
> newest kernel.
> 
> Alternatively if there is an open sourced system available that would be
> great.
> 
> As this patch-set is for the community and some companies that would
> like to use it and not for myself, I am asking for some help from the
> community with a task that when someone has the system in place should
> not take more than 15-20 minutes, maybe even less.

I'm sorry but this is absolutely the wrong way to go about this. Your setup's
problems are yours to figure out and fix, if you are going to send *any* future
patches make absolutely sure they build, run and work as intended.
Please do not send any future patches without them being fully tested and, as
Ido mentioned, cover at least the bare minimum for a submission.

Thanks,
 Nik

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote:
> So the solution would be to not let the DSA layer send the
> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is
> set?

I have never said that.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote:
>> So the solution would be to not let the DSA layer send the
>> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is
>> set?
>
> I have never said that.

No, I was just thinking of a solution based on your previous comment
that dynamic fdb entries with the offloaded flag set should not be aged
out by the bridge as they are now.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Thu, Mar 30, 2023 at 02:59:04PM +0200, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote:
> >> So the solution would be to not let the DSA layer send the
> >> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is
> >> set?
> >
> > I have never said that.
> 
> No, I was just thinking of a solution based on your previous comment
> that dynamic fdb entries with the offloaded flag set should not be aged
> out by the bridge as they are now.

If you were a user of those other drivers, and you ran the command:
"bridge fdb add ... master dynamic"
would you be ok with the behavior: "I don't have dynamic FDB entries,
but here's a static one for you"?

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Thu, Mar 30, 2023 at 16:09, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 02:59:04PM +0200, Hans Schultz wrote:
>> On Thu, Mar 30, 2023 at 15:43, Vladimir Oltean <olteanv@gmail.com> wrote:
>> > On Tue, Mar 28, 2023 at 09:45:26PM +0200, Hans Schultz wrote:
>> >> So the solution would be to not let the DSA layer send the
>> >> SWITCHDEV_FDB_OFFLOADED event in the case when the new dynamic flag is
>> >> set?
>> >
>> > I have never said that.
>> 
>> No, I was just thinking of a solution based on your previous comment
>> that dynamic fdb entries with the offloaded flag set should not be aged
>> out by the bridge as they are now.
>
> If you were a user of those other drivers, and you ran the command:
> "bridge fdb add ... master dynamic"
> would you be ok with the behavior: "I don't have dynamic FDB entries,
> but here's a static one for you"?

I don't know if you have a solution in mind wrt the behaviour of the
offloaded flag if it is not to do as it does now and let the bridge age
out dynamic entries. That led me to conclude that this patch-set cannot
use the offloaded flag, but you seem to suggest otherwise.

If you have a suggestion, feel free.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Thu, Mar 30, 2023 at 04:54:19PM +0200, Hans Schultz wrote:
> I don't know if you have a solution in mind wrt the behaviour of the
> offloaded flag if it is not to do as it does now and let the bridge age
> out dynamic entries. That led me to conclude that this patch-set cannot
> use the offloaded flag, but you seem to suggest otherwise.
> 
> If you have a suggestion, feel free.

Didn't I explain what I would do from the first reply on this thread?
https://patchwork.kernel.org/project/netdevbpf/patch/20230318141010.513424-3-netdev@kapio-technology.com/#25270613

As a bug fix, stop reporting to switchdev those FDB entries with
BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into
"net-next" next Thursday (the ship has sailed for today), add "bool static"
to the switchdev notifier info, and make all switchdev drivers (everywhere
where a SWITCHDEV_FDB_ADD_TO_DEVICE handler appears) ignore the
"added_by_user && !is_static" combination, but by their own choice and
not by switchdev's choice.

Then, make DSA decide whether to handle the "added_by_user && !is_static"
combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC
flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx
driver. When DSA_FDB_FLAG_DYNAMIC is not supported, DSA will not offload
the FDB entry: neither will it call port_fdb_add(), nor will it emit
SWITCHDEV_FDB_OFFLOADED. Ideally, it would also inform user space that
it can't offload this flag by returning an error, but the lack of an
error propagation mechanism from switchdev to the bridge FDB is a known
limitation which is hard to overcome, and is outside the scope of your
patchset I believe. To see whether DSA has acted upon the "master dynamic"
flag or not, it would be good enough for the user to see something
adequate in "bridge fdb show | grep offloaded", I believe.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Then, make DSA decide whether to handle the "added_by_user && !is_static"
> combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC
> flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx
> driver.

Okay, so this will require a new function in the DSA layer that sets
which flags are supported and that the driver will call on
initialization.

Where (in the DSA layer) should such a function be placed and what
should it be called?

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Thu, Mar 30, 2023 at 05:34:44PM +0200, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Then, make DSA decide whether to handle the "added_by_user && !is_static"
> > combination or not, based on the presence of the DSA_FDB_FLAG_DYNAMIC
> > flag, which will be set in ds->supported_fdb_flags only for the mv88e6xxx
> > driver.
> 
> Okay, so this will require a new function in the DSA layer that sets
> which flags are supported and that the driver will call on
> initialization.
> 
> Where (in the DSA layer) should such a function be placed and what
> should it be called?

Don't overthink it, no new function. It's okay to just set
ds->supported_fdb_flags = DSA_FDB_FLAG_DYNAMIC in
mv88e6xxx_register_switch(), near the place where it currently sets
ds->num_lag_ids. Either before dsa_register_switch(), or within the
ds->ops->setup(). Both are fine, since the user network interfaces
haven't been allocated just yet by dsa_slave_create() and so, the
switchdev code path is inaccessible.

Existing drivers will have ds->supported_fdb_flags = 0 by default, since
they allocate the struct dsa_switch with kzalloc(), and DSA will have to
do something sane with that.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Tue, Mar 28, 2023 at 19:40, Ido Schimmel <idosch@nvidia.com> wrote:
> On Sun, Mar 26, 2023 at 05:41:06PM +0200, Hans Schultz wrote:
>> On Mon, Mar 20, 2023 at 10:44, Ido Schimmel <idosch@nvidia.com> wrote:
>> >> +	$MZ $swp1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
>> >> +		-a $mac -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
>> >> +	tc_check_packets "dev $swp2 egress" 1 1
>> >> +	check_fail $? "Dynamic FDB entry did not age out"
>> >
>> > Shouldn't this be check_err()? After the FDB entry was aged you want to
>> > make sure that packets received via $swp1 with SMAC being $mac are no
>> > longer forwarded by the bridge.
>> 
>> I was thinking that check_fail() will pass when tc_check_packets() does
>> not see any packets, thus the test passing here when no packets are forwarded?
>
> What do you mean by "I was *thinking*"? How is it possible that you are
> submitting a selftest that you didn't bother running?!
>
> I see you trimmed my earlier question: "Does this actually work?"
>
> I tried it and it passed:
>
> # ./bridge_locked_port.sh                         
> TEST: Locked port ipv4                                              [ OK ]
> TEST: Locked port ipv6                                              [ OK ]
> TEST: Locked port vlan                                              [ OK ]            
> TEST: Locked port MAB                                               [ OK ]            
> TEST: Locked port MAB roam                                          [ OK ]
> TEST: Locked port MAB configuration                                 [ OK ]
> TEST: Locked port MAB FDB flush                                     [ OK ]
>
> And I couldn't understand how that's even possible. Then I realized that
> the entire test is dead code because the patch is missing this
> fundamental hunk:
>
> ```
> diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> index dbc7017fd45d..5bf6b2aa1098 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
> @@ -9,6 +9,7 @@ ALL_TESTS="
>         locked_port_mab_roam
>         locked_port_mab_config
>         locked_port_mab_flush
> +       locked_port_dyn_fdb
>  "
>  
>  NUM_NETIFS=4
> ```
>
> Which tells me that you didn't even try running it once.

Not true, it reveals that I forgot to put it in the patch, that's all. As
I cannot run several of these tests because of memory constraints I link
the file to a copy in a rw area where I modify the list and just run one
of the subtests at a time. If I try to run the whole it always fails
after a couple of sub-tests with an error.

It seems to me that these scripts are quite memory consuming as they
accumulate memory consuption in relation to what is loaded along the
way. A major problem with my system.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Vladimir Oltean]

On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
> Not true, it reveals that I forgot to put it in the patch, that's all. As
> I cannot run several of these tests because of memory constraints I link
> the file to a copy in a rw area where I modify the list and just run one
> of the subtests at a time. If I try to run the whole it always fails
> after a couple of sub-tests with an error.
> 
> It seems to me that these scripts are quite memory consuming as they
> accumulate memory consuption in relation to what is loaded along the
> way. A major problem with my system.

I'm sorry for perhaps asking something entirely obvious, but have you tried:

kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
board $ cd selftests/drivers/net/dsa/
board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3

?

This is how I always run them, and it worked fine with both Debian
(where it's easy to add missing packages to the rootfs) or with a more
embedded-oriented Buildroot.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
>> Not true, it reveals that I forgot to put it in the patch, that's all. As
>> I cannot run several of these tests because of memory constraints I link
>> the file to a copy in a rw area where I modify the list and just run one
>> of the subtests at a time. If I try to run the whole it always fails
>> after a couple of sub-tests with an error.
>> 
>> It seems to me that these scripts are quite memory consuming as they
>> accumulate memory consuption in relation to what is loaded along the
>> way. A major problem with my system.
>
> I'm sorry for perhaps asking something entirely obvious, but have you tried:
>
> kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
> board $ cd selftests/drivers/net/dsa/
> board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3
>
> ?
>
> This is how I always run them, and it worked fine with both Debian
> (where it's easy to add missing packages to the rootfs) or with a more
> embedded-oriented Buildroot.

I am not entirely clear of your idea. You need somehow to boot into a
system with the patched net-next kernel or you have a virtual machine
boot into a virtual OS. I guess it is the last option you refer to using
Debian?

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 09:07:53PM +0200, Hans Schultz wrote:
>> Not true, it reveals that I forgot to put it in the patch, that's all. As
>> I cannot run several of these tests because of memory constraints I link
>> the file to a copy in a rw area where I modify the list and just run one
>> of the subtests at a time. If I try to run the whole it always fails
>> after a couple of sub-tests with an error.
>> 
>> It seems to me that these scripts are quite memory consuming as they
>> accumulate memory consuption in relation to what is loaded along the
>> way. A major problem with my system.
>
> I'm sorry for perhaps asking something entirely obvious, but have you tried:
>
> kernel-dir $ rsync -avr tools/testing/selftests/ root@$board:selftests/
> board $ cd selftests/drivers/net/dsa/
> board $ ./bridge_locked_port.sh lan0 lan1 lan2 lan3
>
> ?
>
> This is how I always run them, and it worked fine with both Debian
> (where it's easy to add missing packages to the rootfs) or with a more
> embedded-oriented Buildroot.

The memory problems are of course on the embedded target. In that case I
think it would be a very good idea to do something to design the system
better, so that it frees memory between the subtests.

If all tests are always run on the bridge only, I think they don't make
much sense as these patchsets are directed towards switchcores.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Vladimir Oltean]

On Fri, Mar 31, 2023 at 09:43:34AM +0200, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 22:27, Vladimir Oltean <olteanv@gmail.com> wrote:
> > This is how I always run them, and it worked fine with both Debian
> > (where it's easy to add missing packages to the rootfs) or with a more
> > embedded-oriented Buildroot.
> 
> I am not entirely clear of your idea. You need somehow to boot into a
> system with the patched net-next kernel

You have to do that anyway for any kind of kernel work, no?

> or you have a virtual machine boot into a virtual OS. I guess it is
> the last option you refer to using Debian?

You could do that too, but you don't have to. Debian, like many other
Linux distributions, supports a wide variety of CPU architectures; it
can be run on embedded systems just as well as on desktop PCs or VMs.
I didn't say you have to use Debian, though, I just said I ran the
selftests on a Debian-based rootfs and that it was easy to prepare the
environment there. The Debian rootfs and the selftests were deployed to
the target board with the DSA switch on it, in case that wasn't clear.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Vladimir Oltean]

On Fri, Mar 31, 2023 at 10:06:34AM +0200, Hans Schultz wrote:
> The memory problems are of course on the embedded target. In that case I
> think it would be a very good idea to do something to design the system
> better, so that it frees memory between the subtests.

People like Martin Blumenstingl have managed to deploy and run the
networking kselftests on OpenWRT, which typically runs on very
resource-constrained embedded devices.
https://lore.kernel.org/netdev/CAFBinCDX5XRyMyOd-+c_Zkn6dawtBpQ9DaPkA4FDC5agL-t8CA@mail.gmail.com/
https://lore.kernel.org/netdev/20220707135532.1783925-1-martin.blumenstingl@googlemail.com/

Considering that, you'll have to come with a much more concrete description
of why the system should be "designed better" and "free memory between
subtests" (what memory?!) before you could run it on your target system.

Either that, or at least take into serious consideration the fact that you
may be hung up on doing something which isn't necessary for the end goal.

I simply have no clue what you're talking about. It's as if we're talking
about completely different things.

> If all tests are always run on the bridge only, I think they don't make
> much sense as these patchsets are directed towards switchcores.

Is this supposed to mean something, or is it just a random thought you
had, that you believed it would be good to share with us?

The tools/testing/selftests/net/forwarding/lib.sh central framework has
the NETIF_TYPE and NETIF_CREATE variables, which indicate that by default,
veth interfaces are created. When running a bridge selftest with veth as
bridge ports, indeed software bridging should take place, and those
selftests should work fine. In Linux, the software behavior represents a
model for the offload behavior, since offloads are 100% transparent to
the user most of the time.

Below in lib.sh, there is a line which sources "$relative_path/forwarding.config",
a file which can contain customizations of the default variables used by
the framework. Even though it isn't strictly necessary to put the
customized bash variables in a forwarding.config file, it is more
convenient to do this than to specify them as environment variables.

If you "cd tools/testing/selftests/drivers/net/dsa/", you will find
precisely such a forwarding.config file there, which contains the line
"NETIF_CREATE=no", which means that when you run the symlinked sub-group
of forwarding tests relevant to DSA from this folder, the expectation is
that the bridge ports are not veth interfaces created for the test, but
rather, physical ports.

So, by running the command I posted in the earlier email, you actually
run it on the physical DSA user port interfaces, and it should pass
there too. This is based on the equivalency principle between the
software and the hardware data paths that I was talking about.

If you're actively and repeatedly making an effort to work with your eyes
closed, and then build strawmen around the fact that you don't see, then
you're not going to get very friendly reactions from people, me included,
who explain things to you that pertain to your due diligence. This is
because these people know the things that they're explaining to you out
of their own due diligence, and, as a result, are not easily fooled by
your childish excuses.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Fri, Mar 31, 2023 at 12:37, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> So, by running the command I posted in the earlier email, you actually
> run it on the physical DSA user port interfaces, and it should pass
> there too.

Okay, that sounds like a good idea which I have not done before. I am
seeing how I can install Debian in an Qemu or VMWare setup to be able to
test that way.

> This is based on the equivalency principle between the
> software and the hardware data paths that I was talking about.
>
> If you're actively and repeatedly making an effort to work with your eyes
> closed, and then build strawmen around the fact that you don't see, then
> you're not going to get very friendly reactions from people, me included,
> who explain things to you that pertain to your due diligence. This is
> because these people know the things that they're explaining to you out
> of their own due diligence, and, as a result, are not easily fooled by
> your childish excuses.

I am not coming with excuses here, and certainly not childish ones at
that either. I am just pointing out that on my device the tests don't
run well because of memory shortage and my reasoning why I think it is
so.
I will as long as the system is as it is with these selftests, just run
single subtests at a time on target, but if I have new phy problems like
the one you have seen I have had before, then testing on target becomes
off limits.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote:
> As a bug fix, stop reporting to switchdev those FDB entries with
> BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into
> "net-next" next Thursday (the ship has sailed for today), add "bool static"

It is probably too late today (now I have a Debian based VM that can do
the selftests), but with this bug fix I have 1) not submitted bug fixes
before and 2) it probably needs an appropriate explanation, where I
don't know the problem well enough for general switchcores to submit
with a suitable text.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Vladimir Oltean]

On Thu, Apr 06, 2023 at 05:17:46PM +0200, Hans Schultz wrote:
> On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote:
> > As a bug fix, stop reporting to switchdev those FDB entries with
> > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into
> > "net-next" next Thursday (the ship has sailed for today), add "bool static"
> 
> It is probably too late today (now I have a Debian based VM that can do
> the selftests), but with this bug fix I have 1) not submitted bug fixes
> before and 2) it probably needs an appropriate explanation, where I
> don't know the problem well enough for general switchcores to submit
> with a suitable text.

Do you want me to try to submit this change as a bug fix?

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Vladimir Oltean]

On Fri, Mar 31, 2023 at 02:43:11PM +0200, Hans Schultz wrote:
> I will as long as the system is as it is with these selftests, just run
> single subtests at a time on target, but if I have new phy problems like
> the one you have seen I have had before, then testing on target becomes
> off limits.

Please open a dedicated communication channel (separate email thread on
netdev@vger.kernel.org) with the appropriate maintainers for the PHY
code that is failing for you in To:, and you will get the help that you
need to resolve that and to be able to test on the target board.

Re: [PATCH v2 net-next 2/6] net: dsa: propagate flags down towards drivers

[Hans Schultz]

On Thu, Apr 06, 2023 at 18:21, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Thu, Apr 06, 2023 at 05:17:46PM +0200, Hans Schultz wrote:
>> On Thu, Mar 30, 2023 at 18:07, Vladimir Oltean <olteanv@gmail.com> wrote:
>> > As a bug fix, stop reporting to switchdev those FDB entries with
>> > BR_FDB_ADDED_BY_USER && !BR_FDB_STATIC. Then, after "net" is merged into
>> > "net-next" next Thursday (the ship has sailed for today), add "bool static"
>> 
>> It is probably too late today (now I have a Debian based VM that can do
>> the selftests), but with this bug fix I have 1) not submitted bug fixes
>> before and 2) it probably needs an appropriate explanation, where I
>> don't know the problem well enough for general switchcores to submit
>> with a suitable text.
>
> Do you want me to try to submit this change as a bug fix?

I think that would be fine as you would know the matter best.

Re: [PATCH v2 net-next 6/6] selftests: forwarding: add dynamic FDB test

[Hans Schultz]

On Thu, Apr 06, 2023 at 18:24, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Fri, Mar 31, 2023 at 02:43:11PM +0200, Hans Schultz wrote:
>> I will as long as the system is as it is with these selftests, just run
>> single subtests at a time on target, but if I have new phy problems like
>> the one you have seen I have had before, then testing on target becomes
>> off limits.
>
> Please open a dedicated communication channel (separate email thread on
> netdev@vger.kernel.org) with the appropriate maintainers for the PHY
> code that is failing for you in To:, and you will get the help that you
> need to resolve that and to be able to test on the target board.

The errors from the phy I saw in February. Maybe something was fixed in
the meantime as I did not see the same warning and exception last I
tried to run the newest kernel on target a little over a week ago.