Re: [PATCH net-next 6/7] net/mlx5: E-switch, load reps via work queue after registration

[Mark Bloch <mbloch@nvidia.com>]

On 09/04/2026 14:55, Tariq Toukan wrote:
> From: Mark Bloch <mbloch@nvidia.com>
> 
> mlx5_eswitch_register_vport_reps() merely sets the callbacks. The actual
> representor load/unload requires devlink locking and shouldn’t run from
> the registration context. Queue a work that acquires the devlink lock,
> loads all relevant reps. This lets load happen where the needed locks
> can be taken.
> 

Sashiko writes:

> (Note: Due to system recitation restrictions, this report does not quote the
> original patch code directly. Instead, it provides a free-form summary of
> the questions and observations regarding the changes.)
> commit 8bc15ac3cf1f185bce88ba5499055d1546dae606
> Author: Mark Bloch <mbloch@nvidia.com>
> net/mlx5: E-switch, load reps via work queue after registration
> This commit shifts the representor load operation to an asynchronous
> workqueue so that the necessary devlink locks can be properly acquired,
> avoiding locking issues in the registration context.
> The commit message notes that actual representor load and unload require
> devlink locking and shouldn't run from the registration context. The patch
> successfully defers the load operation, but it appears the unload path
> in mlx5_eswitch_unregister_vport_reps() was not updated. Will
> mlx5_eswitch_unregister_vport_reps_blocked() and __unload_reps_all_vport()
> still be called synchronously without the devlink lock, leaving the
> teardown path unprotected?

This is not worse than the current behavior. The block/unblock mechanism
ensures correctness even without holding the devlink lock, although this
is not ideal.

Fixing the unload path to follow the same model is planned as a follow-up
series

> When mlx5_eswitch_register_vport_reps() queues the asynchronous work item,
> there doesn't seem to be a corresponding cancellation or flush in
> mlx5_eswitch_unregister_vport_reps(). If the registering module
> unregisters the representors and unloads before the work executes, could
> the delayed mlx5_eswitch_reload_reps() run and dereference stale callbacks,
> causing a use-after-free?

Each representor maintains state per rep type, and together with the
E-Switch generation counter we avoid executing work on stale instances.
If the E-Switch generation changes, the queued work will not proceed
with loading representors for the old state.

> The call to mlx5_esw_add_work() to enqueue the representor load operation
> might fail and return an error code like -ENOMEM. Since the return value
> isn't checked in mlx5_eswitch_register_vport_reps(), will a failure
> leave the representors in the REP_REGISTERED state but completely unloaded,
> without notifying the caller?

This change addresses the case where the eswitch is already in
switchdev mode and the IB module is loaded afterwards. Prior to this
patch, no representors would be created in that scenario.

After this change, representors are created when possible, improving the
overall user experience. If workqueue allocation fails (e.g. -ENOMEM),
there is little meaningful recovery we can provide in this context.

> In the new mlx5_eswitch_reload_reps_blocked() function, if loading the
> uplink representor fails, the function returns early. Similarly, any

The uplink representor is a prerequisite for all other representors.
If it fails to load, the rest cannot be meaningfully initialized, so it
is treated as a critical failure.

> failures when loading individual VF representors in the loop are not
> checked. Since this now runs asynchronously, are these errors permanently
> lost, potentially leaving the subsystem in a partially initialized state
> with no fallback?

Failures when loading individual vf representors are intentionally not
treated as fatal. The operation is best-effort, and partial
initialization is acceptable in this context.

Mark

> Signed-off-by: Mark Bloch <mbloch@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  .../mellanox/mlx5/core/eswitch_offloads.c     | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 4b626ffcfa8e..279490c0074c 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -4535,6 +4535,38 @@ mlx5_eswitch_register_vport_reps_blocked(struct mlx5_eswitch *esw,
>  	}
>  }
>  
> +static void mlx5_eswitch_reload_reps_blocked(struct mlx5_eswitch *esw)
> +{
> +	struct mlx5_vport *vport;
> +	unsigned long i;
> +
> +	if (esw->mode != MLX5_ESWITCH_OFFLOADS)
> +		return;
> +
> +	if (mlx5_esw_offloads_rep_load(esw, MLX5_VPORT_UPLINK))
> +		return;
> +
> +	mlx5_esw_for_each_vport(esw, i, vport) {
> +		if (!vport)
> +			continue;
> +		if (!vport->enabled)
> +			continue;
> +		if (vport->vport == MLX5_VPORT_UPLINK)
> +			continue;
> +		if (!mlx5_eswitch_vport_has_rep(esw, vport->vport))
> +			continue;
> +
> +		mlx5_esw_offloads_rep_load(esw, vport->vport);
> +	}
> +}
> +
> +static void mlx5_eswitch_reload_reps(struct mlx5_eswitch *esw)
> +{
> +	mlx5_esw_reps_block(esw);
> +	mlx5_eswitch_reload_reps_blocked(esw);
> +	mlx5_esw_reps_unblock(esw);
> +}
> +
>  void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
>  				      const struct mlx5_eswitch_rep_ops *ops,
>  				      u8 rep_type)
> @@ -4542,6 +4574,8 @@ void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,
>  	mlx5_esw_reps_block(esw);
>  	mlx5_eswitch_register_vport_reps_blocked(esw, ops, rep_type);
>  	mlx5_esw_reps_unblock(esw);
> +
> +	mlx5_esw_add_work(esw, mlx5_eswitch_reload_reps);
>  }
>  EXPORT_SYMBOL(mlx5_eswitch_register_vport_reps);
>