All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net 0/6] FDB fixes for NXP SJA1105
@ 2021-07-30 17:18 Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 1/6] net: dsa: sja1105: fix static FDB writes for SJA1110 Vladimir Oltean
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

I have some upcoming patches that make heavy use of statically installed
FDB entries, and when testing them on SJA1105P/Q/R/S and SJA1110, it
became clear that these switches do not behave reliably at all.

- On SJA1110, a static FDB entry cannot be installed at all
- On SJA1105P/Q/R/S, it is very picky about the inner/outer VLAN type
- Dynamically learned entries will make us not install static ones, or
  even if we do, they might not take effect

Patch 5/6 has a conflict with net-next (sorry), the commit message of
that patch describes how to deal with it. Thanks.

Vladimir Oltean (6):
  net: dsa: sja1105: fix static FDB writes for SJA1110
  net: dsa: sja1105: overwrite dynamic FDB entries with static ones in
    .port_fdb_add
  net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently
    with statically added ones
  net: dsa: sja1105: ignore the FDB entry for unknown multicast when
    adding a new address
  net: dsa: sja1105: be stateless with FDB entries on
    SJA1105P/Q/R/S/SJA1110 too
  net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN
    tag

 .../net/dsa/sja1105/sja1105_dynamic_config.c  | 27 +++---
 drivers/net/dsa/sja1105/sja1105_main.c        | 94 ++++++++++++++-----
 2 files changed, 84 insertions(+), 37 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net 1/6] net: dsa: sja1105: fix static FDB writes for SJA1110
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 2/6] net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add Vladimir Oltean
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

The blamed commit made FDB access on SJA1110 functional only as far as
dumping the existing entries goes, but anything having to do with an
entry's index (adding, deleting) is still broken.

There are in fact 2 problems, all caused by improperly inheriting the
code from SJA1105P/Q/R/S:
- An entry size is SJA1110_SIZE_L2_LOOKUP_ENTRY (24) bytes and not
  SJA1105PQRS_SIZE_L2_LOOKUP_ENTRY (20) bytes
- The "index" field within an FDB entry is at bits 10:1 for SJA1110 and
  not 15:6 as in SJA1105P/Q/R/S

This patch moves the packing function for the cmd->index outside of
sja1105pqrs_common_l2_lookup_cmd_packing() and into the device specific
functions sja1105pqrs_l2_lookup_cmd_packing and
sja1110_l2_lookup_cmd_packing.

Fixes: 74e7feff0e22 ("net: dsa: sja1105: fix dynamic access to L2 Address Lookup table for SJA1110")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../net/dsa/sja1105/sja1105_dynamic_config.c  | 27 ++++++++++---------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
index 56fead68ea9f..147709131c13 100644
--- a/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_dynamic_config.c
@@ -304,6 +304,15 @@ sja1105pqrs_common_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
 			hostcmd = SJA1105_HOSTCMD_INVALIDATE;
 	}
 	sja1105_packing(p, &hostcmd, 25, 23, size, op);
