[PATCH net-next 0/5] net: dsa: mv88e6xxx: Offload bridge port flags

Add support for offloading learning and broadcast flooding flags. With
this in place, mv88e6xx supports offloading of all bridge port flags
that are currently supported by the bridge.

Broadcast flooding is somewhat awkward to control as there is no
per-port bit for this like there is for unknown unicast and unknown
multicast. Instead we have to update the ATU entry for the broadcast
address for all currently used FIDs.

Tobias Waldekranz (5):
  net: dsa: mv88e6xxx: Provide generic VTU iterator
  net: dsa: mv88e6xxx: Remove some bureaucracy around querying the VTU
  net: dsa: mv88e6xxx: Flood all traffic classes on standalone ports
  net: dsa: mv88e6xxx: Offload bridge learning flag
  net: dsa: mv88e6xxx: Offload bridge broadcast flooding flag

 drivers/net/dsa/mv88e6xxx/chip.c | 248 +++++++++++++++++++++----------
 drivers/net/dsa/mv88e6xxx/port.c |  21 +++
 drivers/net/dsa/mv88e6xxx/port.h |   2 +
 3 files changed, 196 insertions(+), 75 deletions(-)

-- 
2.25.1

Move the intricacies of correctly iterating over the VTU to a common
implementation.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 100 ++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 36 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 903d619e08ed..c70d4b7db4f5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1481,6 +1481,37 @@ static int mv88e6xxx_vtu_getnext(struct mv88e6xxx_chip *chip,
 	return chip->info->ops->vtu_getnext(chip, entry);
 }
 
+static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,
+			      int (*cb)(struct mv88e6xxx_chip *chip,
+					const struct mv88e6xxx_vtu_entry *entry,
+					void *priv),
+			      void *priv)
+{
+	struct mv88e6xxx_vtu_entry entry = {
+		.vid = mv88e6xxx_max_vid(chip),
+		.valid = false,
+	};
+	int err;
+
+	if (!chip->info->ops->vtu_getnext)
+		return -EOPNOTSUPP;
+
+	do {
+		err = chip->info->ops->vtu_getnext(chip, &entry);
+		if (err)
+			return err;
+
+		if (!entry.valid)
+			break;
+
+		err = cb(chip, &entry, priv);
+		if (err)
+			return err;
+	} while (entry.vid < mv88e6xxx_max_vid(chip));
+
+	return 0;
+}
+
 static int mv88e6xxx_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 				   struct mv88e6xxx_vtu_entry *entry)
 {

@@ -1490,9 +1521,18 @@ static int mv88e6xxx_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 	return chip->info->ops->vtu_loadpurge(chip, entry);
 }
 
+static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip,
+				  const struct mv88e6xxx_vtu_entry *entry,
+				  void *_fid_bitmap)
+{
+	unsigned long *fid_bitmap = _fid_bitmap;
+
+	set_bit(entry->fid, fid_bitmap);
+	return 0;
+}
+
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
 {
-	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;
 	u16 fid;
 

@@ -1508,21 +1548,7 @@ int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap)
 	}
 
 	/* Set every FID bit used by the VLAN entries */
-	vlan.vid = mv88e6xxx_max_vid(chip);
-	vlan.valid = false;
-
-	do {
-		err = mv88e6xxx_vtu_getnext(chip, &vlan);
-		if (err)
-			return err;
-
-		if (!vlan.valid)
-			break;
-
-		set_bit(vlan.fid, fid_bitmap);
-	} while (vlan.vid < mv88e6xxx_max_vid(chip));
-
-	return 0;
+	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap);
 }
 
 static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)

@@ -2168,10 +2194,30 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
 	return err;
 }
 
+struct mv88e6xxx_port_db_dump_vlan_ctx {
+	int port;
+	dsa_fdb_dump_cb_t *cb;
+	void *data;
+};
+
+static int mv88e6xxx_port_db_dump_vlan(struct mv88e6xxx_chip *chip,
+				       const struct mv88e6xxx_vtu_entry *entry,
+				       void *_data)
+{
+	struct mv88e6xxx_port_db_dump_vlan_ctx *ctx = _data;
+
+	return mv88e6xxx_port_db_dump_fid(chip, entry->fid, entry->vid,
+					  ctx->port, ctx->cb, ctx->data);
+}
+
 static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 				  dsa_fdb_dump_cb_t *cb, void *data)
 {
-	struct mv88e6xxx_vtu_entry vlan;
+	struct mv88e6xxx_port_db_dump_vlan_ctx ctx = {
+		.port = port,
+		.cb = cb,
+		.data = data,
+	};
 	u16 fid;
 	int err;
 

@@ -2184,25 +2230,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	/* Dump VLANs' Filtering Information Databases */
-	vlan.vid = mv88e6xxx_max_vid(chip);
-	vlan.valid = false;
-
-	do {
-		err = mv88e6xxx_vtu_getnext(chip, &vlan);
-		if (err)
-			return err;
-
-		if (!vlan.valid)
-			break;
-
-		err = mv88e6xxx_port_db_dump_fid(chip, vlan.fid, vlan.vid, port,
-						 cb, data);
-		if (err)
-			return err;
-	} while (vlan.vid < mv88e6xxx_max_vid(chip));
-
-	return err;
+	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_db_dump_vlan, &ctx);
 }
 
 static int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,

