[PATCH net-next 00/10] DSA unicast filtering

[PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

This series doesn't attempt anything extremely brave, it just changes
the way in which standalone ports which support FDB isolation work.

Up until now, DSA has recommended that switch drivers configure
standalone ports in a separate VID/FID with learning disabled, and with
the CPU port as the only destination, reached trivially via flooding.
That works, except that standalone ports will deliver all packets to the
CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
flooding towards the CPU port, to force the dropping of packets with
unknown MAC DA.

We handle port promiscuity by re-enabling flooding towards the CPU port.
This is relevant because the bridge puts its automatic (learning +
flooding) ports in promiscuous mode, and this makes some things work
automagically, like for example bridging with a foreign interface.
We don't delve yet into the territory of managing CPU flooding more
aggressively while under a bridge.

The only switch driver that benefits from this work right now is the
NXP LS1028A switch (felix). The others need to implement FDB isolation
first, before DSA is going to install entries to the port's standalone
database. Otherwise, these entries might collide with bridge FDB/MDB
entries.

This work was done mainly to have all the required features in place
before somebody starts seriously architecting DSA support for multiple
CPU ports. Otherwise it is much more difficult to bolt these features on
top of multiple CPU ports.

Vladimir Oltean (10):
  net: dsa: remove workarounds for changing master promisc/allmulti only
    while up
  net: dsa: rename the host FDB and MDB methods to contain the "bridge"
    namespace
  net: dsa: install secondary unicast and multicast addresses as host
    FDB/MDB
  net: dsa: install the primary unicast MAC address as standalone port
    host FDB
  net: dsa: manage flooding on the CPU ports
  net: dsa: felix: migrate host FDB and MDB entries when changing tag
    proto
  net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port
  net: dsa: felix: start off with flooding disabled on the CPU port
  net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q
  net: mscc: ocelot: accept configuring bridge port flags on the NPI
    port

 drivers/net/dsa/ocelot/felix.c     | 241 ++++++++++++++++++++------
 drivers/net/ethernet/mscc/ocelot.c |   3 +
 include/net/dsa.h                  |   7 +
 net/dsa/dsa.c                      |  40 +++++
 net/dsa/dsa_priv.h                 |  53 +++++-
 net/dsa/port.c                     | 160 +++++++++++++-----
 net/dsa/slave.c                    | 261 +++++++++++++++++++++++------
 7 files changed, 609 insertions(+), 156 deletions(-)

-- 
2.25.1

[PATCH net-next 01/10] net: dsa: remove workarounds for changing master promisc/allmulti only while up

[Vladimir Oltean]

Lennert Buytenhek explains in commit df02c6ff2e39 ("dsa: fix master
interface allmulti/promisc handling"), dated Nov 2008, that changing the
promiscuity of interfaces that are down (here the master) is broken.

This fact regarding promisc/allmulti has changed since commit
b6c40d68ff64 ("net: only invoke dev->change_rx_flags when device is UP")
by Vlad Yasevich, dated Nov 2013.

Therefore, DSA now has unnecessary complexity to handle master state
transitions from down to up. In fact, syncing the unicast and multicast
addresses can happen completely asynchronously to the administrative
state changes.

This change reduces that complexity by effectively fully reverting
commit df02c6ff2e39 ("dsa: fix master interface allmulti/promisc
handling").

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 45 ++++++++-------------------------------------
 1 file changed, 8 insertions(+), 37 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 089616206b11..52d1316368c9 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -81,29 +81,12 @@ static int dsa_slave_open(struct net_device *dev)
 			goto out;
 	}
 
-	if (dev->flags & IFF_ALLMULTI) {
-		err = dev_set_allmulti(master, 1);
-		if (err < 0)
-			goto del_unicast;
-	}
-	if (dev->flags & IFF_PROMISC) {
-		err = dev_set_promiscuity(master, 1);
-		if (err < 0)
-			goto clear_allmulti;
-	}
-
 	err = dsa_port_enable_rt(dp, dev->phydev);
 	if (err)
-		goto clear_promisc;
+		goto del_unicast;
 
 	return 0;
 
-clear_promisc:
-	if (dev->flags & IFF_PROMISC)
-		dev_set_promiscuity(master, -1);
-clear_allmulti:
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(master, -1);
 del_unicast:
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
@@ -118,13 +101,6 @@ static int dsa_slave_close(struct net_device *dev)
 
 	dsa_port_disable_rt(dp);
 
-	dev_mc_unsync(master, dev);
-	dev_uc_unsync(master, dev);
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(master, -1);
-	if (dev->flags & IFF_PROMISC)
-		dev_set_promiscuity(master, -1);
-
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
 
@@ -134,14 +110,13 @@ static int dsa_slave_close(struct net_device *dev)
 static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
-	if (dev->flags & IFF_UP) {
-		if (change & IFF_ALLMULTI)
-			dev_set_allmulti(master,
-					 dev->flags & IFF_ALLMULTI ? 1 : -1);
-		if (change & IFF_PROMISC)
-			dev_set_promiscuity(master,
-					    dev->flags & IFF_PROMISC ? 1 : -1);
-	}
+
+	if (change & IFF_ALLMULTI)
+		dev_set_allmulti(master,
+				 dev->flags & IFF_ALLMULTI ? 1 : -1);
+	if (change & IFF_PROMISC)
+		dev_set_promiscuity(master,
+				    dev->flags & IFF_PROMISC ? 1 : -1);
 }
 
 static void dsa_slave_set_rx_mode(struct net_device *dev)
@@ -161,9 +136,6 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	if (!(dev->flags & IFF_UP))
-		goto out;
-
 	if (!ether_addr_equal(addr->sa_data, master->dev_addr)) {
 		err = dev_uc_add(master, addr->sa_data);
 		if (err < 0)
@@ -173,7 +145,6 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
 
-out:
 	eth_hw_addr_set(dev, addr->sa_data);
 
 	return 0;
-- 
2.25.1

[PATCH net-next 02/10] net: dsa: rename the host FDB and MDB methods to contain the "bridge" namespace

[Vladimir Oltean]

We are preparing to add API in port.c that adds FDB and MDB entries that
correspond to the port's standalone database. Rename the existing
methods to make it clear that the FDB and MDB entries offloaded come
from the bridge database.

Since the function names lengthen in dsa_slave_switchdev_event_work(),
we place "addr" and "vid" in temporary variables, to shorten those.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h | 16 ++++++++--------
 net/dsa/port.c     | 16 ++++++++--------
 net/dsa/slave.c    | 30 ++++++++++++------------------
 3 files changed, 28 insertions(+), 34 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 07c0ad52395a..2e05c863d4b4 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -223,10 +223,10 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
-int dsa_port_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-			  u16 vid);
-int dsa_port_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);
+int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+				 u16 vid);
 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,
@@ -236,10 +236,10 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_mdb_del(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
-int dsa_port_host_mdb_add(const struct dsa_port *dp,
-			  const struct switchdev_obj_port_mdb *mdb);
-int dsa_port_host_mdb_del(const struct dsa_port *dp,
-			  const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
 			      struct switchdev_brport_flags flags,
 			      struct netlink_ext_ack *extack);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index d9da425a17fb..4fb282ae049c 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -835,8 +835,8 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_DEL, &info);
 }
 
-int dsa_port_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)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
@@ -867,8 +867,8 @@ int dsa_port_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_ADD, &info);
 }
 
-int dsa_port_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)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
@@ -982,8 +982,8 @@ int dsa_port_mdb_del(const struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_MDB_DEL, &info);
 }
 
-int dsa_port_host_mdb_add(const struct dsa_port *dp,
-			  const struct switchdev_obj_port_mdb *mdb)
+int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb)
 {
 	struct dsa_notifier_mdb_info info = {
 		.sw_index = dp->ds->index,
@@ -1007,8 +1007,8 @@ int dsa_port_host_mdb_add(const struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info);
 }
 
-int dsa_port_host_mdb_del(const struct dsa_port *dp,
-			  const struct switchdev_obj_port_mdb *mdb)
+int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb)
 {
 	struct dsa_notifier_mdb_info info = {
 		.sw_index = dp->ds->index,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 52d1316368c9..f0d8f2f5d923 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -397,7 +397,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx,
 		if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
 			return -EOPNOTSUPP;
 
-		err = dsa_port_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
+		err = dsa_port_bridge_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		if (dsa_port_offloads_bridge_port(dp, obj->orig_dev))
@@ -478,7 +478,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx,
 		if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev))
 			return -EOPNOTSUPP;
 
-		err = dsa_port_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
+		err = dsa_port_bridge_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		if (dsa_port_offloads_bridge_port(dp, obj->orig_dev))
@@ -2356,7 +2356,9 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work)
 {
 	struct dsa_switchdev_event_work *switchdev_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 vid = switchdev_work->vid;
 	struct dsa_switch *ds;
 	struct dsa_port *dp;
 	int err;
@@ -2367,19 +2369,15 @@ 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_host_fdb_add(dp, switchdev_work->addr,
-						    switchdev_work->vid);
+			err = dsa_port_bridge_host_fdb_add(dp, addr, vid);
 		else if (dp->lag)
-			err = dsa_port_lag_fdb_add(dp, switchdev_work->addr,
-						   switchdev_work->vid);
+			err = dsa_port_lag_fdb_add(dp, addr, vid);
 		else
-			err = dsa_port_fdb_add(dp, switchdev_work->addr,
-					       switchdev_work->vid);
+			err = dsa_port_fdb_add(dp, addr, vid);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to add %pM vid %d to fdb: %d\n",
-				dp->index, switchdev_work->addr,
-				switchdev_work->vid, err);
+				dp->index, addr, vid, err);
 			break;
 		}
 		dsa_fdb_offload_notify(switchdev_work);
@@ -2387,19 +2385,15 @@ 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_host_fdb_del(dp, switchdev_work->addr,
-						    switchdev_work->vid);
+			err = dsa_port_bridge_host_fdb_del(dp, addr, vid);
 		else if (dp->lag)
-			err = dsa_port_lag_fdb_del(dp, switchdev_work->addr,
-						   switchdev_work->vid);
+			err = dsa_port_lag_fdb_del(dp, addr, vid);
 		else
-			err = dsa_port_fdb_del(dp, switchdev_work->addr,
-					       switchdev_work->vid);
+			err = dsa_port_fdb_del(dp, addr, vid);
 		if (err) {
 			dev_err(ds->dev,
 				"port %d failed to delete %pM vid %d from fdb: %d\n",
-				dp->index, switchdev_work->addr,
-				switchdev_work->vid, err);
+				dp->index, addr, vid, err);
 		}
 
 		break;
-- 
2.25.1

[PATCH net-next 03/10] net: dsa: install secondary unicast and multicast addresses as host FDB/MDB

[Vladimir Oltean]

In preparation of disabling flooding towards the CPU in standalone ports
mode, identify the addresses requested by upper interfaces and use the
new API for DSA FDB isolation to request the hardware driver to offload
these as FDB or MDB objects. The objects belong to the user port's
database, and are installed pointing towards the CPU port.

Because dev_uc_add()/dev_mc_add() is VLAN-unaware, we offload to the
port standalone database addresses with VID 0 (also VLAN-unaware).
So this excludes switches with global VLAN filtering from supporting
unicast filtering, because there, it is possible for a port of a switch
to join a VLAN-aware bridge, and this changes the VLAN awareness of
standalone ports, requiring VLAN-aware standalone host FDB entries.
For the same reason, hellcreek, which requires VLAN awareness in
standalone mode, is also exempted from unicast filtering.

We create "standalone" variants of dsa_port_host_fdb_add() and
dsa_port_host_mdb_add() (and the _del coresponding functions).

We also create a separate work item type for handling deferred
standalone host FDB/MDB entries compared to the switchdev one.
This is done for the purpose of clarity - the procedure for offloading a
bridge FDB entry is different than offloading a standalone one, and
the switchdev event work handles only FDBs anyway, not MDBs.
Deferral is needed for standalone entries because ndo_set_rx_mode runs
in atomic context. We could probably optimize things a little by first
queuing up all entries that need to be offloaded, and scheduling the
work item just once, but the data structures that we can pass through
__dev_uc_sync() and __dev_mc_sync() are limiting (there is nothing like
a void *priv), so we'd have to keep the list of queued events somewhere
in struct dsa_switch, and possibly a lock for it. Too complicated for
now.

Adding the address to the master is handled by dev_uc_sync(), adding it
to the hardware is handled by __dev_uc_sync(). So this is the reason why
dsa_port_standalone_host_fdb_add() does not call dev_uc_add(). Not that
it had the rtnl_mutex anyway - ndo_set_rx_mode has it, but is atomic.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  37 +++++++++++
 net/dsa/port.c     | 160 +++++++++++++++++++++++++++++++++------------
 net/dsa/slave.c    | 116 ++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+), 40 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2e05c863d4b4..c3c7491ace72 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -144,6 +144,21 @@ struct dsa_switchdev_event_work {
 	bool host_addr;
 };
 
+enum dsa_standalone_event {
+	DSA_UC_ADD,
+	DSA_UC_DEL,
+	DSA_MC_ADD,
+	DSA_MC_DEL,
+};
+
+struct dsa_standalone_event_work {
+	struct work_struct work;
+	struct net_device *dev;
+	enum dsa_standalone_event event;
+	unsigned char addr[ETH_ALEN];
+	u16 vid;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
@@ -223,6 +238,10 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
+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);
 int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
