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

[Simon Horman <horms@kernel.org>]

On Thu, Nov 06, 2025 at 04:08:30PM -0800, Saeed Mahameed wrote:
> From: Saeed Mahameed <saeedm@nvidia.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@nvidia.com>
> Signed-off-by: Adithya Jayachandran <ajayachandra@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.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;
> +}

...