+}
+
+static void
+sja1105pqrs_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
+				  enum packing_op op)
+{
+	int entry_size = SJA1105PQRS_SIZE_L2_LOOKUP_ENTRY;
+
+	sja1105pqrs_common_l2_lookup_cmd_packing(buf, cmd, op, entry_size);
 
 	/* Hack - The hardware takes the 'index' field within
 	 * struct sja1105_l2_lookup_entry as the index on which this command
@@ -313,26 +322,18 @@ sja1105pqrs_common_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
 	 * such that our API doesn't need to ask for a full-blown entry
 	 * structure when e.g. a delete is requested.
 	 */
-	sja1105_packing(buf, &cmd->index, 15, 6,
-			SJA1105PQRS_SIZE_L2_LOOKUP_ENTRY, op);
-}
-
-static void
-sja1105pqrs_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
-				  enum packing_op op)
-{
-	int size = SJA1105PQRS_SIZE_L2_LOOKUP_ENTRY;
-
-	return sja1105pqrs_common_l2_lookup_cmd_packing(buf, cmd, op, size);
+	sja1105_packing(buf, &cmd->index, 15, 6, entry_size, op);
 }
 
 static void
 sja1110_l2_lookup_cmd_packing(void *buf, struct sja1105_dyn_cmd *cmd,
 			      enum packing_op op)
 {
-	int size = SJA1110_SIZE_L2_LOOKUP_ENTRY;
+	int entry_size = SJA1110_SIZE_L2_LOOKUP_ENTRY;
+
+	sja1105pqrs_common_l2_lookup_cmd_packing(buf, cmd, op, entry_size);
 
-	return sja1105pqrs_common_l2_lookup_cmd_packing(buf, cmd, op, size);
+	sja1105_packing(buf, &cmd->index, 10, 1, entry_size, op);
 }
 
 /* The switch is so retarded that it makes our command/entry abstraction
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 2/6] net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 1/6] net: dsa: sja1105: fix static FDB writes for SJA1110 Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 3/6] net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones Vladimir Oltean
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

The SJA1105 switch family leaves it up to software to decide where
within the FDB to install a static entry, and to concatenate destination
ports for already existing entries (the FDB is also used for multicast
entries), it is not as simple as just saying "please add this entry".

This means we first need to search for an existing FDB entry before
adding a new one. The driver currently manages to fool itself into
thinking that if an FDB entry already exists, there is nothing to be
done. But that FDB entry might be dynamically learned, case in which it
should be replaced with a static entry, but instead it is left alone.

This patch checks the LOCKEDS ("locked/static") bit from found FDB
entries, and lets the code "goto skip_finding_an_index;" if the FDB
entry was not static. So we also need to move the place where we set
LOCKEDS = true, to cover the new case where a dynamic FDB entry existed
but was dynamic.

Fixes: 291d1e72b756 ("net: dsa: sja1105: Add support for FDB and MDB management")
Fixes: 1da73821343c ("net: dsa: sja1105: Add FDB operations for P/Q/R/S series")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index e2dc997580a8..cc4a22ee1474 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1333,7 +1333,7 @@ int sja1105et_fdb_add(struct dsa_switch *ds, int port,
 		 * mask? If yes, we need to do nothing. If not, we need
 		 * to rewrite the entry by adding this port to it.
 		 */
-		if (l2_lookup.destports & BIT(port))
+		if ((l2_lookup.destports & BIT(port)) && l2_lookup.lockeds)
 			return 0;
 		l2_lookup.destports |= BIT(port);
 	} else {
@@ -1364,6 +1364,7 @@ int sja1105et_fdb_add(struct dsa_switch *ds, int port,
 						     index, NULL, false);
 		}
 	}
+	l2_lookup.lockeds = true;
 	l2_lookup.index = sja1105et_fdb_index(bin, way);
 
 	rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_LOOKUP,
