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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2564cd0a-c236-427a-abd7-e9933adff5cc@redhat.com>
Date: Tue, 27 Aug 2024 12:57:35 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Pawel Dembicki <paweldembicki@...il.com>, netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
 Vladimir Oltean <olteanv@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2] net: dsa: vsc73xx: implement FDB operations

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@...il.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