@@ -236,6 +255,10 @@ int dsa_port_mdb_add(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_mdb_del(const struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_standalone_host_mdb_add(const struct dsa_port *dp,
+				     const struct switchdev_obj_port_mdb *mdb);
+int dsa_port_standalone_host_mdb_del(const struct dsa_port *dp,
+				     const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
 				 const struct switchdev_obj_port_mdb *mdb);
 int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
@@ -502,6 +525,20 @@ static inline void *dsa_etype_header_pos_tx(struct sk_buff *skb)
 int dsa_switch_register_notifier(struct dsa_switch *ds);
 void dsa_switch_unregister_notifier(struct dsa_switch *ds);
 
+static inline bool dsa_switch_supports_uc_filtering(struct dsa_switch *ds)
+{
+	return ds->ops->port_fdb_add && ds->ops->port_fdb_del &&
+	       ds->fdb_isolation && !ds->vlan_filtering_is_global &&
+	       !ds->needs_standalone_vlan_filtering;
+}
+
+static inline bool dsa_switch_supports_mc_filtering(struct dsa_switch *ds)
+{
+	return ds->ops->port_mdb_add && ds->ops->port_mdb_del &&
+	       ds->fdb_isolation && !ds->vlan_filtering_is_global &&
+	       !ds->needs_standalone_vlan_filtering;
+}
+
 /* dsa2.c */
 void dsa_lag_map(struct dsa_switch_tree *dst, struct dsa_lag *lag);
 void dsa_lag_unmap(struct dsa_switch_tree *dst, struct dsa_lag *lag);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 4fb282ae049c..58291df14cdb 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -835,20 +835,43 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_DEL, &info);
 }
 
-int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid)
+static int dsa_port_host_fdb_add(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid,
+				 struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
 		.addr = addr,
 		.vid = vid,
-		.db = {
-			.type = DSA_DB_BRIDGE,
-			.bridge = *dp->bridge,
-		},
+		.db = db,
+	};
+
+	if (!dp->ds->fdb_isolation)
+		info.db.bridge.num = 0;
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_ADD, &info);
+}
+
+int dsa_port_standalone_host_fdb_add(struct dsa_port *dp,
+				     const unsigned char *addr, u16 vid)
+{
+	struct dsa_db db = {
+		.type = DSA_DB_PORT,
+		.dp = dp,
 	};
+
+	return dsa_port_host_fdb_add(dp, addr, vid, db);
+}
+
+int dsa_port_bridge_host_fdb_add(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid)
+{
 	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct dsa_db db = {
+		.type = DSA_DB_BRIDGE,
+		.bridge = *dp->bridge,
+	};
 	int err;
 
 	/* Avoid a call to __dev_set_promiscuity() on the master, which
@@ -861,26 +884,46 @@ int dsa_port_bridge_host_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 			return err;
 	}
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
-	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_ADD, &info);
+	return dsa_port_host_fdb_add(dp, addr, vid, db);
 }
 
-int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
-				 u16 vid)
+static int dsa_port_host_fdb_del(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid,
+				 struct dsa_db db)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
 		.addr = addr,
 		.vid = vid,
-		.db = {
-			.type = DSA_DB_BRIDGE,
-			.bridge = *dp->bridge,
-		},
+		.db = db,
+	};
+
+	if (!dp->ds->fdb_isolation)
+		info.db.bridge.num = 0;
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_DEL, &info);
+}
+
+int dsa_port_standalone_host_fdb_del(struct dsa_port *dp,
+				     const unsigned char *addr, u16 vid)
+{
+	struct dsa_db db = {
+		.type = DSA_DB_PORT,
+		.dp = dp,
 	};
+
+	return dsa_port_host_fdb_del(dp, addr, vid, db);
+}
+
+int dsa_port_bridge_host_fdb_del(struct dsa_port *dp,
+				 const unsigned char *addr, u16 vid)
+{
 	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct dsa_db db = {
+		.type = DSA_DB_BRIDGE,
+		.bridge = *dp->bridge,
+	};
 	int err;
 
 	if (cpu_dp->master->priv_flags & IFF_UNICAST_FLT) {
@@ -889,10 +932,7 @@ int dsa_port_bridge_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 			return err;
 	}
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
-	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_DEL, &info);
+	return dsa_port_host_fdb_del(dp, addr, vid, db);
 }
 
 int dsa_port_lag_fdb_add(struct dsa_port *dp, const unsigned char *addr,
@@ -982,54 +1022,94 @@ int dsa_port_mdb_del(const struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_MDB_DEL, &info);
 }
 
-int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
-				 const struct switchdev_obj_port_mdb *mdb)
+static int dsa_port_host_mdb_add(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb,
+				 struct dsa_db db)
 {
 	struct dsa_notifier_mdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
 		.mdb = mdb,
-		.db = {
-			.type = DSA_DB_BRIDGE,
-			.bridge = *dp->bridge,
-		},
+		.db = db,
+	};
+
+	if (!dp->ds->fdb_isolation)
+		info.db.bridge.num = 0;
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info);
+}
+
+int dsa_port_standalone_host_mdb_add(const struct dsa_port *dp,
+				     const struct switchdev_obj_port_mdb *mdb)
+{
+	struct dsa_db db = {
+		.type = DSA_DB_PORT,
+		.dp = dp,
 	};
+
+	return dsa_port_host_mdb_add(dp, mdb, db);
+}
+
+int dsa_port_bridge_host_mdb_add(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb)
+{
 	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct dsa_db db = {
+		.type = DSA_DB_BRIDGE,
+		.bridge = *dp->bridge,
+	};
 	int err;
 
 	err = dev_mc_add(cpu_dp->master, mdb->addr);
 	if (err)
 		return err;
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
-	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info);
+	return dsa_port_host_mdb_add(dp, mdb, db);
 }
 
-int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
-				 const struct switchdev_obj_port_mdb *mdb)
+static int dsa_port_host_mdb_del(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb,
+				 struct dsa_db db)
 {
 	struct dsa_notifier_mdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
 		.mdb = mdb,
-		.db = {
-			.type = DSA_DB_BRIDGE,
-			.bridge = *dp->bridge,
-		},
+		.db = db,
+	};
+
+	if (!dp->ds->fdb_isolation)
+		info.db.bridge.num = 0;
+
+	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_DEL, &info);
+}
+
+int dsa_port_standalone_host_mdb_del(const struct dsa_port *dp,
+				     const struct switchdev_obj_port_mdb *mdb)
+{
+	struct dsa_db db = {
+		.type = DSA_DB_PORT,
+		.dp = dp,
 	};
+
+	return dsa_port_host_mdb_del(dp, mdb, db);
+}
+
+int dsa_port_bridge_host_mdb_del(const struct dsa_port *dp,
+				 const struct switchdev_obj_port_mdb *mdb)
+{
 	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct dsa_db db = {
+		.type = DSA_DB_BRIDGE,
+		.bridge = *dp->bridge,
+	};
 	int err;
 
 	err = dev_mc_del(cpu_dp->master, mdb->addr);
 	if (err)
 		return err;
 
-	if (!dp->ds->fdb_isolation)
-		info.db.bridge.num = 0;
-
-	return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_DEL, &info);
+	return dsa_port_host_mdb_del(dp, mdb, db);
 }
 
 int dsa_port_vlan_add(struct dsa_port *dp,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f0d8f2f5d923..1402f93b1de2 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -23,6 +23,114 @@
 
 #include "dsa_priv.h"
 
+static void dsa_slave_standalone_event_work(struct work_struct *work)
+{
+	struct dsa_standalone_event_work *standalone_work =
+		container_of(work, struct dsa_standalone_event_work, work);
+	const unsigned char *addr = standalone_work->addr;
+	struct net_device *dev = standalone_work->dev;
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_mdb mdb;
+	struct dsa_switch *ds = dp->ds;
+	u16 vid = standalone_work->vid;
+	int err;
+
+	switch (standalone_work->event) {
+	case DSA_UC_ADD:
+		err = dsa_port_standalone_host_fdb_add(dp, addr, vid);
+		if (err) {
+			dev_err(ds->dev,
+				"port %d failed to add %pM vid %d to fdb: %d\n",
+				dp->index, addr, vid, err);
+			break;
+		}
+		break;
+
+	case DSA_UC_DEL:
+		err = dsa_port_standalone_host_fdb_del(dp, addr, vid);
+		if (err) {
+			dev_err(ds->dev,
+				"port %d failed to delete %pM vid %d from fdb: %d\n",
+				dp->index, addr, vid, err);
+		}
+
+		break;
+	case DSA_MC_ADD:
+		ether_addr_copy(mdb.addr, addr);
+		mdb.vid = vid;
+
+		err = dsa_port_standalone_host_mdb_add(dp, &mdb);
+		if (err) {
+			dev_err(ds->dev,
+				"port %d failed to add %pM vid %d to mdb: %d\n",
+				dp->index, addr, vid, err);
+			break;
+		}
+		break;
+	case DSA_MC_DEL:
+		ether_addr_copy(mdb.addr, addr);
+		mdb.vid = vid;
+
+		err = dsa_port_standalone_host_mdb_del(dp, &mdb);
+		if (err) {
+			dev_err(ds->dev,
+				"port %d failed to delete %pM vid %d from mdb: %d\n",
+				dp->index, addr, vid, err);
+		}
+
+		break;
+	}
+
+	kfree(standalone_work);
+}
+
+static int dsa_slave_schedule_standalone_work(struct net_device *dev,
+					      enum dsa_standalone_event event,
+					      const unsigned char *addr,
+					      u16 vid)
+{
+	struct dsa_standalone_event_work *standalone_work;
+
+	standalone_work = kzalloc(sizeof(*standalone_work), GFP_ATOMIC);
+	if (!standalone_work)
+		return -ENOMEM;
+
+	INIT_WORK(&standalone_work->work, dsa_slave_standalone_event_work);
+	standalone_work->event = event;
+	standalone_work->dev = dev;
+
+	ether_addr_copy(standalone_work->addr, addr);
+	standalone_work->vid = vid;
+
+	dsa_schedule_work(&standalone_work->work);
+
+	return 0;
+}
+
+static int dsa_slave_sync_uc(struct net_device *dev,
+			     const unsigned char *addr)
+{
+	return dsa_slave_schedule_standalone_work(dev, DSA_UC_ADD, addr, 0);
+}
+
+static int dsa_slave_unsync_uc(struct net_device *dev,
+			       const unsigned char *addr)
+{
+	return dsa_slave_schedule_standalone_work(dev, DSA_UC_DEL, addr, 0);
+}
+
+static int dsa_slave_sync_mc(struct net_device *dev,
+			     const unsigned char *addr)
+{
+	return dsa_slave_schedule_standalone_work(dev, DSA_MC_ADD, addr, 0);
+}
+
+static int dsa_slave_unsync_mc(struct net_device *dev,
+			       const unsigned char *addr)
+{
+	return dsa_slave_schedule_standalone_work(dev, DSA_MC_DEL, addr, 0);
+}
+
 /* slave mii_bus handling ***************************************************/
 static int dsa_slave_phy_read(struct mii_bus *bus, int addr, int reg)
 {
@@ -122,9 +230,15 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 static void dsa_slave_set_rx_mode(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 
 	dev_mc_sync(master, dev);
 	dev_uc_sync(master, dev);
+	if (dsa_switch_supports_mc_filtering(ds))
+		__dev_mc_sync(dev, dsa_slave_sync_mc, dsa_slave_unsync_mc);
+	if (dsa_switch_supports_uc_filtering(ds))
+		__dev_uc_sync(dev, dsa_slave_sync_uc, dsa_slave_unsync_uc);
 }
 
 static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
@@ -1919,6 +2033,8 @@ int dsa_slave_create(struct dsa_port *port)
 	else
 		eth_hw_addr_inherit(slave_dev, master);
 	slave_dev->priv_flags |= IFF_NO_QUEUE;
+	if (dsa_switch_supports_uc_filtering(ds))
+		slave_dev->priv_flags |= IFF_UNICAST_FLT;
 	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
 	if (ds->ops->port_max_mtu)
 		slave_dev->max_mtu = ds->ops->port_max_mtu(ds, port->index);
-- 
2.25.1

[PATCH net-next 04/10] net: dsa: install the primary unicast MAC address as standalone port host FDB

[Vladimir Oltean]

To be able to safely turn off CPU flooding for standalone ports, we need
to ensure that the dev_addr of each DSA slave interface is installed as
a standalone host FDB entry for compatible switches.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1402f93b1de2..6eb728efac43 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -175,6 +175,7 @@ static int dsa_slave_open(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 	int err;
 
 	err = dev_open(master, NULL);
@@ -183,10 +184,16 @@ static int dsa_slave_open(struct net_device *dev)
 		goto out;
 	}
 
+	if (dsa_switch_supports_uc_filtering(ds)) {
+		err = dsa_port_standalone_host_fdb_add(dp, dev->dev_addr, 0);
+		if (err)
+			goto out;
+	}
+
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr)) {
 		err = dev_uc_add(master, dev->dev_addr);
 		if (err < 0)
-			goto out;
+			goto del_host_addr;
 	}
 
 	err = dsa_port_enable_rt(dp, dev->phydev);
@@ -198,6 +205,9 @@ static int dsa_slave_open(struct net_device *dev)
 del_unicast:
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
+del_host_addr:
+	if (dsa_switch_supports_uc_filtering(ds))
+		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);
 out:
 	return err;
 }
@@ -206,12 +216,16 @@ static int dsa_slave_close(struct net_device *dev)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 
 	dsa_port_disable_rt(dp);
 
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
 
+	if (dsa_switch_supports_uc_filtering(ds))
+		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);
+
 	return 0;
 }
 
@@ -244,24 +258,41 @@ static void dsa_slave_set_rx_mode(struct net_device *dev)
 static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 	struct sockaddr *addr = a;
 	int err;
 
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	if (dsa_switch_supports_uc_filtering(ds)) {
+		err = dsa_port_standalone_host_fdb_add(dp, addr->sa_data, 0);
+		if (err)
+			return err;
+	}
+
 	if (!ether_addr_equal(addr->sa_data, master->dev_addr)) {
 		err = dev_uc_add(master, addr->sa_data);
 		if (err < 0)
-			return err;
+			goto del_unicast;
 	}
 
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr))
 		dev_uc_del(master, dev->dev_addr);
 
+	if (dsa_switch_supports_uc_filtering(ds))
+		dsa_port_standalone_host_fdb_del(dp, dev->dev_addr, 0);
+
 	eth_hw_addr_set(dev, addr->sa_data);
 
 	return 0;
+
+del_unicast:
+	if (dsa_switch_supports_uc_filtering(ds))
+		dsa_port_standalone_host_fdb_del(dp, addr->sa_data, 0);
+
+	return err;
 }
 
 struct dsa_slave_dump_ctx {
-- 
2.25.1

[PATCH net-next 05/10] net: dsa: manage flooding on the CPU ports

[Vladimir Oltean]

DSA can treat IFF_PROMISC and IFF_ALLMULTI on standalone user ports as
signifying whether packets with an unknown MAC DA will be received or
not. Since known MAC DAs are handled by FDB/MDB entries, this means that
promiscuity is analogous to including/excluding the CPU port from the
flood domain of those packets.

There are two ways to signal CPU flooding to drivers.

The first (chosen here) is to synthesize a call to
ds->ops->port_bridge_flags() for the CPU port, with a mask of
BR_FLOOD | BR_MCAST_FLOOD. This has the effect of turning on egress
flooding on the CPU port regardless of source.

The alternative would be to create a new ds->ops->port_host_flood()
which is called per user port. Some switches (sja1105) have a flood
domain that is managed per {ingress port, egress port} pair, so it would
make more sense for this kind of switch to not flood the CPU from port A
if just port B requires it. Nonetheless, the sja1105 has other quirks
that prevent it from making use of unicast filtering, and without a
concrete user making use of this feature, I chose not to implement it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 6eb728efac43..42436ac6993b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -229,9 +229,44 @@ static int dsa_slave_close(struct net_device *dev)
 	return 0;
 }
 
+/* Keep flooding enabled towards this port's CPU port as long as it serves at
+ * least one port in the tree that requires it.
+ */
+static void dsa_port_manage_cpu_flood(struct dsa_port *dp)
+{
+	struct switchdev_brport_flags flags = {
+		.mask = BR_FLOOD | BR_MCAST_FLOOD,
+	};
+	struct dsa_switch_tree *dst = dp->ds->dst;
+	struct dsa_port *cpu_dp = dp->cpu_dp;
+	struct dsa_port *other_dp;
+	int err;
+
+	list_for_each_entry(other_dp, &dst->ports, list) {
+		if (!dsa_port_is_user(other_dp))
+			continue;
+
+		if (other_dp->cpu_dp != cpu_dp)
+			continue;
+
+		if (other_dp->slave->flags & IFF_ALLMULTI)
+			flags.val |= BR_MCAST_FLOOD;
+		if (other_dp->slave->flags & IFF_PROMISC)
+			flags.val |= BR_FLOOD;
+	}
+
+	err = dsa_port_pre_bridge_flags(dp, flags, NULL);
+	if (err)
+		return;
+
+	dsa_port_bridge_flags(cpu_dp, flags, NULL);
+}
+
 static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 {
 	struct net_device *master = dsa_slave_to_master(dev);
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
 
 	if (change & IFF_ALLMULTI)
 		dev_set_allmulti(master,
@@ -239,6 +274,10 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 	if (change & IFF_PROMISC)
 		dev_set_promiscuity(master,
 				    dev->flags & IFF_PROMISC ? 1 : -1);
+
+	if (dsa_switch_supports_uc_filtering(ds) &&
+	    dsa_switch_supports_mc_filtering(ds))
+		dsa_port_manage_cpu_flood(dp);
 }
 
 static void dsa_slave_set_rx_mode(struct net_device *dev)
-- 
2.25.1

[PATCH net-next 06/10] net: dsa: felix: migrate host FDB and MDB entries when changing tag proto

[Vladimir Oltean]

The "ocelot" and "ocelot-8021q" tagging protocols make use of different
hardware resources, and host FDB entries have different destination
ports in the switch analyzer module, practically speaking.

So when the user requests a tagging protocol change, the driver must
migrate all host FDB and MDB entries from the NPI port (in fact CPU port
module) towards the same physical port, but this time used as a regular
port.

It is pointless for the felix driver to keep a copy of the host
addresses, when we can create and export DSA helpers for walking through
the addresses that it already needs to keep on the CPU port, for
refcounting purposes.

felix_classify_db() is moved up to avoid a forward declaration.

We pass "bool change" because dp->fdbs and dp->mdbs are uninitialized
lists when felix_setup() first calls felix_set_tag_protocol(), so we
need to avoid calling dsa_port_walk_fdbs() during probe time.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c | 171 ++++++++++++++++++++++++++++-----
 include/net/dsa.h              |   7 ++
 net/dsa/dsa.c                  |  40 ++++++++
 3 files changed, 192 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index badb5b9ba790..47320bfbaac1 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -25,6 +25,104 @@
 #include <net/dsa.h>
 #include "felix.h"
 
+/* Translate the DSA database API into the ocelot switch library API,
+ * which uses VID 0 for all ports that aren't part of a bridge,
+ * and expects the bridge_dev to be NULL in that case.
+ */
+static struct net_device *felix_classify_db(struct dsa_db db)
+{
+	switch (db.type) {
+	case DSA_DB_PORT:
+	case DSA_DB_LAG:
+		return NULL;
+	case DSA_DB_BRIDGE:
+		return db.bridge.dev;
+	default:
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+}
+
+/* We are called before felix_npi_port_init(), so ocelot->npi is -1. */
+static int felix_migrate_fdbs_to_npi_port(struct dsa_switch *ds, int port,
+					  const unsigned char *addr, u16 vid,
+					  struct dsa_db db)
+{
+	struct net_device *bridge_dev = felix_classify_db(db);
+	struct ocelot *ocelot = ds->priv;
+	int cpu = ocelot->num_phys_ports;
+	int err;
+
+	err = ocelot_fdb_del(ocelot, port, addr, vid, bridge_dev);
+	if (err)
+		return err;
+
+	return ocelot_fdb_add(ocelot, cpu, addr, vid, bridge_dev);
+}
+
+static int felix_migrate_mdbs_to_npi_port(struct dsa_switch *ds, int port,
+					  const unsigned char *addr, u16 vid,
+					  struct dsa_db db)
+{
+	struct net_device *bridge_dev = felix_classify_db(db);
+	struct switchdev_obj_port_mdb mdb;
+	struct ocelot *ocelot = ds->priv;
+	int cpu = ocelot->num_phys_ports;
+	int err;
+
+	memset(&mdb, 0, sizeof(mdb));
+	ether_addr_copy(mdb.addr, addr);
+	mdb.vid = vid;
+
+	err = ocelot_port_mdb_del(ocelot, port, &mdb, bridge_dev);
+	if (err)
+		return err;
+
+	return ocelot_port_mdb_add(ocelot, cpu, &mdb, bridge_dev);
+}
+
+/* ocelot->npi was already set to -1 by felix_npi_port_deinit, so
+ * ocelot_fdb_add() will not redirect FDB entries towards the
+ * CPU port module here, which is what we want.
+ */
+static int
+felix_migrate_fdbs_to_tag_8021q_port(struct dsa_switch *ds, int port,
+				     const unsigned char *addr, u16 vid,
+				     struct dsa_db db)
+{
+	struct net_device *bridge_dev = felix_classify_db(db);
+	struct ocelot *ocelot = ds->priv;
+	int cpu = ocelot->num_phys_ports;
+	int err;
+
+	err = ocelot_fdb_del(ocelot, cpu, addr, vid, bridge_dev);
+	if (err)
+		return err;
+
+	return ocelot_fdb_add(ocelot, port, addr, vid, bridge_dev);
+}
+
+static int
+felix_migrate_mdbs_to_tag_8021q_port(struct dsa_switch *ds, int port,
+				     const unsigned char *addr, u16 vid,
+				     struct dsa_db db)
+{
+	struct net_device *bridge_dev = felix_classify_db(db);
+	struct switchdev_obj_port_mdb mdb;
+	struct ocelot *ocelot = ds->priv;
+	int cpu = ocelot->num_phys_ports;
+	int err;
+
+	memset(&mdb, 0, sizeof(mdb));
+	ether_addr_copy(mdb.addr, addr);
+	mdb.vid = vid;
+
+	err = ocelot_port_mdb_del(ocelot, cpu, &mdb, bridge_dev);
+	if (err)
+		return err;
+
+	return ocelot_port_mdb_add(ocelot, port, &mdb, bridge_dev);
+}
+
 /* Set up VCAP ES0 rules for pushing a tag_8021q VLAN towards the CPU such that
  * the tagger can perform RX source port identification.
  */
@@ -327,7 +425,7 @@ static int felix_update_trapping_destinations(struct dsa_switch *ds,
 	return 0;
 }
 
-static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
+static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu, bool change)
 {
 	struct ocelot *ocelot = ds->priv;
 	unsigned long cpu_flood;
@@ -365,9 +463,21 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
 	if (err)
 		return err;
 
+	if (change) {
+		err = dsa_port_walk_fdbs(ds, cpu,
+					 felix_migrate_fdbs_to_tag_8021q_port);
+		if (err)
+			goto out_tag_8021q_unregister;
+
+		err = dsa_port_walk_mdbs(ds, cpu,
+					 felix_migrate_mdbs_to_tag_8021q_port);
+		if (err)
+			goto out_migrate_fdbs;
+	}
+
 	err = felix_update_trapping_destinations(ds, true);
 	if (err)
-		goto out_tag_8021q_unregister;
+		goto out_migrate_mdbs;
 
 	/* The ownership of the CPU port module's queues might have just been
 	 * transferred to the tag_8021q tagger from the NPI-based tagger.
@@ -380,6 +490,12 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu)
 
 	return 0;
 
+out_migrate_mdbs:
+	if (change)
+		dsa_port_walk_mdbs(ds, cpu, felix_migrate_mdbs_to_npi_port);
+out_migrate_fdbs:
+	if (change)
+		dsa_port_walk_fdbs(ds, cpu, felix_migrate_fdbs_to_npi_port);
 out_tag_8021q_unregister:
 	dsa_tag_8021q_unregister(ds);
 	return err;
@@ -454,10 +570,23 @@ static void felix_npi_port_deinit(struct ocelot *ocelot, int port)
 	ocelot_fields_write(ocelot, port, SYS_PAUSE_CFG_PAUSE_ENA, 1);
 }
 
-static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu)
+static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu, bool change)
 {
 	struct ocelot *ocelot = ds->priv;
 	unsigned long cpu_flood;
+	int err;
+
+	if (change) {
+		err = dsa_port_walk_fdbs(ds, cpu,
+					 felix_migrate_fdbs_to_npi_port);
+		if (err)
+			return err;
+
+		err = dsa_port_walk_mdbs(ds, cpu,
+					 felix_migrate_mdbs_to_npi_port);
+		if (err)
+			goto out_migrate_fdbs;
+	}
 
 	felix_npi_port_init(ocelot, cpu);
 
@@ -478,6 +607,13 @@ static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu)
 	ocelot_rmw_rix(ocelot, cpu_flood, cpu_flood, ANA_PGID_PGID, PGID_BC);
 
 	return 0;
+
+out_migrate_fdbs:
+	if (change)
+		dsa_port_walk_fdbs(ds, cpu,
+				   felix_migrate_fdbs_to_tag_8021q_port);
+
+	return err;
 }
 
 static void felix_teardown_tag_npi(struct dsa_switch *ds, int cpu)
@@ -488,17 +624,17 @@ static void felix_teardown_tag_npi(struct dsa_switch *ds, int cpu)
 }
 
 static int felix_set_tag_protocol(struct dsa_switch *ds, int cpu,
-				  enum dsa_tag_protocol proto)
+				  enum dsa_tag_protocol proto, bool change)
 {
 	int err;
 
 	switch (proto) {
 	case DSA_TAG_PROTO_SEVILLE:
 	case DSA_TAG_PROTO_OCELOT:
-		err = felix_setup_tag_npi(ds, cpu);
+		err = felix_setup_tag_npi(ds, cpu, change);
 		break;
 	case DSA_TAG_PROTO_OCELOT_8021Q:
-		err = felix_setup_tag_8021q(ds, cpu);
+		err = felix_setup_tag_8021q(ds, cpu, change);
 		break;
 	default:
 		err = -EPROTONOSUPPORT;
@@ -542,9 +678,9 @@ static int felix_change_tag_protocol(struct dsa_switch *ds, int cpu,
 
 	felix_del_tag_protocol(ds, cpu, old_proto);
 
-	err = felix_set_tag_protocol(ds, cpu, proto);
+	err = felix_set_tag_protocol(ds, cpu, proto, true);
 	if (err) {
-		felix_set_tag_protocol(ds, cpu, old_proto);
+		felix_set_tag_protocol(ds, cpu, old_proto, true);
 		return err;
 	}
 
@@ -592,23 +728,6 @@ static int felix_fdb_dump(struct dsa_switch *ds, int port,
 	return ocelot_fdb_dump(ocelot, port, cb, data);
 }
 
-/* Translate the DSA database API into the ocelot switch library API,
- * which uses VID 0 for all ports that aren't part of a bridge,
- * and expects the bridge_dev to be NULL in that case.
- */
-static struct net_device *felix_classify_db(struct dsa_db db)
-{
-	switch (db.type) {
-	case DSA_DB_PORT:
-	case DSA_DB_LAG:
-		return NULL;
-	case DSA_DB_BRIDGE:
-		return db.bridge.dev;
-	default:
-		return ERR_PTR(-EOPNOTSUPP);
-	}
-}
-
 static int felix_fdb_add(struct dsa_switch *ds, int port,
 			 const unsigned char *addr, u16 vid,
 			 struct dsa_db db)
@@ -1260,7 +1379,7 @@ static int felix_setup(struct dsa_switch *ds)
 		/* The initial tag protocol is NPI which always returns 0, so
 		 * there's no real point in checking for errors.
 		 */
-		felix_set_tag_protocol(ds, dp->index, felix->tag_proto);
+		felix_set_tag_protocol(ds, dp->index, felix->tag_proto, false);
 		break;
 	}
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index cfedcfb86350..71cc363dbbd4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1219,6 +1219,13 @@ struct dsa_switch_driver {
 
 struct net_device *dsa_dev_to_net_device(struct device *dev);
 
+typedef int dsa_fdb_walk_cb_t(struct dsa_switch *ds, int port,
+			      const unsigned char *addr, u16 vid,
+			      struct dsa_db db);
+
+int dsa_port_walk_fdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb);
+int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb);
+
 /* Keep inline for faster access in hot path */
 static inline bool netdev_uses_dsa(const struct net_device *dev)
 {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index c43f7446a75d..06d5de28a43e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -467,6 +467,46 @@ struct dsa_port *dsa_port_from_netdev(struct net_device *netdev)
 }
 EXPORT_SYMBOL_GPL(dsa_port_from_netdev);
 
+int dsa_port_walk_fdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct dsa_mac_addr *a;
+	int err;
+
+	mutex_lock(&dp->addr_lists_lock);
+
+	list_for_each_entry(a, &dp->fdbs, list) {
+		err = cb(ds, port, a->addr, a->vid, a->db);
+		if (err)
+			break;
+	}
+
+	mutex_unlock(&dp->addr_lists_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(dsa_port_walk_fdbs);
+
+int dsa_port_walk_mdbs(struct dsa_switch *ds, int port, dsa_fdb_walk_cb_t cb)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct dsa_mac_addr *a;
+	int err;
+
+	mutex_lock(&dp->addr_lists_lock);
+
+	list_for_each_entry(a, &dp->mdbs, list) {
+		err = cb(ds, port, a->addr, a->vid, a->db);
+		if (err)
+			break;
+	}
+
+	mutex_unlock(&dp->addr_lists_lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(dsa_port_walk_mdbs);
+
 static int __init dsa_init_module(void)
 {
 	int rc;
-- 
2.25.1

[PATCH net-next 07/10] net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port

[Vladimir Oltean]

When the tagging protocol changes from "ocelot" to "ocelot-8021q" or in
reverse, the DSA promiscuity setting that was applied for the old CPU
port must be transferred to the new one.

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

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 47320bfbaac1..f263712a007b 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -80,6 +80,43 @@ static int felix_migrate_mdbs_to_npi_port(struct dsa_switch *ds, int port,
 	return ocelot_port_mdb_add(ocelot, cpu, &mdb, bridge_dev);
 }
 
+static void felix_migrate_pgid_bit(struct dsa_switch *ds, int from, int to,
+				   int pgid)
+{
+	struct ocelot *ocelot = ds->priv;
+	bool on;
+	u32 val;
+
+	val = ocelot_read_rix(ocelot, ANA_PGID_PGID, pgid);
+	on = !!(val & BIT(from));
+	val &= ~BIT(from);
+	if (on)
+		val |= BIT(to);
+	else
+		val &= ~BIT(to);
+
+	ocelot_write_rix(ocelot, val, ANA_PGID_PGID, pgid);
+}
+
+static void felix_migrate_flood_to_npi_port(struct dsa_switch *ds, int port)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	felix_migrate_pgid_bit(ds, port, ocelot->num_phys_ports, PGID_UC);
+	felix_migrate_pgid_bit(ds, port, ocelot->num_phys_ports, PGID_MC);
+	felix_migrate_pgid_bit(ds, port, ocelot->num_phys_ports, PGID_BC);
+}
+
+static void
+felix_migrate_flood_to_tag_8021q_port(struct dsa_switch *ds, int port)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	felix_migrate_pgid_bit(ds, ocelot->num_phys_ports, port, PGID_UC);
+	felix_migrate_pgid_bit(ds, ocelot->num_phys_ports, port, PGID_MC);
+	felix_migrate_pgid_bit(ds, ocelot->num_phys_ports, port, PGID_BC);
+}
+
 /* ocelot->npi was already set to -1 by felix_npi_port_deinit, so
  * ocelot_fdb_add() will not redirect FDB entries towards the
  * CPU port module here, which is what we want.
@@ -473,11 +510,13 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu, bool change)
 					 felix_migrate_mdbs_to_tag_8021q_port);
 		if (err)
 			goto out_migrate_fdbs;
+
+		felix_migrate_flood_to_tag_8021q_port(ds, cpu);
 	}
 
 	err = felix_update_trapping_destinations(ds, true);
 	if (err)
-		goto out_migrate_mdbs;
+		goto out_migrate_flood;
 
 	/* The ownership of the CPU port module's queues might have just been
 	 * transferred to the tag_8021q tagger from the NPI-based tagger.
@@ -490,7 +529,9 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu, bool change)
 
 	return 0;
 
-out_migrate_mdbs:
+out_migrate_flood:
+	if (change)
+		felix_migrate_flood_to_npi_port(ds, cpu);
 	if (change)
 		dsa_port_walk_mdbs(ds, cpu, felix_migrate_mdbs_to_npi_port);
 out_migrate_fdbs:
@@ -586,6 +627,8 @@ static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu, bool change)
 					 felix_migrate_mdbs_to_npi_port);
 		if (err)
 			goto out_migrate_fdbs;
+
+		felix_migrate_flood_to_npi_port(ds, cpu);
 	}
 
 	felix_npi_port_init(ocelot, cpu);
-- 
2.25.1

[PATCH net-next 08/10] net: dsa: felix: start off with flooding disabled on the CPU port

[Vladimir Oltean]

The driver probes with all ports as standalone, and it supports unicast
filtering. So DSA will call port_fdb_add() for all necessary addresses
on the current CPU port. We also handle migrations when the CPU port
hardware resource changes (on tagging protocol change), so there should
not be any unknown address that we have to receive while not promiscuous.

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

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f263712a007b..e9ce0d687713 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -614,7 +614,6 @@ static void felix_npi_port_deinit(struct ocelot *ocelot, int port)
 static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu, bool change)
 {
 	struct ocelot *ocelot = ds->priv;
-	unsigned long cpu_flood;
 	int err;
 
 	if (change) {
@@ -633,22 +632,6 @@ static int felix_setup_tag_npi(struct dsa_switch *ds, int cpu, bool change)
 
 	felix_npi_port_init(ocelot, cpu);
 
-	/* Include the CPU port module (and indirectly, the NPI port)
-	 * in the forwarding mask for unknown unicast - the hardware
-	 * default value for ANA_FLOODING_FLD_UNICAST excludes
-	 * BIT(ocelot->num_phys_ports), and so does ocelot_init,
-	 * since Ocelot relies on whitelisting MAC addresses towards
-	 * PGID_CPU.
-	 * We do this because DSA does not yet perform RX filtering,
-	 * and the NPI port does not perform source address learning,
-	 * so traffic sent to Linux is effectively unknown from the
-	 * switch's perspective.
-	 */
-	cpu_flood = ANA_PGID_PGID_PGID(BIT(ocelot->num_phys_ports));
-	ocelot_rmw_rix(ocelot, cpu_flood, cpu_flood, ANA_PGID_PGID, PGID_UC);
-	ocelot_rmw_rix(ocelot, cpu_flood, cpu_flood, ANA_PGID_PGID, PGID_MC);
-	ocelot_rmw_rix(ocelot, cpu_flood, cpu_flood, ANA_PGID_PGID, PGID_BC);
-
 	return 0;
 
 out_migrate_fdbs:
-- 
2.25.1

[PATCH net-next 09/10] net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q

[Vladimir Oltean]

felix_migrate_flood_to_tag_8021q_port() takes care of clearing the
flooding bits on the old CPU port (which was the CPU port module), so
manually clearing this bit from PGID_UC, PGID_MC, PGID_BC is redundant.

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

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index e9ce0d687713..638f420bf599 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -465,7 +465,6 @@ static int felix_update_trapping_destinations(struct dsa_switch *ds,
 static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu, bool change)
 {
 	struct ocelot *ocelot = ds->priv;
-	unsigned long cpu_flood;
 	struct dsa_port *dp;
 	int err;
 
@@ -487,15 +486,6 @@ static int felix_setup_tag_8021q(struct dsa_switch *ds, int cpu, bool change)
 				 ANA_PORT_CPU_FWD_BPDU_CFG, dp->index);
 	}
 
-	/* In tag_8021q mode, the CPU port module is unused, except for PTP
-	 * frames. So we want to disable flooding of any kind to the CPU port
-	 * module, since packets going there will end in a black hole.
-	 */
-	cpu_flood = ANA_PGID_PGID_PGID(BIT(ocelot->num_phys_ports));
-	ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_UC);
-	ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_MC);
-	ocelot_rmw_rix(ocelot, 0, cpu_flood, ANA_PGID_PGID, PGID_BC);
-
 	err = dsa_tag_8021q_register(ds, htons(ETH_P_8021AD));
 	if (err)
 		return err;
-- 
2.25.1

[PATCH net-next 10/10] net: mscc: ocelot: accept configuring bridge port flags on the NPI port

[Vladimir Oltean]

