Re: [PATCH net v3 6/8] net: dsa: b53: reference count shared mirror source bits

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] This patch adds a per-port software counter of mirror source
  references (struct b53_port::mirror_count[2]).
- [Low] At this commit, the first-reference path in b53_mirror_add()
  (b53_common.c:2573-2582) ignores the return values of b53_read16(loc),…

--

net: dsa: b53: reference count shared mirror source bits

This commit adds a per-port, per-direction reference count for the b53
mirror source bits. The hardware bit is programmed on the first
reference and cleared only when the last reference is removed. A
delete with no matching reference is ignored.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 75033b28100ec..b16167c9510b8 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c

[ ... ]

> @@ -2559,6 +2560,11 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (*mirror_count) {
> +		(*mirror_count)++;
> +		return 0;
> +	}
> +

[Severity: Medium]
Can mirror_count get out of sync with the hardware after a suspend/resume
on bcm_sf2?

On resume, the switch is reset and setup runs again:

drivers/net/dsa/bcm_sf2.c:bcm_sf2_sw_resume() {
    ...
	ret = bcm_sf2_sw_rst(priv);
    ...
	ret = bcm_sf2_cfp_resume(ds);
    ...
	ds->ops->setup(ds);
    ...
}

bcm_sf2_sw_rst() does a watchdog chip reset, or a reset_control
assert/deassert on BCM7278. That puts B53_MIR_CAP_CTL, B53_IG_MIR_CTL
and B53_EG_MIR_CTL back to their defaults. Later in the series,
bcm_sf2_sw_setup() also calls b53_mirror_init(), which writes 0 to all
three registers.

The only place dev->ports[] gets cleared is in b53_reset_switch():

	memset(priv->ports, 0, sizeof(*priv->ports) * priv->num_ports);

and only b53_setup() reaches that, not bcm_sf2_sw_setup(). Unlike CFP
with bcm_sf2_cfp_resume(), nothing replays the mirror configuration.
DSA also keeps the tc matchall filters installed across resume.

So after resume, a port can have mirror_count[dir] >= 1 while its source
bit and MIRROR_EN are both clear. If a new matchall mirror filter is
added on that port and direction, the destination check passes because
the registers read back as 0. It then reaches the early return above
and reports success without programming any register.

Wouldn't the filter show as offloaded while nothing is mirrored, until
every filter on that port and direction is removed?

Before this patch, every add did the full read-modify-write, so adding a
filter after resume reprogrammed the hardware. The later patches
"net: dsa: b53: harden mirror register programming" and "net: dsa: b53:
fix mirror port encoding on BCM5325 and BCM5365" keep this early return.

>  	if (ingress)
>  		loc = B53_IG_MIR_CTL;
>  	else
> @@ -2573,6 +2579,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  	cap_ctl |= MIRROR_EN;
>  	b53_write16(dev, B53_MGMT_PAGE, B53_MIR_CAP_CTL, cap_ctl);
>  
> +	(*mirror_count)++;
> +

[Severity: Low]
At this commit, b53_mirror_add() ignores the return values of
b53_read16(loc), b53_write16(loc) and b53_write16(B53_MIR_CAP_CTL), and
then increments mirror_count anyway.

If one of those bus accesses fails, the count says the source is
programmed when it isn't. Every later add on that port and direction
then takes the if (*mirror_count) shortcut. Will that keep the source
unprogrammed?

In b53_mirror_del(), the count is also decremented before the unchecked
clear.

The later patch "net: dsa: b53: harden mirror register programming" in
this series fixes this. It checks every read and write and only
increments mirror_count after programming succeeds. On the delete side
it logs failures, since .port_mirror_del returns void. A failed clear
there leaves a stale bit with a count of 0. That can be recovered,
because the next add goes from 0 to 1 and reprograms the bit.

>  	return 0;
>  }
>  EXPORT_SYMBOL(b53_mirror_add);

[ ... ]

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