[PATCH net v2 0/5] net: dsa: b53: Preliminary fixes

[PATCH net v2 0/5] net: dsa: b53: Preliminary fixes

[Florian Fainelli]

This patch series contains fixes to issues reported by Sashiko on the
b53 PCS bug fixes being submitted, and then some more that came from
code review and testing.

Except for issue #2, all of the other issues did trigger in testing and
needed to be corrected.

Changes in v2:

- collected Jonas' R-b tags
- corrected logic in patch #3 to propagate -ENOENT, memset after the
  debug print, translate -ENOSPC to -ENOENT when deleting

Florian Fainelli (5):
  net: dsa: b53: initialize the jumbo frame size register offset
  net: dsa: b53: fix the SRAB grant timeout check
  net: dsa: b53: clear stale state when creating an ARL entry
  net: dsa: b53: reject conflicting mirror destinations
  net: dsa: b53: use the correct override register 2 Gbps bit

 drivers/net/dsa/b53/b53_common.c | 44 ++++++++++++++++++++++----------
 drivers/net/dsa/b53/b53_srab.c   |  2 +-
 2 files changed, 31 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH net v2 1/5] net: dsa: b53: initialize the jumbo frame size register offset

[Florian Fainelli]

The chip data specifies both the jumbo port mask and frame size register
offsets, but b53_switch_init() only copies the former into the device.
Since the device is zero-initialized, b53_set_jumbo() writes the frame
size to offset zero instead of the chip-specific size register at 0x05
or 0x08.

Copy jumbo_size_reg from the chip data along with jumbo_pm_reg so frame
size updates reach the intended register.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 0880310c9ce3..559d4b360c82 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -3109,6 +3109,7 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->vta_regs[1] = chip->vta_regs[1];
 			dev->vta_regs[2] = chip->vta_regs[2];
 			dev->jumbo_pm_reg = chip->jumbo_pm_reg;
+			dev->jumbo_size_reg = chip->jumbo_size_reg;
 			dev->imp_port = chip->imp_port;
 			dev->num_vlans = chip->vlans;
 			dev->num_arl_bins = chip->arl_bins;
-- 
2.53.0

[PATCH net v2 2/5] net: dsa: b53: fix the SRAB grant timeout check

[Florian Fainelli]

b53_srab_request_grant() polls for a grant up to 20 times, but checks for
an iteration count of five to detect a timeout. This rejects a grant
received on the sixth poll and reports success when all 20 polls finish
without a grant. The latter allows a register access to proceed without
ownership of the switch resources.

Check for exhaustion of all 20 iterations instead.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_srab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
index 83a03fca142a..f4689078ccd5 100644
--- a/drivers/net/dsa/b53/b53_srab.c
+++ b/drivers/net/dsa/b53/b53_srab.c
@@ -105,7 +105,7 @@ static int b53_srab_request_grant(struct b53_device *dev)
 			break;
 		usleep_range(10, 100);
 	}
-	if (WARN_ON(i == 5))
+	if (WARN_ON(i == 20))
 		return -EIO;
 
 	return 0;
-- 
2.53.0

[PATCH net v2 3/5] net: dsa: b53: clear stale state when creating an ARL entry

[Florian Fainelli]

When an ARL lookup does not find the requested MAC and VID, b53_arl_read()
returns the first free bin index but leaves the entry populated with the
last bin examined. The multicast add path then ORs the requested port into
that unrelated entry's forwarding bitmap, potentially forwarding packets
to ports which are not members of the multicast group.

Clear the entry before creating a new one. Also return without writing
anything when deleting an entry which is not present, instead of making
a new entry from stale bin contents.

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

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 559d4b360c82..c38e8c5838bd 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1990,11 +1990,15 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 	case -ENOSPC:
 		dev_dbg(dev->dev, "{%pM,%.4d} no space left in ARL\n",
 			addr, vid);
-		return is_valid ? ret : 0;
+		return is_valid ? ret : -ENOENT;
 	case -ENOENT:
+		if (!is_valid)
+			return ret;
+
 		/* We could not find a matching MAC, so reset to a new entry */
 		dev_dbg(dev->dev, "{%pM,%.4d} not found, using idx: %d\n",
 			addr, vid, idx);
+		memset(&ent, 0, sizeof(ent));
 		break;
 	default:
 		dev_dbg(dev->dev, "{%pM,%.4d} found, using idx: %d\n",
-- 
2.53.0

[PATCH net v2 4/5] net: dsa: b53: reject conflicting mirror destinations

[Florian Fainelli]

The mirror capture control register selects one destination for all
ingress and egress mirror sources. Adding a rule for a different capture
port currently overwrites that field and silently redirects all existing
mirror rules.

Read the capture control before changing any source masks and reject a
different destination while mirroring is enabled. Return an extack which
describes the hardware limitation, and propagate a failed capture-control
read rather than making the decision from uninitialized data.

Clear the capture-port field when the final source is removed so a later
rule can select a new destination.

Fixes: ed3af5fd08eb ("net: dsa: b53: Add support for port mirroring")
Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index c38e8c5838bd..378f47e7250c 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2543,7 +2543,19 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
 		   struct netlink_ext_ack *extack)
 {
 	struct b53_device *dev = ds->priv;
-	u16 reg, loc;
+	u16 reg, loc, cap_ctl;
+	int ret;
+
+	ret = b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &cap_ctl);
+	if (ret)
+		return ret;
+
+	if ((cap_ctl & MIRROR_EN) &&
+	    (cap_ctl & CAP_PORT_MASK) != mirror->to_local_port) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Only one mirroring destination is supported");
+		return -EOPNOTSUPP;
+	}
 
 	if (ingress)
 		loc = B53_IG_MIR_CTL;
@@ -2554,11 +2566,10 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
 	reg |= BIT(port);
 	b53_write16(dev, B53_MGMT_PAGE, loc, reg);
 
-	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
-	reg &= ~CAP_PORT_MASK;
-	reg |= mirror->to_local_port;
-	reg |= MIRROR_EN;
-	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);
+	cap_ctl &= ~CAP_PORT_MASK;
+	cap_ctl |= mirror->to_local_port;
+	cap_ctl |= MIRROR_EN;
+	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, cap_ctl);
 
 	return 0;
 }
@@ -2595,10 +2606,8 @@ void b53_mirror_del(struct dsa_switch *ds, int port,
 
 	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
 	/* Both no longer have ports, let's disable mirroring */
-	if (loc_disable && other_loc_disable) {
-		reg &= ~MIRROR_EN;
-		reg &= ~mirror->to_local_port;
-	}
+	if (loc_disable && other_loc_disable)
+		reg &= ~(MIRROR_EN | CAP_PORT_MASK);
 	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);
 }
 EXPORT_SYMBOL(b53_mirror_del);
-- 
2.53.0

[PATCH net v2 5/5] net: dsa: b53: use the correct override register 2 Gbps bit

[Florian Fainelli]

The IMP and per-port GMII override registers swap the override-enable and 2
Gbps speed bits. Using the IMP speed bit on a GMII port clears its override
enable and leaves any previous 2 Gbps selection intact. Select the speed
bit alongside the register and enable bit so lower speeds clear the correct
bit and 2 Gbps sets it.

