Re: [PATCH net-next v2 7/9] net: dsa: lan9645x: add mac table integration

[Jens Emil Schulz Ostergaard]

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.
> ---
> net: dsa: lan9645x: add mac table integration
> 
> Add MAC table support, and dsa fdb callback integration. The mactable is
> keyed on (vid,mac) and each bucket has 4 slots. A mac table entry typically
> points to a PGID index, the first 9 of which represent a front port.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> > index 000000000000..6335714dca21
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> [ ... ]
> > +int lan9645x_mact_entry_add(struct lan9645x *lan9645x, int pgid,
> > +                         const unsigned char *mac, u16 vid)
> > +{
> > +     struct lan9645x_mact_entry *entry;
> > +     int err;
> > +
> > +     mutex_lock(&lan9645x->mact_lock);
> > +
> > +     /* Users can not move (vid,mac) to a different port, without removing
> > +      * the original entry first. But we overwrite entry in HW, and update
> > +      * software pgid for good measure.
> > +      */
> > +     entry = lan9645x_mact_entry_find(lan9645x, mac, vid);
> > +     if (entry) {
> > +             entry->common.pgid = pgid;
> > +             goto mac_learn;
> > +     }
> > +
> > +     entry = lan9645x_mact_entry_alloc(lan9645x, mac, vid, pgid,
> > +                                       ENTRYTYPE_LOCKED);
> > +     if (!entry) {
> > +             mutex_unlock(&lan9645x->mact_lock);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     list_add_tail(&entry->list, &lan9645x->mac_entries);
> > +
> > +mac_learn:
> > +     err = __lan9645x_mact_learn(lan9645x, pgid, mac, vid, ENTRYTYPE_LOCKED);
> > +     if (err)
> > +             lan9645x_mact_entry_dealloc(lan9645x, entry);
> 
> If a pre-existing entry is updated but the hardware learning command fails,
> will this inadvertently delete the valid software entry and cause the hardware
> and software tracking to become desynchronized?
> 

I will rearrange so they stay synced.

> > +
> > +     mutex_unlock(&lan9645x->mact_lock);
> > +     return err;
> > +}
> > +
> > +int lan9645x_mact_entry_del(struct lan9645x *lan9645x, int pgid,
> > +                         const unsigned char *mac, u16 vid)
> > +{
> > +     struct lan9645x_mact_entry *entry;
> > +     int err;
> > +
> > +     mutex_lock(&lan9645x->mact_lock);
> > +
> > +     entry = lan9645x_mact_entry_find(lan9645x, mac, vid);
> > +     if (!entry) {
> > +             mutex_unlock(&lan9645x->mact_lock);
> > +             return -ENOENT;
> > +     }
> > +
> > +     WARN_ON(entry->common.pgid != pgid);
> > +     lan9645x_mact_entry_dealloc(lan9645x, entry);
> > +     err = __lan9645x_mact_forget(lan9645x, mac, vid, ENTRYTYPE_LOCKED);
> 
> Does this permanently remove the software entry even if the hardware
> command to forget the entry fails?
> 

Same here, I can rearrange so they stay synced.

> > +
> > +     mutex_unlock(&lan9645x->mact_lock);
> > +     return err;
> > +}
> [ ... ]
> > +void lan9645x_mac_deinit(struct lan9645x *lan9645x)
> > +{
> > +     mutex_destroy(&lan9645x->mact_lock);
> > +}
> 
> Are the static MAC entries in the mac_entries list leaked here during driver
> teardown, or are they freed elsewhere?
> 

I thought DSA core would have removed them for me, but it looks like that may not be
the case. I will add dealloc during deinit.

> > +
> > +int lan9645x_mact_dsa_dump(struct lan9645x *lan9645x, int port,
> > +                        dsa_fdb_dump_cb_t *cb, void *data)
> > +{
> > +     struct lan9645x_mact_entry entry = {};
> > +     u32 mach, macl, maca;
> > +     int err = 0;
> > +     u32 autoage;
> [ ... ]
> > +     while (1) {
> > +             /* NOTE: we rely on mach, macl and type being set correctly in
> > +              * the registers from previous round, vis a vis the GET_NEXT
> > +              * semantics, so locking entire loop is important.
> > +              */
> > +             lan_wr(ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_GET_NEXT) |
> > +                    ANA_MACACCESS_ENTRYTYPE_SET(entry.common.type),
> > +                    lan9645x, ANA_MACACCESS);
> > +
> > +             if (lan9645x_mac_wait_for_completion(lan9645x, &maca))
> > +                     break;
> 
> If the hardware completion times out, the loop breaks out and returns the
> local err variable which is initialized to 0. Will this suppress the hardware
> timeout error code and incorrectly report a successful dump?
> 

Yes, I will return the error.

> > +
> > +             if (ANA_MACACCESS_VALID_GET(maca) == 0)
> > +                     break;
> > +
> > +             mach = lan_rd(lan9645x, ANA_MACHDATA);
> > +             macl = lan_rd(lan9645x, ANA_MACLDATA);
> > +
> > +             lan9645x_mact_parse(mach, macl, maca, &entry.common);
> > +
> > +             if (ANA_MACACCESS_DEST_IDX_GET(maca) == port &&
> > +                 entry.common.type == ENTRYTYPE_NORMAL) {
> 
> Since static FDB entries are programmed with ENTRYTYPE_LOCKED, will this
> condition exclude them from the FDB dump, preventing them from showing up
> during bridge FDB queries?
> 

Yes that is intentional. This is for the dynamic entries which the bridge core does not
know about.

> > +                     if (entry.common.key.vid > VLAN_MAX)
> > +                             entry.common.key.vid = 0;
> > +
> > +                     err = cb(entry.common.key.mac, entry.common.key.vid,
> > +                              false, data);
> > +                     if (err)
> > +                             break;
> > +             }
> > +     }
> > +
> > +     /* Remove aging filters and restore aging */
> > +     lan_wr(0, lan9645x, ANA_ANAGEFIL);
> > +     lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(ANA_AUTOAGE_AGE_PERIOD_GET(autoage)),
> > +             ANA_AUTOAGE_AGE_PERIOD,
> > +             lan9645x, ANA_AUTOAGE);
> 
> Does restoring the ANA_AUTOAGE register using the autoage value saved at the
> start of the function race with other operations setting the aging time?
> 
> If the aging time is updated concurrently without taking mact_lock, its newly
> configured aging period might be overwritten here.
> 

