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

[Jonas Gorski]

On Wed, Oct 28, 2026 at 1:10 AM Florian Fainelli
<florian.fainelli@xxxxxxxxxxxx> wrote:
>
> IGMPv2 reports and MLDv1 reports are addressed to the multicast group
> itself. Once the first listener is installed in the ARL table, subsequent
> reports can follow that entry without reaching the CPU. The bridge then
> fails to learn additional listeners, although static MDB forwarding works.
>
> Enable the high-level protocol classifiers so that IGMP and MLD control
> packets are copied to the CPU independently of the multicast ARL lookup.
> Retain normal L2 forwarding by clearing the CPU-only forwarding modes,
> and preserve unrelated protocol-control fields.
>
> Handle the BCM5395's 16-bit register and different classifier fields
> separately from the 32-bit layout on the BCM531xx, BCM5301x and newer
> integrated RoboSwitch families, including BCM7445, BCM7278 and BCM4908.
> Export the helper and call it from bcm_sf2_sw_setup() as well, since SF2
> uses its own setup operation instead of b53_setup(). This also restores
> the classifiers after the switch reset on resume. Leave older devices
> with different IGMP controls untouched, and propagate register access
> failures during setup.
>
> The SF2 setup integration and BCM4908 chip gate were updated with
> assistance from OpenAI Codex in response to review feedback.
>
> Fixes: 5d65b64a3d97 ("net: dsa: b53: Add support for MDB")
> Signed-off-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
> ---
> Changes in v2:
>
> - export the helper and call it from bcm_sf2.c as well as spotted by
>   Sashiko
>
>  drivers/net/dsa/b53/b53_common.c | 52 ++++++++++++++++++++++++++++++++
>  drivers/net/dsa/b53/b53_priv.h   |  1 +
>  drivers/net/dsa/b53/b53_regs.h   | 17 +++++++++++
>  drivers/net/dsa/bcm_sf2.c        |  5 +++
>  4 files changed, 75 insertions(+)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0880310c9ce3..51784432d266 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -836,6 +836,54 @@ void b53_brcm_hdr_setup(struct dsa_switch *ds, int port)
>  }
>  EXPORT_SYMBOL(b53_brcm_hdr_setup);
>
> +int b53_enable_mcast_snooping(struct b53_device *dev)
> +{
> +       u32 val;
> +       u16 val16;
> +       int ret;

I'm surprised to be the first to say that, but ... reverse christmas
tree? Or is this not a thing anymore?

> +
> +       /* BCM5395 has a narrower register and a single IGMP classifier.
> +        * Its ICMPv6 classifier also covers MLD.
> +        */
> +       if (dev->chip_id == BCM5395_DEVICE_ID) {
> +               ret = b53_read16(dev, B53_MGMT_PAGE, B53_HL_PRTC_CTRL, &val16);
> +               if (ret)
> +                       return ret;
> +
> +               val16 |= HL_PRTC_IGMP_IP_EN_5395 | HL_PRTC_ICMPV6_EN;
> +               val16 &= ~(HL_PRTC_IGMP_FWD_MODE_5395 |
> +                          HL_PRTC_ICMPV6_FWD_MODE);
> +
> +               return b53_write16(dev, B53_MGMT_PAGE, B53_HL_PRTC_CTRL,
> +                                  val16);
> +       }
> +
> +       /* Older switches use different IGMP controls, not HL_PRTC_CTRL. */
> +       if (!is531x5(dev) && !is5301x(dev) && !is58xx(dev) &&
> +           dev->chip_id != BCM4908_DEVICE_ID)
> +               return 0;

I would move this to top, and probably check for those that don't have
it. Basically go from oldest to newest generation code wise.

> +
> +       ret = b53_read32(dev, B53_MGMT_PAGE, B53_HL_PRTC_CTRL, &val);
> +       if (ret)
> +               return ret;
> +
> +       /* 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.
> +        * Copy control traffic to the CPU independently of the ARL lookup,
> +        * retaining L2 forwarding rather than selecting CPU-only traps.
> +        */

Should this comment be also in the 5395 case? Feels like it would apply to both.

> +       val |= HL_PRTC_IGMP_RPTLVE_EN | HL_PRTC_IGMP_QRY_EN |
> +              HL_PRTC_IGMP_UKN_EN | HL_PRTC_MLD_RPTDONE_EN |
> +              HL_PRTC_MLD_QRY_EN;

I would also break out the "Copy control" part and move it here, to
make it more obvious that these bits control this, and clearer that 0
means copy to CPU instead of trap to CPU.

> +       val &= ~(HL_PRTC_IGMP_RPTLVE_FWD_MODE | HL_PRTC_IGMP_QRY_FWD_MODE |
> +                HL_PRTC_IGMP_UKN_FWD_MODE | HL_PRTC_MLD_RPTDONE_FWD_MODE |
> +                HL_PRTC_MLD_QRY_FWD_MODE);
> +
> +       return b53_write32(dev, B53_MGMT_PAGE, B53_HL_PRTC_CTRL, val);
> +}
> +EXPORT_SYMBOL(b53_enable_mcast_snooping);
> +
>  static void b53_enable_cpu_port(struct b53_device *dev, int port)
>  {
>         u8 port_ctrl;
> @@ -1334,6 +1382,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);
>  }

(snip)

Best regards,
Jonas