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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: E-switch, load reps via work queue after registration

This change defers loading of newly registered representors to the
E-Switch work queue so the handler can acquire the devlink lock that is
not held by the auxiliary driver registration context. The unregister
path continues to unload representors synchronously.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 8f656253981b..f26d1652dd05 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -4563,6 +4563,38 @@ mlx5_eswitch_register_vport_reps_blocked(struct mlx5_eswitch *esw,

[ ... ]

> +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);
> +}
> +
>  static void
>  mlx5_eswitch_register_vport_reps_locked(struct mlx5_eswitch *esw,
>  					const struct mlx5_eswitch_rep_ops *ops,
> @@ -4574,6 +4606,8 @@ mlx5_eswitch_register_vport_reps_locked(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);

The return value of mlx5_esw_add_work() is discarded here.

Looking at mlx5_esw_add_work():

    static int mlx5_esw_add_work(struct mlx5_eswitch *esw,
                                 void (*func)(struct mlx5_eswitch *esw))
    {
            struct mlx5_host_work *host_work;

            host_work = kzalloc_obj(*host_work, GFP_ATOMIC);
            if (!host_work)
                    return -ENOMEM;
            ...
    }

the allocation is GFP_ATOMIC and can fail, in which case no work is
queued.

If mlx5_esw_add_work() returns -ENOMEM while the E-Switch is already in
MLX5_ESWITCH_OFFLOADS mode, the reload that mlx5_eswitch_reload_reps()
was meant to perform never runs, so the newly registered rep type will
not be loaded for the already-enabled vports — which is the exact case
this commit is introducing infrastructure to handle.

Registration still returns success to the auxiliary driver
(mlx5e_rep_probe / mlx5r_rep_probe), with no log message, no retry, and
no way for the caller to notice the dropped reload.

The other existing caller, mlx5_esw_funcs_changed_handler(), does check
the return value of mlx5_esw_add_work(), suggesting allocation failure
is expected to be handled.

Should this call check the return value and at least emit a warning or
retry, so that a GFP_ATOMIC allocation failure during switchdev-mode
registration does not silently leave the representors unloaded?

>  }
>  
>  void mlx5_eswitch_register_vport_reps(struct mlx5_eswitch *esw,