Re: [PATCH net-next v3 08/12] net: dsa: rzn1-a5psw: add FDB support

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, May 04, 2022 at 11:29:56AM +0200, Clément Léger wrote:
> +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port,
> +			      const unsigned char *addr, u16 vid,
> +			      struct dsa_db db)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	union lk_data lk_data = {0};
> +	bool clear = false;
> +	int ret = 0;
> +	u16 entry;
> +	u32 reg;
> +
> +	ether_addr_copy(lk_data.entry.mac, addr);
> +
> +	spin_lock(&a5psw->lk_lock);
> +
> +	ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry);
> +	if (ret) {
> +		dev_err(a5psw->dev, "Failed to lookup mac address\n");
> +		goto lk_unlock;
> +	}
> +
> +	lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI);
> +	if (!lk_data.entry.valid) {
> +		dev_err(a5psw->dev, "Tried to remove non-existing entry\n");
> +		ret = -ENOENT;
> +		goto lk_unlock;

These error messages can happen under quite normal use with your hardware.
For example:

ip link add br0 type bridge && ip link set br0 master br0
bridge fdb add dev swp0 00:01:02:03:04:05 vid 1 master static
bridge fdb add dev swp0 00:01:02:03:04:05 vid 2 master static
bridge fdb del dev swp0 00:01:02:03:04:05 vid 2 master static
bridge fdb del dev swp0 00:01:02:03:04:05 vid 1 master static

Because the driver ignores the VID, then as soon as the VID 2 FDB entry
is removed, the VID 1 FDB entry doesn't exist anymore, either.

The above is a bit artificial. More practically, the bridge installs
local FDB entries (MAC address of bridge device, MAC addresses of ports)
once in the VLAN-unaware database (VID 0), and once per VLAN installed
on br0.

When the MAC address of br0 is different from that of the ports, and it
is changed by the user, you'll get these errors too, since those changes
translate into a deletion of the old local FDB entries (installed as FDB
entries towards the CPU port). Do you want the users to see them and
think something is wrong?  I mean, something _is_ wrong (the hardware),
but you have to work with that somehow.

> +	}
> +
> +	lk_data.entry.port_mask &= ~BIT(port);
> +	/* If there is no more port in the mask, clear the entry */
> +	if (lk_data.entry.port_mask == 0)
> +		clear = true;
> +
> +	a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi);
> +
> +	reg = entry;
> +	if (clear)
> +		reg |= A5PSW_LK_ADDR_CTRL_CLEAR;
> +	else
> +		reg |= A5PSW_LK_ADDR_CTRL_WRITE;
> +
> +	ret = a5psw_lk_execute_ctrl(a5psw, &reg);
> +	if (ret)
> +		goto lk_unlock;
> +
> +	/* Decrement LEARNCOUNT */
> +	if (clear) {
> +		reg = A5PSW_LK_LEARNCOUNT_MODE_DEC;
> +		a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg);
> +	}
> +
> +lk_unlock:
> +	spin_unlock(&a5psw->lk_lock);
> +
> +	return ret;
> +}