@@ -1434,10 +1435,10 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
 					 SJA1105_SEARCH, &l2_lookup);
 	if (rc == 0) {
-		/* Found and this port is already in the entry's
+		/* Found a static entry and this port is already in the entry's
 		 * port mask => job done
 		 */
-		if (l2_lookup.destports & BIT(port))
+		if ((l2_lookup.destports & BIT(port)) && l2_lookup.lockeds)
 			return 0;
 		/* l2_lookup.index is populated by the switch in case it
 		 * found something.
@@ -1460,10 +1461,11 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 		dev_err(ds->dev, "FDB is full, cannot add entry.\n");
 		return -EINVAL;
 	}
-	l2_lookup.lockeds = true;
 	l2_lookup.index = i;
 
 skip_finding_an_index:
+	l2_lookup.lockeds = true;
+
 	rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_LOOKUP,
 					  l2_lookup.index, &l2_lookup,
 					  true);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 3/6] net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 1/6] net: dsa: sja1105: fix static FDB writes for SJA1110 Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 2/6] net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 4/6] net: dsa: sja1105: ignore the FDB entry for unknown multicast when adding a new address Vladimir Oltean
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

The procedure to add a static FDB entry in sja1105 is concurrent with
dynamic learning performed on all bridge ports and the CPU port.

The switch looks up the FDB from left to right, and also learns
dynamically from left to right, so it is possible that between the
moment when we pick up a free slot to install an FDB entry, another slot
to the left of that one becomes free due to an address ageing out, and
that other slot is then immediately used by the switch to learn
dynamically the same address as we're trying to add statically.

The result is that we succeeded to add our static FDB entry, but it is
being shadowed by a dynamic FDB entry to its left, and the switch will
behave as if our static FDB entry did not exist.

We cannot really prevent this from happening unless we make the entire
process to add a static FDB entry a huge critical section where address
learning is temporarily disabled on _all_ ports, and then re-enabled
according to the configuration done by sja1105_port_set_learning.
However, that is kind of disruptive for the operation of the network.

What we can do alternatively is to simply read back the FDB for dynamic
entries located before our newly added static one, and delete them.
This will guarantee that our static FDB entry is now operational. It
will still not guarantee that there aren't dynamic FDB entries to the
_right_ of that static FDB entry, but at least those entries will age
out by themselves since they aren't hit, and won't bother anyone.

Fixes: 291d1e72b756 ("net: dsa: sja1105: Add support for FDB and MDB management")
Fixes: 1da73821343c ("net: dsa: sja1105: Add FDB operations for P/Q/R/S series")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 57 +++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index cc4a22ee1474..5a4c7789ca43 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1318,10 +1318,11 @@ static int sja1105et_is_fdb_entry_in_bin(struct sja1105_private *priv, int bin,
 int sja1105et_fdb_add(struct dsa_switch *ds, int port,
 		      const unsigned char *addr, u16 vid)
 {
-	struct sja1105_l2_lookup_entry l2_lookup = {0};
+	struct sja1105_l2_lookup_entry l2_lookup = {0}, tmp;
 	struct sja1105_private *priv = ds->priv;
 	struct device *dev = ds->dev;
 	int last_unused = -1;
+	int start, end, i;
 	int bin, way, rc;
 
 	bin = sja1105et_fdb_hash(priv, addr, vid);
@@ -1373,6 +1374,29 @@ int sja1105et_fdb_add(struct dsa_switch *ds, int port,
 	if (rc < 0)
 		return rc;
 
+	/* Invalidate a dynamically learned entry if that exists */
+	start = sja1105et_fdb_index(bin, 0);
+	end = sja1105et_fdb_index(bin, way);
+
+	for (i = start; i < end; i++) {
+		rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
+						 i, &tmp);
+		if (rc == -ENOENT)
+			continue;
+		if (rc)
+			return rc;
+
+		if (tmp.macaddr != ether_addr_to_u64(addr) || tmp.vlanid != vid)
+			continue;
+
+		rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_LOOKUP,
+						  i, NULL, false);
+		if (rc)
+			return rc;
+
+		break;
+	}
+
 	return sja1105_static_fdb_change(priv, port, &l2_lookup, true);
 }
 
@@ -1414,7 +1438,7 @@ int sja1105et_fdb_del(struct dsa_switch *ds, int port,
 int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 			const unsigned char *addr, u16 vid)
 {
-	struct sja1105_l2_lookup_entry l2_lookup = {0};
+	struct sja1105_l2_lookup_entry l2_lookup = {0}, tmp;
 	struct sja1105_private *priv = ds->priv;
 	int rc, i;
 
@@ -1472,6 +1496,35 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	if (rc < 0)
 		return rc;
 
+	/* The switch learns dynamic entries and looks up the FDB left to
+	 * right. It is possible that our addition was concurrent with the
+	 * dynamic learning of the same address, so now that the static entry
+	 * has been installed, we are certain that address learning for this
+	 * particular address has been turned off, so the dynamic entry either
+	 * is in the FDB at an index smaller than the static one, or isn't (it
+	 * can also be at a larger index, but in that case it is inactive
+	 * because the static FDB entry will match first, and the dynamic one
+	 * will eventually age out). Search for a dynamically learned address
+	 * prior to our static one and invalidate it.
+	 */
+	tmp = l2_lookup;
+
+	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
+					 SJA1105_SEARCH, &tmp);
+	if (rc < 0) {
+		dev_err(ds->dev,
+			"port %d failed to read back entry for %pM vid %d: %pe\n",
+			port, addr, vid, ERR_PTR(rc));
+		return rc;
+	}
+
+	if (tmp.index < l2_lookup.index) {
+		rc = sja1105_dynamic_config_write(priv, BLK_IDX_L2_LOOKUP,
+						  tmp.index, NULL, false);
+		if (rc < 0)
+			return rc;
+	}
+
 	return sja1105_static_fdb_change(priv, port, &l2_lookup, true);
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 4/6] net: dsa: sja1105: ignore the FDB entry for unknown multicast when adding a new address
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
                   ` (2 preceding siblings ...)
  2021-07-30 17:18 ` [PATCH net 3/6] net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 5/6] net: dsa: sja1105: be stateless with FDB entries on SJA1105P/Q/R/S/SJA1110 too Vladimir Oltean
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

