[PATCH net-next 0/5] Fast ageing support for SJA1105 DSA driver

[PATCH net-next 0/5] Fast ageing support for SJA1105 DSA driver

[Vladimir Oltean]

While adding support for flushing dynamically learned FDB entries in the
sja1105 driver, I noticed a few things that could be improved in DSA.
Most notably, drivers could omit a fast age when address learning is
turned off, which might mean that ports leaving a bridge and becoming
standalone could still have FDB entries pointing towards them. Secondly,
when DSA fast ages a port after the 'learning' flag has been turned off,
the software bridge still has the dynamically learned 'master' FDB
entries installed, and those should be deleted too.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
Cc: Alexandra Winter <wintera@linux.ibm.com>
Cc: Julian Wiedmann <jwi@linux.ibm.com>
Cc: Roopa Prabhu <roopa@nvidia.com>
Cc: Nikolay Aleksandrov <nikolay@nvidia.com>

Vladimir Oltean (5):
  net: dsa: centralize fast ageing when address learning is turned off
  net: dsa: don't fast age bridge ports with learning turned off
  net: dsa: flush the dynamic FDB of the software bridge when fast
    ageing a port
  net: dsa: sja1105: rely on DSA core tracking of port learning state
  net: dsa: sja1105: add FDB fast ageing support

 drivers/net/dsa/mv88e6xxx/chip.c       |  7 ---
 drivers/net/dsa/sja1105/sja1105.h      |  1 -
 drivers/net/dsa/sja1105/sja1105_main.c | 73 +++++++++++++++++++-------
 include/net/dsa.h                      |  2 +
 net/dsa/dsa_priv.h                     |  2 +-
 net/dsa/port.c                         | 55 +++++++++++++++++--
 6 files changed, 108 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH net-next 1/5] net: dsa: centralize fast ageing when address learning is turned off

[Vladimir Oltean]

Currently DSA leaves it down to device drivers to fast age the FDB on a
port when address learning is disabled on it. There are 2 reasons for
doing that in the first place:

- when address learning is disabled by user space, through
  IFLA_BRPORT_LEARNING or the brport_attr_learning sysfs, what user
  space typically wants to achieve is to operate in a mode with no
  dynamic FDB entry on that port. But if the port is already up, some
  addresses might have been already learned on it, and it seems silly to
  wait for 5 minutes for them to expire until something useful can be
  done.

- when a port leaves a bridge and becomes standalone, DSA turns off
  address learning on it. This also has the nice side effect of flushing
  the dynamically learned bridge FDB entries on it, which is a good idea
  because standalone ports should not have bridge FDB entries on them.

We let drivers manage fast ageing under this condition because if DSA
were to do it, it would need to track each port's learning state, and
act upon the transition, which it currently doesn't.

But there are 2 reasons why doing it is better after all:

- drivers might get it wrong and not do it (see b53_port_set_learning)

- we would like to flush the dynamic entries from the software bridge
  too, and letting drivers do that would be another pain point

So track the port learning state and trigger a fast age process
automatically within DSA.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c |  7 -------
 include/net/dsa.h                |  1 +
 net/dsa/dsa_priv.h               |  2 +-
 net/dsa/port.c                   | 35 ++++++++++++++++++++++++++++----
 4 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c2c5f1573fe5..c45ca2473743 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5797,7 +5797,6 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 				       struct netlink_ext_ack *extack)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
-	bool do_fast_age = false;
 	int err = -EOPNOTSUPP;
 
 	mv88e6xxx_reg_lock(chip);
@@ -5809,9 +5808,6 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 		err = mv88e6xxx_port_set_assoc_vector(chip, port, pav);
 		if (err)
 			goto out;
