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

[Jakub Kicinski]

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?

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

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

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

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

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

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