-- 
2.25.1

Allow a user to control automatic learning per port.

Many chips have an explicit "LearningDisable"-bit that can be used for
this, but we opt for setting/clearing the PAV instead, as it works on
all devices at least as far back as 6083.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 29 +++++++++++++++++++++--------
 drivers/net/dsa/mv88e6xxx/port.c | 21 +++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h |  2 ++
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 01e4ac32d1e5..48e65f22641e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -2689,15 +2689,20 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 			return err;
 	}
 
-	/* Port Association Vector: when learning source addresses
-	 * of packets, add the address to the address database using
-	 * a port bitmap that has only the bit for this port set and
-	 * the other bits clear.
+	/* Port Association Vector: disable automatic address learning
+	 * on all user ports since they start out in standalone
+	 * mode. When joining a bridge, learning will be configured to
+	 * match the bridge port settings. Enable learning on all
+	 * DSA/CPU ports. NOTE: FROM_CPU frames always bypass the
+	 * learning process.
+	 *
+	 * Disable HoldAt1, IntOnAgeOut, LockedPort, IgnoreWrongData,
+	 * and RefreshLocked. I.e. setup standard automatic learning.
 	 */
-	reg = 1 << port;
-	/* Disable learning for CPU port */
-	if (dsa_is_cpu_port(ds, port))
+	if (dsa_is_user_port(ds, port))
 		reg = 0;
+	else
+		reg = 1 << port;
 
 	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
 				   reg);

@@ -5426,7 +5431,7 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	const struct mv88e6xxx_ops *ops;
 
-	if (flags.mask & ~(BR_FLOOD | BR_MCAST_FLOOD))
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
 		return -EINVAL;
 
 	ops = chip->info->ops;

@@ -5449,6 +5454,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 
 	mv88e6xxx_reg_lock(chip);
 
+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+
 	if (flags.mask & BR_FLOOD) {
 		bool unicast = !!(flags.val & BR_FLOOD);
 

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 4561f289ab76..d716cd61b6c6 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c

@@ -1171,6 +1171,27 @@ int mv88e6097_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port)
 				    0x0001);
 }
 
+/* Offset 0x0B: Port Association Vector */
+
+int mv88e6xxx_port_set_assoc_vector(struct mv88e6xxx_chip *chip, int port,
+				    u16 pav)
+{
+	u16 reg, mask;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
+				  &reg);
+	if (err)
+		return err;
+
+	mask = GENMASK(mv88e6xxx_num_ports(chip), 0);
+	reg &= ~mask;
+	reg |= pav & mask;
+
+	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
+				    reg);
+}
+
 /* Offset 0x0C: Port ATU Control */
 
 int mv88e6xxx_port_disable_learn_limit(struct mv88e6xxx_chip *chip, int port)

diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index e6d0eaa6aa1d..635b6571a0e9 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h

@@ -361,6 +361,8 @@ int mv88e6165_port_set_jumbo_size(struct mv88e6xxx_chip *chip, int port,
 				  size_t size);
 int mv88e6095_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port);
 int mv88e6097_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port);
+int mv88e6xxx_port_set_assoc_vector(struct mv88e6xxx_chip *chip, int port,
+				    u16 pav);
 int mv88e6097_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in,
 			       u8 out);
 int mv88e6390_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in,

-- 
2.25.1

These switches have two modes of classifying broadcast:

1. Broadcast is multicast.
2. Broadcast is its own unique thing that is always flooded
   everywhere.

This driver uses the first option, making sure to load the broadcast
address into all active databases. Because of this, we can support
per-port broadcast flooding by (1) making sure to only set the subset
of ports that have it enabled whenever joining a new bridge or VLAN,
and (2) by updating all active databases whenever the setting is
changed on a port.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 68 +++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 48e65f22641e..e6987c501fb7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1950,6 +1950,18 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	int err;
 
 	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		struct dsa_port *dp = dsa_to_port(chip->ds, port);
+
+		if (dsa_is_unused_port(chip->ds, port))
+			continue;
+
+		if (dsa_is_user_port(chip->ds, port) && dp->bridge_dev &&
+		    !br_port_flag_is_set(dp->slave, BR_BCAST_FLOOD))
+			/* Skip bridged user ports where broadcast
+			 * flooding is disabled.
+			 */
+			continue;
+
 		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
 		if (err)
 			return err;

@@ -1958,6 +1970,51 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	return 0;
 }
 
+struct mv88e6xxx_port_broadcast_sync_ctx {
+	int port;
+	bool flood;
+};
+
+static int
+mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
+				   const struct mv88e6xxx_vtu_entry *vlan,
+				   void *_ctx)
+{
+	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+	struct mv88e6xxx_port_broadcast_sync_ctx *ctx = _ctx;
+	u8 state;
+
+	if (ctx->flood)
+		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC;
+	else
+		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_UNUSED;
+
+	return mv88e6xxx_port_db_load_purge(chip, ctx->port, broadcast,
+					    vlan->vid, state);
+}
+
+static int mv88e6xxx_port_broadcast_sync(struct mv88e6xxx_chip *chip, int port,
+					 bool flood)
+{
+	struct mv88e6xxx_port_broadcast_sync_ctx ctx = {
+		.port = port,
+		.flood = flood,
+	};
+	struct mv88e6xxx_vtu_entry vid0 = {
+		.vid = 0,
+	};
+	int err;
+
+	/* Update the port's private database... */
+	err = mv88e6xxx_port_broadcast_sync_vlan(chip, &vid0, &ctx);
+	if (err)
+		return err;
+
+	/* ...and the database for all VLANs. */
+	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_broadcast_sync_vlan,
+				  &ctx);
+}
+
 static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 				    u16 vid, u8 member, bool warn)
 {

@@ -5431,7 +5488,8 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	const struct mv88e6xxx_ops *ops;
 
-	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
+			   BR_BCAST_FLOOD))
 		return -EINVAL;
 
 	ops = chip->info->ops;

