[PATCH net 0/4] net: dsa: b53: Various ARL fixes

[PATCH net 0/4] net: dsa: b53: Various ARL fixes

[Florian Fainelli]

Hi David, Andrew, Vivien, Jakub,

This patch series fixes a number of short comings in the existing b53
driver ARL management logic in particular:

- we were not looking up the {MAC,VID} tuples against their VID, despite
  having VLANs enabled

- the MDB entries (multicast) would lose their validity as soon as a
  single port in the vector would leave the entry

- the ARL was currently under utilized because we would always place new
  entries in bin index #1, instead of using all possible bins available,
  thus reducing the ARL effective size by 50% or 75% depending on the
  switch generation

- it was possible to overwrite the ARL entries because no proper space
  verification was done

This patch series addresses all of these issues.

Florian Fainelli (4):
  net: dsa: b53: Lookup VID in ARL searches when VLAN is enabled
  net: dsa: b53: Fix valid setting for MDB entries
  net: dsa: b53: Fix ARL register definitions
  net: dsa: b53: Rework ARL bin logic

 drivers/net/dsa/b53/b53_common.c | 31 ++++++++++++++++++++++++++-----
 drivers/net/dsa/b53/b53_regs.h   |  4 ++--
 2 files changed, 28 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH net 1/4] net: dsa: b53: Lookup VID in ARL searches when VLAN is enabled

[Florian Fainelli]

When VLAN is enabled, and an ARL search is issued, we also need to
compare the full {MAC,VID} tuple before returning a successful search
result.

Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 68e2381694b9..fa9b9aca7b56 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1505,6 +1505,9 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
 			continue;
 		if ((mac_vid & ARLTBL_MAC_MASK) != mac)
 			continue;
+		if (dev->vlan_enabled &&
+		    ((mac_vid >> ARLTBL_VID_S) & ARLTBL_VID_MASK) != vid)
+			continue;
 		*idx = i;
 	}
 
-- 
2.17.1

Re: [PATCH net 1/4] net: dsa: b53: Lookup VID in ARL searches when VLAN is enabled

[Andrew Lunn]

On Mon, Apr 13, 2020 at 09:16:27PM -0700, Florian Fainelli wrote:
> When VLAN is enabled, and an ARL search is issued, we also need to
> compare the full {MAC,VID} tuple before returning a successful search
> result.
> 
> Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

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

    Andrew

[PATCH net 2/4] net: dsa: b53: Fix valid setting for MDB entries

[Florian Fainelli]

When support for the MDB entries was added, the valid bit was correctly
changed to be assigned depending on the remaining port bitmask, that is,
if there were no more ports added to the entry's port bitmask, the entry
now becomes invalid. There was another assignment a few lines below that
would override this which would invalidate entries even when there were
still multiple ports left in the MDB entry.

Fixes: 5d65b64a3d97 ("net: dsa: b53: Add support for MDB")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index fa9b9aca7b56..e937bf365490 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1561,7 +1561,6 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 		ent.is_valid = !!(ent.port);
 	}
 
-	ent.is_valid = is_valid;
 	ent.vid = vid;
 	ent.is_static = true;
 	ent.is_age = false;
-- 
2.17.1

Re: [PATCH net 2/4] net: dsa: b53: Fix valid setting for MDB entries

[Andrew Lunn]

On Mon, Apr 13, 2020 at 09:16:28PM -0700, Florian Fainelli wrote:
> When support for the MDB entries was added, the valid bit was correctly
> changed to be assigned depending on the remaining port bitmask, that is,
> if there were no more ports added to the entry's port bitmask, the entry
> now becomes invalid. There was another assignment a few lines below that
> would override this which would invalidate entries even when there were
> still multiple ports left in the MDB entry.
> 
> Fixes: 5d65b64a3d97 ("net: dsa: b53: Add support for MDB")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

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

    Andrew

[PATCH net 3/4] net: dsa: b53: Fix ARL register definitions