Fixes: b6a8a5477fe9 ("net: dsa: b53: fix resetting speed and pause on forced link")
Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 378f47e7250c..75033b28100e 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1370,17 +1370,19 @@ static void b53_force_port_config(struct b53_device *dev, int port,
 				  int speed, int duplex,
 				  bool tx_pause, bool rx_pause)
 {
-	u8 reg, val, off;
+	u8 reg, val, off, speed2000;
 
 	/* Override the port settings */
 	if (port == dev->imp_port) {
 		off = B53_PORT_OVERRIDE_CTRL;
 		val = PORT_OVERRIDE_EN;
+		speed2000 = PORT_OVERRIDE_SPEED_2000M;
 	} else if (is5325(dev)) {
 		return;
 	} else {
 		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
 		val = GMII_PO_EN;
+		speed2000 = GMII_PO_SPEED_2000M;
 	}
 
 	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
@@ -1392,11 +1394,11 @@ static void b53_force_port_config(struct b53_device *dev, int port,
 
 	reg &= ~(0x3 << GMII_PO_SPEED_S);
 	if (is5301x(dev) || is58xx(dev))
-		reg &= ~PORT_OVERRIDE_SPEED_2000M;
+		reg &= ~speed2000;
 
 	switch (speed) {
 	case 2000:
-		reg |= PORT_OVERRIDE_SPEED_2000M;
+		reg |= speed2000;
 		fallthrough;
 	case SPEED_1000:
 		reg |= PORT_OVERRIDE_SPEED_1000M;
-- 
2.53.0

Re: [PATCH net v2 2/5] net: dsa: b53: fix the SRAB grant timeout check

[Daniel Machon]

> b53_srab_request_grant() polls for a grant up to 20 times, but checks for
> an iteration count of five to detect a timeout. This rejects a grant
> received on the sixth poll and reports success when all 20 polls finish
> without a grant. The latter allows a register access to proceed without
> ownership of the switch resources.
> 
> Check for exhaustion of all 20 iterations instead.
> 
> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/dsa/b53/b53_srab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index 83a03fca142a..f4689078ccd5 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -105,7 +105,7 @@ static int b53_srab_request_grant(struct b53_device *dev)
>  			break;
>  		usleep_range(10, 100);
>  	}
> -	if (WARN_ON(i == 5))
> +	if (WARN_ON(i == 20))

I wondered why the initial check was i == 5. Seems like b53_srab_op(), just
below this one, has more or less the same loop, but with 5 iterations. Probably a
copy paste error.

>  		return -EIO;
>  
>  	return 0;
> -- 
> 2.53.0
> 

Reviewed-by: Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net v2 2/5] net: dsa: b53: fix the SRAB grant timeout check

[Florian Fainelli]

On 10/30/26 04:34, Daniel Machon wrote:
>> b53_srab_request_grant() polls for a grant up to 20 times, but checks for
>> an iteration count of five to detect a timeout. This rejects a grant
>> received on the sixth poll and reports success when all 20 polls finish
>> without a grant. The latter allows a register access to proceed without
>> ownership of the switch resources.
>>
>> Check for exhaustion of all 20 iterations instead.
>>
>> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
>> Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/net/dsa/b53/b53_srab.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
>> index 83a03fca142a..f4689078ccd5 100644
>> --- a/drivers/net/dsa/b53/b53_srab.c
>> +++ b/drivers/net/dsa/b53/b53_srab.c
>> @@ -105,7 +105,7 @@ static int b53_srab_request_grant(struct b53_device *dev)
>>   			break;
>>   		usleep_range(10, 100);
>>   	}
>> -	if (WARN_ON(i == 5))
>> +	if (WARN_ON(i == 20))
> 
> I wondered why the initial check was i == 5. Seems like b53_srab_op(), just
> below this one, has more or less the same loop, but with 5 iterations. Probably a
> copy paste error.

Yes, that seems like it. I will be addressing the late Sashiko feedback 
received on v1 / patch #4 and re-submit a v3.

pw-bot: cr
-- 
Florian

Re: [PATCH net v2 2/5] net: dsa: b53: fix the SRAB grant timeout check

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The grant poll loop in b53_srab_request_grant()
  (drivers/net/dsa/b53/b53_srab.c, lines 102-109) wastes its final sleep.

Pre-existing issues:
- [Medium] Uninitialized stack values are used on the grant-timeout path
  that this patch makes reachable.

--

net: dsa: b53: fix the SRAB grant timeout check

This patch changes the timeout check in b53_srab_request_grant() from
i == 5 to i == 20 so that it matches the 20 iteration poll loop. A grant
that never arrives now returns -EIO, where before the register access
went ahead without ownership of the switch resources.

