* Re: [PATCH net-next v4 7/9] net: dsa: lan9645x: add mac table integration
[not found] <20260430-dsa_lan9645x_switch_driver_base-v4-7-f1b6005fa8b7@microchip.com>
@ 2026-05-06 1:46 ` Jakub Kicinski
2026-05-12 7:42 ` Jens Emil Schulz Ostergaard
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Kicinski @ 2026-05-06 1:46 UTC (permalink / raw)
To: jensemil.schulzostergaard
Cc: Jakub Kicinski, UNGLinuxDriver, andrew, olteanv, davem, edumazet,
pabeni, horms, robh, krzk+dt, conor+dt, woojung.huh, linux,
Steen.Hegelund, daniel.machon, linux-kernel, netdev, devicetree
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH net-next v4 7/9] net: dsa: lan9645x: add mac table integration
2026-05-06 1:46 ` [PATCH net-next v4 7/9] net: dsa: lan9645x: add mac table integration Jakub Kicinski
@ 2026-05-12 7:42 ` Jens Emil Schulz Ostergaard
0 siblings, 0 replies; 2+ messages in thread
From: Jens Emil Schulz Ostergaard @ 2026-05-12 7:42 UTC (permalink / raw)
To: Jakub Kicinski
Cc: UNGLinuxDriver, andrew, olteanv, davem, edumazet, pabeni, horms,
robh, krzk+dt, conor+dt, woojung.huh, linux, Steen.Hegelund,
daniel.machon, linux-kernel, netdev, devicetree
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-12 7:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260430-dsa_lan9645x_switch_driver_base-v4-7-f1b6005fa8b7@microchip.com>
2026-05-06 1:46 ` [PATCH net-next v4 7/9] net: dsa: lan9645x: add mac table integration Jakub Kicinski
2026-05-12 7:42 ` Jens Emil Schulz Ostergaard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox