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

[Jakub Kicinski <kuba@kernel.org>]

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.