[Florian Fainelli]

The ARL {MAC,VID} tuple and the forward entry were off by 0x10 bytes,
which means that when we read/wrote from/to ARL bin index 0, we were
actually accessing the ARLA_RWCTRL register.

Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_regs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index 2a9f421680aa..d914e756cdab 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -304,7 +304,7 @@
  *
  * BCM5325 and BCM5365 share most definitions below
  */
-#define B53_ARLTBL_MAC_VID_ENTRY(n)	(0x10 * (n))
+#define B53_ARLTBL_MAC_VID_ENTRY(n)	((0x10 * (n)) + 0x10)
 #define   ARLTBL_MAC_MASK		0xffffffffffffULL
 #define   ARLTBL_VID_S			48
 #define   ARLTBL_VID_MASK_25		0xff
@@ -316,7 +316,7 @@
 #define   ARLTBL_VALID_25		BIT(63)
 
 /* ARL Table Data Entry N Registers (32 bit) */
-#define B53_ARLTBL_DATA_ENTRY(n)	((0x10 * (n)) + 0x08)
+#define B53_ARLTBL_DATA_ENTRY(n)	((0x10 * (n)) + 0x18)
 #define   ARLTBL_DATA_PORT_ID_MASK	0x1ff
 #define   ARLTBL_TC(tc)			((3 & tc) << 11)
 #define   ARLTBL_AGE			BIT(14)
-- 
2.17.1

Re: [PATCH net 3/4] net: dsa: b53: Fix ARL register definitions

[Andrew Lunn]

On Mon, Apr 13, 2020 at 09:16:29PM -0700, Florian Fainelli wrote:
> The ARL {MAC,VID} tuple and the forward entry were off by 0x10 bytes,
> which means that when we read/wrote from/to ARL bin index 0, we were
> actually accessing the ARLA_RWCTRL register.
> 
> Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

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

    Andrew

[PATCH net 4/4] net: dsa: b53: Rework ARL bin logic

[Florian Fainelli]

When asking the ARL to read a MAC address, we will get a number of bins
returned in a single read. Out of those bins, there can essentially be 3
states:

- all bins are full, we have no space left, and we can either replace an
  existing address or return that full condition

- the MAC address was found, then we need to return its bin index and
  modify that one, and only that one

- the MAC address was not found and we have a least one bin free, we use
  that bin index location then

The code would unfortunately fail on all counts.

Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index e937bf365490..b2b2c4a301bf 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1483,6 +1483,7 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
 			u16 vid, struct b53_arl_entry *ent, u8 *idx,
 			bool is_valid)
 {
+	DECLARE_BITMAP(free_bins, dev->num_arl_entries);
 	unsigned int i;
 	int ret;
 
@@ -1490,6 +1491,8 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
 	if (ret)
 		return ret;
 
+	bitmap_zero(free_bins, dev->num_arl_entries);
+
 	/* Read the bins */
 	for (i = 0; i < dev->num_arl_entries; i++) {
 		u64 mac_vid;
@@ -1501,16 +1504,24 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
 			   B53_ARLTBL_DATA_ENTRY(i), &fwd_entry);
 		b53_arl_to_entry(ent, mac_vid, fwd_entry);
 
-		if (!(fwd_entry & ARLTBL_VALID))
+		if (!(fwd_entry & ARLTBL_VALID)) {
+			set_bit(i, free_bins);
 			continue;
+		}
 		if ((mac_vid & ARLTBL_MAC_MASK) != mac)
 			continue;
 		if (dev->vlan_enabled &&
 		    ((mac_vid >> ARLTBL_VID_S) & ARLTBL_VID_MASK) != vid)
 			continue;
 		*idx = i;
+		return 0;
 	}
 
+	if (bitmap_weight(free_bins, dev->num_arl_entries) == 0)
+		return -ENOSPC;
+
+	*idx = find_first_bit(free_bins, dev->num_arl_entries);
+
 	return -ENOENT;
 }
 
