[PATCH RESEND net-next 0/2] Fix buggy brport flags offload for SJA1105 DSA

[PATCH RESEND net-next 0/2] Fix buggy brport flags offload for SJA1105 DSA

[Vladimir Oltean]

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

I am resending this series because the title and the patches were mixed
up and these patches were lost. This series' cover letter was used as
the merge commit for the unrelated "Fixing build breakage after "Merge
branch 'Propagate-extack-for-switchdev-LANs-from-DSA'"" series, as can
be seen below:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=ca04422afd6998611a81d0ea1b61d5a5f4923f84

while the actual patches from the "Fix buggy brport flags offload for
SJA1105 DSA" series were marked as superseded and not applied:
https://patchwork.kernel.org/project/netdevbpf/cover/20210214155704.1784220-1-olteanv@gmail.com/
which they should have.

I know with so many bugs I introduced it's hard to keep track, I'm sorry.

Original series description:

While testing software bridging on sja1105, I discovered that I managed
to introduce two bugs in a single patch submitted recently to net-next.

Vladimir Oltean (2):
  net: dsa: sja1105: fix configuration of source address learning
  net: dsa: sja1105: fix leakage of flooded frames outside bridging
    domain

 drivers/net/dsa/sja1105/sja1105.h      |   2 +
 drivers/net/dsa/sja1105/sja1105_main.c | 117 ++++++++++++++++---------
 2 files changed, 77 insertions(+), 42 deletions(-)

-- 
2.25.1

[PATCH RESEND net-next 1/2] net: dsa: sja1105: fix configuration of source address learning

[Vladimir Oltean]

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

Due to a mistake, the driver always sets the address learning flag to
the previously stored value, and not to the currently configured one.
The bug is visible only in standalone ports mode, because when the port
is bridged, the issue is masked by .port_stp_state_set which overwrites
the address learning state to the proper value.

