Re: [PATCH net-next v11 8/9] net: dsa: lan9645x: add mdb management

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

This is a reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=8

> --- Patch [8]: [PATCH net-next v11 8/9] net: dsa: lan9645x: add mdb management ---
> commit f98510fb218c27ef1092f8176a5d379f8499735c
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: lan9645x: add mdb management
>
> This adds the DSA port_mdb_add/port_mdb_del callbacks to the lan9645x
> driver, with a new lan9645x_mdb.c tracking software mdb entries and a pool
> of PGID indices. IP multicast entries encode the port mask into the high
> DMAC bytes, while L2 multicast entries point at an allocated PGID index.
>
> > 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..ea37bb660273c
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > @@ -0,0 +1,398 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (C) 2026 Microchip Technology Inc.
> > + */
> > +
> > +#include "lan9645x_main.h"
> > +
> > +/* HW ignores dest_idx for IPv4/IPv6 types, so we use this dummy index */
> > +#define IP_ENTRY_PGID		0
> > +#define PGID_INDEX(pgid)	((pgid) ? (pgid)->index : IP_ENTRY_PGID)
>
> [ ... ]
>
> > +void lan9645x_mdb_init(struct lan9645x *lan9645x)
> > +{
> > +	INIT_LIST_HEAD(&lan9645x->mdb_entries);
> > +	INIT_LIST_HEAD(&lan9645x->pgid_entries);
> > +	mutex_init(&lan9645x->mdb_lock);
> > +
> > +	/* Use CPU queues to communicate frame classification to the CPU */
> > +	lan_rmw(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_IGMP |
> > +		ANA_CPUQ_CFG_CPUQ_MLD |
> > +		ANA_CPUQ_CFG_CPUQ_IPMC_CTRL,
> > +		lan9645x, ANA_CPUQ_CFG);
> > +}
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Could the commit message mention the forwarding behaviour change that comes
> with this patch?  The changelog only says:
>
>     Add support for dsa mdb callbacks. L2 multicast and IP multicast is
>     handled differently.
>
> but besides the callbacks, lan9645x_mdb_init() writes the global
> ANA_CPUQ_CFG register (IGMP and MLD to LAN9645X_CPUQ_TRAP, IPMC_CTRL to
> LAN9645X_CPUQ_COPY), and lan9645x_port_setup() unconditionally sets
> IGMP_REDIR_ENA / MLD_REDIR_ENA / IPMC_CTRL_COPY_ENA on every non-NPI port.
>
> After this patch IGMP/MLD frames arriving on a user port are trapped to the
> CPU instead of being forwarded by hardware, and lan9645x_offload_fwd_mark()
> in net/dsa/tag_lan9645x.c clears skb->offload_fwd_mark for the trap queue:
>
>     if (cpuq & BIT(LAN9645X_CPUQ_TRAP)) {
>             skb->offload_fwd_mark = 0;
>             return;
>     }
>
> so the software bridge takes over forwarding for those frames.  This happens
> for every non-NPI port regardless of whether any mdb entry is ever
> offloaded.
>

I will update the commit message.

> Also, is lan9645x_mdb_init() the right place for a write to a global
> classification register, given the name suggests only software list
> initialisation?
>

I will move the register write to lan9645x_setup().