Currently, when sja1105pqrs_fdb_add() is called for a host-joined IPv6
MDB entry such as 33:33:00:00:00:6a, the search for that address will
return the FDB entry for SJA1105_UNKNOWN_MULTICAST, which has a
destination MAC of 01:00:00:00:00:00 and a mask of 01:00:00:00:00:00.
It returns that entry because, well, it matches, in the sense that
unknown multicast is supposed by design to match it...

But the issue is that we then proceed to overwrite this entry with the
one for our precise host-joined multicast address, and the unknown
multicast entry is no longer there - unknown multicast is now flooded to
the same group of ports as broadcast, which does not look up the FDB.

To solve this problem, we should ignore searches that return the unknown
multicast address as the match, and treat them as "no match" which will
result in the entry being installed to hardware.

For this to work properly, we need to put the result of the FDB search
in a temporary variable in order to avoid overwriting the l2_lookup
entry we want to program. The l2_lookup entry returned by the search
might not have the same set of DESTPORTS and not even the same MACADDR
as the entry we're trying to add.

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 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5a4c7789ca43..5d8739b30d8c 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1456,14 +1456,19 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	}
 	l2_lookup.destports = BIT(port);
 
+	tmp = l2_lookup;
+
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
-					 SJA1105_SEARCH, &l2_lookup);
-	if (rc == 0) {
+					 SJA1105_SEARCH, &tmp);
+	if (rc == 0 && tmp.index != SJA1105_MAX_L2_LOOKUP_COUNT - 1) {
 		/* Found a static entry and this port is already in the entry's
 		 * port mask => job done
 		 */
-		if ((l2_lookup.destports & BIT(port)) && l2_lookup.lockeds)
+		if ((tmp.destports & BIT(port)) && tmp.lockeds)
 			return 0;
+
+		l2_lookup = tmp;
+
 		/* l2_lookup.index is populated by the switch in case it
 		 * found something.
 		 */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 5/6] net: dsa: sja1105: be stateless with FDB entries on SJA1105P/Q/R/S/SJA1110 too
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
                   ` (3 preceding siblings ...)
  2021-07-30 17:18 ` [PATCH net 4/6] net: dsa: sja1105: ignore the FDB entry for unknown multicast when adding a new address Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-07-30 17:18 ` [PATCH net 6/6] net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN tag Vladimir Oltean
  2021-08-02 13:30 ` [PATCH net 0/6] FDB fixes for NXP SJA1105 patchwork-bot+netdevbpf
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

Similar but not quite the same with what was done in commit b11f0a4c0c81
("net: dsa: sja1105: be stateless when installing FDB entries") for
SJA1105E/T, it is desirable to drop the priv->vlan_aware check and
simply go ahead and install FDB entries in the VLAN that was given by
the bridge.

As opposed to SJA1105E/T, in SJA1105P/Q/R/S and SJA1110, the FDB is a
maskable TCAM, and we are installing VLAN-unaware FDB entries with the
VLAN ID masked off. However, such FDB entries might completely obscure
VLAN-aware entries where the VLAN ID is included in the search mask,
because the switch looks up the FDB from left to right and picks the
first entry which results in a masked match. So it depends on whether
the bridge installs first the VLAN-unaware or the VLAN-aware FDB entries.