@@ -5480,6 +5538,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 			goto out;
 	}
 
+	if (flags.mask & BR_BCAST_FLOOD) {
+		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
+
+		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
+		if (err)
+			goto out;
+	}
+
 out:
 	mv88e6xxx_reg_unlock(chip);
 

-- 
2.25.1

In accordance with the comment in dsa_port_bridge_leave, standalone
ports shall be configured to flood all types of traffic. This change
aligns the mv88e6xxx driver with that policy.

Previously a standalone port would initially not egress any unknown
traffic, but after joining and then leaving a bridge, it would.

This does not matter that much since we only ever send FROM_CPUs on
standalone ports, but it seems prudent to make sure that the initial
values match those that are applied after a bridging/unbridging cycle.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 86027e98d83d..01e4ac32d1e5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -2457,19 +2457,15 @@ static int mv88e6xxx_setup_message_port(struct mv88e6xxx_chip *chip, int port)
 
 static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 {
-	struct dsa_switch *ds = chip->ds;
-	bool flood;
 	int err;
 
-	/* Upstream ports flood frames with unknown unicast or multicast DA */
-	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
 	if (chip->info->ops->port_set_ucast_flood) {
-		err = chip->info->ops->port_set_ucast_flood(chip, port, flood);
+		err = chip->info->ops->port_set_ucast_flood(chip, port, true);
 		if (err)
 			return err;
 	}
 	if (chip->info->ops->port_set_mcast_flood) {
-		err = chip->info->ops->port_set_mcast_flood(chip, port, flood);
+		err = chip->info->ops->port_set_mcast_flood(chip, port, true);
 		if (err)
 			return err;
 	}

-- 
2.25.1

The hardware has a somewhat quirky protocol for reading out the VTU
entry for a particular VID. But there is no reason why we cannot
create a better API for ourselves in the driver.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 45 ++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c70d4b7db4f5..86027e98d83d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1472,13 +1472,23 @@ static int mv88e6xxx_vtu_setup(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_vtu_flush(chip);
 }
 
-static int mv88e6xxx_vtu_getnext(struct mv88e6xxx_chip *chip,
-				 struct mv88e6xxx_vtu_entry *entry)
+static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
+			     struct mv88e6xxx_vtu_entry *entry)
 {
+	int err;
+
 	if (!chip->info->ops->vtu_getnext)
 		return -EOPNOTSUPP;
 
-	return chip->info->ops->vtu_getnext(chip, entry);
+	entry->vid = vid - 1;
+	entry->valid = false;
+
+	err = chip->info->ops->vtu_getnext(chip, entry);
+
+	if (entry->vid != vid)
+		entry->valid = false;
+
+	return err;
 }
 
 static int mv88e6xxx_vtu_walk(struct mv88e6xxx_chip *chip,

@@ -1585,19 +1595,13 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		return 0;
 
-	vlan.vid = vid - 1;
-	vlan.valid = false;
-
-	err = mv88e6xxx_vtu_getnext(chip, &vlan);
+	err = mv88e6xxx_vtu_get(chip, vid, &vlan);
 	if (err)
 		return err;
 
 	if (!vlan.valid)
 		return 0;
 
-	if (vlan.vid != vid)
-		return 0;
-
 	for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) {
 		if (dsa_is_dsa_port(ds, i) || dsa_is_cpu_port(ds, i))
 			continue;

@@ -1679,15 +1683,12 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 		if (err)
 			return err;
 	} else {
-		vlan.vid = vid - 1;
-		vlan.valid = false;
-
-		err = mv88e6xxx_vtu_getnext(chip, &vlan);
+		err = mv88e6xxx_vtu_get(chip, vid, &vlan);
 		if (err)
 			return err;
 
 		/* switchdev expects -EOPNOTSUPP to honor software VLANs */
-		if (vlan.vid != vid || !vlan.valid)
+		if (!vlan.valid)
 			return -EOPNOTSUPP;
 
 		fid = vlan.fid;

@@ -1964,14 +1965,11 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 	struct mv88e6xxx_vtu_entry vlan;
 	int i, err;
 
-	vlan.vid = vid - 1;
-	vlan.valid = false;
-
-	err = mv88e6xxx_vtu_getnext(chip, &vlan);
+	err = mv88e6xxx_vtu_get(chip, vid, &vlan);
 	if (err)
 		return err;
 
-	if (vlan.vid != vid || !vlan.valid) {
+	if (!vlan.valid) {
 		memset(&vlan, 0, sizeof(vlan));
 
 		err = mv88e6xxx_atu_new(chip, &vlan.fid);

@@ -2067,17 +2065,14 @@ static int mv88e6xxx_port_vlan_leave(struct mv88e6xxx_chip *chip,
 	if (!vid)
 		return -EOPNOTSUPP;
 
-	vlan.vid = vid - 1;
-	vlan.valid = false;
-
-	err = mv88e6xxx_vtu_getnext(chip, &vlan);
+	err = mv88e6xxx_vtu_get(chip, vid, &vlan);
 	if (err)
 		return err;
 
 	/* If the VLAN doesn't exist in hardware or the port isn't a member,
 	 * tell switchdev that this VLAN is likely handled in software.
 	 */
-	if (vlan.vid != vid || !vlan.valid ||
+	if (!vlan.valid ||
 	    vlan.member[port] == MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_NON_MEMBER)
 		return -EOPNOTSUPP;
 

-- 
2.25.1

On Mon, Mar 15, 2021 at 10:13:56PM +0100, Tobias Waldekranz wrote:

Can the mv88e6xxx_port_db_dump_fid(VLAN 0) located above this call be
covered by the same mv88e6xxx_vtu_walk?

On Mon, Mar 15, 2021 at 10:13:57PM +0100, Tobias Waldekranz wrote:

+static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
+			     struct mv88e6xxx_vtu_entry *entry)
 {
+	int err;
+
 	if (!chip->info->ops->vtu_getnext)
 		return -EOPNOTSUPP;
 
-	return chip->info->ops->vtu_getnext(chip, entry);
+	entry->vid = vid - 1;

Should the vtu_get API not work with vid 0 as well? Shouldn't we
initialize entry->vid to mv88e6xxx_max_vid(chip) in that case?

+	entry->valid = false;
+
+	err = chip->info->ops->vtu_getnext(chip, entry);
+
+	if (entry->vid != vid)
+		entry->valid = false;
+
+	return err;
 }

On Mon, Mar 15, 2021 at 10:13:58PM +0100, Tobias Waldekranz wrote:

In accordance with the comment in dsa_port_bridge_leave, standalone
ports shall be configured to flood all types of traffic. This change
aligns the mv88e6xxx driver with that policy.

Previously a standalone port would initially not egress any unknown
traffic, but after joining and then leaving a bridge, it would.

This does not matter that much since we only ever send FROM_CPUs on
standalone ports, but it seems prudent to make sure that the initial
values match those that are applied after a bridging/unbridging cycle.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

Can this be refactored to use mv88e6xxx_port_set_assoc_vector too?

mv88e6xxx_num_ports(chip) - 1, maybe?

On Mon, Mar 15, 2021 at 10:14:00PM +0100, Tobias Waldekranz wrote:

What if dp->slave is not the bridge port, but a LAG? br_port_flag_is_set
will return false.

Speaking of, shouldn't mv88e6xxx_port_vlan_join also be called from
mv88e6xxx_port_bridge_join somehow, or are we waiting for the bridge
facility to replay VLANs added to the LAG when we emit the offload
notification for it?

MAC addresses are usually defined as unsigned char[ETH_ALEN]. You can
also use eth_broadcast_addr(broadcast) for initialization.

On Tue, Mar 16, 2021 at 10:42, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 15, 2021 at 10:13:56PM +0100, Tobias Waldekranz wrote:

quoted

@@ -2184,25 +2230,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	/* Dump VLANs' Filtering Information Databases */
-	vlan.vid = mv88e6xxx_max_vid(chip);
-	vlan.valid = false;
-
-	do {
-		err = mv88e6xxx_vtu_getnext(chip, &vlan);
-		if (err)
-			return err;
-
-		if (!vlan.valid)
-			break;
-
-		err = mv88e6xxx_port_db_dump_fid(chip, vlan.fid, vlan.vid, port,
-						 cb, data);
-		if (err)
-			return err;
-	} while (vlan.vid < mv88e6xxx_max_vid(chip));
-
-	return err;
+	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_db_dump_vlan, &ctx);
 }

Can the mv88e6xxx_port_db_dump_fid(VLAN 0) located above this call be
covered by the same mv88e6xxx_vtu_walk?

The port's default default FID does not belong to any VLAN, so it is
never loaded in the VTU. That is why it handled separately. So, no :)

On Tue, Mar 16, 2021 at 11:17, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 15, 2021 at 10:13:57PM +0100, Tobias Waldekranz wrote:

quoted

+static int mv88e6xxx_vtu_get(struct mv88e6xxx_chip *chip, u16 vid,
+			     struct mv88e6xxx_vtu_entry *entry)
 {
+	int err;
+
 	if (!chip->info->ops->vtu_getnext)
 		return -EOPNOTSUPP;
 
-	return chip->info->ops->vtu_getnext(chip, entry);
+	entry->vid = vid - 1;

Should the vtu_get API not work with vid 0 as well? Shouldn't we
initialize entry->vid to mv88e6xxx_max_vid(chip) in that case?

Good catch. We never load VID 0 to the VTU today, but this function
should be ready for it, if that ever changes. Fixing in v2.

quoted

+	entry->valid = false;
+
+	err = chip->info->ops->vtu_getnext(chip, entry);
+
+	if (entry->vid != vid)
+		entry->valid = false;
+
+	return err;
 }

On Tue, Mar 16, 2021 at 11:27, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

quoted

Allow a user to control automatic learning per port.

Many chips have an explicit "LearningDisable"-bit that can be used for
this, but we opt for setting/clearing the PAV instead, as it works on
all devices at least as far back as 6083.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 29 +++++++++++++++++++++--------
 drivers/net/dsa/mv88e6xxx/port.c | 21 +++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h |  2 ++
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 01e4ac32d1e5..48e65f22641e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -2689,15 +2689,20 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
 			return err;
 	}
 
