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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/31/26 14:56, netdev-bot+sashiko@kernel.org wrote:
> 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);
> 
> [ ... ]
> 

Agreed. The reference counts survive SF2 resume while hardware mirror 
state is cleared. I’ll restore the destination and active source masks 
during setup, preserving the counts for filters DSA retains, and 
propagate setup failures from resume.

I’ll also fold in the register-access hardening so counts advance only 
after successful programming.

pw-bot: cr
-- 
Florian