Anyway, if we had a VLAN-unaware FDB entry towards one set of DESTPORTS
and a VLAN-aware one towards other set of DESTPORTS, the result is that
the packets in VLAN-aware mode will be forwarded towards the DESTPORTS
specified by the VLAN-unaware entry.

To solve this, simply do not use the masked matching ability of the FDB
for VLAN ID, and always match precisely on it. In VLAN-unaware mode, we
configure the switch for shared VLAN learning, so the VLAN ID will be
ignored anyway during lookup, so it is redundant to mask it off in the
TCAM.

This patch conflicts with net-next commit 0fac6aa098ed ("net: dsa: sja1105:
delete the best_effort_vlan_filtering mode") which changed this line:
	if (priv->vlan_state != SJA1105_VLAN_UNAWARE) {
into:
	if (priv->vlan_aware) {

When merging with net-next, the lines added by this patch should take
precedence in the conflict resolution (i.e. the "if" condition should be
deleted in both cases).

Fixes: 1da73821343c ("net: dsa: sja1105: Add FDB operations for P/Q/R/S series")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5d8739b30d8c..335b608bad11 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1447,13 +1447,8 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	l2_lookup.vlanid = vid;
 	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
-	if (priv->vlan_state != SJA1105_VLAN_UNAWARE) {
-		l2_lookup.mask_vlanid = VLAN_VID_MASK;
-		l2_lookup.mask_iotag = BIT(0);
-	} else {
-		l2_lookup.mask_vlanid = 0;
-		l2_lookup.mask_iotag = 0;
-	}
+	l2_lookup.mask_vlanid = VLAN_VID_MASK;
+	l2_lookup.mask_iotag = BIT(0);
 	l2_lookup.destports = BIT(port);
 
 	tmp = l2_lookup;
@@ -1545,13 +1540,8 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
 	l2_lookup.vlanid = vid;
 	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
-	if (priv->vlan_state != SJA1105_VLAN_UNAWARE) {
-		l2_lookup.mask_vlanid = VLAN_VID_MASK;
-		l2_lookup.mask_iotag = BIT(0);
-	} else {
-		l2_lookup.mask_vlanid = 0;
-		l2_lookup.mask_iotag = 0;
-	}
+	l2_lookup.mask_vlanid = VLAN_VID_MASK;
+	l2_lookup.mask_iotag = BIT(0);
 	l2_lookup.destports = BIT(port);
 
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH net 6/6] net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN tag
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
                   ` (4 preceding siblings ...)
  2021-07-30 17:18 ` [PATCH net 5/6] net: dsa: sja1105: be stateless with FDB entries on SJA1105P/Q/R/S/SJA1110 too Vladimir Oltean
@ 2021-07-30 17:18 ` Vladimir Oltean
  2021-08-02 13:30 ` [PATCH net 0/6] FDB fixes for NXP SJA1105 patchwork-bot+netdevbpf
  6 siblings, 0 replies; 8+ messages in thread
From: Vladimir Oltean @ 2021-07-30 17:18 UTC (permalink / raw)
  To: netdev, Jakub Kicinski, David S. Miller
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot

On SJA1105P/Q/R/S and SJA1110, the L2 Lookup Table entries contain a
maskable "inner/outer tag" bit which means:
- when set to 1: match single-outer and double tagged frames
- when set to 0: match untagged and single-inner tagged frames
- when masked off: match all frames regardless of the type of tag

This driver does not make any meaningful distinction between inner tags
(matches on TPID) and outer tags (matches on TPID2). In fact, all VLAN
table entries are installed as SJA1110_VLAN_D_TAG, which means that they
match on both inner and outer tags.

So it does not make sense that we install FDB entries with the IOTAG bit
set to 1.

In VLAN-unaware mode, we set both TPID and TPID2 to 0xdadb, so the
switch will see frames as outer-tagged or double-tagged (never inner).
So the FDB entries will match if IOTAG is set to 1.

In VLAN-aware mode, we set TPID to 0x8100 and TPID2 to 0x88a8. So the
switch will see untagged and 802.1Q-tagged packets as inner-tagged, and
802.1ad-tagged packets as outer-tagged. So untagged and 802.1Q-tagged
packets will not match FDB entries if IOTAG is set to 1, but 802.1ad
tagged packets will. Strange.

To fix this, simply mask off the IOTAG bit from FDB entries, and make
them match regardless of whether the VLAN tag is inner or outer.

Fixes: 1da73821343c ("net: dsa: sja1105: Add FDB operations for P/Q/R/S series")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 335b608bad11..8667c9754330 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1445,10 +1445,8 @@ int sja1105pqrs_fdb_add(struct dsa_switch *ds, int port,
 	/* Search for an existing entry in the FDB table */
 	l2_lookup.macaddr = ether_addr_to_u64(addr);
 	l2_lookup.vlanid = vid;
-	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
 	l2_lookup.mask_vlanid = VLAN_VID_MASK;
-	l2_lookup.mask_iotag = BIT(0);
 	l2_lookup.destports = BIT(port);
 
 	tmp = l2_lookup;
@@ -1538,10 +1536,8 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
 
 	l2_lookup.macaddr = ether_addr_to_u64(addr);
 	l2_lookup.vlanid = vid;
-	l2_lookup.iotag = SJA1105_S_TAG;
 	l2_lookup.mask_macaddr = GENMASK_ULL(ETH_ALEN * 8 - 1, 0);
 	l2_lookup.mask_vlanid = VLAN_VID_MASK;
-	l2_lookup.mask_iotag = BIT(0);
 	l2_lookup.destports = BIT(port);
 
 	rc = sja1105_dynamic_config_read(priv, BLK_IDX_L2_LOOKUP,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net 0/6] FDB fixes for NXP SJA1105
  2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
                   ` (5 preceding siblings ...)
  2021-07-30 17:18 ` [PATCH net 6/6] net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN tag Vladimir Oltean
@ 2021-08-02 13:30 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-02 13:30 UTC (permalink / raw)
  To: Vladimir Oltean; +Cc: netdev, kuba, davem, andrew, f.fainelli, vivien.didelot

Hello:

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

On Fri, 30 Jul 2021 20:18:09 +0300 you wrote:
> I have some upcoming patches that make heavy use of statically installed
> FDB entries, and when testing them on SJA1105P/Q/R/S and SJA1110, it
> became clear that these switches do not behave reliably at all.
> 
> - On SJA1110, a static FDB entry cannot be installed at all
> - On SJA1105P/Q/R/S, it is very picky about the inner/outer VLAN type
> - Dynamically learned entries will make us not install static ones, or
>   even if we do, they might not take effect
> 
> [...]

Here is the summary with links:
  - [net,1/6] net: dsa: sja1105: fix static FDB writes for SJA1110
    https://git.kernel.org/netdev/net/c/cb81698fddbc
  - [net,2/6] net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add
    https://git.kernel.org/netdev/net/c/e11e865bf84e
  - [net,3/6] net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones
    https://git.kernel.org/netdev/net/c/6c5fc159e092
  - [net,4/6] net: dsa: sja1105: ignore the FDB entry for unknown multicast when adding a new address
    https://git.kernel.org/netdev/net/c/728db843df88
  - [net,5/6] net: dsa: sja1105: be stateless with FDB entries on SJA1105P/Q/R/S/SJA1110 too
    https://git.kernel.org/netdev/net/c/589918df9322
  - [net,6/6] net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN tag
    https://git.kernel.org/netdev/net/c/47c2c0c23121

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-08-02 13:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 17:18 [PATCH net 0/6] FDB fixes for NXP SJA1105 Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 1/6] net: dsa: sja1105: fix static FDB writes for SJA1110 Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 2/6] net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 3/6] net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 4/6] net: dsa: sja1105: ignore the FDB entry for unknown multicast when adding a new address Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 5/6] net: dsa: sja1105: be stateless with FDB entries on SJA1105P/Q/R/S/SJA1110 too Vladimir Oltean
2021-07-30 17:18 ` [PATCH net 6/6] net: dsa: sja1105: match FDB entries regardless of inner/outer VLAN tag Vladimir Oltean
2021-08-02 13:30 ` [PATCH net 0/6] FDB fixes for NXP SJA1105 patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.