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

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

On Tue, 2026-05-05 at 18:46 -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
> 
> This patch adds MAC table support and hooks it into the DSA FDB callbacks
> (port_fdb_add, port_fdb_del, port_fdb_dump, port_fast_age), along with a
> port-filtered flush path and a GET_NEXT-based dump helper.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> > new file mode 100644
> > index 000000000000..7ae23d3464f8
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> 
> [ ... ]
> 
> > +void lan9645x_mac_init(struct lan9645x *lan9645x)
> > +{
> > +     u32 val;
> > +
> > +     mutex_init(&lan9645x->mact_lock);
> > +
> > +     /* Clear the MAC table */
> > +     lan_wr(ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_INIT),
> > +            lan9645x, ANA_MACACCESS);
> > +
> > +     if (lan9645x_rd_poll_timeout(lan9645x, ANA_MACACCESS, val,
> > +                                  ANA_MACACCESS_MAC_TABLE_CMD_GET(val) ==
> > +                                  CMD_IDLE))
> > +             dev_err(lan9645x->dev, "mac init timeout\n");
> > +}
> 
> Should lan9645x_mac_init() propagate the CMD_INIT timeout instead of
> logging and returning?
> 
> It is declared void and the only timeout handling is a dev_err().  The
> caller in lan9645x_setup() invokes it without a return value and then
> proceeds to configure flooding PGIDs, per-port CPU forwarding, and
> register the dsa_switch_ops callbacks (.port_fdb_add, .port_fdb_del,
> .port_fast_age, .port_fdb_dump) while the MAC table command engine may
> still be busy.
> 
> For comparison, lan9645x_vlan_init() (called just above in setup) treats
> the structurally identical clear-table timeout as fatal:
> 
>         lan_wr(ANA_VLANACCESS_VLAN_TBL_CMD_SET(VLANACCESS_CMD_INIT),
>                lan9645x, ANA_VLANACCESS);
> 
>         err = lan9645x_vlan_wait_for_completion(lan9645x);
>         if (err) {
>                 dev_err(lan9645x->dev, "Vlan clear table failed\n");
>                 return err;
>         }
> 
> Would returning an int from lan9645x_mac_init() and checking it in
> lan9645x_setup() be more consistent?
> 

Yes I think that is a good idea, I will return an error here.

> [ ... ]
> 
> > +int lan9645x_mact_dsa_dump(struct lan9645x *lan9645x, int port,
> > +                        dsa_fdb_dump_cb_t *cb, void *data)
> > +{
> > +     u8 mac[ETH_ALEN] __aligned(2);
> > +     u32 mach, macl, maca;
> > +     int err = 0;
> > +     u32 autoage;
> > +     u64 addr;
> > +     u16 vid;
> > +     u8 type;
> 
> [ ... ]
> 
> > +             type = ANA_MACACCESS_ENTRYTYPE_GET(maca);
> > +             mach = lan_rd(lan9645x, ANA_MACHDATA);
> > +             macl = lan_rd(lan9645x, ANA_MACLDATA);
> > +
> > +             if (ANA_MACACCESS_DEST_IDX_GET(maca) == port &&
> > +                 type == ENTRYTYPE_NORMAL) {
> > +                     addr = (u64)ANA_MACHDATA_MACHDATA_GET(mach) << 32 |
> > +                            macl;
> > +                     u64_to_ether_addr(addr, mac);
> > +                     vid = ANA_MACHDATA_VID_GET(mach);
> > +                     if (vid > VLAN_MAX)
> > +                             vid = 0;
> > +
> > +                     err = cb(mac, vid, false, data);
> > +                     if (err)
> > +                             break;
> > +             }
> > +     }
> 
> Does this filter cause user-added FDB entries to be invisible to
> 'bridge fdb show'?
> 
> lan9645x_fdb_add() unconditionally programs entries as ENTRYTYPE_LOCKED:
> 
>         return lan9645x_mact_learn(lan9645x, dest, addr, vid, ENTRYTYPE_LOCKED);
> 
> but the dump above only reports entries matching ENTRYTYPE_NORMAL, so any
> entry installed via 'bridge fdb add dev swpN <mac> self [static]' is
> present in hardware and not reported through port_fdb_dump.
> 
> For NTF_SELF entries the DSA core path
> (net/dsa/user.c:dsa_user_port_fdb_do_dump) depends on port_fdb_dump to
> surface entries that are not tracked in the bridge software FDB.
> 
> Separately, the callback hardcodes is_static=false for every reported
> entry:
> 
>         err = cb(mac, vid, false, data);
> 
> If the ENTRYTYPE_LOCKED filter is relaxed, would is_static need to be
> derived from the entry type, similar to ocelot_fdb_dump() in
> drivers/net/ethernet/mscc/ocelot.c which uses
> (entry.type == ENTRYTYPE_LOCKED)?


This is intentional, we only dump the dynamically learned entries which
the network stack can not know about. The static entries are already
reported, for instance

bridge fdb add dev swpN <mac> self

will show up on the conduit, and get programmed toward the CPU_PORT in
hardware.

Including these would duplicate entries in bridge fdb show. I will add
a comment about this.


> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index c99189ce586e..b21e1bf25b0c 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > @@ -139,6 +140,7 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >       err = lan9645x_vlan_init(lan9645x);
> >       if (err)
> >               return err;
> > +     lan9645x_mac_init(lan9645x);
> 
> Related to the lan9645x_mac_init() comment above, should this call
> check a return value and abort setup on failure rather than continuing
> to register switch ops with a potentially wedged MAC table engine?
> 

Yes I will add this.

> [ ... ]
> 
> > +static int lan9645x_fdb_add(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 dest;
> > +
> > +     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);
> > +
> > +     dest = dsa_port_is_cpu(dp) ? PGID_CPU : port;
> > +
> > +     return lan9645x_mact_learn(lan9645x, dest, addr, vid, ENTRYTYPE_LOCKED);
> > +}
> 
> See the question above about lan9645x_mact_dsa_dump() filtering on
> ENTRYTYPE_NORMAL: entries created here with ENTRYTYPE_LOCKED do not
> appear to be reported by port_fdb_dump.

But they are reported by other means already, so we try to avoid the
duplication.