Re: [PATCH net] net/mlx5e: Move representor vnic reporter to eswitch devlink port

[Carolina Jubran <cjubran@nvidia.com>]

On 31/07/2026 19:57, Simon Horman wrote:
> On Wed, Jul 29, 2026 at 10:00:59AM +0300, Tariq Toukan wrote:
>
> ...
>
>> @@ -252,6 +275,16 @@ int mlx5_esw_offloads_devlink_port_register(struct mlx5_eswitch *esw, struct mlx
>>   		mlx5_core_dbg(dev, "Failed to register port resources: %d\n",
>>   			      err);
>>   
>> +	rep = mlx5_eswitch_vport_rep(esw, vport_num);
>> +	reporter = devl_port_health_reporter_create(
>> +		&dl_port->dl_port, &mlx5_esw_rep_vnic_reporter_ops, rep);
> Hi Caroila, and Tariq,
>
> I'm wondering if you could take a look over the following which appears at
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260729070059.2421788-1-tariqt%40nvidia.com
>
>    Can rep or rep->esw be NULL at this point?
>
>    mlx5_eswitch_vport_rep() is a plain lookup with no NULL check here:
>
>    eswitch_offloads.c:mlx5_eswitch_get_rep() {
> 	return xa_load(&esw->offloads.vport_reps, vport);
>    }
>
>    and rep->esw is only assigned lazily. In mlx5_esw_offloads_rep_add() the
>    assignment sits after the continue that is taken whenever no rep_ops are
>    registered yet, which is the state at probe time when
>    esw_offloads_init_reps() creates the reps:
>
>    eswitch_offloads.c:mlx5_esw_offloads_rep_add() {
> 	rep = kzalloc_obj(*rep);
> 	...
> 	for (rep_type = 0; rep_type < NUM_REP_TYPES; rep_type++) {
> 		if (!esw->offloads.rep_ops[rep_type]) {
> 			atomic_set(&rep->rep_data[rep_type].state,
> 				   REP_UNREGISTERED);
> 			continue;
> 		}
> 		...
> 		rep->esw = esw;
> 	}
>    }
>
>    The only other writer is mlx5_eswitch_register_vport_reps_blocked(),
>    which runs from the mlx5_core.eth-rep auxiliary driver probe
>    (mlx5e_rep_probe() -> mlx5_eswitch_register_vport_reps()):
>
>    eswitch_offloads.c:mlx5_eswitch_register_vport_reps_blocked() {
> 	esw->offloads.rep_ops[rep_type] = ops;
> 	mlx5_esw_for_each_rep(esw, i, rep) {
> 		if (likely(mlx5_eswitch_vport_has_rep(esw, rep->vport))) {
> 			rep->esw = esw;
> 			...
>    }
>
>    Is that probe guaranteed to have succeeded before ports are registered?
>    esw_mode_change() discards the mlx5_rescan_drivers_locked() return value
>    and sets esw->mode = mode regardless, and add_drivers() only warns when
>    add_adev() fails:
>
>    dev.c:add_drivers() {
> 	priv->adev[i] = add_adev(dev, i);
> 	if (IS_ERR(priv->adev[i])) {
> 		mlx5_core_warn(dev, "Device[%d] (%s) failed to load\n", ...);
> 		/* We continue to rescan drivers and leave to the caller ... */
>
>    In that case rep_ops[REP_ETH] stays NULL, rep->esw stays NULL, and
>    mlx5_eswitch_load_vport() -> mlx5_esw_offloads_load_rep() ->
>    mlx5_esw_offloads_devlink_port_register() still installs a live reporter.
>    Would a subsequent "devlink health diagnose <dev>/<port> reporter vnic"
>    then oops on rep->esw->dev in mlx5_esw_rep_vnic_reporter_diagnose(),
>    with the devlink instance lock held?
>
>    The old code could not hit this because the reporter was created from the
>    REP_ETH load callback, i.e. only after rep_ops were registered and
>    rep->esw was set. Note also that mlx5_esw_offloads_rep_remove() does
>    check the same lookup result for NULL, which this call site does not.
>
>    Since struct mlx5_devlink_port already carries a struct mlx5_vport
>    pointer, which gives both vport->dev and vport->vport and whose lifetime
>    matches the port exactly, could dl_port->vport be used as the reporter
>    priv instead of the rep?


Good point, will fix it. Thanks

>
>> +	if (IS_ERR(reporter))
>> +		mlx5_core_dbg(dev,
>> +			      "Failed to create vnic health reporter for vport %d: %pe\n",
>> +			      vport_num, reporter);
>> +	else
>> +		dl_port->vnic_reporter = reporter;
>> +
>>   	return 0;
>>   
>>   rate_err:
> ...