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

[netdev-bot+sashiko@kernel.org]

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?

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