> diff --git a/drivers/net/dsa/b53/b53_srab.c b/drivers/net/dsa/b53/b53_srab.c
> index 83a03fca142a0..f4689078ccd51 100644
> --- a/drivers/net/dsa/b53/b53_srab.c
> +++ b/drivers/net/dsa/b53/b53_srab.c
> @@ -105,7 +105,7 @@ static int b53_srab_request_grant(struct b53_device *dev)
>  			break;
>  		usleep_range(10, 100);
>  	}
> -	if (WARN_ON(i == 5))
> +	if (WARN_ON(i == 20))
>  		return -EIO;

[Severity: Medium]
This is a pre-existing issue, but this patch makes the grant timeout
path reachable. Several callers then use uninitialized values.

When b53_srab_request_grant() returns -EIO, b53_srab_read8() (and the
read16/read32/read48/read64 variants) skip the *val assignment:

drivers/net/dsa/b53/b53_srab.c:b53_srab_read8() {
	ret = b53_srab_request_grant(dev);
	if (ret)
		goto err;
	...
	*val = readl(regs + B53_SRAB_RD_L) & 0xff;

err:
	b53_srab_release_grant(dev);
	...
}

The b53_build_op() wrappers in b53_priv.h pass the error through and do
not initialize *val. Many callers in b53_common.c ignore the return
value. One example is b53_get_ethtool_stats():

		} else {
			u32 val32;

			b53_read32(dev, B53_MIB_PAGE(port), s->offset,
				   &val32);
			val = val32;
		}
		data[i] = (u64)val;

Can this copy uninitialized kernel stack data into the ETHTOOL_GSTATS
buffer that is returned to userspace?

Other callers follow the same pattern:

  b53_eee_enable_set() writes an uninitialized u16 reg back to
  B53_EEE_EN_CTRL with b53_write16().

  b53_enable_vlan() read-modify-writes the uninitialized mgmt, vc0, vc1
  and vc5 into the switch mode and VLAN control registers.

  b53_arl_search_read_89() (and the _25/_63xx/_95 variants) decode the
  uninitialized mac_vid and fwd_entry into a b53_arl_entry.
  b53_fdb_dump() then passes that entry to b53_fdb_copy() and the FDB
  dump callback.

Before this patch, a grant timeout fell through to b53_srab_op() and
*val got written. The same uninitialized use could already happen
through the GORDYN timeout in b53_srab_op(). That path also returns
-EIO and skips the assignment.

Should the SRAB read accessors zero *val on error? The other option is
for these callers to check the b53_read*() return value.

[Severity: Low]
Can this reject a grant that arrives during the last sleep?

On the i == 19 iteration, the loop reads B53_SRAB_CTRLS and sees no
RCAGNT. It then calls usleep_range(10, 100) and exits with i == 20,
without reading CTRLS again:

	for (i = 0; i < 20; i++) {
		ctrls = readl(regs + B53_SRAB_CTRLS);
		if (ctrls & B53_SRAB_CTRLS_RCAGNT)
			break;
		usleep_range(10, 100);
	}

Suppose the arbiter asserts RCAGNT during that last sleep. The function
now WARNs and returns -EIO. The caller's err: path then calls
b53_srab_release_grant() and clears RCAREQ, even though access was
granted. In practice the wait covers 19 polls, not 20.

Before this patch the timeout check never fired, so this had no effect.

Would readl_poll_timeout() avoid this? A final read of CTRLS after the
loop would also work.

>  
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028212310.3847553-1-florian.fainelli%40broadcom.com

