Re: [PATCH net v3 7/8] net: dsa: b53: harden mirror register programming

[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 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 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] This patch adds an unconditional b53_mirror_init() call to
>    bcm_sf2_sw_setup() (drivers/net/dsa/bcm_sf2.c).
> 
> --
> 
> net: dsa: b53: harden mirror register programming
> 
> This commit clears the mirror capture control register and both
> source-control registers during b53 and bcm_sf2 setup. It checks every
> mirror register access and only increments the source reference count
> once hardware programming has succeeded. It also decides whether the
> destination is occupied from the source masks instead of MIRROR_EN.
> 
>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>> index 9e5713015187b..9f54cd414c18f 100644
>> --- a/drivers/net/dsa/bcm_sf2.c
>> +++ b/drivers/net/dsa/bcm_sf2.c
>> @@ -1059,6 +1059,11 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
>>   {
>>   	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>>   	unsigned int port;
>> +	int ret;
>> +
>> +	ret = b53_mirror_init(ds);
>> +	if (ret)
>> +		return ret;
>>   
>>   	/* Enable all valid ports and disable those unused */
>>   	for (port = 0; port < priv->hw_params.num_ports; port++) {
> 
> [Severity: Medium]
> Can this lose the installed mirror rules across suspend/resume?
> 
> The resume path also calls bcm_sf2_sw_setup():
> 
> drivers/net/dsa/bcm_sf2.c:bcm_sf2_sw_resume() {
>      ...
> 	ret = bcm_sf2_cfp_resume(ds);
>      ...
> 	ds->ops->setup(ds);
> 
> 	return 0;
> }
> 
> So every resume now writes 0 to B53_MIR_CAP_CTL, B53_IG_MIR_CTL and
> B53_EG_MIR_CTL. The tc matchall mirror rules stay installed.
> 
> dev->ports[].mirror_count[] also keeps its value. The only thing that
> clears it is the memset in b53_reset_switch(), and the bcm_sf2 path never
> calls that.
> 
> CFP gets restored by bcm_sf2_cfp_resume(), but nothing reprograms the
> mirror registers after resume.
> 
> After resume, b53_mirror_add() would hit this:
> 
> 	if (*mirror_count) {
> 		(*mirror_count)++;
> 		return 0;
> 	}
> 
> A new rule for a port and direction that already has a reference would
> then return success without programming the hardware.
> 
> The new occupancy check would also read all-zero source masks:
> 
> 	if (((reg | other_reg) & MIRROR_MASK) &&
> 	    (cap_ctl & CAP_PORT_MASK) != mirror->to_local_port) {
> 
> That would accept a rule with a different destination while software
> still tracks rules for the old one.
> 
> Doesn't this break the rule that mirror_count[] follows the hardware
> source mask bits? The commit message relies on that when it says
> "Reference counting must not record a first source reference until
> hardware programming has succeeded".
> 
> bcm_sf2_sw_rst() may already clear these registers through EN_CHIP_RST.
> If so, the stale count came from the earlier patch in this series, "net:
> dsa: b53: reference count shared mirror source bits", and this patch makes
> the wipe unconditional.
> 
> If the reset does not clear them, which is the case the commit message
> describes, this patch causes the loss on resume.
> 
> The problem is still there at the end of the series. "net: dsa: b53: fix
> mirror port encoding on BCM5325 and BCM5365" only changes the port and
> capture encoding.
> 
> A related question: bcm_sf2_sw_resume() ignores the return value of
> ds->ops->setup(). If b53_mirror_init() fails during resume, the new early
> return skips the port, VLAN and ACB setup below and no error is reported.
> Is that intended?
> 

Agreed. This overlaps with the patch #6 finding. The respin will restore 
the destination and source masks from retained software state during 
setup, preserving reference counts for installed filters. It will also 
propagate setup failures from bcm_sf2_sw_resume().

pw-bot: cr
-- 
Florian