Fixes: 4d9423549501 ("net: dsa: sja1105: offload bridge port flags to device")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 1dad94540cc9..3d3e2794655d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3282,7 +3282,7 @@ static int sja1105_port_set_learning(struct sja1105_private *priv, int port,
 
 	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
 
-	mac[port].dyn_learn = !!(priv->learn_ena & BIT(port));
+	mac[port].dyn_learn = enabled;
 
 	rc = sja1105_dynamic_config_write(priv, BLK_IDX_MAC_CONFIG, port,
 					  &mac[port], true);
-- 
2.25.1

[PATCH RESEND net-next 2/2] net: dsa: sja1105: fix leakage of flooded frames outside bridging domain

[Vladimir Oltean]

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

Quite embarrasingly, I managed to fool myself into thinking that the
flooding domain of sja1105 source ports is restricted by the forwarding
domain, which it isn't. Frames which match an FDB entry are forwarded
towards that entry's DESTPORTS restricted by REACH_PORT[SRC_PORT], while
frames that don't match any FDB entry are forwarded towards
FL_DOMAIN[SRC_PORT] or BC_DOMAIN[SRC_PORT].

This means we can't get away with doing the simple thing, and we must
manage the flooding domain ourselves such that it is restricted by the
forwarding domain. This new function must be called from the
.port_bridge_join and .port_bridge_leave methods too, not just from
.port_bridge_flags as we did before.

Fixes: 4d9423549501 ("net: dsa: sja1105: offload bridge port flags to device")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105.h      |   2 +
 drivers/net/dsa/sja1105/sja1105_main.c | 115 ++++++++++++++++---------
 2 files changed, 76 insertions(+), 41 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 15a0893d0ff1..494d17117b94 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -206,6 +206,8 @@ struct sja1105_private {
 	bool rgmii_tx_delay[SJA1105_NUM_PORTS];
 	bool best_effort_vlan_filtering;
 	unsigned long learn_ena;
+	unsigned long ucast_egress_floods;
+	unsigned long bcast_egress_floods;
 	const struct sja1105_info *info;
 	struct gpio_desc *reset_gpio;
 	struct spi_device *spidev;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 3d3e2794655d..7eef96fab214 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -50,6 +50,12 @@ sja1105_port_allow_traffic(struct sja1105_l2_forwarding_entry *l2_fwd,
 		l2_fwd[from].reach_port &= ~BIT(to);
 }
 
+static bool sja1105_can_forward(struct sja1105_l2_forwarding_entry *l2_fwd,
+				int from, int to)
+{
+	return !!(l2_fwd[from].reach_port & BIT(to));
+}
+
 /* Structure used to temporarily transport device tree
  * settings into sja1105_setup
  */
@@ -408,6 +414,12 @@ static int sja1105_init_l2_forwarding(struct sja1105_private *priv)
 		for (j = 0; j < SJA1105_NUM_TC; j++)
 			l2fwd[i].vlan_pmap[j] = j;
 
+		/* All ports start up with egress flooding enabled,
+		 * including the CPU port.
+		 */
+		priv->ucast_egress_floods |= BIT(i);
+		priv->bcast_egress_floods |= BIT(i);
+
 		if (i == upstream)
 			continue;
 
@@ -1571,6 +1583,50 @@ static int sja1105_mdb_del(struct dsa_switch *ds, int port,
 	return sja1105_fdb_del(ds, port, mdb->addr, mdb->vid);
 }
 
+/* Common function for unicast and broadcast flood configuration.
+ * Flooding is configured between each {ingress, egress} port pair, and since
+ * the bridge's semantics are those of "egress flooding", it means we must
+ * enable flooding towards this port from all ingress ports that are in the
+ * same forwarding domain.
+ */
+static int sja1105_manage_flood_domains(struct sja1105_private *priv)
+{
+	struct sja1105_l2_forwarding_entry *l2_fwd;
+	struct dsa_switch *ds = priv->ds;
+	int from, to, rc;
+
+	l2_fwd = priv->static_config.tables[BLK_IDX_L2_FORWARDING].entries;
+
+	for (from = 0; from < ds->num_ports; from++) {
+		u64 fl_domain = 0, bc_domain = 0;
+
+		for (to = 0; to < priv->ds->num_ports; to++) {
+			if (!sja1105_can_forward(l2_fwd, from, to))
+				continue;
+
+			if (priv->ucast_egress_floods & BIT(to))
+				fl_domain |= BIT(to);
+			if (priv->bcast_egress_floods & BIT(to))
+				bc_domain |= BIT(to);
+		}
+
+		/* Nothing changed, nothing to do */
+		if (l2_fwd[from].fl_domain == fl_domain &&
+		    l2_fwd[from].bc_domain == bc_domain)
+			continue;
+
+		l2_fwd[from].fl_domain = fl_domain;
+		l2_fwd[from].bc_domain = bc_domain;
+
+		rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_FORWARDING,
+						  from, &l2_fwd[from], true);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
 static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 				 struct net_device *br, bool member)
 {
@@ -1608,8 +1664,12 @@ static int sja1105_bridge_member(struct dsa_switch *ds, int port,
 			return rc;
 	}
 
-	return sja1105_dynamic_config_write(priv, BLK_IDX_L2_FORWARDING,
-					    port, &l2_fwd[port], true);
+	rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_FORWARDING,
+					  port, &l2_fwd[port], true);
+	if (rc)
+		return rc;
+
+	return sja1105_manage_flood_domains(priv);
 }
 
 static void sja1105_bridge_stp_state_set(struct dsa_switch *ds, int port,
@@ -3297,51 +3357,24 @@ static int sja1105_port_set_learning(struct sja1105_private *priv, int port,
 	return 0;
 }
 
-/* Common function for unicast and broadcast flood configuration.
- * Flooding is configured between each {ingress, egress} port pair, and since
- * the bridge's semantics are those of "egress flooding", it means we must
- * enable flooding towards this port from all ingress ports that are in the
- * same bridge. In practice, we just enable flooding from all possible ingress
- * ports regardless of whether they're in the same bridge or not, since the
- * reach_port configuration will not allow flooded frames to leak across
- * bridging domains anyway.
- */
 static int sja1105_port_ucast_bcast_flood(struct sja1105_private *priv, int to,
 					  struct switchdev_brport_flags flags)
 {
-	struct sja1105_l2_forwarding_entry *l2_fwd;
-	int from, rc;
-
-	l2_fwd = priv->static_config.tables[BLK_IDX_L2_FORWARDING].entries;
-
-	for (from = 0; from < priv->ds->num_ports; from++) {
-		if (dsa_is_unused_port(priv->ds, from))
-			continue;
-		if (from == to)
-			continue;
-
-		/* Unicast */
-		if (flags.mask & BR_FLOOD) {
-			if (flags.val & BR_FLOOD)
-				l2_fwd[from].fl_domain |= BIT(to);
-			else
-				l2_fwd[from].fl_domain &= ~BIT(to);
-		}
-		/* Broadcast */
-		if (flags.mask & BR_BCAST_FLOOD) {
-			if (flags.val & BR_BCAST_FLOOD)
-				l2_fwd[from].bc_domain |= BIT(to);
-			else
-				l2_fwd[from].bc_domain &= ~BIT(to);
-		}
+	if (flags.mask & BR_FLOOD) {
+		if (flags.val & BR_FLOOD)
+			priv->ucast_egress_floods |= BIT(to);
+		else
+			priv->ucast_egress_floods |= BIT(to);
+	}
 
-		rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_FORWARDING,
-						  from, &l2_fwd[from], true);
-		if (rc < 0)
-			return rc;
+	if (flags.mask & BR_BCAST_FLOOD) {
+		if (flags.val & BR_BCAST_FLOOD)
+			priv->bcast_egress_floods |= BIT(to);
+		else
+			priv->bcast_egress_floods |= BIT(to);
 	}
 
-	return 0;
+	return sja1105_manage_flood_domains(priv);
 }
 
 static int sja1105_port_mcast_flood(struct sja1105_private *priv, int to,
-- 
2.25.1

Re: [PATCH RESEND net-next 1/2] net: dsa: sja1105: fix configuration of source address learning

[Florian Fainelli]

On 2/16/2021 3:41 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Due to a mistake, the driver always sets the address learning flag to
> the previously stored value, and not to the currently configured one.
> The bug is visible only in standalone ports mode, because when the port
> is bridged, the issue is masked by .port_stp_state_set which overwrites
> the address learning state to the proper value.
> 
> Fixes: 4d9423549501 ("net: dsa: sja1105: offload bridge port flags to device")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

Re: [PATCH RESEND net-next 0/2] Fix buggy brport flags offload for SJA1105 DSA

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 16 Feb 2021 13:41:17 +0200 you wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> I am resending this series because the title and the patches were mixed
> up and these patches were lost. This series' cover letter was used as
> the merge commit for the unrelated "Fixing build breakage after "Merge
> branch 'Propagate-extack-for-switchdev-LANs-from-DSA'"" series, as can
> be seen below:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=ca04422afd6998611a81d0ea1b61d5a5f4923f84
> 
> [...]

Here is the summary with links:
  - [RESEND,net-next,1/2] net: dsa: sja1105: fix configuration of source address learning
    https://git.kernel.org/netdev/net-next/c/4c44fc5e9400
  - [RESEND,net-next,2/2] net: dsa: sja1105: fix leakage of flooded frames outside bridging domain
    https://git.kernel.org/netdev/net-next/c/7f7ccdea8c73

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