Re: [PATCH net-next V2 4/7] net/mlx5: E-Switch, serialize representor lifecycle

[Mark Bloch <mbloch@nvidia.com>]

On 03/05/2026 4:42, Jakub Kicinski wrote:
> 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, serialize representor lifecycle
> 
> This change replaces the ad-hoc bit/wait-queue coordination of E-Switch
> representor lifecycle with the per-E-Switch reps_lock, adds nested
> wrappers for the shared-FDB master IB path, and clears the rep_ops
> pointer on unregister.
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> index 66a773a99876..f70737437954 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> 
> [ ... ]
> 
>> @@ -1825,7 +1831,11 @@ void mlx5_eswitch_disable(struct mlx5_eswitch *esw)
>>  
>>  	devl_assert_locked(priv_to_devlink(esw->dev));
>>  	mlx5_lag_disable_change(esw->dev);
>> +
>> +	mlx5_esw_reps_block(esw);
>>  	mlx5_eswitch_disable_locked(esw);
>> +	mlx5_esw_reps_unblock(esw);
>> +
>>  	esw->mode = MLX5_ESWITCH_LEGACY;
>>  	mlx5_lag_enable_change(esw->dev);
>>  }
> 
> The commit message says "Take the representor lock around the mode
> transition", but the assignment esw->mode = MLX5_ESWITCH_LEGACY happens
> here after reps_lock has been dropped.
> 
> Meanwhile mlx5_eswitch_unregister_vport_reps_blocked() now reads
> esw->mode under reps_lock to decide whether to call
> __unload_reps_all_vport():
> 
>     if (esw->mode == MLX5_ESWITCH_OFFLOADS)
>             __unload_reps_all_vport(esw, rep_type);
> 
> Should the mode store be moved inside the reps_block/reps_unblock
> region so the reader's assumption about stability under reps_lock
> holds?
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>> index 6a5143b63dfd..d4ac07c995b9 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> 
> [ ... ]
> 
>> @@ -2425,21 +2436,22 @@ void mlx5_esw_reps_unblock(struct mlx5_eswitch *esw)
>>  
>>  static void esw_mode_change(struct mlx5_eswitch *esw, u16 mode)
>>  {
>> +	mlx5_esw_reps_unblock(esw);
>>  	mlx5_devcom_comp_lock(esw->dev->priv.hca_devcom_comp);
>>  	if (esw->dev->priv.flags & MLX5_PRIV_FLAGS_DISABLE_IB_ADEV ||
>>  	    mlx5_core_mp_enabled(esw->dev)) {
>>  		esw->mode = mode;
>> -		mlx5_rescan_drivers_locked(esw->dev);
>> -		mlx5_devcom_comp_unlock(esw->dev->priv.hca_devcom_comp);
>> -		return;
>> +		goto out;
>>  	}
>>  
>>  	esw->dev->priv.flags |= MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
>>  	mlx5_rescan_drivers_locked(esw->dev);
>>  	esw->mode = mode;
>>  	esw->dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_IB_ADEV;
>> +out:
>>  	mlx5_rescan_drivers_locked(esw->dev);
>>  	mlx5_devcom_comp_unlock(esw->dev->priv.hca_devcom_comp);
>> +	mlx5_esw_reps_block(esw);
>>  }
> 
> Both assignments to esw->mode in this function are reached after
> reps_lock has been dropped at the top of the function.
> 
> mlx5_rescan_drivers_locked() can invoke driver bind/unbind which routes
> through mlx5_eswitch_unregister_vport_reps(), and that path now takes
> reps_lock and reads esw->mode to decide whether to call
> __unload_reps_all_vport().
> 
> Can a rep-unregister that runs during the rescan observe a stale
> esw->mode here and make the wrong unload decision?
> 
> [ ... ]
> 
>> @@ -4496,9 +4524,10 @@ mlx5_eswitch_vport_has_rep(const struct mlx5_eswitch *esw, u16 vport_num)
> 
> [ ... ]
> 
>> @@ -4525,9 +4585,35 @@ void mlx5_eswitch_unregister_vport_reps(struct mlx5_eswitch *esw, u8 rep_type)
>>  
>>  	mlx5_esw_for_each_rep(esw, i, rep)
>>  		atomic_set(&rep->rep_data[rep_type].state, REP_UNREGISTERED);
>> +
>> +	esw->offloads.rep_ops[rep_type] = NULL;
>> +}
> 
> This new NULL store on unregister appears to introduce a concurrent
> NULL dereference in readers that do not hold reps_lock. Before this
> patch, rep_ops[rep_type] was set once at register and never cleared,
> so a REP_LOADED state check was sufficient to know the pointer was
> valid.
> 
> mlx5_eswitch_get_proto_dev() is one such reader:
> 
>     rep = mlx5_eswitch_get_rep(esw, vport);
>     if (atomic_read(&rep->rep_data[rep_type].state) == REP_LOADED &&
>         esw->offloads.rep_ops[rep_type]->get_proto_dev)
>             return esw->offloads.rep_ops[rep_type]->get_proto_dev(rep);
> 
> The state check and each pointer load here are independent reads. Can
> a concurrent unregister on another CPU run between the state load and
> the ->get_proto_dev dereference, transitioning the rep through
> REP_REGISTERED/REP_UNREGISTERED and then storing NULL into rep_ops,
> so the second load observes NULL?
> 
> mlx5_esw_offloads_rep_event_unpair() and mlx5_esw_offloads_pair() have
> the same shape:
> 
>     ops = esw->offloads.rep_ops[rep_type];
>     if (atomic_read(&rep->rep_data[rep_type].state) == REP_LOADED &&
>         ops->event)
>             ops->event(esw, rep, ...);
> 
> Neither path holds reps_lock; they run from the devcom PAIR/UNPAIR
> event handler. Can ops be NULL here once an unregister has cleared
> rep_ops?
> 
> If readers are expected to be safe, should they either take reps_lock,
> be converted to RCU, or skip the NULL clear on unregister?
> 
> [ ... ]

Good point. The unregister path should not key the unload decision off
esw->mode. esw->mode is not the lifetime predicate for representor ops,
especially because mode transitions intentionally drop reps_lock around
auxiliary bus rescans. The actual invariant we need is that unregister
unloads any currently loaded reps before the ops pointer can be cleared
or the ops owner can go away.

I will change mlx5_eswitch_unregister_vport_reps_blocked() to always
call __unload_reps_all_vport(). That helper is already state-gated,
so this is a no-op when no reps are loaded.

With that change, the lockless readers do not get a new live
rep_ops == NULL window. unregister first synchronously unloads the
loaded reps, and those unload callbacks tear down the representor-owned
contexts that can call get_proto_dev() / rep events. Clearing rep_op
after that removes a stale pointer keeping it would be wrong because
the ops memory may be going away.

Mark