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

[Simon Horman <horms@kernel.org>]

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?

> +	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:

...