Re: [PATCH] net: dsa: b53: Enable IGMP and MLD control packet CPU copies

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The new IGMP/MLD CPU-copy setup in b53_enable_mcast_snooping()
  (drivers/net/dsa/b53/b53_common.c) is static and has one caller,…

--

net: dsa: b53: Enable IGMP and MLD control packet CPU copies

This commit enables the IGMP and MLD high-level protocol classifiers in
B53_HL_PRTC_CTRL, so control packets get copied to the CPU even when a
multicast ARL entry matches. This lets the bridge keep learning listeners
after the first MDB entry has been offloaded. BCM5395 uses a 16-bit layout,
BCM531xx/5301x/58xx use a 32-bit layout, and older chips are left alone.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0880310c9ce3f..54844cfadf019 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -836,6 +836,52 @@ void b53_brcm_hdr_setup(struct dsa_switch *ds, int port)
>  }
>  EXPORT_SYMBOL(b53_brcm_hdr_setup);
>  
> +static int b53_enable_mcast_snooping(struct b53_device *dev)
> +{

[ ... ]

> +	/* Older switches use different IGMP controls, not HL_PRTC_CTRL. */
> +	if (!is531x5(dev) && !is5301x(dev) && !is58xx(dev))
> +		return 0;

[ ... ]

> @@ -1334,6 +1380,10 @@ static int b53_setup(struct dsa_switch *ds)
>  			b53_disable_port(ds, port);
>  	}
>  
> +	ret = b53_enable_mcast_snooping(dev);
> +	if (ret)
> +		return ret;
> +
>  	return b53_setup_devlink_resources(ds);
>  }

[Severity: High]
Does this fix reach the Starfighter 2 switches (BCM7445, BCM7278, BCM4908)?

b53_enable_mcast_snooping() is static, and its only caller is b53_setup().
bcm_sf2_sw_probe() sets the chip id and then swaps in its own ops:

drivers/net/dsa/bcm_sf2.c:bcm_sf2_sw_probe() {
    ...
	pdata->chip_id = priv->type;
    ...
	ds->ops = &bcm_sf2_ops;
    ...
}

bcm_sf2_ops sets .setup to bcm_sf2_sw_setup(), so b53_setup() never runs
on these chips. bcm_sf2_sw_setup() does not program B53_HL_PRTC_CTRL:

drivers/net/dsa/bcm_sf2.c:bcm_sf2_sw_setup() {
    ...
	b53_configure_vlan(ds);
	bcm_sf2_enable_acb(ds);

	return b53_setup_devlink_resources(ds);
}

bcm_sf2_ops still has this entry:

	.port_mdb_add		= b53_mdb_add,

So multicast ARL entries still get installed on SF2. After the first
listener's entry exists, the original problem looks unchanged there: later
IGMPv2/MLDv1 reports sent to the group MAC follow the ARL entry and never
reach the bridge.

The resume path, bcm_sf2_sw_resume() calling ds->ops->setup(), has the same
gap.

The is58xx() part of the gate above matches BCM7445_DEVICE_ID and
BCM7278_DEVICE_ID, but devices with those IDs never reach this function. Is
that check dead code? The commit message also says this covers the "newer
integrated RoboSwitch families".

Should the helper be exported and also called from bcm_sf2_sw_setup()?

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