AFAIK both run under rtnl so we do not have a race. But I will take the mact_lock during
set_ageing_time.

> > +
> > +     mutex_unlock(&lan9645x->mact_lock);
> > +
> > +     return err;
> > +}
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index 046c95a72242..32c0301030a4 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> [ ... ]
> > +static int lan9645x_fdb_del(struct dsa_switch *ds, int port,
> > +                         const unsigned char *addr, u16 vid,
> > +                         struct dsa_db db)
> > +{
> > +     struct net_device *br = lan9645x_db2bridge(db);
> > +     struct dsa_port *dp = dsa_to_port(ds, port);
> > +     struct lan9645x *lan9645x = ds->priv;
> > +     int err;
> > +
> > +     if (IS_ERR(br))
> > +             return PTR_ERR(br);
> > +
> > +     if (dsa_port_is_cpu(dp) && !br &&
> > +         dsa_fdb_present_in_other_db(ds, port, addr, vid, db))
> > +             return 0;
> > +
> > +     if (!vid)
> > +             vid = lan9645x_vlan_unaware_pvid(!!br);
> > +
> > +     if (dsa_port_is_cpu(dp))
> > +             return lan9645x_mact_forget(lan9645x, addr, vid,
> > +                                         ENTRYTYPE_LOCKED);
> > +
> > +     err = lan9645x_mact_entry_del(lan9645x, port, addr, vid);
> > +     if (err == -ENOENT) {
> > +             dev_dbg(lan9645x->dev,
> > +                     "fdb not found port=%d addr=%pM vid=%u\n", port, addr,
> > +                     vid);
> > +             return 0;
> > +     }
> > +
> > +     return err;
> > +}
> 
> Since lan9645x_mact_entry_del returns -ENOENT when a MAC address is not found
> in the software tracking list, and the software list only contains statically
> added entries, won't this intercept attempts to manually delete dynamically
> learned MAC addresses and return success without actually removing them from
> the hardware table?

No I do not think this is how it works. Dynamic entries are flushed with 
.port_fast_age, and port_fdb_del is for static entries. We handle -ENOENT
because DSA core does not refcount fdbs for userports.