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

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

On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> Subject: net: dsa: lan9645x: add mdb management
> 
> Add support for dsa mdb callbacks. L2 multicast and IP multicast is
> handled differently. IP multicast stores the port group mask inline in
> the mac table. L2 multicast points to a PGID index, which encodes the
> port group mask.
> 
> > 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 000000000000..4cf92887a4eb
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > @@ -0,0 +1,391 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (C) 2026 Microchip Technology Inc.
> > + */
> > +
> > +#include "lan9645x_main.h"
> 
> [ ... ]
> 
> > +static void lan9645x_mdb_encode_mac(unsigned char *mac,
> > +                                 struct lan9645x_mdb_entry *mdb_entry,
> > +                                 enum macaccess_entry_type type)
> > +{
> > +     ether_addr_copy(mac, mdb_entry->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) {
> > +             mac[0] = 0;
> > +             mac[1] = mdb_entry->ports >> 8;
> > +             mac[2] = mdb_entry->ports & 0xff;
> > +     } else if (type == ENTRYTYPE_MACV6) {
> > +             mac[0] = mdb_entry->ports >> 8;
> > +             mac[1] = mdb_entry->ports & 0xff;
> > +     }
> > +}
> 
> Does this code need to mask out BIT(CPU_PORT) from mdb_entry->ports?
> 
> For L2 multicast, the CPU port is explicitly stripped from the PGID portmask
> to avoid conflicting instructions, since CPU_PORT forwarding is handled by the
> cpu_copy flag. Here, the IP multicast portmask is encoded directly into the
> MAC address using the full mdb_entry->ports. Could this cause hardware to
> receive conflicting instructions (forwarding to port 9 and copying to the CPU
> extraction queue), leading to duplicate multicast packets for the CPU?
> 

No this is fine.

> [ ... ]
> 
> > +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++) {
> 
> Is it safe to start the search at PGID_GP_START (defined as CPU_PORT)?
> 
> Hardware uses ANA_PGID(DEST_IDX) to forward unicast traffic, and MAC table
> entries for the CPU have DEST_IDX = 9. When the first L2 multicast group is
> added, it appears this loop will allocate PGID index 9, overwriting the
> unicast portmask for the CPU port. Could this cause all unicast traffic
> destined for the CPU to be flooded to the external physical ports in the
> multicast group instead?
> 

No this is safe, PGID 9 is not used to forward anything to the CPU.

> > +             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_add(struct lan9645x *lan9645x, int chip_port,
> > +                           const unsigned char addr[ETH_ALEN], u16 vid,
> > +                           enum macaccess_entry_type type)
> > +{
> > +     struct lan9645x_pgid_entry *old_pgid, *new_pgid;
> > +     struct lan9645x_mdb_entry *mdb_entry;
> > +     unsigned char mac[ETH_ALEN];
> > +     int err, pgid_index;
> > +     bool cpu_copy;
> > +
> > +     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;
> > +
> > +     mdb_entry->ports |= BIT(chip_port);
> > +
> > +     /* Encode mac for IP mc */
> > +     lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> > +
> > +     /* Update PGID ptr for non-IP entries (L2 multicast) */
> > +     old_pgid = mdb_entry->pgid;
> > +     new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> > +     if (IS_ERR(new_pgid)) {
> > +             /* Out of PGIDs or mem. Continue forwarding to old port
> > +              * group, or remove if fresh mdb_entry.
> > +              */
> > +             mdb_entry->ports &= ~BIT(chip_port);
> > +             if (!mdb_entry->ports)
> > +                     lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> > +
> > +             return PTR_ERR(new_pgid);
> > +     }
> > +     mdb_entry->pgid = new_pgid;
> > +
> > +     cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> > +     pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> > +
> > +     /* Make sure to write on top of existing entry, so we do not disrupt
> > +      * flowing traffic.
> > +      */
> > +     err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> > +                                        mdb_entry->vid, type, cpu_copy);
> > +     lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +     return err;
> > +}
> 
> What happens if lan9645x_mact_learn_cpu_copy() fails and returns an error?
> 
> The software MDB state (mdb_entry->ports and mdb_entry->pgid) is updated and
> the old PGID reference is dropped via lan9645x_pgid_entry_put() before
> verifying if the hardware MAC table update succeeds.
> 
> If the hardware update fails, the software state isn't rolled back, leaving
> them out of sync. Furthermore, the dropped old_pgid reference could allow the
> index to be reallocated to a new multicast group while the hardware still
> points to the old PGID index, potentially causing silent traffic cross-talk.
> 


I will change the mac table write error path, so hw/sw stay synced.


> > +
> > +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 *old_pgid, *new_pgid;
> > +     struct lan9645x_mdb_entry *mdb_entry;
> > +     unsigned char mac[ETH_ALEN];
> > +     int err, pgid_index;
> > +     bool cpu_copy;
> > +
> > +     mdb_entry = lan9645x_mdb_entry_lookup(lan9645x, addr, vid);
> > +     if (!mdb_entry)
> > +             return -ENOENT;
> > +
> > +     if (!(mdb_entry->ports & BIT(chip_port)))
> > +             return 0;
> > +
> > +     mdb_entry->ports &= ~BIT(chip_port);
> > +
> > +     /* Encode mac for IP mc */
> > +     lan9645x_mdb_encode_mac(mac, mdb_entry, type);
> > +
> > +     /* Update PGID ptr for non-IP entries (L2 multicast) */
> > +     old_pgid = mdb_entry->pgid;
> > +     new_pgid = lan9645x_mdb_pgid_entry_get(lan9645x, mdb_entry, type);
> > +     if (IS_ERR(new_pgid)) {
> > +             /* Continue forwarding to old port group. */
> > +             mdb_entry->ports |= BIT(chip_port);
> > +             return PTR_ERR(new_pgid);
> > +     }
> > +     mdb_entry->pgid = new_pgid;
> > +
> > +     if (!mdb_entry->ports) {
> > +             lan9645x_mact_forget(lan9645x, mac, mdb_entry->vid, type);
> > +             lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +             lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> > +             return 0;
> > +     }
> 
> Similar to the addition path above, if lan9645x_mact_forget() fails, its
> return value is ignored, and the software MDB entry and PGID are freed
> anyway. This could also lead to hardware/software state desynchronization.
> 
> > +
> > +     cpu_copy = !!(mdb_entry->ports & BIT(CPU_PORT));
> > +     pgid_index = lan9645x_mdb_pgid_index(mdb_entry, type);
> > +
> > +     err = lan9645x_mact_learn_cpu_copy(lan9645x, pgid_index, mac,
> > +                                        mdb_entry->vid, type, cpu_copy);
> > +     lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +     return err;
> > +}