-	/* Port Association Vector: when learning source addresses
-	 * of packets, add the address to the address database using
-	 * a port bitmap that has only the bit for this port set and
-	 * the other bits clear.
+	/* Port Association Vector: disable automatic address learning
+	 * on all user ports since they start out in standalone
+	 * mode. When joining a bridge, learning will be configured to
+	 * match the bridge port settings. Enable learning on all
+	 * DSA/CPU ports. NOTE: FROM_CPU frames always bypass the
+	 * learning process.
+	 *
+	 * Disable HoldAt1, IntOnAgeOut, LockedPort, IgnoreWrongData,
+	 * and RefreshLocked. I.e. setup standard automatic learning.
 	 */
-	reg = 1 << port;
-	/* Disable learning for CPU port */
-	if (dsa_is_cpu_port(ds, port))
+	if (dsa_is_user_port(ds, port))
 		reg = 0;
+	else
+		reg = 1 << port;
 
 	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
 				   reg);

Can this be refactored to use mv88e6xxx_port_set_assoc_vector too?

That was my initial thought as well. But this write also ensures that
the other learning related settings are in a known state. So I settled
for leaving the raw write, but I made sure to document that we depend on
the values of the other flags (second paragraph).

quoted

@@ -5426,7 +5431,7 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	const struct mv88e6xxx_ops *ops;
 
