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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 11/3/26 01:16, Paolo Abeni wrote:
> 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.

This is not a false positive, I came up with this additional patch to 
please the preview AI reviews which I found legitimate, however in 
hindsight this is definitively net-next material.

Can you pick up patch #1 on its own if you and other reviewers are now 
happy about it?

Thanks!
-- 
Florian