netdev - Re: [PATCH net-next V2 2/3] net/mlx5: MPFS, add support for dynamic enable/disable

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aQ9gB4lCBaK19bRo@horms.kernel.org>
Date: Sat, 8 Nov 2025 15:21:43 +0000
From: Simon Horman <horms@...nel.org>
To: Saeed Mahameed <saeed@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Saeed Mahameed <saeedm@...dia.com>, netdev@...r.kernel.org,
	Tariq Toukan <tariqt@...dia.com>, Gal Pressman <gal@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>, Jiri Pirko <jiri@...dia.com>,
	mbloch@...dia.com, Adithya Jayachandran <ajayachandra@...dia.com>
Subject: Re: [PATCH net-next V2 2/3] net/mlx5: MPFS, add support for dynamic
 enable/disable

On Thu, Nov 06, 2025 at 04:08:30PM -0800, Saeed Mahameed wrote:
> From: Saeed Mahameed <saeedm@...dia.com>
> 
> MPFS (Multi PF Switch) is enabled by default in Multi-Host environments,
> the driver keeps a list of desired unicast mac addresses of all vports
> (vfs/Sfs) and applied to HW via L2_table FW command.
> 
> Add API to dynamically apply the list of MACs to HW when needed for next
> patches, to utilize this new API in devlink eswitch active/in-active uAPI.
> 
> Issue: 4314625
> Change-Id: I185c144319e514f787811f556888e1b727bdbf35

nit: the issue and Change-Id should be dropped when upstreaming patches.

> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> Signed-off-by: Adithya Jayachandran <ajayachandra@...dia.com>
> Reviewed-by: Jiri Pirko <jiri@...dia.com>
> ---
>  .../ethernet/mellanox/mlx5/core/lib/mpfs.c    | 115 +++++++++++++++---
>  .../ethernet/mellanox/mlx5/core/lib/mpfs.h    |   9 ++
>  2 files changed, 107 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/mpfs.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/mpfs.c

...

> @@ -148,30 +151,34 @@ int mlx5_mpfs_add_mac(struct mlx5_core_dev *dev, u8 *mac)

...

> +	if (mpfs->enabled) {
> +		err = alloc_l2table_index(mpfs, &index);
> +		if (err)
> +			goto hash_del;
> +		err = set_l2table_entry_cmd(dev, index, mac);
> +		if (err)
> +			goto free_l2table_index;
> +		mlx5_core_dbg(dev, "MPFS entry %pM, set @index (%d)\n",
> +			      l2addr->node.addr, l2addr->index);

nit: the following patch updates the line above to:

			      l2addr->node.addr, index);

     I don't think that change belongs in the following patch.

...

> +int mlx5_mpfs_enable(struct mlx5_core_dev *dev)
> +{
> +	struct mlx5_mpfs *mpfs = dev->priv.mpfs;
> +	struct l2table_node *l2addr;
> +	struct hlist_node *n;
> +	int err = 0;
> +
> +	if (!mpfs)
> +		return -ENODEV;
> +
> +	mutex_lock(&mpfs->lock);
> +	if (mpfs->enabled)
> +		goto out;
> +	mpfs->enabled = true;
> +	mlx5_core_dbg(dev, "MPFS enabling mpfs\n");
> +
> +	mlx5_mpfs_foreach(l2addr, n, mpfs) {
> +		u32 index;
> +
> +		err = alloc_l2table_index(mpfs, &index);
> +		if (err) {
> +			mlx5_core_err(dev, "Failed to allocated MPFS index for %pM, err(%d)\n",
> +				      l2addr->node.addr, err);
> +			goto out;
> +		}
> +
> +		err = set_l2table_entry_cmd(dev, index, l2addr->node.addr);
> +		if (err) {
> +			mlx5_core_err(dev, "Failed to set MPFS l2table entry for %pM index=%d, err(%d)\n",
> +				      l2addr->node.addr, index, err);
> +			free_l2table_index(mpfs, index);
> +			goto out;
> +		}
> +
> +		l2addr->index = index;
> +		mlx5_core_dbg(dev, "MPFS entry %pM, set @index (%d)\n",
> +			      l2addr->node.addr, l2addr->index);
> +	}
> +out:
> +	mutex_unlock(&mpfs->lock);

I realise that error handling can be complex at best, and particularly
so when configuration ends up being partially applied. But I am wondering
if the cleanup here is sufficient.

In a similar vein, I also note that although this function returns an
error, it is ignored by callers which are added by the following patch.
Likewise for mlx5_esw_fdb_drop_create() which is also added by the
following patch.

> +	return err;
> +}

...