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

On Thu, 2026-08-06 at 14:38 +0000, [email protected] 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 2 potential issue(s) to consider:
> - [Medium] Duplicate CPU packet delivery on IP multicast MDB matches
> - [Low] Inverted teardown sequence between MAC and MDB initialization
> --
> 
> --- Patch [8]: [PATCH net-next v11 8/9] net: dsa: lan9645x: add mdb management ---
> commit c8677da78fd630673d16e31ac732c8854e853dbe
> Author: Jens Emil Schulz Østergaard <[email protected]>
> 
> 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_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index af158d4461aee..fc604edb309b9 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > @@ -70,6 +70,7 @@ static void lan9645x_teardown(struct dsa_switch *ds)
> >       destroy_workqueue(lan9645x->owq);
> >       lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
> >       lan9645x_mac_deinit(lan9645x);
> > +     lan9645x_mdb_deinit(lan9645x);
> 
> [Severity: Low]
> In lan9645x_setup(), lan9645x_mac_init() is called before lan9645x_mdb_init().
> However, lan9645x_teardown() and the err_mac path destroy these in the forward
> order (lan9645x_mac_deinit() followed by lan9645x_mdb_deinit()).
> 
> Since lan9645x_mac_deinit() destroys mact_lock, could calling it before
> lan9645x_mdb_deinit() cause a use-after-destroy if MDB cleanup ever needs to
> take mact_lock?
> 

I will change the order in the next version.

> [ ... ]
> 
> > 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
> [ ... ]
> > +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));
> 
> [Severity: Medium]
> For IP multicast entries, the port mask is stored inline in the high bytes of
> the MAC address via lan9645x_mdb_encode_mac().
> 
> Because new_ports includes the CPU port bit, the hardware will forward the
> packet to the CPU port according to the inline mask. The code also sets the
> MAC_CPU_COPY bit via cpu_copy.
> 
> If the hardware acts on both the inline port mask and the MAC_CPU_COPY bit,
> will this result in duplicate packets being delivered to the CPU?
> 
> Should the CPU port bit be stripped from the new_ports mask before encoding it
> into the MAC address, similarly to how it is stripped from L2 multicast PGID
> entries in lan9645x_mdb_pgid_entry_get()?

No, it will not cause duplicate packets. The chip does not act on both,
the inline mask in the mac table has no effect for the CPU port. The
MAC_CPU_COPY flag in is how IP multicast entries each the CPU.

For L2MC where we must use the limited PGID resource, we strip the CPU
port from the PGID mask and let the mac table entries use cpu_copy as
necessary. This allows better sharing of PGID entries, because two L2
MDB groups differing only in the CPU port can reuse the same PGID.


> 
> > +     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);
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a@microchip.com?part=8