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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250828113417.GB10519@horms.kernel.org>
Date: Thu, 28 Aug 2025 12:34:17 +0100
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>, 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 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);

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

>  	int                     mode;
>  	u16                     manager_vport;
>  	u16                     first_host_vport;

...