Re: [PATCH net v3 2/2] net: dsa: b53: Enable legacy IGMP snooping controls

[netdev-bot+sashiko@kernel.org]

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 commit message says: 'These legacy classifiers have fixed
  forwarding behavior, including query flooding which excludes the IMP…

--

net: dsa: b53: Enable legacy IGMP snooping controls

This enables the IP-layer IGMP classifier (bit 3 of the global management
configuration register) on BCM5325E, BCM5365, BCM5389, BCM5397/5398 and
BCM63xx, which were previously skipped in b53_enable_mcast_snooping(). On
BCM6318 and BCM63268 it also clears the IGMP redirection enable to select
snooping mode. BCM5325M and unidentified BCM5325 variants remain excluded.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index e54f6d4d1f7bb..957a1501d3f49 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -841,12 +841,29 @@ int b53_enable_mcast_snooping(struct b53_device *dev)
>  	u16 val16;
>  	u32 val;
>  	int ret;
> +	u8 val8;
>  
> -	/* Older switches use different IGMP controls, not HL_PRTC_CTRL. */
> -	if (is5325(dev) || is5365(dev) || dev->chip_id == BCM5389_DEVICE_ID ||
> -	    is5397_98(dev) || is63xx(dev))
> +	/* BCM5325M snooping uses the mirroring engine instead. */
> +	if (is5325(dev) && !is5325e(dev))
>  		return 0;
>  
> +	/* Older switches classify IGMP in the global management register.
> +	 * They do not provide the separate MLD controls of newer switches.
> +	 */
> +	if (is5325e(dev) || is5365(dev) ||
> +	    dev->chip_id == BCM5389_DEVICE_ID || is5397_98(dev) || is63xx(dev)) {
> +		ret = b53_read8(dev, B53_MGMT_PAGE, B53_GLOBAL_CONFIG, &val8);
> +		if (ret)
> +			return ret;
> +
> +		val8 |= GC_IGMP_IP_EN;
> +		/* Disable the separate IGMP redirection mode on BCM6318/63268. */
> +		if (is6318_268(dev))
> +			val8 &= ~GC_IGMP_RED_EN_6318_268;
> +
> +		return b53_write8(dev, B53_MGMT_PAGE, B53_GLOBAL_CONFIG, val8);
> +	}

[Severity: High]
The commit message says:

  These legacy classifiers have fixed forwarding behavior, including
  query flooding which excludes the IMP on BCM5397/5398 and BCM63xx.

The message describes this as fixed hardware behavior. Isn't it actually
something this patch turns on?

Before this change, b53_enable_mcast_snooping() returned early on
BCM5397/5398 and BCM63xx, so GC_IGMP_IP_EN stayed clear. IGMP general
queries to 01:00:5e:00:00:01 were then forwarded as ordinary multicast.
The IMP has PORT_CTRL_RX_MCST_EN set, and the bridge never installs MDB
entries for 224.0.0.x, so those queries reached the CPU.

This path now runs unconditionally at setup:

  b53_setup()
    b53_enable_mcast_snooping()
      val8 |= GC_IGMP_IP_EN;
      b53_write8(dev, B53_MGMT_PAGE, B53_GLOBAL_CONFIG, val8);

On BCM6318/63268, snooping mode is also selected explicitly. With that in
place, would queries from an external querier on any switch port stop
reaching the CPU on these chips?

If so, these cases look affected:

  - the bridge can no longer learn multicast router ports or detect other
    queriers from the queries it receives

  - the host IGMP stack on br0, or on standalone user ports (for example
    an igmpproxy upstream on a WAN port), never answers general queries,
    so the upstream router would expire the host's memberships after the
    group membership interval

  - on standalone ports, where the port VLAN mask only allows the IMP, a
    query flooded excluding the IMP would reach no port at all

This patch carries a Fixes: tag and would likely be picked up for stable.
Should BCM5397/5398 and BCM63xx keep returning early until queries can
still be delivered to the IMP with the classifier enabled?

> +
>  	/* IGMPv2 and MLDv1 reports use the group's destination MAC. Once
>  	 * the first listener is offloaded, ordinary multicast forwarding
>  	 * can keep subsequent reports from reaching the bridge CPU.

[ ... ]

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