Re: [PATCH net V3 1/4] net/mlx5: SD: Serialize init/cleanup

[Shay Drori]

On 23/04/2026 15:31, Tariq Toukan wrote:
> From: Shay Drory <shayd@xxxxxxxxxx>
>
> mlx5_sd_init() / mlx5_sd_cleanup() may run from multiple PFs in the same
> Socket-Direct group. This can cause the SD bring-up/tear-down sequence
> to be executed more than once or interleaved across PFs.
>
> Protect SD init/cleanup with mlx5_devcom_comp_lock() and track the SD
> group state on the primary device. Skip init if the primary is already
> UP, and skip cleanup unless the primary is UP.

Sashiko:
"The commit message mentions skipping cleanup unless the primary is UP.
However, it appears this state check is missing from mlx5_sd_cleanup()
in the diff below."

The above sentence is leftover and should be removed.
will drop in next version.

> In addition, move mlx5_devcom_comp_set_ready(false) from sd_unregister()
> into the cleanup's locked section. A concurrent init acquiring the
> devcom lock will now observe devcom is no longer ready and bail out
> immediately.
>
> Fixes: 381978d28317 ("net/mlx5e: Create single netdev per SD group")
> Signed-off-by: Shay Drory <shayd@xxxxxxxxxx>
> Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> ---
>   .../net/ethernet/mellanox/mlx5/core/lib/sd.c  | 32 +++++++++++++++----
>   1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> index 762c783156b4..96b4316f570e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> @@ -18,6 +18,7 @@ struct mlx5_sd {
>   	u8 host_buses;
>   	struct mlx5_devcom_comp_dev *devcom;
>   	struct dentry *dfs;
> +	u8 state;
>   	bool primary;
>   	union {
>   		struct { /* primary */
> @@ -31,6 +32,11 @@ struct mlx5_sd {
>   	};
>   };
>   
> +enum mlx5_sd_state {
> +	MLX5_SD_STATE_DOWN = 0,
> +	MLX5_SD_STATE_UP,
> +};
> +
>   static int mlx5_sd_get_host_buses(struct mlx5_core_dev *dev)
>   {
>   	struct mlx5_sd *sd = mlx5_get_sd(dev);
> @@ -270,9 +276,6 @@ static void sd_unregister(struct mlx5_core_dev *dev)
>   {
>   	struct mlx5_sd *sd = mlx5_get_sd(dev);
>   
> -	mlx5_devcom_comp_lock(sd->devcom);
> -	mlx5_devcom_comp_set_ready(sd->devcom, false);
> -	mlx5_devcom_comp_unlock(sd->devcom);
>   	mlx5_devcom_unregister_component(sd->devcom);
>   }
>   
> @@ -426,6 +429,7 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
>   	struct mlx5_core_dev *primary, *pos, *to;
>   	struct mlx5_sd *sd = mlx5_get_sd(dev);
>   	u8 alias_key[ACCESS_KEY_LEN];
> +	struct mlx5_sd *primary_sd;
>   	int err, i;
>   
>   	err = sd_init(dev);
> @@ -440,10 +444,15 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
>   	if (err)
>   		goto err_sd_cleanup;
>   
> +	mlx5_devcom_comp_lock(sd->devcom);
>   	if (!mlx5_devcom_comp_is_ready(sd->devcom))
> -		return 0;
> +		goto out;

Sashiko:
"Can primary be NULL here?
In sd_register(), the devcom ready state is published under the devcom
lock, but the lock is then released before the peer_sd->primary_dev
pointers are initialized.
If a concurrent thread executing mlx5_sd_init() or mlx5_sd_cleanup()
acquires the lock and observes the ready state, could it read an
uninitialized primary_dev before the loop in sd_register() completes?"

No, this is impossible. concurrent init will always set primary before
accessing it, and cleanup is always after successful init, so again-
primary is set.
and the next comment is also impossible seqence

>   
>   	primary = mlx5_sd_get_primary(dev);
> +	primary_sd = mlx5_get_sd(primary);
> +
> +	if (primary_sd->state != MLX5_SD_STATE_DOWN)
> +		goto out;
>   
>   	for (i = 0; i < ACCESS_KEY_LEN; i++)
>   		alias_key[i] = get_random_u8();
> @@ -472,6 +481,9 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
>   		sd->group_id, mlx5_devcom_comp_get_size(sd->devcom));
>   	sd_print_group(primary);
>   
> +	primary_sd->state = MLX5_SD_STATE_UP;
> +out:
> +	mlx5_devcom_comp_unlock(sd->devcom);
>   	return 0;
>   
>   err_unset_secondaries:
> @@ -481,6 +493,8 @@ int mlx5_sd_init(struct mlx5_core_dev *dev)
>   	sd_cmd_unset_primary(primary);
>   	debugfs_remove_recursive(sd->dfs);
>   err_sd_unregister:
> +	mlx5_devcom_comp_set_ready(sd->devcom, false);
> +	mlx5_devcom_comp_unlock(sd->devcom);
>   	sd_unregister(dev);
>   err_sd_cleanup:
>   	sd_cleanup(dev);
> @@ -491,22 +505,28 @@ void mlx5_sd_cleanup(struct mlx5_core_dev *dev)
>   {
>   	struct mlx5_sd *sd = mlx5_get_sd(dev);
>   	struct mlx5_core_dev *primary, *pos;
> +	struct mlx5_sd *primary_sd;
>   	int i;
>   
>   	if (!sd)
>   		return;
>   
> +	mlx5_devcom_comp_lock(sd->devcom);
>   	if (!mlx5_devcom_comp_is_ready(sd->devcom))
> -		goto out;
> +		goto out_unlock;
>   
>   	primary = mlx5_sd_get_primary(dev);
> +	primary_sd = mlx5_get_sd(primary);
>   	mlx5_sd_for_each_secondary(i, primary, pos)
>   		sd_cmd_unset_secondary(pos);
>   	sd_cmd_unset_primary(primary);
>   	debugfs_remove_recursive(sd->dfs);
>   
>   	sd_info(primary, "group id %#x, uncombined\n", sd->group_id);
> -out:
> +	primary_sd->state = MLX5_SD_STATE_DOWN;
> +	mlx5_devcom_comp_set_ready(sd->devcom, false);
> +out_unlock:
> +	mlx5_devcom_comp_unlock(sd->devcom);
>   	sd_unregister(dev);
>   	sd_cleanup(dev);
>   }