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

Jakub Kicinski
2026-05-06T01:46:16Z

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?

[ ... ]

> +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)?

> 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?

[ ... ]

> +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.