-	if (flags.mask & ~(BR_FLOOD | BR_MCAST_FLOOD))
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
 		return -EINVAL;
 
 	ops = chip->info->ops;

@@ -5449,6 +5454,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 
 	mv88e6xxx_reg_lock(chip);
 
+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+
 	if (flags.mask & BR_FLOOD) {
 		bool unicast = !!(flags.val & BR_FLOOD);
 

diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 4561f289ab76..d716cd61b6c6 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c

@@ -1171,6 +1171,27 @@ int mv88e6097_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port)
 				    0x0001);
 }
 
+/* Offset 0x0B: Port Association Vector */
+
+int mv88e6xxx_port_set_assoc_vector(struct mv88e6xxx_chip *chip, int port,
+				    u16 pav)
+{
+	u16 reg, mask;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
+				  &reg);
+	if (err)
+		return err;
+
+	mask = GENMASK(mv88e6xxx_num_ports(chip), 0);

mv88e6xxx_num_ports(chip) - 1, maybe?

Ahh thanks. This made me think that there should be a helper for this;
turns out Vivien added mv88e6xxx_port_mask four years ago :)

quoted

+	reg &= ~mask;
+	reg |= pav & mask;
+
+	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_ASSOC_VECTOR,
+				    reg);
+}
+
 /* Offset 0x0C: Port ATU Control */
 
 int mv88e6xxx_port_disable_learn_limit(struct mv88e6xxx_chip *chip, int port)

diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index e6d0eaa6aa1d..635b6571a0e9 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h

@@ -361,6 +361,8 @@ int mv88e6165_port_set_jumbo_size(struct mv88e6xxx_chip *chip, int port,
 				  size_t size);
 int mv88e6095_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port);
 int mv88e6097_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port);
+int mv88e6xxx_port_set_assoc_vector(struct mv88e6xxx_chip *chip, int port,
+				    u16 pav);
 int mv88e6097_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in,
 			       u8 out);
 int mv88e6390_port_pause_limit(struct mv88e6xxx_chip *chip, int port, u8 in,

-- 
2.25.1

On Tue, Mar 16, 2021 at 11:39, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 15, 2021 at 10:14:00PM +0100, Tobias Waldekranz wrote:

quoted

These switches have two modes of classifying broadcast:

1. Broadcast is multicast.
2. Broadcast is its own unique thing that is always flooded
   everywhere.

This driver uses the first option, making sure to load the broadcast
address into all active databases. Because of this, we can support
per-port broadcast flooding by (1) making sure to only set the subset
of ports that have it enabled whenever joining a new bridge or VLAN,
and (2) by updating all active databases whenever the setting is
changed on a port.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 68 +++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 48e65f22641e..e6987c501fb7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1950,6 +1950,18 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	int err;
 
 	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		struct dsa_port *dp = dsa_to_port(chip->ds, port);
+
+		if (dsa_is_unused_port(chip->ds, port))
+			continue;
+
+		if (dsa_is_user_port(chip->ds, port) && dp->bridge_dev &&
+		    !br_port_flag_is_set(dp->slave, BR_BCAST_FLOOD))

What if dp->slave is not the bridge port, but a LAG? br_port_flag_is_set
will return false.

Good point. I see two ways forward:

- My first idea was to cache a vector per switch that would act as the
  template when creating a new entry. This avoids having the driver
  layer knowing about stacked netdevs etc. But I think that Andrew is
  generally opposed to caching?

- Add a new helper at the dsa layer that takes a dp and returns the
  netdev that is attached to the bridge, if any:

  struct net_device *dsa_port_to_bridge_port(struct dsa_port *dp)

Any preference or other ideas?

Speaking of, shouldn't mv88e6xxx_port_vlan_join also be called from
mv88e6xxx_port_bridge_join somehow, or are we waiting for the bridge
facility to replay VLANs added to the LAG when we emit the offload
notification for it?

I do not think so. VLANs are always added via the .port_vlan_add
callback, no?

Potentially the bridge is of the non-filtering variety, so it could be
that no VLANs are ever added.

Or do you mean (the most confusingly named feature Marvell LinkStreet
devices) port-based VLANs? Those are setup on a bridge join via
mv88e6xxx_port_vlan_map and mv88e6xxx_pvt_map.

quoted

+			/* Skip bridged user ports where broadcast
+			 * flooding is disabled.
+			 */
+			continue;
+
 		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
 		if (err)
 			return err;

@@ -1958,6 +1970,51 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	return 0;
 }
 