> [ ... ]
>
> > +static void lan9645x_mdb_encode_mac(unsigned char *dst, unsigned char *mac,
> > +				    u16 ports, enum macaccess_entry_type type)
> > +{
> > +	ether_addr_copy(dst, mac);
> > +
> > +	/* The HW encodes the portmask in the high bits of the mac for ip
> > +	 * multicast entries, to save on the limited PGID resources.
> > +	 *
> > +	 * IPv4 Multicast DMAC: 0x01005Exxxxxx
> > +	 * IPv6 Multicast DMAC: 0x3333xxxxxxxx
> > +	 *
> > +	 * which gives us 24 or 16 bits to encode the portmask.
> > +	 */
> > +	if (type == ENTRYTYPE_MACV4) {
> > +		dst[0] = 0;
> > +		dst[1] = ports >> 8;
> > +		dst[2] = ports & 0xff;
> > +	} else if (type == ENTRYTYPE_MACV6) {
> > +		dst[0] = ports >> 8;
> > +		dst[1] = ports & 0xff;
> > +	}
> > +}
>
> [ ... ]
>
> > +static struct lan9645x_pgid_entry *
> > +lan9645x_mdb_pgid_entry_create(struct lan9645x *lan9645x, u16 ports)
> > +{
> > +	struct lan9645x_pgid_entry *pgid_entry = NULL;
> > +	int index;
> > +
> > +	for (index = PGID_GP_START; index < PGID_GP_END; index++) {
> > +		bool used = false;
> > +
> > +		list_for_each_entry(pgid_entry, &lan9645x->pgid_entries, list) {
> > +			if (pgid_entry->index == index) {
> > +				used = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!used)
> > +			return lan9645x_pgid_entry_alloc(lan9645x, index,
> > +							 ports);
> > +	}
> > +
> > +	return ERR_PTR(-ENOSPC);
> > +}
>
> [ ... ]
>
> > +static int lan9645x_mdb_update_dest(struct lan9645x *lan9645x,
> > +				    struct lan9645x_mdb_entry *mdb_entry,
> > +				    enum macaccess_entry_type type,
> > +				    struct lan9645x_pgid_entry *new_pgid,
> > +				    u16 new_ports)
> > +{
> > +	unsigned char mac[ETH_ALEN] __aligned(2);
> > +	struct lan9645x_pgid_entry *old_pgid;
> > +	int err, pgid_index;
> > +	bool cpu_copy;
> > +
> > +	old_pgid = mdb_entry->pgid;
> > +	lan9645x_mdb_encode_mac(mac, mdb_entry->mac, new_ports, type);
> > +	cpu_copy = !!(new_ports & BIT(lan9645x->num_phys_ports));
> > +	pgid_index = PGID_INDEX(new_pgid);
> > +
> > +	/* For IP multicast, the hardware lookup uses the DMAC
> > +	 * (01:00:5E:.. / 33:33:..) as the (mac, vid) key, not the encoded mac.
> > +	 * Therefore, this CMD_LEARN will atomically rewrite the existing
> > +	 * hardware entry. We intentionally do not do a forget before learn
> > +	 * sequence, as that would not be atomic, and leave a forwarding gap.
> > +	 */
> > +	err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> > +					   mdb_entry->vid, type, cpu_copy);
> > +	if (err) {
> > +		lan9645x_pgid_entry_put(lan9645x, new_pgid);
> > +		return err;
> > +	}
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is it safe to assume here that a failing lan9645x_mact_learn_cpu_copy()
> means nothing reached the hardware?
>

On a healthy bus I expect no errors at all. An error means the bus
misbehaved, and we can not infer from it whether the command landed.

The mac commands CMD_LEARN/CMD_FORGET complete in a few microseconds, so the
only way to get here with an entry actually written is for the register bus to
fail between the command and the status read. I think that is close to
impossible. But the forget costs nothing here: it is a noop if the entry was
never written, and it fails harmlessly if the bus is still down. I will add the
forget.

> Looking at __lan9645x_mact_learn_cpu_copy() in lan9645x_mac.c, the
> CMD_LEARN is written before the completion poll, and the only failure
> returned is from the poll itself:
>
>     lan_wr(... ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_LEARN), lan9645x, ANA_MACACCESS);
>
>     return lan9645x_mac_wait_for_completion(lan9645x, NULL);
>
> If the LEARN did take effect while the poll timed out, this error path calls
> lan9645x_pgid_entry_put(lan9645x, new_pgid), which for a freshly created
> PGID drops the last reference and returns the index to the free pool used by
> lan9645x_mdb_pgid_entry_create(), and __lan9645x_mdb_add() then destroys the
> fresh mdb entry:
>
>     err = lan9645x_mdb_update_dest(lan9645x, mdb_entry, type, new_pgid,
>                                    new_ports);
>     if (err && !mdb_entry->ports)
>             lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
>
> Does that leave a hardware MAC table entry whose DEST_IDX points at an index
> no software entry references any more, so it can never be forgotten?  When
> lan9645x_pgid_entry_alloc() later reuses that index and rewrites
> ANA_PGID(index) with a different mask, would the orphaned entry start
> forwarding that group to an unrelated port set?
>
> Would attempting lan9645x_mact_forget() for the (mac, vid) before dropping
> the software state be enough to keep the two views in sync?
>