@@ -1537,13 +1548,21 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 
 	ret = b53_arl_read(dev, mac, vid, &ent, &idx, is_valid);
 	/* If this is a read, just finish now */
-	if (op)
+	if (op && ret != -ENOENT)
 		return ret;
 
 	/* We could not find a matching MAC, so reset to a new entry */
-	if (ret) {
+	switch (ret) {
+	case -ENOSPC:
+		dev_warn(dev->dev, "no space left in ARL\n");
+		return ret;
+	case -ENOENT:
+		dev_dbg(dev->dev, "{%pM,%.4d} not found, using idx: %d\n", addr, vid, idx);
 		fwd_entry = 0;
-		idx = 1;
+		break;
+	default:
+		dev_dbg(dev->dev, "{%pM,%.4d} found, using idx: %d\n", addr, vid, idx);
+		break;
 	}
 
 	/* For multicast address, the port is a bitmask and the validity
-- 
2.17.1

Re: [PATCH net 4/4] net: dsa: b53: Rework ARL bin logic

[Andrew Lunn]

On Mon, Apr 13, 2020 at 09:16:30PM -0700, Florian Fainelli wrote:
> When asking the ARL to read a MAC address, we will get a number of bins
> returned in a single read. Out of those bins, there can essentially be 3
> states:
> 
> - all bins are full, we have no space left, and we can either replace an
>   existing address or return that full condition
> 
> - the MAC address was found, then we need to return its bin index and
>   modify that one, and only that one
> 
> - the MAC address was not found and we have a least one bin free, we use
>   that bin index location then
> 
> The code would unfortunately fail on all counts.
> 
> Fixes: 1da6df85c6fb ("net: dsa: b53: Implement ARL add/del/dump operations")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

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

    Andrew

Re: [PATCH net 0/4] net: dsa: b53: Various ARL fixes

[Florian Fainelli]

David, Jakub,

On 4/13/2020 9:16 PM, Florian Fainelli wrote:
> Hi David, Andrew, Vivien, Jakub,
> 
> This patch series fixes a number of short comings in the existing b53
> driver ARL management logic in particular:
> 
> - we were not looking up the {MAC,VID} tuples against their VID, despite
>   having VLANs enabled
> 
> - the MDB entries (multicast) would lose their validity as soon as a
>   single port in the vector would leave the entry
> 
> - the ARL was currently under utilized because we would always place new
>   entries in bin index #1, instead of using all possible bins available,
>   thus reducing the ARL effective size by 50% or 75% depending on the
>   switch generation

Please do not apply this just yet, this patch issues a dev_warn() which
would be seen even when deleting an entry from the ARL while the table
is full this is undesirable and I also need to update the callers of
b53_arl_op() to check for -ENOSPC specifically. v2 coming, thanks!

> 
> - it was possible to overwrite the ARL entries because no proper space
>   verification was done
> 
> This patch series addresses all of these issues.
> 
> Florian Fainelli (4):
>   net: dsa: b53: Lookup VID in ARL searches when VLAN is enabled
>   net: dsa: b53: Fix valid setting for MDB entries
>   net: dsa: b53: Fix ARL register definitions
>   net: dsa: b53: Rework ARL bin logic
> 
>  drivers/net/dsa/b53/b53_common.c | 31 ++++++++++++++++++++++++++-----
>  drivers/net/dsa/b53/b53_regs.h   |  4 ++--
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 

-- 
Florian

Re: [PATCH net 0/4] net: dsa: b53: Various ARL fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 14 Apr 2020 08:47:15 -0700

> Please do not apply this just yet, this patch issues a dev_warn() which
> would be seen even when deleting an entry from the ARL while the table
> is full this is undesirable and I also need to update the callers of
> b53_arl_op() to check for -ENOSPC specifically. v2 coming, thanks!

Ok, marked as supercered in patchwork.