Re: [PATCH net-next v2] net: dsa: vsc73xx: implement FDB operations

[Paolo Abeni <pabeni@redhat.com>]

On 8/22/24 16:23, Pawel Dembicki wrote:
> This commit introduces implementations of three functions:
> .port_fdb_dump
> .port_fdb_add
> .port_fdb_del
> 
> The FDB database organization is the same as in other old Vitesse chips:
> It has 2048 rows and 4 columns (buckets). The row index is calculated by
> the hash function 'vsc73xx_calc_hash' and the FDB entry must be placed
> exactly into row[hash]. The chip selects the bucket number by itself.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>

The patch LGTM, but I think you can deduplicate the code a bit, creating 
a few additional helpers.

> +static int vsc73xx_port_read_mac_table_row(struct vsc73xx *vsc, u16 index,
> +					   struct vsc73xx_fdb *fdb)
> +{
> +	int ret, i;
> +	u32 val;
> +
> +	if (!fdb)
> +		return -EINVAL;
> +	if (index >= VSC73XX_NUM_FDB_ROWS)
> +		return -EINVAL;
> +
> +	for (i = 0; i < VSC73XX_NUM_BUCKETS; i++) {
> +		ret = vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				    VSC73XX_MACTINDX,
> +				    (i ? 0 : VSC73XX_MACTINDX_SHADOW) |
> +				    FIELD_PREP(VSC73XX_MACTINDX_BUCKET_MSK, i) |
> +				    index);
> +		if (ret)
> +			return ret;
> +
> +		ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> +		if (ret)
> +			return ret;

the sequence:
	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, ...)
	vsc73xx_port_wait_for_mac_table_cmd()

could have its own helper

> +		ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +					  VSC73XX_MACACCESS,
> +					  VSC73XX_MACACCESS_CMD_MASK,
> +					  VSC73XX_MACACCESS_CMD_READ_ENTRY);
> +		if (ret)
> +			return ret;
> +
> +		ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> +		if (ret)
> +			return ret;

and even:

	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, ...)
	vsc73xx_port_wait_for_mac_table_cmd()

[...]
> +static int vsc73xx_fdb_del_entry(struct vsc73xx *vsc, int port,
> +				 const unsigned char *addr, u16 vid)
> +{
> +	struct vsc73xx_fdb fdb[VSC73XX_NUM_BUCKETS];
> +	u16 hash = vsc73xx_calc_hash(addr, vid);
> +	int bucket, ret;
> +
> +	mutex_lock(&vsc->fdb_lock);
> +
> +	ret = vsc73xx_port_read_mac_table_row(vsc, hash, fdb);
> +	if (ret)
> +		goto err;
> +
> +	for (bucket = 0; bucket < VSC73XX_NUM_BUCKETS; bucket++) {
> +		if (fdb[bucket].valid && fdb[bucket].port == port &&
> +		    ether_addr_equal(addr, fdb[bucket].mac))
> +			break;
> +	}
> +
> +	if (bucket == VSC73XX_NUM_BUCKETS) {
> +		/* Can't find MAC in MAC table */
> +		ret = -ENODATA;
> +		goto err;
> +	}
> +
> +	ret = vsc73xx_fdb_insert_mac(vsc, addr, vid);
> +	if (ret)
> +		goto err;
> +
> +	ret = vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_MACTINDX,
> +			    hash);
> +	if (ret)
> +		goto err;
> +
> +	ret = vsc73xx_port_wait_for_mac_table_cmd(vsc);
> +	if (ret)
> +		goto err;
> +
> +	ret = vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
> +				  VSC73XX_MACACCESS,
> +				  VSC73XX_MACACCESS_CMD_MASK,
> +				  VSC73XX_MACACCESS_CMD_FORGET);
> +	if (ret)
> +		goto err;
> +
> +	ret =  vsc73xx_port_wait_for_mac_table_cmd(vsc);

AFAICS both fdb_add and fdb_del use the same sequence:

	vsc73xx_fdb_insert_mac()
	vsc73xx_write(... VSC73XX_MACTINDX, hash);
	vsc73xx_port_wait_for_mac_table_cmd
	vsc73xx_update_bits(... <variable part>)
	vsc73xx_port_wait_for_mac_table_cmd()

perhaps it would be worthy to factor it out - also using the above 
mentioned helpers.
	
Thanks,

Paolo