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

[Paolo Abeni <pabeni@redhat.com>]

On 11/2/26 01:09, netdev-bot+sashiko@kernel.org wrote:
> 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
@Florian: could you please comment here? I'm unsure how much legit the above
comment is.

Also this series is IMHO borderline for 'net' as it ends up enabling a feature
that never worked. Would you consider it for net-next without fixes tags?

In any case, if the AI comments is a false positive, no need to repost.

/P