Re: [PATCH net-next v14 7/9] net: dsa: lan9645x: add mdb management

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-10-12 at 11:24 +0000, netdev-bot+sashiko@kernel.org wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 2
> 
> 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).
> 
> - [Medium] Separate IP multicast groups that map to the same multicast
>   MAC collapse into one per-port bit, so leaving one group removes…
> - [Medium] Several error paths deliberately keep a PGID reference, but
>   nothing owns it afterwards and it is only freed in…
> - [Low] A comment in lan9645x_setup()
>   (drivers/net/dsa/microchip/lan9645x/lan9645x_main.c, lines 210-214,…
> - [Low] PGID destination-mask writes in lan9645x_pgid_entry_alloc() and
>   in the overwrite-in-place branch of __lan9645x_mdb_del() go through…
> 
> --
> 
> net: dsa: lan9645x: add mdb management
> 
> This patch adds .port_mdb_add and .port_mdb_del support to the lan9645x DSA
> driver. IP multicast entries store the port mask inline in the mac table
> entry. L2 multicast entries point at a refcounted PGID, and CPU membership
> is carried by MAC_CPU_COPY. The patch also redirects IGMP and MLD to the
> CPU and copies IPv4 multicast control frames to it.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index c55161f8bafff..a0f48f3ad9aff 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> 
> [ ... ]
> 
> > @@ -253,6 +255,17 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >                       lan9645x, ANA_CPUQ_8021_CFG(i));
> >       }
> > 
> > +     /* Use CPU queues to communicate frame classification to the CPU */
> > +     lan_rmw(ANA_CPUQ_CFG_CPUQ_MAC_COPY_SET(LAN9645X_CPUQ_DEF) |
> > +             ANA_CPUQ_CFG_CPUQ_IGMP_SET(LAN9645X_CPUQ_TRAP) |
> > +             ANA_CPUQ_CFG_CPUQ_MLD_SET(LAN9645X_CPUQ_TRAP) |
> > +             ANA_CPUQ_CFG_CPUQ_IPMC_CTRL_SET(LAN9645X_CPUQ_COPY),
> > +             ANA_CPUQ_CFG_CPUQ_MAC_COPY |
> > +             ANA_CPUQ_CFG_CPUQ_IGMP |
> > +             ANA_CPUQ_CFG_CPUQ_MLD |
> > +             ANA_CPUQ_CFG_CPUQ_IPMC_CTRL,
> > +             lan9645x, ANA_CPUQ_CFG);
> > +
> 
> [Severity: Low]
> 
> Is the comment above the ANA_FLOODING_IPMC write in lan9645x_setup() still
> accurate after this change?
> 
> lan9645x_setup() {
>     ...
>         /* Setup flooding PGIDs for IPv4/IPv6 multicast. Control and dataplane
>          * use the same masks. Control frames are redirected to CPU, and
>          * the network stack is responsible for forwarding these.
>     ...
> }
> 
> This patch copies IPv4 multicast control frames to the CPU instead of
> redirecting them. lan9645x_port_setup() sets
> ANA_CPU_FWD_CFG_IPMC_CTRL_COPY_ENA, and IPMC_CTRL is mapped to
> LAN9645X_CPUQ_COPY here.
> 
> lan9645x_offload_fwd_mark() in net/dsa/tag_lan9645x.c only clears
> skb->offload_fwd_mark for LAN9645X_CPUQ_TRAP:
> 
>         if (cpuq & BIT(LAN9645X_CPUQ_TRAP)) {
>                 skb->offload_fwd_mark = 0;
>                 return;
>         }
> 
> So the hardware forwards these copied frames through
> FLD_MC4_CTRL=PGID_MC, and the stack does not. Only IGMP and MLD are
> redirected.
> 
> The commit message gets this right. Could the code comment be updated to
> match?
> 
> >       /* Reserve 1664 bytes, 26 buffer cells, per (port, prio) for source
> >        * tracking (resource 0, indices 0..95) and destination tracking
> >        * (resource 2, indices 512..607). These are access watermarks, not
> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > new file mode 100644
> > index 0000000000000..ca333d43904fd
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > @@ -0,0 +1,582 @@
> 
> [ ... ]
> 
> > +static struct lan9645x_pgid_entry *
> > +lan9645x_pgid_entry_alloc(struct lan9645x *lan9645x, int index, u16 ports)
> > +{
> 
> [ ... ]
> 
> > +     dev_dbg(lan9645x->dev, "index=%d ports=0x%x\n", pgid_entry->index,
> > +             pgid_entry->ports);
> > +
> > +     lan_rmw(ANA_PGID_PGID_SET(pgid_entry->ports),
> > +             ANA_PGID_PGID, lan9645x,
> > +             ANA_PGID(pgid_entry->index));
> > +
> > +     return pgid_entry;
> > +}
> 
> [Severity: Low]
> 
> lan9645x_pgid_entry_put() leaves the old destination mask in hardware on
> purpose and relies on this write to replace it when the index is reused.
> lan_rmw() only does WARN_ON_ONCE() when regmap_update_bits() fails, and
> lan9645x_pgid_entry_alloc() returns success either way.
> 
> If this write is lost, would the new group forward to the previous group's
> ports?
> 
> The overwrite in place branch in __lan9645x_mdb_del() has the same shape.
> It updates mdb_entry->pgid->ports before an unchecked lan_rmw(), so the
> departing port would keep receiving traffic.
> 
> This follows the lan_wr()/lan_rmw() convention used across the driver, and
> in lan966x and ocelot too. It is also not clear that this regmap can fail
> at all. So this is more of a hardening question than a defect in the mdb
> logic.
> 
> [ ... ]
> 
> > +static int lan9645x_mdb_widen_dest(struct lan9645x *lan9645x,
> > +                                struct lan9645x_mdb_entry *mdb_entry,
> > +                                enum macaccess_entry_type type,
> > +                                struct lan9645x_pgid_entry *new_pgid,
> > +                                int pgid_index, u16 new_ports)
> > +{
> > +     struct lan9645x_pgid_entry *old_pgid = mdb_entry->pgid;
> > +     int err;
> > +
> > +     lockdep_assert_held(&lan9645x->mdb_lock);
> > +
> > +     err = lan9645x_mdb_write_dest(lan9645x, mdb_entry, type, pgid_index,
> > +                                   new_ports);
> > +     if (err) {
> 
> [ ... ]
> 
> > +             /* A failed write leaves the hardware state unknown, the mac
> > +              * entry may point at old_pgid or new_pgid. Keep the reference
> > +              * taken on new_pgid, so its index cannot be handed to an
> > +              * unrelated group. If the entry already holds new_pgid, e.g.
> > +              * when the CPU port module joins, drop the extra reference.
> > +              */
> > +             if (new_pgid == old_pgid)
> > +                     lan9645x_pgid_entry_put(lan9645x, new_pgid);
> > +             return err;
> > +     }
> > +     mdb_entry->pgid = new_pgid;
> > +     mdb_entry->ports = new_ports;
> > +     lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +     return 0;
> > +}
> 
> [Severity: Medium]
> 
> If lan9645x_mdb_write_dest() fails and new_pgid != old_pgid, the reference
> on new_pgid is kept but never stored in mdb_entry->pgid. What drops it
> later?
> 
> Later successful updates only put mdb_entry->pgid, so this reference seems
> to be released only in lan9645x_mdb_deinit().
> 
> A few other error paths do the same.
> 
> lan9645x_mdb_narrow_dest() switches mdb_entry->pgid to new_pgid before the
> write. On failure it returns without putting old_pgid:
> 
>                 if (old_pgid != new_pgid)
>                         return;
> 
> In __lan9645x_mdb_del(), when CMD_FORGET fails for the last port:
> 
>                         mdb_entry->pgid = NULL;
>                 }
>                 lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> 
> In __lan9645x_mdb_add(), when both the learn and the cleanup forget fail
> for a fresh entry, the new_pgid reference is kept and the mdb entry is
> freed.
> 
> Each transient ANA_MACACCESS poll timeout or regmap read error would then
> use up one index from the PGID_GP_START..PGID_GP_END pool for good. A
> shared PGID that holds an orphaned reference can also never get back to
> refcount 1, which disables the overwrite in place branch in
> __lan9645x_mdb_del().
> 
> The v14 changelog explains why the reference is held while the hardware
> state is uncertain. Is there anything that releases it once a later
> successful CMD_LEARN or CMD_FORGET for the same (mac, vid) makes the state
> known again? Without that, could L2 multicast adds eventually fail with
> -ENOSPC?
> 