I will add the forget here and a comment.

> > +	mdb_entry->pgid = new_pgid;
> > +	mdb_entry->ports = new_ports;
> > +	lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +	return 0;
> > +}
>
> [ ... ]
>
> > +static int __lan9645x_mdb_del(struct lan9645x *lan9645x, int chip_port,
> > +			      const unsigned char addr[ETH_ALEN], u16 vid,
> > +			      enum macaccess_entry_type type)
> > +{
> > +	struct lan9645x_pgid_entry *new_pgid;
> > +	struct lan9645x_mdb_entry *mdb_entry;
> > +	u16 new_ports;
> > +	int err;
> > +
> > +	mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> > +	if (!mdb_entry)
> > +		return -ENOENT;
> > +
> > +	if (!(mdb_entry->ports & BIT(chip_port)))
> > +		return 0;
> > +
> > +	new_ports = mdb_entry->ports & ~BIT(chip_port);
> > +
> > +	if (!new_ports) {
> > +		/* For IP multicast hardware uses DMAC as key (mac,vid) not
> > +		 * encoded mac.
> > +		 */
> > +		err = lan9645x_mact_forget(lan9645x, mdb_entry->mac,
> > +					   mdb_entry->vid, type);
> > +		if (err)
> > +			return err;
> > +		lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> > +		return 0;
> > +	}
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> For MACV4/MACV6 entries, the CMD_LEARN in lan9645x_mdb_update_dest() uses
> the mask-encoded address from lan9645x_mdb_encode_mac(), but this
> CMD_FORGET uses the plain mdb_entry->mac.  Can the two commands address the
> same hardware entry?

Yes the two commands address the same hardware entry. The encoded bytes are not
part of the (mac, vid) key used for lookups, for entries of type 2 and 3.
Otherwise the HW could not work, because ingressing frames do not carry the
port mask. The MAC which is used for the key depends on the mac table
entry type.

Type 2 (IPv4 multicast):
KEY_MAC = 0x01005E000000 | MACLDATA[23:0]

Type 3 (IPv6 multicast):
KEY_MAC = 0x333300000000 | MACLDATA[31:0]

The CPU access path is no different. Encoding the mac before the forget makes no
difference, because it changes bytes which are unused for the lookup.

>
> The comment says the encoded bytes are not part of the (mac, vid) key, but
> both in-tree drivers for this silicon family re-encode the current mask
> before forgetting.  ocelot_port_mdb_del() in
> drivers/net/ethernet/mscc/ocelot.c does:
>
>     ocelot_encode_ports_to_mdb(addr, mc);
>     ocelot_mact_forget(ocelot, addr, vid);
>
> and lan966x_mdb_ip_del() in
> drivers/net/ethernet/microchip/lan966x/lan966x_mdb.c does the same:
>
>     lan966x_mdb_encode_mac(mac, mdb_entry, type);
>     lan966x_mac_forget(lan966x, mac, mdb_entry->vid, type);
>
> ocelot also does forget-then-learn on every mask change rather than relying
> on LEARN rewriting an entry keyed by a different address.

I intentionally do not forget then learn, as that leaves a forwarding gap.
Overwriting an existing entry updates the group mask atomically.

>
> If the encoded bytes do participate in the MAC table key on the CPU access
> path,

But they do not. The encoding is necessary when learning because you need to
store the mask in the entry for the forwarding engine. But the hash for the
lookup is calculated based on the KEY_MAC.

> then deleting the last member of an IPv4/IPv6 group here frees the
> software mdb_entry while the hardware entry survives and keeps forwarding to
> the old port mask, and every mask change inserts an extra entry instead of
> rewriting the existing one.  Could the encoded address be used for
> CMD_FORGET as the peer drivers do, or the comment be backed by the
> datasheet's MAC table key definition for the MACv4/MACv6 entry types?
>

I will add the mac key derivation above for type 2 and 3 in the comment.

> > +
> > +	/* Update PGID ptr for non-IP entries (L2 multicast) */
> > +	new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, new_ports, type);
> > +	if (IS_ERR(new_pgid))
> > +		/* Continue forwarding to old port group. */
> > +		return PTR_ERR(new_pgid);
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Can this early return leave an mdb entry that can never be reclaimed?
>