+struct mv88e6xxx_port_broadcast_sync_ctx {
+	int port;
+	bool flood;
+};
+
+static int
+mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
+				   const struct mv88e6xxx_vtu_entry *vlan,
+				   void *_ctx)
+{
+	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

MAC addresses are usually defined as unsigned char[ETH_ALEN]. You can
also use eth_broadcast_addr(broadcast) for initialization.

I was going for uniformity with mv88e6xxx_port_add_broadcast. But I will
add a clean-up commit that fixes the existing code first, and then adds
this definition in the proper way.

quoted

+	struct mv88e6xxx_port_broadcast_sync_ctx *ctx = _ctx;
+	u8 state;
+
+	if (ctx->flood)
+		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC;
+	else
+		state = MV88E6XXX_G1_ATU_DATA_STATE_MC_UNUSED;
+
+	return mv88e6xxx_port_db_load_purge(chip, ctx->port, broadcast,
+					    vlan->vid, state);
+}
+
+static int mv88e6xxx_port_broadcast_sync(struct mv88e6xxx_chip *chip, int port,
+					 bool flood)
+{
+	struct mv88e6xxx_port_broadcast_sync_ctx ctx = {
+		.port = port,
+		.flood = flood,
+	};
+	struct mv88e6xxx_vtu_entry vid0 = {
+		.vid = 0,
+	};
+	int err;
+
+	/* Update the port's private database... */
+	err = mv88e6xxx_port_broadcast_sync_vlan(chip, &vid0, &ctx);
+	if (err)
+		return err;
+
+	/* ...and the database for all VLANs. */
+	return mv88e6xxx_vtu_walk(chip, mv88e6xxx_port_broadcast_sync_vlan,
+				  &ctx);
+}
+
 static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port,
 				    u16 vid, u8 member, bool warn)
 {

@@ -5431,7 +5488,8 @@ static int mv88e6xxx_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	const struct mv88e6xxx_ops *ops;
 
-	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD))
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
+			   BR_BCAST_FLOOD))
 		return -EINVAL;
 
 	ops = chip->info->ops;

@@ -5480,6 +5538,14 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 			goto out;
 	}
 
+	if (flags.mask & BR_BCAST_FLOOD) {
+		bool broadcast = !!(flags.val & BR_BCAST_FLOOD);
+
+		err = mv88e6xxx_port_broadcast_sync(chip, port, broadcast);
+		if (err)
+			goto out;
+	}
+
 out:
 	mv88e6xxx_reg_unlock(chip);
 

-- 
2.25.1

On Wed, Mar 17, 2021 at 12:14:18PM +0100, Tobias Waldekranz wrote:

Good point. I see two ways forward:

- My first idea was to cache a vector per switch that would act as the
  template when creating a new entry. This avoids having the driver
  layer knowing about stacked netdevs etc. But I think that Andrew is
  generally opposed to caching?

- Add a new helper at the dsa layer that takes a dp and returns the
  netdev that is attached to the bridge, if any:

  struct net_device *dsa_port_to_bridge_port(struct dsa_port *dp)

Any preference or other ideas?

I vote for dsa_port_to_bridge_port. We'll need it anyway for my software
bridging series with sandwiched net devices.

quoted

Speaking of, shouldn't mv88e6xxx_port_vlan_join also be called from
mv88e6xxx_port_bridge_join somehow, or are we waiting for the bridge
facility to replay VLANs added to the LAG when we emit the offload
notification for it?

I do not think so. VLANs are always added via the .port_vlan_add
callback, no?

I got things mixed up in my head while thinking about it.
Yes, of course, the bridge pvid is added via .port_vlan_add as soon as
the port joins the bridge.
What I meant to say is that this sequence of events:

ip link add br0 type bridge
ip link add bond0 type bonding
ip link set bond0 master br0
ip link set lan0 master bond0

will cause lan0 to miss all sorts of information about the bridge port
switchdev objects, including the pvid. My patch series for
SWITCHDEV_BRPORT_OFFLOADED catches this case and asks for a replay.

Potentially the bridge is of the non-filtering variety, so it could be
that no VLANs are ever added.

That will not happen unless you set ds->configure_vlan_while_not_filtering = false,
which mv88e6xxx no longer does, so we're in the clear.

Or do you mean (the most confusingly named feature Marvell LinkStreet
devices) port-based VLANs? Those are setup on a bridge join via
mv88e6xxx_port_vlan_map and mv88e6xxx_pvt_map.

Nope, I wasn't thinking about PVTs.

quoted

quoted