Re: [PATCH net v2 3/5] net: dsa: b53: clear stale state when creating an ARL entry

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In b53_arl_op() (drivers/net/dsa/b53/b53_common.c), the patch
  changes the result of deleting an ARL entry that is not present (op ==…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but in
  b53_arl_op() (drivers/net/dsa/b53/b53_common.c) the unicast delete…

--

net: dsa: b53: clear stale state when creating an ARL entry

This patch clears the b53_arl_entry in b53_arl_op() before creating a new
ARL entry. As a result, a multicast add no longer ORs the port into an
unrelated bin's forwarding bitmap. It also makes b53_arl_op() return early,
without writing anything, when asked to delete an entry that is not
present.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 559d4b360c828..c38e8c5838bd8 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1990,11 +1990,15 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
>  	case -ENOSPC:
>  		dev_dbg(dev->dev, "{%pM,%.4d} no space left in ARL\n",
>  			addr, vid);
> -		return is_valid ? ret : 0;
> +		return is_valid ? ret : -ENOENT;
>  	case -ENOENT:
> +		if (!is_valid)
> +			return ret;
> +

[Severity: Medium]
Is it intended that deleting an absent entry now fails with -ENOENT?

Before this patch, the -ENOSPC delete case returned 0. The -ENOENT delete
case returned the result of b53_arl_rw_op(), which is normally 0.

The commit message only says the function will "return without writing
anything". In practice both paths now return -ENOENT to b53_fdb_del() and
b53_mdb_del(), and from there to the DSA core for both b53 and bcm_sf2.

On BCM5325/BCM5365, b53_mdb_add() never programs anything:

	if (is5325(priv) || is5365(priv))
		return -EOPNOTSUPP;

b53_mdb_del() has no matching check. Every RTM_DELMDB that comes through
br_switchdev_mdb_notify() -> switchdev_port_obj_del() will now hit this
path and log:

	if (ret)
		dev_err(ds->dev, "failed to delete MDB entry\n");

switchdev_port_obj_del_deferred() then logs a second error, because the
code is not -EOPNOTSUPP. Ordinary IGMP/MLD group leaves or timeouts on a
bridged user port would produce these messages.

The same thing happens when deleting an fdb/mdb entry whose earlier add
failed with -ENOSPC.

CPU and DSA ports see a further effect. In net/dsa/switch.c,
dsa_port_do_fdb_del() keeps the tracking record when the driver returns an
error, and dsa_port_do_mdb_del() does the same:

	err = ds->ops->port_fdb_del(ds, port, addr, vid, db);
	trace_dsa_fdb_del_hw(dp, addr, vid, &db, err);
	if (err) {
		refcount_set(&a->refcount, 1);
		goto out;
	}

A later add of the same address finds that record and calls
refcount_inc(). It never calls port_fdb_add()/port_mdb_add() again, so the
ARL entry stays missing until teardown. Before this patch, the delete
returned 0, the record was freed, and the next add reprogrammed the
hardware.

There are two ways the hardware can be missing an entry that DSA still
tracks:

  - bcm_sf2_sw_resume() resets the switch via bcm_sf2_sw_rst() and does
    not replay the host FDB/MDB lists.
  - b53 has no FDB isolation. A host entry and a user-port static entry
    with the same MAC/VID share one ARL slot, so deleting the user-port
    entry invalidates both.

Documentation/networking/dsa/dsa.rst says port_fdb_del/port_mdb_del remove
the address "if it was mapped". That reads as if a missing entry is not an
error.

Would returning 0 here (perhaps with a dev_dbg()), while still skipping the
write, keep the fix without these side effects?

>  		/* We could not find a matching MAC, so reset to a new entry */
>  		dev_dbg(dev->dev, "{%pM,%.4d} not found, using idx: %d\n",
>  			addr, vid, idx);
> +		memset(&ent, 0, sizeof(ent));
>  		break;
>  	default:
>  		dev_dbg(dev->dev, "{%pM,%.4d} found, using idx: %d\n",

[Severity: Medium]
This isn't a bug introduced by this patch, but should the unicast delete in
the found case check that the entry belongs to the requesting port?

After the switch statement, b53_arl_op() does:

	if (!is_multicast_ether_addr(addr)) {
		ent.port = port;
		ent.is_valid = is_valid;
	} else {

It then writes the bin back as invalid. It never compares the ent.port read
from hardware against port.

dsa.rst says port_fdb_del should delete the address "if it was mapped into
this port forwarding database".

b53 does not set ds->fdb_isolation, so dsa_port_fdb_del() forces
info.db.bridge.num to 0 and the driver ignores the dsa_db. Suppose the same
MAC/VID is added on port A and then on port B. This can happen with two
VLAN-unaware bridges using vid 0, or with a host FDB entry on the CPU port
plus a user-port static entry.

The single ARL slot then points at B. A later delete on port A removes B's
entry, while the bridge and DSA still believe it is installed.

This patch only reworks the absent-entry branches, so the found-entry path
is unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028212310.3847553-1-florian.fainelli%40broadcom.com

Re: [PATCH net v2 4/5] net: dsa: b53: reject conflicting mirror destinations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (3) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] b53_mirror_add() now treats the live B53_MIR_CAP_CTL contents
  (MIRROR_EN plus CAP_PORT_MASK) as a record of rules this driver…

Pre-existing issues:
- [Medium] Only the first B53_MIR_CAP_CTL read in b53_mirror_add() is
  checked.
- [Medium] Mirror register writes are still unchecked, so a failed write
  can leave the source masks and the shared capture control out of sync.
- [Medium] b53_mdio_read16() (drivers/net/dsa/b53/b53_mdio.c:95-106)
  assigns the int result of mdiobus_read_nested(bus,…

--

net: dsa: b53: reject conflicting mirror destinations

b53_mirror_add() now reads B53_MIR_CAP_CTL first. If mirroring is
already enabled with a different capture port, it rejects the new rule
with an extack, and it returns an error if that read fails.
b53_mirror_del() now clears the whole CAP_PORT_MASK field when the last
mirror source is removed.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index c38e8c5838bd8..378f47e7250cd 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -2543,7 +2543,19 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  		   struct netlink_ext_ack *extack)
>  {
>  	struct b53_device *dev = ds->priv;
> -	u16 reg, loc;
> +	u16 reg, loc, cap_ctl;
> +	int ret;
> +
> +	ret = b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &cap_ctl);
> +	if (ret)
> +		return ret;

[Severity: Medium]
This isn't a bug introduced by this patch, but on switches attached over
MDIO, can cap_ctl still hold a bogus value even when this returns 0?

drivers/net/dsa/b53/b53_mdio.c:b53_mdio_read16() {
	...
	ret = b53_mdio_op(dev, page, reg, REG_MII_ADDR_READ);
	if (ret)
		return ret;

	*val = mdiobus_read_nested(bus, BRCM_PSEUDO_PHY_ADDR, REG_MII_DATA0);

	return 0;
}

Suppose b53_mdio_op() succeeds and then the REG_MII_DATA0 read fails. The
negative errno is truncated into the u16 and reported as success. For
example, -ETIMEDOUT becomes 0xff92, which has MIRROR_EN set and
CAP_PORT_MASK equal to 2.

The new check below would then compare against a capture port of 2. It
could accept a conflicting rule or reject a valid one.

b53_mdio_read8/32/48/64 have the same pattern, so this probably needs a
separate fix in b53_mdio.c: check the signed result before assigning
*val.

> +
> +	if ((cap_ctl & MIRROR_EN) &&
> +	    (cap_ctl & CAP_PORT_MASK) != mirror->to_local_port) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Only one mirroring destination is supported");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Can this check reject valid rules because of mirror state left in the
hardware from before probe?

As far as I can see, b53 probe, setup and reset never put
B53_MIR_CAP_CTL, B53_IG_MIR_CTL or B53_EG_MIR_CTL into a known state.
Only b53_mirror_add() and b53_mirror_del() touch them.

b53_switch_reset() soft-resets only is539x() and BCM58XX/BCM583XX chips:

	if (is539x(dev)) {
		b53_write8(dev, B53_CTRL_PAGE, B53_SOFTRESET, 0x83);
	...

Other chips (5325, 5365, 531x5, 63xx) without a reset GPIO keep their
earlier register contents.

kexec is one way to get there. device_shutdown()->dsa_switch_shutdown()
only detaches and unlinks the user ports and never calls
port_mirror_del. MIRROR_EN and the capture port therefore survive into
the next kernel.

The new DSA instance starts with an empty mall_tc_list, so userspace has
no rule it can delete to clear that state. After that, every tc matchall
mirred rule to a port other than the leftover capture port fails with
-EOPNOTSUPP and "Only one mirroring destination is supported". Before
this patch, the next add simply overwrote the leftover capture port.

Would it make sense to clear B53_MIR_CAP_CTL and the IG/EG mirror masks
during setup? Another option is to base the check on state tracked in
software, as mt7530 does with priv->mirror_rx/mirror_tx.

[ ... ]

> @@ -2554,11 +2566,10 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  	reg |= BIT(port);
>  	b53_write16(dev, B53_MGMT_PAGE, loc, reg);

[Severity: Medium]
This is a pre-existing issue, but only the first B53_MIR_CAP_CTL read is
checked. Can the other unchecked accesses in the mirror paths still
write uninitialized or stale values to the hardware?

In b53_mirror_add(), the read just before this is unchecked:

	b53_read16(dev, B53_MGMT_PAGE, loc, &reg);
	reg |= BIT(port);
	b53_write16(dev, B53_MGMT_PAGE, loc, reg);

reg is an uninitialized u16, and the SPI and MDIO backends leave *val
untouched on error (b53_spi_read16() only assigns when !ret). A failed
read therefore writes garbage | BIT(port) into the IG/EG mirror mask.
The writes are not checked either, and the function returns 0 either
way.

In b53_mirror_del(), none of the three b53_read16() calls are checked:

- If the first read fails, the uninitialized reg & ~BIT(port) is written
  back to the IG/EG mask.
- If the read of the other direction fails, other_loc_disable is
  computed from stale data. With this patch, that decision now also
  clears CAP_PORT_MASK.
- If the CAP_CTL read fails, reg still holds the other direction's
  source mask, and that value is written into B53_MIR_CAP_CTL:

	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
	/* Both no longer have ports, let's disable mirroring */
	if (loc_disable && other_loc_disable)
		reg &= ~(MIRROR_EN | CAP_PORT_MASK);
	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);

Ignoring register access errors is common in b53_common.c, where only a
handful of the roughly 180 accesses are checked. The commit message only
claims to propagate the capture-control read used for the conflict
decision, and the patch does that.

>  
> -	b53_read16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, &reg);
> -	reg &= ~CAP_PORT_MASK;
> -	reg |= mirror->to_local_port;
> -	reg |= MIRROR_EN;
> -	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);
> +	cap_ctl &= ~CAP_PORT_MASK;
> +	cap_ctl |= mirror->to_local_port;
> +	cap_ctl |= MIRROR_EN;
> +	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, cap_ctl);
>  
>  	return 0;

[Severity: Medium]
This isn't a bug introduced by this patch, but the write results are
still ignored. Can a failed write leave the source masks and
B53_MIR_CAP_CTL out of sync in a way that gets past the new check?

Suppose the loc write succeeds and this CAP_CTL write fails.
b53_mirror_add() still returns 0, dsa_user_add_cls_matchall_mirred()
records the rule, and MIRROR_EN stays clear. A later rule to a
different destination then passes the MIRROR_EN check, and both sources
end up mirrored to the new port.

The opposite case can happen in b53_mirror_del():

	if (loc_disable && other_loc_disable)
		reg &= ~(MIRROR_EN | CAP_PORT_MASK);
	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, reg);

If this write fails after the source masks were cleared, MIRROR_EN stays
set with the old port. The new check then rejects other destinations
even though no sources remain.

Writes can fail before the register is touched. For example,
b53_spi_write16() returns early when b53_prepare_reg_access() fails.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028212310.3847553-1-florian.fainelli%40broadcom.com