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

[Simon Horman <horms@kernel.org>]

On Sun, Aug 11, 2024 at 09:56:49PM +0200, 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 row number by itself.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 302 +++++++++++++++++++++++++
>  drivers/net/dsa/vitesse-vsc73xx.h      |   2 +
>  2 files changed, 304 insertions(+)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index a82b550a9e40..7da1641b8bab 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -46,6 +46,8 @@
>  #define VSC73XX_BLOCK_MII_EXTERNAL	0x1 /* External MDIO subblock */
>  
>  #define CPU_PORT	6 /* CPU port */
> +#define VSC73XX_NUM_FDB_RECORDS	2048
> +#define VSC73XX_NUM_BUCKETS	4
>  
>  /* MAC Block registers */
>  #define VSC73XX_MAC_CFG		0x00
> @@ -197,6 +199,21 @@
>  #define VSC73XX_SRCMASKS_MIRROR			BIT(26)
>  #define VSC73XX_SRCMASKS_PORTS_MASK		GENMASK(7, 0)
>  
> +#define VSC73XX_MACHDATA_VID			GENMASK(27, 16)
> +#define VSC73XX_MACHDATA_VID_SHIFT		16
> +#define VSC73XX_MACHDATA_MAC0_SHIFT		8
> +#define VSC73XX_MACHDATA_MAC1_SHIFT		0
> +#define VSC73XX_MACLDATA_MAC2_SHIFT		24
> +#define VSC73XX_MACLDATA_MAC3_SHIFT		16
> +#define VSC73XX_MACLDATA_MAC4_SHIFT		8
> +#define VSC73XX_MACLDATA_MAC5_SHIFT		0
> +#define VSC73XX_MAC_BYTE_MASK			GENMASK(7, 0)
> +
> +#define VSC73XX_MACTINDX_SHADOW			BIT(13)
> +#define VSC73XX_MACTINDX_BUCKET_MASK		GENMASK(12, 11)
> +#define VSC73XX_MACTINDX_BUCKET_MASK_SHIFT	11
> +#define VSC73XX_MACTINDX_INDEX_MASK		GENMASK(10, 0)
> +
>  #define VSC73XX_MACACCESS_CPU_COPY		BIT(14)
>  #define VSC73XX_MACACCESS_FWD_KILL		BIT(13)
>  #define VSC73XX_MACACCESS_IGNORE_VLAN		BIT(12)
> @@ -204,6 +221,7 @@
>  #define VSC73XX_MACACCESS_VALID			BIT(10)
>  #define VSC73XX_MACACCESS_LOCKED		BIT(9)
>  #define VSC73XX_MACACCESS_DEST_IDX_MASK		GENMASK(8, 3)
> +#define VSC73XX_MACACCESS_DEST_IDX_MASK_SHIFT	3
>  #define VSC73XX_MACACCESS_CMD_MASK		GENMASK(2, 0)
>  #define VSC73XX_MACACCESS_CMD_IDLE		0
>  #define VSC73XX_MACACCESS_CMD_LEARN		1
> @@ -329,6 +347,13 @@ struct vsc73xx_counter {
>  	const char *name;
>  };
>  
> +struct vsc73xx_fdb {
> +	u16 vid;
> +	u8 port;
> +	u8 mac[6];
> +	bool valid;
> +};
> +
>  /* Counters are named according to the MIB standards where applicable.
>   * Some counters are custom, non-standard. The standard counters are
>   * named in accordance with RFC2819, RFC2021 and IEEE Std 802.3-2002 Annex
> @@ -1829,6 +1854,278 @@ static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
>  		vsc73xx_refresh_fwd_map(ds, port, state);
>  }
>  
> +static u16 vsc73xx_calc_hash(const unsigned char *addr, u16 vid)
> +{
> +	/* VID 5-0, MAC 47-44 */
> +	u16 hash = ((vid & GENMASK(5, 0)) << 4) | (addr[0] >> 4);
> +
> +	/* MAC 43-33 */
> +	hash ^= ((addr[0] & GENMASK(3, 0)) << 7) | (addr[1] >> 1);
> +	/* MAC 32-22 */
> +	hash ^= ((addr[1] & BIT(0)) << 10) | (addr[2] << 2) | (addr[3] >> 6);
> +	/* MAC 21-11 */
> +	hash ^= ((addr[3] & GENMASK(5, 0)) << 5) | (addr[4] >> 3);
> +	/* MAC 10-0 */
> +	hash ^= ((addr[4] & GENMASK(2, 0)) << 8) | addr[5];

In this function and throughout this patchset I see a lot of mask nd shift
operations, sometimes combined, here and elsewhere in this patchset. It
seems likely that using FIELD_PREP, in conjunction with GENMASK and BIT,
would be appropriate instead. If combined with #defines for the GENMASK and
BIT constants with meaningful names this may significantly improve
readability.

> +
> +	return hash;
> +}

...