+			/* Skip bridged user ports where broadcast
+			 * flooding is disabled.
+			 */
+			continue;
+
 		err = mv88e6xxx_port_add_broadcast(chip, port, vid);
 		if (err)
 			return err;

@@ -1958,6 +1970,51 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	return 0;
 }
 
+struct mv88e6xxx_port_broadcast_sync_ctx {
+	int port;
+	bool flood;
+};
+
+static int
+mv88e6xxx_port_broadcast_sync_vlan(struct mv88e6xxx_chip *chip,
+				   const struct mv88e6xxx_vtu_entry *vlan,
+				   void *_ctx)
+{
+	const char broadcast[6] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

MAC addresses are usually defined as unsigned char[ETH_ALEN]. You can
also use eth_broadcast_addr(broadcast) for initialization.

I was going for uniformity with mv88e6xxx_port_add_broadcast. But I will
add a clean-up commit that fixes the existing code first, and then adds
this definition in the proper way.

Thanks.

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+

If flags.val & BR_LEARNING is off, could you please call
mv88e6xxx_port_fast_age too? This ensures that existing ATU entries that
were automatically learned are purged.

On Wed, Mar 17, 2021 at 16:12, Vladimir Oltean [off-list ref] wrote:

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

quoted

+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+

If flags.val & BR_LEARNING is off, could you please call
mv88e6xxx_port_fast_age too? This ensures that existing ATU entries that
were automatically learned are purged.

This opened up another can of worms.

It turns out that the hardware is incapable of fast aging a LAG. I can
see two workarounds. Both are awful in their own special ways:

1. Iterate over all entries of all FIDs in the ATU, removing all
   matching dynamic entries. This will accomplish the same thing, but it
   is a very expensive operation, and having that in the control path of
   STP does not feel quite right.

2. Flushing all dynamic entries in the entire ATU. Fast, but obviously
   results in a period of lots of flooded packets.

Any opinion on which approach you think would hurt less? Or, even
better, if there is a third way that I have missed.

For this series I am leaning towards making mv88e6xxx_port_fast_age a
no-op for LAG ports. We could then come back to this problem when we add
other LAG-related FDB operations like static FDB entries. Acceptable?

On Wed, Mar 17, 2021 at 07:45:46PM +0100, Tobias Waldekranz wrote:

On Wed, Mar 17, 2021 at 16:12, Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

quoted

+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+

If flags.val & BR_LEARNING is off, could you please call
mv88e6xxx_port_fast_age too? This ensures that existing ATU entries that
were automatically learned are purged.

This opened up another can of worms.

It turns out that the hardware is incapable of fast aging a LAG.

You sound pretty definitive about it, do you know why?

I can see two workarounds. Both are awful in their own special ways:

1. Iterate over all entries of all FIDs in the ATU, removing all
   matching dynamic entries. This will accomplish the same thing, but it
   is a very expensive operation, and having that in the control path of
   STP does not feel quite right.

When does it ever feel right? :)

I think of it like a faster 'bridge fdb' command (since 'bridge fdb'
traverses the ATU super inefficiently, it dumps the whole table for each
port).

On my system with 24 mv88e6xxx ports, 'time bridge fdb' takes around 34
seconds. So that means a 'slow age' will take around 1.4 seconds for a
single LAG.

On the other hand, on my system with 7 sja1105 ports, I have no choice
but to do slow ageing - the hardware simply doesn't have the concept of
'fast ageing'. There, 'time bridge fdb' returns 1.781s, so I expect a
slow age would take around 0.25 seconds. Of course I'm not happy about
it, but I think I'll bite the bullet.

2. Flushing all dynamic entries in the entire ATU. Fast, but obviously
   results in a period of lots of flooded packets.

This one seems like an overreaction to me. Would that even solve the
problem? Couly you destroy and re-create the trunk?

Any opinion on which approach you think would hurt less? Or, even
better, if there is a third way that I have missed.

For this series I am leaning towards making mv88e6xxx_port_fast_age a
no-op for LAG ports. We could then come back to this problem when we add
other LAG-related FDB operations like static FDB entries. Acceptable?

Yeah, I guess that's fair.

On Wed, Mar 17, 2021 at 21:29, Vladimir Oltean [off-list ref] wrote:

On Wed, Mar 17, 2021 at 07:45:46PM +0100, Tobias Waldekranz wrote:

quoted

On Wed, Mar 17, 2021 at 16:12, Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Mar 15, 2021 at 10:13:59PM +0100, Tobias Waldekranz wrote:

quoted

+	if (flags.mask & BR_LEARNING) {
+		u16 pav = (flags.val & BR_LEARNING) ? (1 << port) : 0;
+
+		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
+		if (err)
+			goto out;
+	}
+

If flags.val & BR_LEARNING is off, could you please call
mv88e6xxx_port_fast_age too? This ensures that existing ATU entries that
were automatically learned are purged.

This opened up another can of worms.

It turns out that the hardware is incapable of fast aging a LAG.

You sound pretty definitive about it, do you know why?

The big note saying "Entries associated with a LAG cannot be moved or
removed using these commands" was the first clue :)

This is pure speculation on my part.

In the first iterations of this family of devices (before it was bought
by Marvell), cross-chip LAGs where not supported. You would simply set
multiple bits in the PAV for all LAG members to associate stations with
the LAG. In that scenario you can fast-age a LAG by fast-aging each
individual port.