-
-		if (!learning)
-			do_fast_age = true;
 	}
 
 	if (flags.mask & BR_FLOOD) {
@@ -5843,9 +5839,6 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
 out:
 	mv88e6xxx_reg_unlock(chip);
 
-	if (do_fast_age)
-		mv88e6xxx_port_fast_age(ds, port);
-
 	return err;
 }
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index d7dc26d316ea..995e9d3f9cfc 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -254,6 +254,7 @@ struct dsa_port {
 	struct device_node	*dn;
 	unsigned int		ageing_time;
 	bool			vlan_filtering;
+	bool			learning;
 	u8			stp_state;
 	struct net_device	*bridge_dev;
 	int			bridge_num;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 8dad40b2cf5c..9575cabd3ec3 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -241,7 +241,7 @@ int dsa_port_host_mdb_del(const struct dsa_port *dp,
 int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
 			      struct switchdev_brport_flags flags,
 			      struct netlink_ext_ack *extack);
-int dsa_port_bridge_flags(const struct dsa_port *dp,
+int dsa_port_bridge_flags(struct dsa_port *dp,
 			  struct switchdev_brport_flags flags,
 			  struct netlink_ext_ack *extack);
 int dsa_port_vlan_add(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ef5e08b09bb7..d6a35a03acd6 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -30,6 +30,16 @@ static int dsa_port_notify(const struct dsa_port *dp, unsigned long e, void *v)
 	return dsa_tree_notify(dp->ds->dst, e, v);
 }
 
+static void dsa_port_fast_age(const struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->port_fast_age)
+		return;
+
+	ds->ops->port_fast_age(ds, dp->index);
+}
+
 int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
 {
 	struct dsa_switch *ds = dp->ds;
@@ -40,7 +50,7 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
 
 	ds->ops->port_stp_state_set(ds, port, state);
 
-	if (do_fast_age && ds->ops->port_fast_age) {
+	if (do_fast_age) {
 		/* Fast age FDB entries or flush appropriate forwarding database
 		 * for the given port, if we are moving it from Learning or
 		 * Forwarding state, to Disabled or Blocking or Listening state.
@@ -54,7 +64,7 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
 		    (state == BR_STATE_DISABLED ||
 		     state == BR_STATE_BLOCKING ||
 		     state == BR_STATE_LISTENING))
-			ds->ops->port_fast_age(ds, port);
+			dsa_port_fast_age(dp);
 	}
 
 	dp->stp_state = state;
@@ -633,16 +643,33 @@ int dsa_port_pre_bridge_flags(const struct dsa_port *dp,
 	return ds->ops->port_pre_bridge_flags(ds, dp->index, flags, extack);
 }
 
-int dsa_port_bridge_flags(const struct dsa_port *dp,
+int dsa_port_bridge_flags(struct dsa_port *dp,
 			  struct switchdev_brport_flags flags,
 			  struct netlink_ext_ack *extack)
 {
 	struct dsa_switch *ds = dp->ds;
+	int err;
 
 	if (!ds->ops->port_bridge_flags)
 		return -EOPNOTSUPP;
 
-	return ds->ops->port_bridge_flags(ds, dp->index, flags, extack);
+	err = ds->ops->port_bridge_flags(ds, dp->index, flags, extack);
+	if (err)
+		return err;
+
+	if (flags.mask & BR_LEARNING) {
+		bool learning = flags.val & BR_LEARNING;
+
+		if (learning == dp->learning)
+			return 0;
+
+		if (dp->learning && !learning)
+			dsa_port_fast_age(dp);
+
+		dp->learning = learning;
+	}
+
+	return 0;
 }
 
 int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu,
-- 
2.25.1

[PATCH net-next 2/5] net: dsa: don't fast age bridge ports with learning turned off

[Vladimir Oltean]

On topology changes, stations that were dynamically learned on ports
that are no longer part of the active topology must be flushed - this is
described by clause "17.11 Updating learned station location information"
of IEEE 802.1D-2004.

However, when address learning on the bridge port is turned off in the
first place, there is nothing to flush, so skip a potentially expensive
operation.

We can finally do this now since DSA is aware of the learning state of
its bridged ports.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index d6a35a03acd6..a4c8d19a76e2 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -50,7 +50,7 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
 
 	ds->ops->port_stp_state_set(ds, port, state);
 
-	if (do_fast_age) {
+	if (do_fast_age && dp->learning) {
 		/* Fast age FDB entries or flush appropriate forwarding database
 		 * for the given port, if we are moving it from Learning or
 		 * Forwarding state, to Disabled or Blocking or Listening state.
-- 
2.25.1

[PATCH net-next 3/5] net: dsa: flush the dynamic FDB of the software bridge when fast ageing a port

[Vladimir Oltean]

Currently, when DSA performs fast ageing on a port, 'bridge fdb' shows
us that the 'self' entries (corresponding to the hardware bridge, as
printed by dsa_slave_fdb_dump) are deleted, but the 'master' entries
(corresponding to the software bridge) aren't.

Indeed, searching through the bridge driver, neither the
brport_attr_learning handler nor the IFLA_BRPORT_LEARNING handler call
br_fdb_delete_by_port. However, br_stp_disable_port does, which is one
of the paths which DSA uses to trigger a fast ageing process anyway.

There is, however, one other very promising caller of
br_fdb_delete_by_port, and that is the bridge driver's handler of the
SWITCHDEV_FDB_FLUSH_TO_BRIDGE atomic notifier. Currently the s390/qeth
HiperSockets card driver is the only user of this.

I can't say I understand that driver's architecture or interaction with
the bridge, but it appears to not be a switchdev driver in the traditional
sense of the word. Nonetheless, the mechanism it provides is a useful
way for DSA to express the fact that it performs fast ageing too, in a
way that does not change the existing behavior for other drivers.

Cc: Alexandra Winter <wintera@linux.ibm.com>
Cc: Julian Wiedmann <jwi@linux.ibm.com>
Cc: Roopa Prabhu <roopa@nvidia.com>
Cc: Nikolay Aleksandrov <nikolay@nvidia.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/port.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/dsa/port.c b/net/dsa/port.c
index a4c8d19a76e2..96a4de67eccb 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -30,6 +30,24 @@ static int dsa_port_notify(const struct dsa_port *dp, unsigned long e, void *v)
 	return dsa_tree_notify(dp->ds->dst, e, v);
 }
 
+static void dsa_port_notify_bridge_fdb_flush(const struct dsa_port *dp)
+{
+	struct net_device *brport_dev = dsa_port_to_bridge_port(dp);
+	struct switchdev_notifier_fdb_info info = {
+		/* flush all VLANs */
+		.vid = 0,
+	};
+
+	/* When the port becomes standalone it has already left the bridge.
+	 * Don't notify the bridge in that case.
+	 */
+	if (!brport_dev)
+		return;
+
+	call_switchdev_notifiers(SWITCHDEV_FDB_FLUSH_TO_BRIDGE,
+				 brport_dev, &info.info, NULL);
+}
+
 static void dsa_port_fast_age(const struct dsa_port *dp)
 {
 	struct dsa_switch *ds = dp->ds;
@@ -38,6 +56,8 @@ static void dsa_port_fast_age(const struct dsa_port *dp)
 		return;
 
 	ds->ops->port_fast_age(ds, dp->index);
+
+	dsa_port_notify_bridge_fdb_flush(dp);
 }
 
 int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age)
-- 
2.25.1

[PATCH net-next 4/5] net: dsa: sja1105: rely on DSA core tracking of port learning state

[Vladimir Oltean]

Now that DSA keeps track of the port learning state, it becomes
superfluous to keep an additional variable with this information in the
sja1105 driver. Remove it.

The DSA core's learning state is present in struct dsa_port *dp.
To avoid the antipattern where we iterate through a DSA switch's
ports and then call dsa_to_port to obtain the "dp" reference (which is
bad because dsa_to_port iterates through the DSA switch tree once
again), just iterate through the dst->ports and operate on those
directly.

The sja1105 had an extra use of priv->learn_ena on non-user ports. DSA
does not touch the learning state of those ports - drivers are free to
do what they wish on them. Mark that information with a comment in
struct dsa_port and let sja1105 set dp->learning for cascade ports.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105.h      |  1 -
 drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++---------------
 include/net/dsa.h                      |  1 +
 3 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 9cd7dbdd7db9..2e899c9f036d 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -233,7 +233,6 @@ struct sja1105_private {
 	phy_interface_t phy_mode[SJA1105_MAX_NUM_PORTS];
 	bool fixed_link[SJA1105_MAX_NUM_PORTS];
 	bool vlan_aware;
-	unsigned long learn_ena;
 	unsigned long ucast_egress_floods;
 	unsigned long bcast_egress_floods;
 	const struct sja1105_info *info;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 241fd25b0b86..87e279be89c9 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -176,7 +176,7 @@ static int sja1105_init_mac_settings(struct sja1105_private *priv)
 	struct sja1105_mac_config_entry *mac;
 	struct dsa_switch *ds = priv->ds;
 	struct sja1105_table *table;
-	int i;
+	struct dsa_port *dp;
 
 	table = &priv->static_config.tables[BLK_IDX_MAC_CONFIG];
 
@@ -195,8 +195,11 @@ static int sja1105_init_mac_settings(struct sja1105_private *priv)
 
 	mac = table->entries;
 
-	for (i = 0; i < ds->num_ports; i++) {
-		mac[i] = default_mac;
+	list_for_each_entry(dp, &ds->dst->ports, list) {
+		if (dp->ds != ds)
+			continue;
+
+		mac[dp->index] = default_mac;
 
 		/* Let sja1105_bridge_stp_state_set() keep address learning
 		 * enabled for the DSA ports. CPU ports use software-assisted
@@ -205,8 +208,8 @@ static int sja1105_init_mac_settings(struct sja1105_private *priv)
 		 * CPU ports in a cross-chip topology if multiple CPU ports
 		 * exist.
 		 */
-		if (dsa_is_dsa_port(ds, i))
-			priv->learn_ena |= BIT(i);
+		if (dsa_port_is_dsa(dp))
+			dp->learning = true;
 	}
 
 	return 0;
@@ -1899,6 +1902,7 @@ static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 static void sja1105_bridge_stp_state_set(struct dsa_switch *ds, int port,
 					 u8 state)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct sja1105_private *priv = ds->priv;
 	struct sja1105_mac_config_entry *mac;
 
@@ -1924,12 +1928,12 @@ static void sja1105_bridge_stp_state_set(struct dsa_switch *ds, int port,
 	case BR_STATE_LEARNING:
 		mac[port].ingress   = true;
 		mac[port].egress    = false;
-		mac[port].dyn_learn = !!(priv->learn_ena & BIT(port));
+		mac[port].dyn_learn = dp->learning;
 		break;
 	case BR_STATE_FORWARDING:
 		mac[port].ingress   = true;
 		mac[port].egress    = true;
-		mac[port].dyn_learn = !!(priv->learn_ena & BIT(port));
+		mac[port].dyn_learn = dp->learning;
 		break;
 	default:
 		dev_err(ds->dev, "invalid STP state: %d\n", state);
@@ -2891,23 +2895,13 @@ static int sja1105_port_set_learning(struct sja1105_private *priv, int port,
 				     bool enabled)
 {
 	struct sja1105_mac_config_entry *mac;
-	int rc;
 
 	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
 
 	mac[port].dyn_learn = enabled;
 
-	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
-					  &mac[port], true);
-	if (rc)
-		return rc;
-
-	if (enabled)
-		priv->learn_ena |= BIT(port);
-	else
-		priv->learn_ena &= ~BIT(port);
-
-	return 0;
+	return sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
+					    &mac[port], true);
 }
 
 static int sja1105_port_ucast_bcast_flood(struct sja1105_private *priv, int to,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 995e9d3f9cfc..0c2cba45fa79 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -254,6 +254,7 @@ struct dsa_port {
 	struct device_node	*dn;
 	unsigned int		ageing_time;
 	bool			vlan_filtering;
+	/* Managed by DSA on user ports and by drivers on CPU and DSA ports */
 	bool			learning;
 	u8			stp_state;
 	struct net_device	*bridge_dev;
-- 
2.25.1

[PATCH net-next 5/5] net: dsa: sja1105: add FDB fast ageing support

[Vladimir Oltean]

Delete the dynamically learned FDB entries when the STP state changes
and when address learning is disabled.

On sja1105 there is no shorthand SPI command for this, so we need to
walk through the entire FDB to delete.

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

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 87e279be89c9..6a52db1ef24c 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1794,6 +1794,46 @@ static int sja1105_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void sja1105_fast_age(struct dsa_switch *ds, int port)
+{
+	struct sja1105_private *priv = ds->priv;
+	int i;
+
+	for (i = 0; i < SJA1105_MAX_L2_LOOKUP_COUNT; i++) {
+		struct sja1105_l2_lookup_entry l2_lookup = {0};
+		u8 macaddr[ETH_ALEN];
+		int rc;
+
+		rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
+						 i, &l2_lookup);
+		/* No fdb entry at i, not an issue */
+		if (rc == -ENOENT)
+			continue;
+		if (rc) {
+			dev_err(ds->dev, "Failed to read FDB: %pe\n",
+				ERR_PTR(rc));
+			return;
+		}
+
+		if (!(l2_lookup.destports & BIT(port)))
+			continue;
+
+		/* Don't delete static FDB entries */
+		if (l2_lookup.lockeds)
+			continue;
+
+		u64_to_ether_addr(l2_lookup.macaddr, macaddr);
+
+		rc = sja1105_fdb_del(ds, port, macaddr, l2_lookup.vlanid);
+		if (rc) {
+			dev_err(ds->dev,
+				"Failed to delete FDB entry %pM vid %lld: %pe\n",
+				macaddr, l2_lookup.vlanid, ERR_PTR(rc));
+			return;
+		}
+	}
+}
+
 static int sja1105_mdb_add(struct dsa_switch *ds, int port,
 			   const struct switchdev_obj_port_mdb *mdb)
 {
@@ -3036,6 +3076,7 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.port_fdb_dump		= sja1105_fdb_dump,
 	.port_fdb_add		= sja1105_fdb_add,
 	.port_fdb_del		= sja1105_fdb_del,
+	.port_fast_age		= sja1105_fast_age,
 	.port_bridge_join	= sja1105_bridge_join,
 	.port_bridge_leave	= sja1105_bridge_leave,
 	.port_pre_bridge_flags	= sja1105_port_pre_bridge_flags,
-- 
2.25.1

Re: [PATCH net-next 0/5] Fast ageing support for SJA1105 DSA driver

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Sun,  8 Aug 2021 17:35:22 +0300 you wrote:
> While adding support for flushing dynamically learned FDB entries in the
> sja1105 driver, I noticed a few things that could be improved in DSA.
> Most notably, drivers could omit a fast age when address learning is
> turned off, which might mean that ports leaving a bridge and becoming
> standalone could still have FDB entries pointing towards them. Secondly,
> when DSA fast ages a port after the 'learning' flag has been turned off,
> the software bridge still has the dynamically learned 'master' FDB
> entries installed, and those should be deleted too.
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] net: dsa: centralize fast ageing when address learning is turned off
    https://git.kernel.org/netdev/net-next/c/045c45d1f598
  - [net-next,2/5] net: dsa: don't fast age bridge ports with learning turned off
    https://git.kernel.org/netdev/net-next/c/4eab90d9737b
  - [net-next,3/5] net: dsa: flush the dynamic FDB of the software bridge when fast ageing a port
    https://git.kernel.org/netdev/net-next/c/9264e4ad2611
  - [net-next,4/5] net: dsa: sja1105: rely on DSA core tracking of port learning state
    https://git.kernel.org/netdev/net-next/c/5313a37b881e
  - [net-next,5/5] net: dsa: sja1105: add FDB fast ageing support
    https://git.kernel.org/netdev/net-next/c/5126ec72a094

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