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

[PATCH net 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.

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 | 42 ++++++++++++++++++++++----------
 drivers/net/dsa/b53/b53_srab.c   |  2 +-
 2 files changed, 30 insertions(+), 14 deletions(-)

-- 
2.53.0

[PATCH net 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")
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 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")
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 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 559d4b360c82..0871a10d8cbc 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1992,6 +1992,10 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 			addr, vid);
 		return is_valid ? ret : 0;
 	case -ENOENT:
+		if (!is_valid)
+			return 0;
+
+		memset(&ent, 0, sizeof(ent));
 		/* 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);
-- 
2.53.0

[PATCH net 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")
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 0871a10d8cbc..b8a7b380bac8 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 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")
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 b8a7b380bac8..7bbbfcd611e1 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 0/5] net: dsa: b53: Preliminary fixes

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Florian Fainelli]

On 10/27/26 13:26, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - Whether the issue was actually triggered, or is only theoretical
>     (e.g. found by code inspection). If it was triggered please include
>     the symptoms, like the stack trace or error messages.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

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

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

[Jonas Gorski]

On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> 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.

That offset is reserved, so (hopefully) was effectively just a NOP.

Which reminds me that I still don't really know what this register is
good for. The (bcm53115) datasheet says it's used when jumbo support
is disabled(?) to determine max good frame size for MIBs, but also
that it controls up to which lengths frames are admitted, then dropped
(and only allows the values 1518 and 2000).

I tried writing various values to it, and never managed to make it do
anything; neither did the switch start dropping frames longer than
that, nor did I see any effect on the MIB counters ... .

> 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")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.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
>

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

[Jonas Gorski]

On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
<florian.fainelli@broadcom.com> 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")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

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

[Jonas Gorski]

On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 559d4b360c82..0871a10d8cbc 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1992,6 +1992,10 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
>                         addr, vid);
>                 return is_valid ? ret : 0;
>         case -ENOENT:
> +               if (!is_valid)
> +                       return 0;

Should we return 0, or should we pass on the -ENOENT? We failed to
remove the entry, so implies there was a desync between hardware state
and software state.

If we don't, we should at least report the issue. If we do, we don't
need to, as dsa will do that. Though we then should also report
-ENOSPC && !is_valid as -ENOENT.

Likewise when trying to add an entry that already exists.

> +
> +               memset(&ent, 0, sizeof(ent));

Just bikeshedding, but semantically it feels the memset should be
below the comment/dev_dbg().

>                 /* 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);
> --
> 2.53.0
>

Best regards,
Jonas

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

[Jonas Gorski]

On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> 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")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

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

[Jonas Gorski]

On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> 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")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

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

[Florian Fainelli]

On 10/28/26 02:52, Jonas Gorski wrote:
> On Tue, Oct 27, 2026 at 9:22 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> 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 | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index 559d4b360c82..0871a10d8cbc 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1992,6 +1992,10 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
>>                          addr, vid);
>>                  return is_valid ? ret : 0;
>>          case -ENOENT:
>> +               if (!is_valid)
>> +                       return 0;
> 
> Should we return 0, or should we pass on the -ENOENT? We failed to
> remove the entry, so implies there was a desync between hardware state
> and software state.
> 
> If we don't, we should at least report the issue. If we do, we don't
> need to, as dsa will do that. Though we then should also report
> -ENOSPC && !is_valid as -ENOENT.
> 
> Likewise when trying to add an entry that already exists.

Agreed, returning -ENOENT seems preferable so the missing hardware entry 
is visible to the caller. I'll propagate it for the -ENOENT case and 
translate -ENOSPC to -ENOENT when deleting.

For additions, finding an existing MAC/VID is not necessarily a 
desynchronization: an FDB add may move an entry or make a dynamically 
learned entry static, and an MDB add may extend the port bitmap. So I 
think we should retain the existing update behavior rather than return 
-EEXIST on a lookup match.

> 
>> +
>> +               memset(&ent, 0, sizeof(ent));
> 
> Just bikeshedding, but semantically it feels the memset should be
> below the comment/dev_dbg().

Yes, I took that change.

Thanks!
-- 
Florian

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

[netdev-bot+sashiko]

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

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

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] Only one capture-control read is checked; the other register
  reads in the mirror helpers still ignore errors.
- [Medium] The new check in b53_mirror_add()
  (drivers/net/dsa/b53/b53_common.c:2553-2558) treats MIRROR_EN and…
- [Medium] b53_mirror_add() ignores the results of both hardware writes
  and always returns 0: the source-mask write `b53_write16(dev,…
- [Low] The commit message justifies the b53_mirror_del() hunk with
  "Clear the capture-port field when the final source is removed so a…

Pre-existing issues:
- [Medium] b53_mirror_add() does not reject a second mirror rule for the
  same source port and direction.

--

net: dsa: b53: reject conflicting mirror destinations

This commit makes b53_mirror_add() read B53_MIR_CAP_CTL before it touches
the source masks. If mirroring is already enabled toward a different
capture port, the new rule is rejected with -EOPNOTSUPP and an extack
message. b53_mirror_del() now clears the whole CAP_PORT_MASK field along
with MIRROR_EN once both source masks are empty.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0871a10d8cbc7..b8a7b380bac87 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;
> +	}

[Severity: Medium]
This check uses only MIRROR_EN and CAP_PORT_MASK in B53_MIR_CAP_CTL. Can
that register hold stale state that this kernel never programmed?

B53_MIR_CAP_CTL, B53_IG_MIR_CTL and B53_EG_MIR_CTL are only accessed in
b53_mirror_add() and b53_mirror_del(). Neither b53_setup()->b53_reset_switch()
nor the bcm_sf2 setup paths initialize them.

b53_switch_reset() only does a soft reset for is539x() and
BCM58XX/BCM583XX, plus a reset GPIO when one is wired:

    b53_switch_reset_gpio(dev);

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

On chips like BCM53115/53125/53128, BCM5325/5365, BCM63xx or BCM5301x
without a reset GPIO, mirror state left by a bootloader or a previous
kernel survives. kexec is one example: dsa_switch_shutdown() never calls
port_mirror_del, so the old kernel's tc mirror rules stay in hardware.

If MIRROR_EN is left set, every rule to a different port fails with "Only
one mirroring destination is supported", even though no mirror rule is
installed.

Adding and then deleting a rule to the stale destination may not clear
it either. b53_mirror_del() clears only BIT(port), and it clears MIRROR_EN
only when both source masks are empty. Leftover source bits for other
ports keep MIRROR_EN set.

Before this patch the destination was simply overwritten. Should the
mirror registers be reset during setup, or should the driver track
installed rules in software?

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue and wasn't introduced by this patch.
b53_mirror_add() accepts a second rule for the same source port and
direction. What happens to the remaining rule when one of the duplicates
is deleted?

dsa_user_add_cls_matchall_mirred() passes every matchall mirror rule to
the driver without a duplicate check. Each tc prio gets its own matchall
tcf_proto, so two rules A and A' (port 1 ingress to port 2) can share one
hardware bit.

Deleting A clears BIT(1) in B53_IG_MIR_CTL. If no other source remains, it
also clears MIRROR_EN and CAP_PORT_MASK. A' is still reported as offloaded
but does nothing in hardware.

A later rule C (port 1 ingress to port 4) then passes the new check, and
A' traffic goes to port 4.

mt753x_port_mirror_add() and qca8k_port_mirror_add() reject duplicates
using software masks:

    if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
        return -EEXIST;

The new check assumes MIRROR_EN reflects the live rule set. Would b53 need
similar tracking?

>  	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;

[Severity: Medium]
Both b53_write16() results here are ignored, and 0 is always returned.
Can a failed write leave the hardware out of sync with what DSA has
recorded?

b53_mdio_write16() can fail before the target register is written, either
from mdiobus_write_nested() or with -EIO from b53_mdio_op(). SPI and SRAB
writes can fail too.

Suppose the source-mask write succeeds but the B53_MIR_CAP_CTL write
fails. MIRROR_EN stays clear, but dsa_user_add_cls_matchall_mirred() still
adds the rule to mall_tc_list because 0 was returned. A later rule to
another destination passes the new check and redirects the leftover
source. That is the redirect this patch is meant to prevent.

The final B53_MIR_CAP_CTL write in b53_mirror_del() is also unchecked. If
it fails, MIRROR_EN and the old capture port stay set with no sources, and
the new check rejects every other destination.

Should these writes be checked, with the source bit rolled back if the
capture-control write fails?

>  }

[ ... ]

> @@ -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);

[Severity: Medium]
The commit message says the patch will "propagate a failed
capture-control read rather than making the decision from uninitialized
data". This capture-control read is still unchecked, though. Is that
intended?

The b53 backends leave *val unchanged on error. b53_mdio_read16() returns
before assigning it:

    ret = b53_mdio_op(dev, page, reg, REG_MII_ADDR_READ);
    if (ret)
        return ret;

b53_spi_read16() only assigns on success.

reg is reused here. If this read fails, reg still holds the other
direction's B53_IG_MIR_CTL or B53_EG_MIR_CTL word, and that word is written
to B53_MIR_CAP_CTL unconditionally. The source bits land in CAP_PORT_MASK,
and the MIRROR_FILTER bits land in BLK_NOT_MIR/MIRROR_EN.

That usually clears MIRROR_EN while rules remain. A later rule to a
different destination then passes the new check in b53_mirror_add() and
silently redirects the remaining sources.

b53_mirror_add() has the same problem, and there reg is uninitialized:

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

If that read fails, stack garbage ORed with BIT(port) goes into the
source-mask register. That can mirror arbitrary ports or set
DIV_EN/MIRROR_FILTER. MIRROR_EN is then set and 0 is returned.

Later deletes only clear BIT(port). The stray bits keep MIRROR_EN set, so
the new check rejects every other destination.

The earlier reads in b53_mirror_del() that set loc_disable and
other_loc_disable are unchecked as well. Should all of these reads be
checked?

>  	/* 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);

[Severity: Low]
This isn't a bug, but the commit message says:

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

Is this hunk needed for that? The new check in b53_mirror_add() only
applies when MIRROR_EN is set, and b53_mirror_add() always rewrites the
field:

    cap_ctl &= ~CAP_PORT_MASK;
    cap_ctl |= mirror->to_local_port;

The old reg &= ~mirror->to_local_port already cleared the field whenever
it held to_local_port. Could the commit message call this a cleanup
rather than a requirement?

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