Later, cross-chip LAGs where added, and enough support was bolted on to
the ATU to support automatic learning, but not enough to support
fast-aging.

quoted

I can see two workarounds. Both are awful in their own special ways:

1. Iterate over all entries of all FIDs in the ATU, removing all
   matching dynamic entries. This will accomplish the same thing, but it
   is a very expensive operation, and having that in the control path of
   STP does not feel quite right.

When does it ever feel right? :)

I think of it like a faster 'bridge fdb' command (since 'bridge fdb'
traverses the ATU super inefficiently, it dumps the whole table for each
port).

On my system with 24 mv88e6xxx ports, 'time bridge fdb' takes around 34
seconds. So that means a 'slow age' will take around 1.4 seconds for a
single LAG.

Well, it also scales linearly with the number of active entries in the
ATU. Here are some times for "bridge fdb" on my system with a single
6390X:

Entries    Time
      1   0.17s
     10   0.48s
    100   3.20s
    200   6.24s
   1000  31.82s

(I used trafgen to generate broadcasts with random SAs)

Then you have to consider that you are not simply walking the ATU, you
also have to write back entries whenever you come across one attached to
the LAG you are fast-aging.

On the other hand, on my system with 7 sja1105 ports, I have no choice
but to do slow ageing - the hardware simply doesn't have the concept of
'fast ageing'. There, 'time bridge fdb' returns 1.781s, so I expect a
slow age would take around 0.25 seconds. Of course I'm not happy about
it, but I think I'll bite the bullet.

quoted

2. Flushing all dynamic entries in the entire ATU. Fast, but obviously
   results in a period of lots of flooded packets.

This one seems like an overreaction to me. Would that even solve the
problem? Couly you destroy and re-create the trunk?

It would make sure that no entries lingered on the port in question,
absolutely. Unfortunately that would also be true for all other ports :)

Adding/removing a LAG in hardware has no connection to ATU entries
unfortunately.

quoted

Any opinion on which approach you think would hurt less? Or, even
better, if there is a third way that I have missed.

For this series I am leaning towards making mv88e6xxx_port_fast_age a
no-op for LAG ports. We could then come back to this problem when we add
other LAG-related FDB operations like static FDB entries. Acceptable?

Yeah, I guess that's fair.

Great. I will try to put together a v2 tomorrow.

On 3/15/2021 2:13 PM, Tobias Waldekranz wrote:

In accordance with the comment in dsa_port_bridge_leave, standalone
ports shall be configured to flood all types of traffic. This change
aligns the mv88e6xxx driver with that policy.

Previously a standalone port would initially not egress any unknown
traffic, but after joining and then leaving a bridge, it would.

This does not matter that much since we only ever send FROM_CPUs on
standalone ports, but it seems prudent to make sure that the initial
values match those that are applied after a bridging/unbridging cycle.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

On Mon, Mar 15, 2021 at 10:13:56PM +0100, Tobias Waldekranz wrote:

Move the intricacies of correctly iterating over the VTU to a common
implementation.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Mon, Mar 15, 2021 at 10:13:58PM +0100, Tobias Waldekranz wrote:

In accordance with the comment in dsa_port_bridge_leave, standalone
ports shall be configured to flood all types of traffic. This change
aligns the mv88e6xxx driver with that policy.

Previously a standalone port would initially not egress any unknown
traffic, but after joining and then leaving a bridge, it would.

This does not matter that much since we only ever send FROM_CPUs on
standalone ports, but it seems prudent to make sure that the initial
values match those that are applied after a bridging/unbridging cycle.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Wed, Mar 17, 2021 at 12:14:18PM +0100, Tobias Waldekranz wrote:

On Tue, Mar 16, 2021 at 11:39, Vladimir Oltean [off-list ref] wrote:

quoted

On Mon, Mar 15, 2021 at 10:14:00PM +0100, Tobias Waldekranz wrote:

quoted

These switches have two modes of classifying broadcast:

1. Broadcast is multicast.
2. Broadcast is its own unique thing that is always flooded
   everywhere.

This driver uses the first option, making sure to load the broadcast
address into all active databases. Because of this, we can support
per-port broadcast flooding by (1) making sure to only set the subset
of ports that have it enabled whenever joining a new bridge or VLAN,
and (2) by updating all active databases whenever the setting is
changed on a port.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 68 +++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 48e65f22641e..e6987c501fb7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c

@@ -1950,6 +1950,18 @@ static int mv88e6xxx_broadcast_setup(struct mv88e6xxx_chip *chip, u16 vid)
 	int err;
 
 	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		struct dsa_port *dp = dsa_to_port(chip->ds, port);
+
+		if (dsa_is_unused_port(chip->ds, port))
+			continue;
+
+		if (dsa_is_user_port(chip->ds, port) && dp->bridge_dev &&
+		    !br_port_flag_is_set(dp->slave, BR_BCAST_FLOOD))

What if dp->slave is not the bridge port, but a LAG? br_port_flag_is_set
will return false.

Good point. I see two ways forward:

- My first idea was to cache a vector per switch that would act as the
  template when creating a new entry. This avoids having the driver
  layer knowing about stacked netdevs etc. But I think that Andrew is
  generally opposed to caching?

Hi Tobias

What i'm mostly against is dynamic memory allocation. If you can
allocate the space for this vector during probe, i have no problems
with that.

     Andrew