netdev - Re: [PATCH net-next V2 3/7] net/mlx5: E-Switch, Add support for adjacent functions vports discovery

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: <aLIi3ToMmFXNO0V5@x130>
Date: Fri, 29 Aug 2025 14:59:57 -0700
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>, mbloch@...dia.com,
	Adithya Jayachandran <ajayachandra@...dia.com>
Subject: Re: [PATCH net-next V2 3/7] net/mlx5: E-Switch, Add support for
 adjacent functions vports discovery

On 28 Aug 12:34, Simon Horman wrote:
>On Tue, Aug 26, 2025 at 09:45:12PM -0700, Saeed Mahameed wrote:
>> From: Adithya Jayachandran <ajayachandra@...dia.com>
>>
>> Adding driver support to query adjacent functions vports, AKA
>> delegated vports.
>>
>> Adjacent functions can delegate their sriov vfs to other sibling PF in
>> the system, to be managed by the eswitch capable sibling PF.
>> E.g, ECPF to Host PF, multi host PF between each other, etc.
>>
>> Only supported in switchdev mode.
>>
>> Signed-off-by: Adithya Jayachandran <ajayachandra@...dia.com>
>> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
>
>...
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>
>...
>
>> +static int
>> +mlx5_eswitch_load_adj_vf_vports(struct mlx5_eswitch *esw,
>> +				enum mlx5_eswitch_vport_event enabled_events)
>> +{
>> +	struct mlx5_vport *vport;
>> +	unsigned long i;
>> +	int err;
>> +
>> +	mlx5_esw_for_each_vf_vport(esw, i, vport, U16_MAX) {
>> +		if (!vport->adjacent)
>> +			continue;
>> +		err = mlx5_eswitch_load_pf_vf_vport(esw, vport->vport,
>> +						    enabled_events);
>> +		if (err)
>> +			goto unload_adj_vf_vport;
>> +	}
>> +
>> +	return 0;
>> +
>> +unload_adj_vf_vport:
>> +	mlx5_eswitch_unload_adj_vf_vports(esw);
>> +	return err;
>> +}
>> +
>>  int mlx5_eswitch_load_vf_vports(struct mlx5_eswitch *esw, u16 num_vfs,
>>  				enum mlx5_eswitch_vport_event enabled_events)
>>  {
>> @@ -1345,7 +1382,15 @@ mlx5_eswitch_enable_pf_vf_vports(struct mlx5_eswitch *esw,
>>  					  enabled_events);
>>  	if (ret)
>>  		goto vf_err;
>> +
>> +	/* Enable adjacent VF vports */
>> +	ret = mlx5_eswitch_load_adj_vf_vports(esw, enabled_events);
>> +	if (ret)
>> +		goto unload_adj_vf_vports;
>> +
>>  	return 0;
>> +unload_adj_vf_vports:
>> +	mlx5_eswitch_unload_adj_vf_vports(esw);
>>
>
>Hi Adithya and Saeed,
>
>mlx5_eswitch_load_adj_vf_vports() already unwinds on error,
>calling mlx5_eswitch_unload_adj_vf_vports().
>
>While resources allocated by the preceding call to
>mlx5_eswitch_load_vf_vports() (just before this hunk) seem to be leaked.
>
>So I wonder if the above two lines should be:
>
>unload_vf_vports:
>	mlx5_eswitch_unload_vf_vports(esw, num_vfs);
>

Yep, looks incorrect, let me double check and submit v3.

>>  vf_err:
>>  	if (mlx5_core_ec_sriov_enabled(esw->dev))
>
>...
>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
>> index cfd6b1b8c6f4..9f8bb397eae5 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
>> @@ -216,6 +216,7 @@ struct mlx5_vport {
>>  	u32                     metadata;
>>  	int                     vhca_id;
>>
>> +	bool adjacent; /* delegated vhca from adjacent function */
>>  	struct mlx5_vport_info  info;
>>
>>  	/* Protected with the E-Switch qos domain lock. The Vport QoS can
>> @@ -384,6 +385,7 @@ struct mlx5_eswitch {
>>
>>  	struct mlx5_esw_bridge_offloads *br_offloads;
>>  	struct mlx5_esw_offload offloads;
>> +	u32 last_vport_idx; /* ++ every time a vport is created */
>
>The comment above documents one operation that can occur
>on this field. But the code also performs others: decrement and set.
>
>So perhaps this is more appropriate?
>
>	u32 last_vport_idx; /* Number of vports created */
>
>Or dropping the comment entirely: it seems obvious enough.
>

Agreed, will drop.