Yes this is right. Continuing to forward to the old group is a bad
fallback strategy. We need the mdb_entry->ports to be a subset of the kernel
mdb entry.

Instead I will fallback to forward to the multicast flood mask, when we run out
of PGIDs.

I will change __lan9645x_mdb_del() like so:

If lan9645x_mdb_pgid_entry_get() fails due to exhaustion, then if the pgid is
not shared, we can safely overwrite it and update destinations.

If it is not shared, we are exhausted and can not service the mdb_del. We
fallback to using PGID_MC, and keep mdb_entry->ports updated. And in
__lan9645x_mdb_add() handle the entries on the fallback pgid. This should make
sure entries do not get wedged, we can still configure forwarding to the CPU
on a fallback entry and if a fallback entry is later reconfigured while there is
PGID space available, it will become offloaded again.

> For an L2 multicast group (ENTRYTYPE_LOCKED) with more than one member,
> new_ports is non-zero, so lan9645x_mdb_pgid_entry_get() runs and can return
> -ENOSPC once all general purpose indices between PGID_GP_START and
> PGID_GP_END are taken, or -ENOMEM from lan9645x_pgid_entry_alloc().  On that
> path BIT(chip_port) is never cleared from mdb_entry->ports.
>
> The bridge has already removed the port from the group, and for user ports
> dsa_port_do_mdb_del() in net/dsa/switch.c just propagates the error with no
> refcount bookkeeping and no retry:
>
>     if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) {
>             err = ds->ops->port_mdb_del(ds, port, mdb, db);
>             ...
>             return err;
>     }
>
> so the driver is never asked to delete that port again.  Does hardware then
> keep forwarding the group to a port that has left it, including after that
> port leaves the bridge entirely?
>
> And since mdb_entry->ports can no longer reach zero, is the
>
>     if (!new_ports) {
>
> branch above (the one that calls lan9645x_mact_forget() and
> lan9645x_mdb_entry_dealloc()) unreachable for this (mac, vid), leaking the
> struct lan9645x_mdb_entry, its reference on a PGID index and the hardware
> MAC table entry until lan9645x_mdb_deinit() at teardown?
>
> ocelot_port_mdb_del() orders this differently: it forgets the hardware entry
> and updates mc->ports before attempting the PGID allocation, so an -ENOSPC
> there cannot leave stale forwarding.  The add path here does clean up on
> failure; would the same be appropriate for the delete path?
>
> > +
> > +	return lan9645x_mdb_update_dest(lan9645x, mdb_entry, type, new_pgid,
> > +					new_ports);
> > +}
>
> [ ... ]
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > index 0548904dc4bd2..407d7eb1d0640 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > @@ -210,8 +210,17 @@ int lan9645x_port_setup(struct dsa_switch *ds, int port)
> >  		ANA_PORT_CFG_PORTID_VAL,
> >  		lan9645x, ANA_PORT_CFG(p->chip_port));
> >  
> > -	if (p->chip_port != lan9645x->npi)
> > +	if (p->chip_port != lan9645x->npi) {
> >  		lan9645x_vlan_set_hostmode(p);
> >  
> > +		lan_rmw(ANA_CPU_FWD_CFG_IGMP_REDIR_ENA_SET(true) |
> > +			ANA_CPU_FWD_CFG_MLD_REDIR_ENA_SET(true) |
> > +			ANA_CPU_FWD_CFG_IPMC_CTRL_COPY_ENA_SET(true),
> > +			ANA_CPU_FWD_CFG_IGMP_REDIR_ENA |
> > +			ANA_CPU_FWD_CFG_MLD_REDIR_ENA |
> > +			ANA_CPU_FWD_CFG_IPMC_CTRL_COPY_ENA,
> > +			lan9645x, ANA_CPU_FWD_CFG(p->chip_port));
> > +	}
> > +
> >  	return 0;
> >  }
>
> This is the per-port half of the forwarding change discussed above at
> lan9645x_mdb_init().
>
> Cross-instance finding from sashiko-gemini
(0d61a27e1fab704dbadd735055165f5f04176e88a680485b744c1f2e2be051cf):
> Duplicate CPU packet delivery on IP multicast MDB matches