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

[Saeed Mahameed <saeed@kernel.org>]

On 08 Nov 15:21, Simon Horman wrote:
>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.

Removed in V3. 

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

You are right, still in V3 also I will fix this.

>...
>
>> +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.
>

Cleanup is sufficient, the use of index -1 is an indication of the entry was
not successfully written to HW, so only if index is positive we will delete
it from hw on cleanup. 

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

This is best effort as there could be a lot of l2 entries and we might run
out of space, we don't want to cripple the whole system just because one VF
mac didn't make it to the mpfs table, the approach here is similar to
set_rx_mode ndo expectation, which this function also serves.