Dismissed, intended.

Correct, and intended. These paths are only reached when a MAC table
command fails, i.e. a register bus error or a command that never
completes. The hardware may then still reference the PGID, and handing
its index to another group could forward that group to the wrong ports.
Keeping the index reserved until the driver is unbound is the safe
outcome, and it costs one of the 48 general purpose PGIDs per failure.
When the pool runs out, a new group gets -ENOSPC and is not offloaded,
and a group already on the flood mask stays on PGID_MC, so nothing is
forwarded incorrectly.


Releasing the reference after a later successful command would need
per entry state for a stale PGID, and would still not cover the delete
and fresh add paths, where the entry is freed and no later command for
it follows. As for the VLAN table, the driver does not attempt runtime
recovery after a register bus failure.



> [ ... ]
> 
> > +static int __lan9645x_mdb_add(struct lan9645x *lan9645x, int chip_port,
> > +                           const unsigned char addr[ETH_ALEN], u16 vid,
> > +                           enum macaccess_entry_type type)
> > +{
> 
> [ ... ]
> 
> > +     mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> > +     if (!mdb_entry) {
> > +             mdb_entry = lan9645x_mdb_entry_alloc(lan9645x, addr, vid);
> > +             if (IS_ERR(mdb_entry))
> > +                     return PTR_ERR(mdb_entry);
> > +     }
> > +
> > +     if (mdb_entry->ports & BIT(chip_port))
> > +             return 0;
> 
> [Severity: Medium]
> 
> What happens when two IP groups on the same port map to the same multicast
> MAC?
> 
> br_switchdev_mdb_populate() converts each group with ip_eth_mc_map() or
> ipv6_eth_mc_map(). For example, 239.1.1.1 and 239.129.1.1 both become
> 01:00:5e:01:01:01.
> 
> For user ports, DSA passes the call straight to the driver without
> refcounting:
> 
> net/dsa/switch.c:dsa_port_do_mdb_add() {
>     ...
>         /* No need to bother with refcounting for user ports */
>         if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) {
>                 err = ds->ops->port_mdb_add(ds, port, mdb, db);
>     ...
> }
> 
> The second join returns 0 here and nothing records it. When the first
> group leaves or expires, __lan9645x_mdb_del() runs:
> 
>         new_ports = mdb_entry->ports & ~BIT(chip_port);
> 
> It then either narrows the destination or forgets the entry. Does the port
> then stop receiving traffic for the group it is still a member of?
> 
> The bridge does not send another add while the remaining group keeps being
> refreshed. The loss would last until that group expires and is created
> again. With the bridge support patch later in the series, ordinary
> IGMP/MLD snooping looks like enough to trigger this.
> 
> Does the mdb entry need a per-port membership count here?
> 

Dismissed, preexisting.

This is an existing issue that can only be fixed in the bridge core.
The limitations is shared with ocelot/felix, mv88e6xxx, lan966x
and sparx5, which also keep one bit per port. A per port count is not
correct either. The bridge sends SWITCHDEV_OBJ_ID_PORT_MDB adds for port
groups that already exist without a matching delete, e.g. on mdb
replace, on IGMPv3/MLDv2 source list changes, on source timer expiry and
on (S, G) blocked flag changes:

  bridge mdb add dev br0 port swp1 grp 01:02:03:04:05:06 permanent vid 10
  bridge mdb replace dev br0 port swp1 grp 01:02:03:04:05:06 permanent vid 10
  bridge mdb del dev br0 port swp1 grp 01:02:03:04:05:06 vid 10

reaches the driver as add, add, del. Counting adds would leave the port
in the group after the bridge removed it. Since switchdev_obj_port_mdb
only carries (MAC, VID), the driver cannot tell an aliasing group from a
re-notification, so neither scheme is right in the driver.


> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com