In order for the Felix DSA driver to be able to turn on/off flooding
towards its CPU port, we need to redirect calls on the NPI port to
actually act upon the index in the analyzer block that corresponds to
the CPU port module. This was never necessary until now because DSA
(or the bridge) never called ocelot_port_bridge_flags() for the NPI
port.

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

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 0af321f6fb54..21134125a6e4 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -2886,6 +2886,9 @@ EXPORT_SYMBOL(ocelot_port_pre_bridge_flags);
 void ocelot_port_bridge_flags(struct ocelot *ocelot, int port,
 			      struct switchdev_brport_flags flags)
 {
+	if (port == ocelot->npi)
+		port = ocelot->num_phys_ports;
+
 	if (flags.mask & BR_LEARNING)
 		ocelot_port_set_learning(ocelot, port,
 					 !!(flags.val & BR_LEARNING));
-- 
2.25.1

Re: [PATCH net-next 00/10] DSA unicast filtering

[Florian Fainelli]

Hi Vladimir,

On 3/2/2022 11:14 AM, Vladimir Oltean wrote:
> This series doesn't attempt anything extremely brave, it just changes
> the way in which standalone ports which support FDB isolation work.
> 
> Up until now, DSA has recommended that switch drivers configure
> standalone ports in a separate VID/FID with learning disabled, and with
> the CPU port as the only destination, reached trivially via flooding.
> That works, except that standalone ports will deliver all packets to the
> CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> flooding towards the CPU port, to force the dropping of packets with
> unknown MAC DA.
> 
> We handle port promiscuity by re-enabling flooding towards the CPU port.
> This is relevant because the bridge puts its automatic (learning +
> flooding) ports in promiscuous mode, and this makes some things work
> automagically, like for example bridging with a foreign interface.
> We don't delve yet into the territory of managing CPU flooding more
> aggressively while under a bridge.
> 
> The only switch driver that benefits from this work right now is the
> NXP LS1028A switch (felix). The others need to implement FDB isolation
> first, before DSA is going to install entries to the port's standalone
> database. Otherwise, these entries might collide with bridge FDB/MDB
> entries.
> 
> This work was done mainly to have all the required features in place
> before somebody starts seriously architecting DSA support for multiple
> CPU ports. Otherwise it is much more difficult to bolt these features on
> top of multiple CPU ports.

Thanks a lot for submitting this, really happy to see a solution being 
brought upstream. I will be reviewing this in more details later on, but 
from where I left a few years ago, the two challenges that I had are 
outlined below, and I believe we have not quite addressed them yet:

- for switches that implement global VLAN filtering, upper VLAN 
interfaces on top of standalone ports would require programming FDB and 
MDB entries with the appropriate VLAN ID, however there is no such 
tracking today AFAICT, so we are not yet solving those use cases yet, right?

- what if the switch does not support FDB/MDB isolation, what would be 
our options here? As you might remember from a few months ago, the 
Broadcom roboswitch do not have any isolation, but what they can do is 
internally tag Ethernet frames with two VLAN tags, an that may be used 
as a form of isolation

> 
> Vladimir Oltean (10):
>    net: dsa: remove workarounds for changing master promisc/allmulti only
>      while up
>    net: dsa: rename the host FDB and MDB methods to contain the "bridge"
>      namespace
>    net: dsa: install secondary unicast and multicast addresses as host
>      FDB/MDB
>    net: dsa: install the primary unicast MAC address as standalone port
>      host FDB
>    net: dsa: manage flooding on the CPU ports
>    net: dsa: felix: migrate host FDB and MDB entries when changing tag
>      proto
>    net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port
>    net: dsa: felix: start off with flooding disabled on the CPU port
>    net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q
>    net: mscc: ocelot: accept configuring bridge port flags on the NPI
>      port
> 
>   drivers/net/dsa/ocelot/felix.c     | 241 ++++++++++++++++++++------
>   drivers/net/ethernet/mscc/ocelot.c |   3 +
>   include/net/dsa.h                  |   7 +
>   net/dsa/dsa.c                      |  40 +++++
>   net/dsa/dsa_priv.h                 |  53 +++++-
>   net/dsa/port.c                     | 160 +++++++++++++-----
>   net/dsa/slave.c                    | 261 +++++++++++++++++++++++------
>   7 files changed, 609 insertions(+), 156 deletions(-)
> 

-- 
Florian

Re: [PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

Hi Florian,

On Wed, Mar 02, 2022 at 11:30:41AM -0800, Florian Fainelli wrote:
> Hi Vladimir,
> 
> On 3/2/2022 11:14 AM, Vladimir Oltean wrote:
> > This series doesn't attempt anything extremely brave, it just changes
> > the way in which standalone ports which support FDB isolation work.
> > 
> > Up until now, DSA has recommended that switch drivers configure
> > standalone ports in a separate VID/FID with learning disabled, and with
> > the CPU port as the only destination, reached trivially via flooding.
> > That works, except that standalone ports will deliver all packets to the
> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> > flooding towards the CPU port, to force the dropping of packets with
> > unknown MAC DA.
> > 
> > We handle port promiscuity by re-enabling flooding towards the CPU port.
> > This is relevant because the bridge puts its automatic (learning +
> > flooding) ports in promiscuous mode, and this makes some things work
> > automagically, like for example bridging with a foreign interface.
> > We don't delve yet into the territory of managing CPU flooding more
> > aggressively while under a bridge.
> > 
> > The only switch driver that benefits from this work right now is the
> > NXP LS1028A switch (felix). The others need to implement FDB isolation
> > first, before DSA is going to install entries to the port's standalone
> > database. Otherwise, these entries might collide with bridge FDB/MDB
> > entries.
> > 
> > This work was done mainly to have all the required features in place
> > before somebody starts seriously architecting DSA support for multiple
> > CPU ports. Otherwise it is much more difficult to bolt these features on
> > top of multiple CPU ports.
> 
> Thanks a lot for submitting this, really happy to see a solution being
> brought upstream. I will be reviewing this in more details later on, but
> from where I left a few years ago, the two challenges that I had are
> outlined below, and I believe we have not quite addressed them yet:
> 
> - for switches that implement global VLAN filtering, upper VLAN interfaces
> on top of standalone ports would require programming FDB and MDB entries
> with the appropriate VLAN ID, however there is no such tracking today
> AFAICT, so we are not yet solving those use cases yet, right?

Yes, here we're entering "brave" territory. With this patch set we have:

static inline bool dsa_switch_supports_uc_filtering(struct dsa_switch *ds)
{
	return ds->ops->port_fdb_add && ds->ops->port_fdb_del &&
	       ds->fdb_isolation && !ds->vlan_filtering_is_global &&
	       !ds->needs_standalone_vlan_filtering;
}

so yes, the use cases you mention are disqualified. To remove that
restriction, every time our .ndo_vlan_rx_add_vid is called, we'd need to
walk over our address lists (kept by struct net_device :: uc and mc),
and replay them in the newly added VLAN. This is the easy part; the
harder part is that we'd need to keep one more list of standalone VLANs
added on a port, in order to do the reverse - every time dsa_slave_sync_uc()
is called, we program that address not only in VID 0, but also in all
VIDs from the list of standalone VLANs on that port. It might be
possible to use vlan_for_each() for this. In any case it is incremental
complexity which I've not yet added.

This solution would install more host FDB entries than needed - the
Cartesian product of RX VLANs and secondary MAC addresses. I'm ok with
that, but there's also Ivan Khoronzhuk's patch series for Independent
Virtual Device Filtering (IVDF), which basically annotates each MAC
address from struct net_device with a VID. That one really seems
excessive, at this stage, to me.

> - what if the switch does not support FDB/MDB isolation, what would be our
> options here? As you might remember from a few months ago, the Broadcom
> roboswitch do not have any isolation, but what they can do is internally tag
> Ethernet frames with two VLAN tags, an that may be used as a form of
> isolation

The central thing I had in mind is that DSA doesn't really enforce that
packets are sent as 'control traffic'. The users of tag_8021q are the
closest example to me - routing towards the correct egress port is done
through a VID added to the packet, later stripped. The essential
characteristic of a packet sent by a DSA tagger as a 'data packet' is
that the FDB lookup is unavoidable.

The reason why I added FDB isolation as a requirement is this:

   +--------------------+
   |                    |
   | +------+ +------+  |
   | | swp0 | | swp1 |  |
   +-+------+-+------+--+
        |        |
        +--------+

swp0 has 192.168.100.1/24, swp3 has 192.168.100.2/24, they are in
different net namespaces or vrfs and want to ping each other.

Without FDB isolation, the mantra is that DSA will keep the FDB devoid
of swp0's and swp1's MAC addresses. Flooding is enabled on all ports and
the CPU port, and learning is disabled.

With FDB isolation and this patch series, DSA will install the MAC
address of swp0 on the CPU port in swp0's private database, and the
address of swp1 on the CPU port in swp1's private database. It is the
driver's responsibility to then distinguish between those 2 databases
the best it can. VID, FID, EFID, you name it.

When swp0 sends a packet to swp1, it has swp0's MAC SA and swp1's MAC DA.
If DSA doesn't know in the general sense that there will be no FDB lookup,
then the best it can do is to ensure that swp0's and swp1's address are
at least not in the same database. Because otherwise, the CPU port will
see a packet with swp1's MAC DA, but the destination for that address is
the CPU port => drop.

Sadly I don't know if this scenario is hypothetical or not.
With sja1105, it wasn't until I converted it to isolate the FDBs
(packets are sent as 'data traffic', but every standalone port has a
different pvid, and packets sent from the CPU follow the egress port's
pvid). If there are more which I haven't caught (there are many DSA
drivers which I don't know very well), this would break them.
So I wanted to be on the conservative side, but I suppose I could be
convinced otherwise.

Please note that there is hope. On ocelot, standalone ports share the
same pvid of 0, and yet it still declares FDB isolation. This is because
traffic sent over the NPI port (using the "ocelot" tagger) bypasses the
FDB lookup and it will always reach its intended destination. And using
the "ocelot-8021q" tagger, packets hit a VCAP IS2 TCAM rule on the
ingress of the CPU port that redirects them to the desired egress port,
before the packet processing reaches the VLAN table.

As for the Roboswitch discussion:
https://lore.kernel.org/all/04d700b6-a4f7-b108-e711-d400ef01cc2e@bang-olufsen.dk/T/#mb2c185003d8a58c691918e056cf09fd0ef6d1bd0

that was mainly about TX forwarding offload, i.o.w. "how to use the FDB
lookup of data plane packets to your benefit, instead of against you".
In this context of FDB isolation I think it would be more beneficial to
you to keep sending control packets, if those bypass the FDB.

It is the bridge TX forwarding offload that I don't see how you could do.

My takeaway from that discussion is that the IMP port processes the VID
from the packet if VLAN awareness is turned on globally, and probably
reverts to a single default VID otherwise.

So even though you could arrange for each VLAN-unaware bridge to have a
driver-private port pvid, and your tagger could wrap packets in an outer
VLAN tag with this value, the fundamental problem seems to be that VLAN
awareness is global in the first place. So either your user ports can
offload a VLAN-unaware bridge, case in which the IMP can't see the VID
from the packets, or the IMP can see the VID from the packets, but you
can't offload a VLAN-unaware bridge.

In this case and if I have understood all the information, I think what
you can do is deny multiple bridges (regardless of VLAN awareness),
configure standalone ports to use pvid 0 or 4095 (and different from the
pvid of VLAN-unaware bridge ports) and just declare that you support FDB
isolation.

Re: [PATCH net-next 00/10] DSA unicast filtering

[Alvin Šipraga]

Hi Vladimir,

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> This series doesn't attempt anything extremely brave, it just changes
> the way in which standalone ports which support FDB isolation work.
>
> Up until now, DSA has recommended that switch drivers configure
> standalone ports in a separate VID/FID with learning disabled, and with
> the CPU port as the only destination, reached trivially via flooding.
> That works, except that standalone ports will deliver all packets to the
> CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> flooding towards the CPU port, to force the dropping of packets with
> unknown MAC DA.
>
> We handle port promiscuity by re-enabling flooding towards the CPU port.
> This is relevant because the bridge puts its automatic (learning +
> flooding) ports in promiscuous mode, and this makes some things work
> automagically, like for example bridging with a foreign interface.
> We don't delve yet into the territory of managing CPU flooding more
> aggressively while under a bridge.
>
> The only switch driver that benefits from this work right now is the
> NXP LS1028A switch (felix). The others need to implement FDB isolation
> first, before DSA is going to install entries to the port's standalone
> database. Otherwise, these entries might collide with bridge FDB/MDB
> entries.
>
> This work was done mainly to have all the required features in place
> before somebody starts seriously architecting DSA support for multiple
> CPU ports. Otherwise it is much more difficult to bolt these features on
> top of multiple CPU ports.

So, previously FDB entries were only installed on bridged ports. Now you
also want to install FDB entries on standalone ports so that flooding
can be disabled on standalone ports for the reasons stated in your cover
letter.

To implement FDB isolation in a DSA driver, a typical approach might be
to use a filter ID (FID) for the FDB entries that is unique per
bridge. That is, since FDB entries were only added on bridged ports
(through learning or static entries added by software), the DSA driver
could readily use the bridge_num of the bridge that is being offloaded
to select the FID. The same bridge_num/FID would be used by the hardware
for lookup/learning on the given port.

If the above general statements are correct-ish, then my question here
is: what should be the FID - or other equivalent unique identifier used
by the hardware for FDB isolation - when the port is not offloading a
bridge, but is standalone? If FDB isolation is implemented in hardware
with something like FIDs, then do all standalone ports need to have a
unique FID?

For some context: I have been working on implementing offload features
for the rtl8365mb driver and I can also support FDB isolation between
bridged ports. The number of offloaded bridges is bounded by the number
of FIDs available, which is 8. For standalone ports I use a reserved
FID=0 which currently would never match any entries in the FDB, because
learning is disabled on standalone ports and Linux does not install any
FDB entries. When placed in a bridge, the FID of that port is then set
to bridge_num, which - rather conveniently - is indexed by 1.

Your change seems to introduce a more generic concept of per-port
FDB. How should one model the per-port FDB in hardware which uses FIDs?
Should I ensure that all ports - standalone by default - start with a
unique FID? That will be OK for switches with up to 8 ports, but for
switches with more ports, I'm a but puzzled as to what I can do. Do I
then have to declare that FDB isolation is unsupported
(fdb_isolation=0)?

Hope the question makes sense.

Kind regards,
Alvin

Re: [PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

Hi Alvin,

On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin Šipraga wrote:
> Hi Vladimir,
> 
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> 
> > This series doesn't attempt anything extremely brave, it just changes
> > the way in which standalone ports which support FDB isolation work.
> >
> > Up until now, DSA has recommended that switch drivers configure
> > standalone ports in a separate VID/FID with learning disabled, and with
> > the CPU port as the only destination, reached trivially via flooding.
> > That works, except that standalone ports will deliver all packets to the
> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> > flooding towards the CPU port, to force the dropping of packets with
> > unknown MAC DA.
> >
> > We handle port promiscuity by re-enabling flooding towards the CPU port.
> > This is relevant because the bridge puts its automatic (learning +
> > flooding) ports in promiscuous mode, and this makes some things work
> > automagically, like for example bridging with a foreign interface.
> > We don't delve yet into the territory of managing CPU flooding more
> > aggressively while under a bridge.
> >
> > The only switch driver that benefits from this work right now is the
> > NXP LS1028A switch (felix). The others need to implement FDB isolation
> > first, before DSA is going to install entries to the port's standalone
> > database. Otherwise, these entries might collide with bridge FDB/MDB
> > entries.
> >
> > This work was done mainly to have all the required features in place
> > before somebody starts seriously architecting DSA support for multiple
> > CPU ports. Otherwise it is much more difficult to bolt these features on
> > top of multiple CPU ports.
> 
> So, previously FDB entries were only installed on bridged ports. Now you
> also want to install FDB entries on standalone ports so that flooding
> can be disabled on standalone ports for the reasons stated in your cover
> letter.

Not "on standalone ports", but on the CPU ports, for the standalone
ports' addresses. Otherwise, yes.

> To implement FDB isolation in a DSA driver, a typical approach might be
> to use a filter ID (FID) for the FDB entries that is unique per
> bridge. That is, since FDB entries were only added on bridged ports
> (through learning or static entries added by software), the DSA driver
> could readily use the bridge_num of the bridge that is being offloaded
> to select the FID. The same bridge_num/FID would be used by the hardware
> for lookup/learning on the given port.

Yes and no. "FID" is a double-edged sword, it will actually depend upon
hardware implementation. FID in general is a mechanism for FDB
partitioning. If the VID->FID translation is keyed only by VID, then the
only intended use case for that is to support shared VLAN learning,
where all VIDs use the same FID (=> the same database for addresses).
This isn't very interesting for us.
If the VID->FID translation is keyed by {port, VID}, it is then possible
to make VIDs on different ports (part of different bridges) use
different FIDs, and that is what is interesting.

> If the above general statements are correct-ish, then my question here
> is: what should be the FID - or other equivalent unique identifier used
> by the hardware for FDB isolation - when the port is not offloading a
> bridge, but is standalone? If FDB isolation is implemented in hardware
> with something like FIDs, then do all standalone ports need to have a
> unique FID?

Not necessarily, although as far as the DSA core is concerned, we treat
drivers supporting FDB isolation as though each port has its own database.
For example, the sja1105 driver really has unique VIDs (=> FIDs) per
standalone port, so if we didn't treat it that way, it wouldn't work.

It is important to visualize that there are only 2 directions for a
packet to travel in a standalone port's FID: either from the line side
to the CPU, or from the CPU to the line side.

If the "CPU to line side" direction is unimpeded by FDB lookups, then
only the direction towards the CPU matters.

Further assuming that there is a single CPU port (you didn't ask about
multiple CPU ports, so I won't tell), the only practical implication of
standalone ports sharing the same FID is that they will, in effect,
share the same MAC DA filters. So, if you have 4 slave interfaces, each
with its own unicast and multicast address lists, what will happen is
that each port will accept the union of all others too. It's not
perfect, in the sense that unnecessary packets would still reach the
CPU, but they would still be dropped there.
For the felix/ocelot driver, I was ok with that => that driver uses the
same VID/FID of 0 for all standalone ports.

> For some context: I have been working on implementing offload features
> for the rtl8365mb driver and I can also support FDB isolation between
> bridged ports. The number of offloaded bridges is bounded by the number
> of FIDs available, which is 8. For standalone ports I use a reserved
> FID=0 which currently would never match any entries in the FDB, because
> learning is disabled on standalone ports and Linux does not install any
> FDB entries. When placed in a bridge, the FID of that port is then set
> to bridge_num, which - rather conveniently - is indexed by 1.
> 
> Your change seems to introduce a more generic concept of per-port
> FDB. How should one model the per-port FDB in hardware which uses FIDs?
> Should I ensure that all ports - standalone by default - start with a
> unique FID? That will be OK for switches with up to 8 ports, but for
> switches with more ports, I'm a but puzzled as to what I can do. Do I
> then have to declare that FDB isolation is unsupported
> (fdb_isolation=0)?
> 
> Hope the question makes sense.

As long as
(a) tag_rtl8_4.c sends packets to standalone ports in a way in which FDB
    lookups are bypassed
(a) you're not concerned about the hardware MAC DA filters being a bit
    more relaxed than software expects
then I wouldn't worry too much about it, and keep using FID 0 for all
standalone ports.

Re: [PATCH net-next 00/10] DSA unicast filtering

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed,  2 Mar 2022 21:14:07 +0200 you wrote:
> This series doesn't attempt anything extremely brave, it just changes
> the way in which standalone ports which support FDB isolation work.
> 
> Up until now, DSA has recommended that switch drivers configure
> standalone ports in a separate VID/FID with learning disabled, and with
> the CPU port as the only destination, reached trivially via flooding.
> That works, except that standalone ports will deliver all packets to the
> CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> flooding towards the CPU port, to force the dropping of packets with
> unknown MAC DA.
> 
> [...]

Here is the summary with links:
  - [net-next,01/10] net: dsa: remove workarounds for changing master promisc/allmulti only while up
    https://git.kernel.org/netdev/net-next/c/35aae5ab9121
  - [net-next,02/10] net: dsa: rename the host FDB and MDB methods to contain the "bridge" namespace
    https://git.kernel.org/netdev/net-next/c/68d6d71eafd1
  - [net-next,03/10] net: dsa: install secondary unicast and multicast addresses as host FDB/MDB
    https://git.kernel.org/netdev/net-next/c/5e8a1e03aa4d
  - [net-next,04/10] net: dsa: install the primary unicast MAC address as standalone port host FDB
    https://git.kernel.org/netdev/net-next/c/499aa9e1b332
  - [net-next,05/10] net: dsa: manage flooding on the CPU ports
    https://git.kernel.org/netdev/net-next/c/7569459a52c9
  - [net-next,06/10] net: dsa: felix: migrate host FDB and MDB entries when changing tag proto
    https://git.kernel.org/netdev/net-next/c/f9cef64fa23f
  - [net-next,07/10] net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port
    https://git.kernel.org/netdev/net-next/c/b903a6bd2e19
  - [net-next,08/10] net: dsa: felix: start off with flooding disabled on the CPU port
    https://git.kernel.org/netdev/net-next/c/90897569beb1
  - [net-next,09/10] net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q
    https://git.kernel.org/netdev/net-next/c/0cc369800e5f
  - [net-next,10/10] net: mscc: ocelot: accept configuring bridge port flags on the NPI port
    https://git.kernel.org/netdev/net-next/c/ac4552096023

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 00/10] DSA unicast filtering

[Alvin Šipraga]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> Hi Alvin,
>
> On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin Šipraga wrote:
>> Hi Vladimir,
>> 
>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> 
>> > This series doesn't attempt anything extremely brave, it just changes
>> > the way in which standalone ports which support FDB isolation work.
>> >
>> > Up until now, DSA has recommended that switch drivers configure
>> > standalone ports in a separate VID/FID with learning disabled, and with
>> > the CPU port as the only destination, reached trivially via flooding.
>> > That works, except that standalone ports will deliver all packets to the
>> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
>> > flooding towards the CPU port, to force the dropping of packets with
>> > unknown MAC DA.
>> >
>> > We handle port promiscuity by re-enabling flooding towards the CPU port.
>> > This is relevant because the bridge puts its automatic (learning +
>> > flooding) ports in promiscuous mode, and this makes some things work
>> > automagically, like for example bridging with a foreign interface.
>> > We don't delve yet into the territory of managing CPU flooding more
>> > aggressively while under a bridge.
>> >
>> > The only switch driver that benefits from this work right now is the
>> > NXP LS1028A switch (felix). The others need to implement FDB isolation
>> > first, before DSA is going to install entries to the port's standalone
>> > database. Otherwise, these entries might collide with bridge FDB/MDB
>> > entries.
>> >
>> > This work was done mainly to have all the required features in place
>> > before somebody starts seriously architecting DSA support for multiple
>> > CPU ports. Otherwise it is much more difficult to bolt these features on
>> > top of multiple CPU ports.
>> 
>> So, previously FDB entries were only installed on bridged ports. Now you
>> also want to install FDB entries on standalone ports so that flooding
>> can be disabled on standalone ports for the reasons stated in your cover
>> letter.
>
> Not "on standalone ports", but on the CPU ports, for the standalone
> ports' addresses. Otherwise, yes.
>
>> To implement FDB isolation in a DSA driver, a typical approach might be
>> to use a filter ID (FID) for the FDB entries that is unique per
>> bridge. That is, since FDB entries were only added on bridged ports
>> (through learning or static entries added by software), the DSA driver
>> could readily use the bridge_num of the bridge that is being offloaded
>> to select the FID. The same bridge_num/FID would be used by the hardware
>> for lookup/learning on the given port.
>
> Yes and no. "FID" is a double-edged sword, it will actually depend upon
> hardware implementation. FID in general is a mechanism for FDB
> partitioning. If the VID->FID translation is keyed only by VID, then the
> only intended use case for that is to support shared VLAN learning,
> where all VIDs use the same FID (=> the same database for addresses).
> This isn't very interesting for us.
> If the VID->FID translation is keyed by {port, VID}, it is then possible
> to make VIDs on different ports (part of different bridges) use
> different FIDs, and that is what is interesting.
>
>> If the above general statements are correct-ish, then my question here
>> is: what should be the FID - or other equivalent unique identifier used
>> by the hardware for FDB isolation - when the port is not offloading a
>> bridge, but is standalone? If FDB isolation is implemented in hardware
>> with something like FIDs, then do all standalone ports need to have a
>> unique FID?
>
> Not necessarily, although as far as the DSA core is concerned, we treat
> drivers supporting FDB isolation as though each port has its own database.
> For example, the sja1105 driver really has unique VIDs (=> FIDs) per
> standalone port, so if we didn't treat it that way, it wouldn't work.

I think I'm not quite grokking what it means for a port to have its own
database. Above you corrected me by stating that this patchset does not
install FDB entries "on standalone ports", but on the CPU ports. The way
I parse this is: you are adding an FDB entry to the database of the
CPU port. But how would that help with the "line side to CPU side"
direction? When a packet from the "line side" enters one of the
(standalone) user ports, the switch would appeal to the forwarding
database of that port, not the CPU port, no? So how does it help to
install FDB entries on the CPU port(s)?

Basically for a switch with two ports, swp0 and swp1, with MAC addresses
foo and bar respectively, we want the following FDB entries to be
installed when both ports are in standalone mode:

    MAC | VID | PORT
    ----+-----+-------------
    foo |  0  | cpu_port_num
    bar |  0  | cpu_port_num

Such that, when swp0 receives a packet with MAC DA 'foo', it knows to
forward that packet to cpu_port_num, i.e. towards the CPU port, instead
of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right?

Now DSA assumes that each port has its own forwarding database, so let
me annotate how I see that by adding another column "PORT DB":

    MAC | VID | PORT         | PORT DB
    ----+-----+--------------+--------------
    foo |  0  | cpu_port_num | swp0_port_num  [*]
    bar |  0  | cpu_port_num | swp1_port_num  [**]

This codifies better that the entry for 'foo' should, ideally, only be
used by the switch when it receives a packet from 'foo' on swp0, but not
if it's received on swp1, etc. Internally this might be weakened to the
same underlying FID, but as you already pointed out, this is acceptable.

Overall this seems logical and OK to me, but I can't reconcile it with
your statement that the FDB entries in question are installed "on the
CPU port". Wouldn't that look like this?

    MAC | VID | PORT         | PORT DB
    ----+-----+--------------+-------------
    foo |  0  | cpu_port_num | cpu_port_num
    bar |  0  | cpu_port_num | cpu_port_num

... which seems wrong.

I'm suspecting/hoping that my above misunderstanding is just a matter
of terminology. For the sake of emphasis, allow me to describe in plain
English the FDB entries [*] and [**] above:

 [*] add an FDB entry on swp0 where [the host/station? with] MAC address
     'foo' is behind the CPU port on VID 0
[**] add an FDB entry on swp1 where [the host/station? with] MAC address
     'bar' is behind the CPU port on VID 0

Merely an attempt, and perhaps not idiomatic, but now you can tell me
what's wrong in describing it that way :-)

>
> It is important to visualize that there are only 2 directions for a
> packet to travel in a standalone port's FID: either from the line side
> to the CPU, or from the CPU to the line side.
>
> If the "CPU to line side" direction is unimpeded by FDB lookups, then
> only the direction towards the CPU matters.
>
> Further assuming that there is a single CPU port (you didn't ask about
> multiple CPU ports, so I won't tell), the only practical implication of
> standalone ports sharing the same FID is that they will, in effect,
> share the same MAC DA filters. So, if you have 4 slave interfaces, each
> with its own unicast and multicast address lists, what will happen is
> that each port will accept the union of all others too. It's not
> perfect, in the sense that unnecessary packets would still reach the
> CPU, but they would still be dropped there.

Right, this is the scenario I had in mind. Fine by me if this is
considered acceptable. I see how it is still better than flooding every
packet.

> For the felix/ocelot driver, I was ok with that => that driver uses the
> same VID/FID of 0 for all standalone ports.
>
>> For some context: I have been working on implementing offload features
>> for the rtl8365mb driver and I can also support FDB isolation between
>> bridged ports. The number of offloaded bridges is bounded by the number
>> of FIDs available, which is 8. For standalone ports I use a reserved
>> FID=0 which currently would never match any entries in the FDB, because
>> learning is disabled on standalone ports and Linux does not install any
>> FDB entries. When placed in a bridge, the FID of that port is then set
>> to bridge_num, which - rather conveniently - is indexed by 1.
>> 
>> Your change seems to introduce a more generic concept of per-port
>> FDB. How should one model the per-port FDB in hardware which uses FIDs?
>> Should I ensure that all ports - standalone by default - start with a
>> unique FID? That will be OK for switches with up to 8 ports, but for
>> switches with more ports, I'm a but puzzled as to what I can do. Do I
>> then have to declare that FDB isolation is unsupported
>> (fdb_isolation=0)?
>> 
>> Hope the question makes sense.
>
> As long as
> (a) tag_rtl8_4.c sends packets to standalone ports in a way in which FDB
>     lookups are bypassed

Yes, it does.

> (a) you're not concerned about the hardware MAC DA filters being a bit
>     more relaxed than software expects

Since they are dropped in software, it doesn't matter to me.

> then I wouldn't worry too much about it, and keep using FID 0 for all
> standalone ports.

Great, thanks a lot for clarifying.

Kind regards,
Alvin

Re: [PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote:
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> 
> > Hi Alvin,
> >
> > On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin Šipraga wrote:
> >> Hi Vladimir,
> >> 
> >> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> >> 
> >> > This series doesn't attempt anything extremely brave, it just changes
> >> > the way in which standalone ports which support FDB isolation work.
> >> >
> >> > Up until now, DSA has recommended that switch drivers configure
> >> > standalone ports in a separate VID/FID with learning disabled, and with
> >> > the CPU port as the only destination, reached trivially via flooding.
> >> > That works, except that standalone ports will deliver all packets to the
> >> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> >> > flooding towards the CPU port, to force the dropping of packets with
> >> > unknown MAC DA.
> >> >
> >> > We handle port promiscuity by re-enabling flooding towards the CPU port.
> >> > This is relevant because the bridge puts its automatic (learning +
> >> > flooding) ports in promiscuous mode, and this makes some things work
> >> > automagically, like for example bridging with a foreign interface.
> >> > We don't delve yet into the territory of managing CPU flooding more
> >> > aggressively while under a bridge.
> >> >
> >> > The only switch driver that benefits from this work right now is the
> >> > NXP LS1028A switch (felix). The others need to implement FDB isolation
> >> > first, before DSA is going to install entries to the port's standalone
> >> > database. Otherwise, these entries might collide with bridge FDB/MDB
> >> > entries.
> >> >
> >> > This work was done mainly to have all the required features in place
> >> > before somebody starts seriously architecting DSA support for multiple
> >> > CPU ports. Otherwise it is much more difficult to bolt these features on
> >> > top of multiple CPU ports.
> >> 
> >> So, previously FDB entries were only installed on bridged ports. Now you
> >> also want to install FDB entries on standalone ports so that flooding
> >> can be disabled on standalone ports for the reasons stated in your cover
> >> letter.
> >
> > Not "on standalone ports", but on the CPU ports, for the standalone
> > ports' addresses. Otherwise, yes.
> >
> >> To implement FDB isolation in a DSA driver, a typical approach might be
> >> to use a filter ID (FID) for the FDB entries that is unique per
> >> bridge. That is, since FDB entries were only added on bridged ports
> >> (through learning or static entries added by software), the DSA driver
> >> could readily use the bridge_num of the bridge that is being offloaded
> >> to select the FID. The same bridge_num/FID would be used by the hardware
> >> for lookup/learning on the given port.
> >
> > Yes and no. "FID" is a double-edged sword, it will actually depend upon
> > hardware implementation. FID in general is a mechanism for FDB
> > partitioning. If the VID->FID translation is keyed only by VID, then the
> > only intended use case for that is to support shared VLAN learning,
> > where all VIDs use the same FID (=> the same database for addresses).
> > This isn't very interesting for us.
> > If the VID->FID translation is keyed by {port, VID}, it is then possible
> > to make VIDs on different ports (part of different bridges) use
> > different FIDs, and that is what is interesting.
> >
> >> If the above general statements are correct-ish, then my question here
> >> is: what should be the FID - or other equivalent unique identifier used
> >> by the hardware for FDB isolation - when the port is not offloading a
> >> bridge, but is standalone? If FDB isolation is implemented in hardware
> >> with something like FIDs, then do all standalone ports need to have a
> >> unique FID?
> >
> > Not necessarily, although as far as the DSA core is concerned, we treat
> > drivers supporting FDB isolation as though each port has its own database.
> > For example, the sja1105 driver really has unique VIDs (=> FIDs) per
> > standalone port, so if we didn't treat it that way, it wouldn't work.
> 
> I think I'm not quite grokking what it means for a port to have its own
> database. Above you corrected me by stating that this patchset does not
> install FDB entries "on standalone ports", but on the CPU ports. The way
> I parse this is: you are adding an FDB entry to the database of the
> CPU port.

No. I am adding an FDB entry _towards_ the CPU port.

Perhaps I need to clarify what I mean by a "database". It is a construct
used by DSA to denote "which subset of ports should match this entry".
The entries from the database of a port should only be matched by that
port when it operates as standalone. Not by other ports, not by bridged
ports.
The entries from the database of a bridge should only be matched by the
ports in that bridge. Not by ports in other bridges, not by standalone
ports.
The entries from the database of a LAG should only be matched by the
ports in that LAG.

The FDB entries added here belong to the database of a standalone port.
This is necessary, because classifying the packet to a FID is done on
the ingress (=> standalone) port. But the destination is the CPU port.

All shared (DSA and CPU) ports serve multiple databases.

> But how would that help with the "line side to CPU side"
> direction? When a packet from the "line side" enters one of the
> (standalone) user ports, the switch would appeal to the forwarding
> database of that port, not the CPU port, no? So how does it help to
> install FDB entries on the CPU port(s)?

See above. This is exactly why we pass the struct dsa_db as argument to
.port_fdb_add(), and why you can't just deduce the database from the
provided "port" argument. The CPU port must serve the databases of
standalone ports.

> Basically for a switch with two ports, swp0 and swp1, with MAC addresses
> foo and bar respectively, we want the following FDB entries to be
> installed when both ports are in standalone mode:
> 
>     MAC | VID | PORT
>     ----+-----+-------------
>     foo |  0  | cpu_port_num
>     bar |  0  | cpu_port_num
> 
> Such that, when swp0 receives a packet with MAC DA 'foo', it knows to
> forward that packet to cpu_port_num, i.e. towards the CPU port, instead
> of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right?
> 
> Now DSA assumes that each port has its own forwarding database, so let
> me annotate how I see that by adding another column "PORT DB":
> 
>     MAC | VID | PORT         | PORT DB
>     ----+-----+--------------+--------------
>     foo |  0  | cpu_port_num | swp0_port_num  [*]
>     bar |  0  | cpu_port_num | swp1_port_num  [**]
> 
> This codifies better that the entry for 'foo' should, ideally, only be
> used by the switch when it receives a packet from 'foo' on swp0, but not
> if it's received on swp1, etc. Internally this might be weakened to the
> same underlying FID, but as you already pointed out, this is acceptable.
> 
> Overall this seems logical and OK to me, but I can't reconcile it with
> your statement that the FDB entries in question are installed "on the
> CPU port". Wouldn't that look like this?
> 
>     MAC | VID | PORT         | PORT DB
>     ----+-----+--------------+-------------
>     foo |  0  | cpu_port_num | cpu_port_num
>     bar |  0  | cpu_port_num | cpu_port_num
> 
> ... which seems wrong.

See above. Basically, when I say "install a FDB entry on port X", I mean
"install it and make it point towards port X". Maybe I should be more
specific.

> I'm suspecting/hoping that my above misunderstanding is just a matter
> of terminology. For the sake of emphasis, allow me to describe in plain
> English the FDB entries [*] and [**] above:
> 
>  [*] add an FDB entry on swp0 where [the host/station? with] MAC address
>      'foo' is behind the CPU port on VID 0
> [**] add an FDB entry on swp1 where [the host/station? with] MAC address
>      'bar' is behind the CPU port on VID 0
> 
> Merely an attempt, and perhaps not idiomatic, but now you can tell me
> what's wrong in describing it that way :-)

Perhaps:

[*]  add an FDB entry to the switch, with MAC address 'foo', which can
     only be matched for packets ingressing swp0 in standalone mode
     (i.e. is in swp0's port database), whose destination is the CPU
     port

[**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1'

Re: [PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

On Thu, Mar 03, 2022 at 04:35:29PM +0200, Vladimir Oltean wrote:
> On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote:
> > Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> > 
> > > Hi Alvin,
> > >
> > > On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin Šipraga wrote:
> > >> Hi Vladimir,
> > >> 
> > >> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> > >> 
> > >> > This series doesn't attempt anything extremely brave, it just changes
> > >> > the way in which standalone ports which support FDB isolation work.
> > >> >
> > >> > Up until now, DSA has recommended that switch drivers configure
> > >> > standalone ports in a separate VID/FID with learning disabled, and with
> > >> > the CPU port as the only destination, reached trivially via flooding.
> > >> > That works, except that standalone ports will deliver all packets to the
> > >> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> > >> > flooding towards the CPU port, to force the dropping of packets with
> > >> > unknown MAC DA.
> > >> >
> > >> > We handle port promiscuity by re-enabling flooding towards the CPU port.
> > >> > This is relevant because the bridge puts its automatic (learning +
> > >> > flooding) ports in promiscuous mode, and this makes some things work
> > >> > automagically, like for example bridging with a foreign interface.
> > >> > We don't delve yet into the territory of managing CPU flooding more
> > >> > aggressively while under a bridge.
> > >> >
> > >> > The only switch driver that benefits from this work right now is the
> > >> > NXP LS1028A switch (felix). The others need to implement FDB isolation
> > >> > first, before DSA is going to install entries to the port's standalone
> > >> > database. Otherwise, these entries might collide with bridge FDB/MDB
> > >> > entries.
> > >> >
> > >> > This work was done mainly to have all the required features in place
> > >> > before somebody starts seriously architecting DSA support for multiple
> > >> > CPU ports. Otherwise it is much more difficult to bolt these features on
> > >> > top of multiple CPU ports.
> > >> 
> > >> So, previously FDB entries were only installed on bridged ports. Now you
> > >> also want to install FDB entries on standalone ports so that flooding
> > >> can be disabled on standalone ports for the reasons stated in your cover
> > >> letter.
> > >
> > > Not "on standalone ports", but on the CPU ports, for the standalone
> > > ports' addresses. Otherwise, yes.
> > >
> > >> To implement FDB isolation in a DSA driver, a typical approach might be
> > >> to use a filter ID (FID) for the FDB entries that is unique per
> > >> bridge. That is, since FDB entries were only added on bridged ports
> > >> (through learning or static entries added by software), the DSA driver
> > >> could readily use the bridge_num of the bridge that is being offloaded
> > >> to select the FID. The same bridge_num/FID would be used by the hardware
> > >> for lookup/learning on the given port.
> > >
> > > Yes and no. "FID" is a double-edged sword, it will actually depend upon
> > > hardware implementation. FID in general is a mechanism for FDB
> > > partitioning. If the VID->FID translation is keyed only by VID, then the
> > > only intended use case for that is to support shared VLAN learning,
> > > where all VIDs use the same FID (=> the same database for addresses).
> > > This isn't very interesting for us.
> > > If the VID->FID translation is keyed by {port, VID}, it is then possible
> > > to make VIDs on different ports (part of different bridges) use
> > > different FIDs, and that is what is interesting.
> > >
> > >> If the above general statements are correct-ish, then my question here
> > >> is: what should be the FID - or other equivalent unique identifier used
> > >> by the hardware for FDB isolation - when the port is not offloading a
> > >> bridge, but is standalone? If FDB isolation is implemented in hardware
> > >> with something like FIDs, then do all standalone ports need to have a
> > >> unique FID?
> > >
> > > Not necessarily, although as far as the DSA core is concerned, we treat
> > > drivers supporting FDB isolation as though each port has its own database.
> > > For example, the sja1105 driver really has unique VIDs (=> FIDs) per
> > > standalone port, so if we didn't treat it that way, it wouldn't work.
> > 
> > I think I'm not quite grokking what it means for a port to have its own
> > database. Above you corrected me by stating that this patchset does not
> > install FDB entries "on standalone ports", but on the CPU ports. The way
> > I parse this is: you are adding an FDB entry to the database of the
> > CPU port.
> 
> No. I am adding an FDB entry _towards_ the CPU port.
> 
> Perhaps I need to clarify what I mean by a "database". It is a construct
> used by DSA to denote "which subset of ports should match this entry".
> The entries from the database of a port should only be matched by that
> port when it operates as standalone. Not by other ports, not by bridged
> ports.
> The entries from the database of a bridge should only be matched by the
> ports in that bridge. Not by ports in other bridges, not by standalone
> ports.
> The entries from the database of a LAG should only be matched by the
> ports in that LAG.
> 
> The FDB entries added here belong to the database of a standalone port.
> This is necessary, because classifying the packet to a FID is done on
> the ingress (=> standalone) port. But the destination is the CPU port.
> 
> All shared (DSA and CPU) ports serve multiple databases.
> 
> > But how would that help with the "line side to CPU side"
> > direction? When a packet from the "line side" enters one of the
> > (standalone) user ports, the switch would appeal to the forwarding
> > database of that port, not the CPU port, no? So how does it help to
> > install FDB entries on the CPU port(s)?
> 
> See above. This is exactly why we pass the struct dsa_db as argument to
> .port_fdb_add(), and why you can't just deduce the database from the
> provided "port" argument. The CPU port must serve the databases of
> standalone ports.
> 
> > Basically for a switch with two ports, swp0 and swp1, with MAC addresses
> > foo and bar respectively, we want the following FDB entries to be
> > installed when both ports are in standalone mode:
> > 
> >     MAC | VID | PORT
> >     ----+-----+-------------
> >     foo |  0  | cpu_port_num
> >     bar |  0  | cpu_port_num
> > 
> > Such that, when swp0 receives a packet with MAC DA 'foo', it knows to
> > forward that packet to cpu_port_num, i.e. towards the CPU port, instead
> > of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right?
> > 
> > Now DSA assumes that each port has its own forwarding database, so let
> > me annotate how I see that by adding another column "PORT DB":
> > 
> >     MAC | VID | PORT         | PORT DB
> >     ----+-----+--------------+--------------
> >     foo |  0  | cpu_port_num | swp0_port_num  [*]
> >     bar |  0  | cpu_port_num | swp1_port_num  [**]
> > 
> > This codifies better that the entry for 'foo' should, ideally, only be
> > used by the switch when it receives a packet from 'foo' on swp0, but not
> > if it's received on swp1, etc. Internally this might be weakened to the
> > same underlying FID, but as you already pointed out, this is acceptable.
> > 
> > Overall this seems logical and OK to me, but I can't reconcile it with
> > your statement that the FDB entries in question are installed "on the
> > CPU port". Wouldn't that look like this?
> > 
> >     MAC | VID | PORT         | PORT DB
> >     ----+-----+--------------+-------------
> >     foo |  0  | cpu_port_num | cpu_port_num
> >     bar |  0  | cpu_port_num | cpu_port_num
> > 
> > ... which seems wrong.
> 
> See above. Basically, when I say "install a FDB entry on port X", I mean
> "install it and make it point towards port X". Maybe I should be more
> specific.
> 
> > I'm suspecting/hoping that my above misunderstanding is just a matter
> > of terminology. For the sake of emphasis, allow me to describe in plain
> > English the FDB entries [*] and [**] above:
> > 
> >  [*] add an FDB entry on swp0 where [the host/station? with] MAC address
> >      'foo' is behind the CPU port on VID 0
> > [**] add an FDB entry on swp1 where [the host/station? with] MAC address
> >      'bar' is behind the CPU port on VID 0
> > 
> > Merely an attempt, and perhaps not idiomatic, but now you can tell me
> > what's wrong in describing it that way :-)
> 
> Perhaps:
> 
> [*]  add an FDB entry to the switch, with MAC address 'foo', which can
>      only be matched for packets ingressing swp0 in standalone mode
>      (i.e. is in swp0's port database), whose destination is the CPU
>      port
> 
> [**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1'

Let me give you a concrete example.

The way in which sja1105 uses tag_8021q is that (assuming 4 ports), the
following are configured as PVIDs in standalone mode:

swp0 has PVID 3072 and MAC address 00:01:02:03:04:00
swp1 has PVID 3073 and MAC address 00:01:02:03:04:01
swp2 has PVID 3074 and MAC address 00:01:02:03:04:02
swp3 has PVID 3075 and MAC address 00:01:02:03:04:03
Port 4 is the CPU port. It doesn't have a PVID - all traffic ingressing
it is VLAN-tagged, what isn't tagged is dropped.

When DSA wants to program the unicast address of swp0 to hardware for
unicast filtering, it calls .port_fdb_add(port 4, address 00:01:02:03:04:00,
VID 0, DSA_DB_PORT with a "dp" of swp0).
The sja1105 driver then translates the database of swp0 to VID/FID 3072,
and installs MAC 00:01:02:03:04:00 & VID 3072 as a FDB entry towards port 4.

This is what we want, because all packets received on swp0 in standalone
mode are classified by the hardware to VID/FID 3072. They will therefore
match the FDB entry and be routed to the CPU port. No other port will
match on that entry, because every other port has a different PVID.

DSA has no underlying knowledge of how the drivers handle their FIDs, it
just passes the information they need such that they can make the
necessary translation when installing FDB entries.

Re: [PATCH net-next 00/10] DSA unicast filtering

[Alvin Šipraga]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote:
>> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> 
>> > Hi Alvin,
>> >
>> > On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin Šipraga wrote:
>> >> Hi Vladimir,
>> >> 
>> >> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
>> >> 
>> >> > This series doesn't attempt anything extremely brave, it just changes
>> >> > the way in which standalone ports which support FDB isolation work.
>> >> >
>> >> > Up until now, DSA has recommended that switch drivers configure
>> >> > standalone ports in a separate VID/FID with learning disabled, and with
>> >> > the CPU port as the only destination, reached trivially via flooding.
>> >> > That works, except that standalone ports will deliver all packets to the
>> >> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
>> >> > flooding towards the CPU port, to force the dropping of packets with
>> >> > unknown MAC DA.
>> >> >
>> >> > We handle port promiscuity by re-enabling flooding towards the CPU port.
>> >> > This is relevant because the bridge puts its automatic (learning +
>> >> > flooding) ports in promiscuous mode, and this makes some things work
>> >> > automagically, like for example bridging with a foreign interface.
>> >> > We don't delve yet into the territory of managing CPU flooding more
>> >> > aggressively while under a bridge.
>> >> >
>> >> > The only switch driver that benefits from this work right now is the
>> >> > NXP LS1028A switch (felix). The others need to implement FDB isolation
>> >> > first, before DSA is going to install entries to the port's standalone
>> >> > database. Otherwise, these entries might collide with bridge FDB/MDB
>> >> > entries.
>> >> >
>> >> > This work was done mainly to have all the required features in place
>> >> > before somebody starts seriously architecting DSA support for multiple
>> >> > CPU ports. Otherwise it is much more difficult to bolt these features on
>> >> > top of multiple CPU ports.
>> >> 
>> >> So, previously FDB entries were only installed on bridged ports. Now you
>> >> also want to install FDB entries on standalone ports so that flooding
>> >> can be disabled on standalone ports for the reasons stated in your cover
>> >> letter.
>> >
>> > Not "on standalone ports", but on the CPU ports, for the standalone
>> > ports' addresses. Otherwise, yes.
>> >
>> >> To implement FDB isolation in a DSA driver, a typical approach might be
>> >> to use a filter ID (FID) for the FDB entries that is unique per
>> >> bridge. That is, since FDB entries were only added on bridged ports
>> >> (through learning or static entries added by software), the DSA driver
>> >> could readily use the bridge_num of the bridge that is being offloaded
>> >> to select the FID. The same bridge_num/FID would be used by the hardware
>> >> for lookup/learning on the given port.
>> >
>> > Yes and no. "FID" is a double-edged sword, it will actually depend upon
>> > hardware implementation. FID in general is a mechanism for FDB
>> > partitioning. If the VID->FID translation is keyed only by VID, then the
>> > only intended use case for that is to support shared VLAN learning,
>> > where all VIDs use the same FID (=> the same database for addresses).
>> > This isn't very interesting for us.
>> > If the VID->FID translation is keyed by {port, VID}, it is then possible
>> > to make VIDs on different ports (part of different bridges) use
>> > different FIDs, and that is what is interesting.
>> >
>> >> If the above general statements are correct-ish, then my question here
>> >> is: what should be the FID - or other equivalent unique identifier used
>> >> by the hardware for FDB isolation - when the port is not offloading a
>> >> bridge, but is standalone? If FDB isolation is implemented in hardware
>> >> with something like FIDs, then do all standalone ports need to have a
>> >> unique FID?
>> >
>> > Not necessarily, although as far as the DSA core is concerned, we treat
>> > drivers supporting FDB isolation as though each port has its own database.
>> > For example, the sja1105 driver really has unique VIDs (=> FIDs) per
>> > standalone port, so if we didn't treat it that way, it wouldn't work.
>> 
>> I think I'm not quite grokking what it means for a port to have its own
>> database. Above you corrected me by stating that this patchset does not
>> install FDB entries "on standalone ports", but on the CPU ports. The way
>> I parse this is: you are adding an FDB entry to the database of the
>> CPU port.
>
> No. I am adding an FDB entry _towards_ the CPU port.
>
> Perhaps I need to clarify what I mean by a "database". It is a construct
> used by DSA to denote "which subset of ports should match this entry".
> The entries from the database of a port should only be matched by that
> port when it operates as standalone. Not by other ports, not by bridged
> ports.
> The entries from the database of a bridge should only be matched by the
> ports in that bridge. Not by ports in other bridges, not by standalone
> ports.
> The entries from the database of a LAG should only be matched by the
> ports in that LAG.
>
> The FDB entries added here belong to the database of a standalone port.
> This is necessary, because classifying the packet to a FID is done on
> the ingress (=> standalone) port. But the destination is the CPU port.
>
> All shared (DSA and CPU) ports serve multiple databases.
>
>> But how would that help with the "line side to CPU side"
>> direction? When a packet from the "line side" enters one of the
>> (standalone) user ports, the switch would appeal to the forwarding
>> database of that port, not the CPU port, no? So how does it help to
>> install FDB entries on the CPU port(s)?
>
> See above. This is exactly why we pass the struct dsa_db as argument to
> .port_fdb_add(), and why you can't just deduce the database from the
> provided "port" argument. The CPU port must serve the databases of
> standalone ports.

Right, this was what was I was missing. Now I follow.

It helped to take another look at how sja1105 handles it:

static int sja1105_fdb_add(struct dsa_switch *ds, int port,
			   const unsigned char *addr, u16 vid,
			   struct dsa_db db)
{
	struct sja1105_private *priv = ds->priv;

	if (!vid) {
		switch (db.type) {
		case DSA_DB_PORT:
			vid = dsa_tag_8021q_standalone_vid(db.dp);
			break;
		case DSA_DB_BRIDGE:
			vid = dsa_tag_8021q_bridge_vid(db.bridge.num);
			break;
		default:
			return -EOPNOTSUPP;
		}
	}

	return priv->info->fdb_add_cmd(ds, port, addr, vid);
}

So, yes, we do call .port_fdb_add() "on" the CPU port (i.e. the 'port'
argument refers to the port number of the CPU port). But this just means
telling the switch to forward packets with MAC DA 'addr' to 'port'. The
actual forwarding database - (standalone) port, or bridge, or LAG
database - is determined by the 'db' parameter. It is up to the DSA
driver to make sense of the info in db.{dp,bridge,lag} in order to
ensure proper FDB isolation, such that the entry is only matched (on
packet ingress) by the ports implied by the given database (a single
standalone port, or a group of ports belonging to the same bridge, or a
group of ports in a LAG).

>
>> Basically for a switch with two ports, swp0 and swp1, with MAC addresses
>> foo and bar respectively, we want the following FDB entries to be
>> installed when both ports are in standalone mode:
>> 
>>     MAC | VID | PORT
>>     ----+-----+-------------
>>     foo |  0  | cpu_port_num
>>     bar |  0  | cpu_port_num
>> 
>> Such that, when swp0 receives a packet with MAC DA 'foo', it knows to
>> forward that packet to cpu_port_num, i.e. towards the CPU port, instead
>> of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right?
>> 
>> Now DSA assumes that each port has its own forwarding database, so let
>> me annotate how I see that by adding another column "PORT DB":
>> 
>>     MAC | VID | PORT         | PORT DB
>>     ----+-----+--------------+--------------
>>     foo |  0  | cpu_port_num | swp0_port_num  [*]
>>     bar |  0  | cpu_port_num | swp1_port_num  [**]
>> 
>> This codifies better that the entry for 'foo' should, ideally, only be
>> used by the switch when it receives a packet from 'foo' on swp0, but not
>> if it's received on swp1, etc. Internally this might be weakened to the
>> same underlying FID, but as you already pointed out, this is acceptable.
>> 
>> Overall this seems logical and OK to me, but I can't reconcile it with
>> your statement that the FDB entries in question are installed "on the
>> CPU port". Wouldn't that look like this?
>> 
>>     MAC | VID | PORT         | PORT DB
>>     ----+-----+--------------+-------------
>>     foo |  0  | cpu_port_num | cpu_port_num
>>     bar |  0  | cpu_port_num | cpu_port_num
>> 
>> ... which seems wrong.
>
> See above. Basically, when I say "install a FDB entry on port X", I mean
> "install it and make it point towards port X". Maybe I should be more
> specific.
>
>> I'm suspecting/hoping that my above misunderstanding is just a matter
>> of terminology. For the sake of emphasis, allow me to describe in plain
>> English the FDB entries [*] and [**] above:
>> 
>>  [*] add an FDB entry on swp0 where [the host/station? with] MAC address
>>      'foo' is behind the CPU port on VID 0
>> [**] add an FDB entry on swp1 where [the host/station? with] MAC address
>>      'bar' is behind the CPU port on VID 0
>> 
>> Merely an attempt, and perhaps not idiomatic, but now you can tell me
>> what's wrong in describing it that way :-)
>
> Perhaps:
>
> [*]  add an FDB entry to the switch, with MAC address 'foo', which can
>      only be matched for packets ingressing swp0 in standalone mode
>      (i.e. is in swp0's port database), whose destination is the CPU
>      port
>
> [**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1'

Superb, thanks as always for the fine explanation.

Kind regards,
Alvin

Re: [PATCH net-next 00/10] DSA unicast filtering

[Vladimir Oltean]

On Thu, Mar 03, 2022 at 03:13:41PM +0000, Alvin Šipraga wrote:
> So, yes, we do call .port_fdb_add() "on" the CPU port (i.e. the 'port'
> argument refers to the port number of the CPU port). But this just means
> telling the switch to forward packets with MAC DA 'addr' to 'port'. The
> actual forwarding database - (standalone) port, or bridge, or LAG
> database - is determined by the 'db' parameter. It is up to the DSA
> driver to make sense of the info in db.{dp,bridge,lag} in order to
> ensure proper FDB isolation, such that the entry is only matched (on
> packet ingress) by the ports implied by the given database (a single
> standalone port, or a group of ports belonging to the same bridge, or a
> group of ports in a LAG).

This is indeed what I was trying to say.