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

Message-ID: <aQ-cWqrZr_1qkgCm@x130>
Date: Sat, 8 Nov 2025 11:39:06 -0800
From: Saeed Mahameed <saeed@...nel.org>
To: Simon Horman <horms@...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 08 Nov 15:21, Simon Horman wrote:
>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.

Removed in V3. 

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

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.