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

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

[Tariq Toukan]

From: Carolina Jubran <cjubran@nvidia.com>

The representor vnic devlink health reporter is created and destroyed
along the representor netdev (un)load path, which is not serialized by
the devlink instance lock. Destroying the reporter from there triggers
a devl_assert_locked() splat on driver unbind:
  WARNING: net/devlink/core.c:259 at devl_assert_locked+0x54/0x70, CPU#2: bash/3758
  Modules linked in: mlx5_vdpa vringh vdpa mlx5_ib mlx5_fwctl mlx5_core ...
  CPU: 2 UID: 0 PID: 3758 Comm: bash Tainted: G        W           6.19.0+ #1 PREEMPT
  Tainted: [W]=WARN
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), ...
  RIP: 0010:devl_assert_locked+0x54/0x70
  Call Trace:
   <TASK>
   devl_health_reporter_destroy+0x3a/0x1b0
   mlx5e_vport_rep_unload+0x12d/0x2b0 [mlx5_core]
   mlx5_eswitch_unregister_vport_reps+0x1b8/0x220 [mlx5_core]
   ? __esw_offloads_unload_rep+0x190/0x190 [mlx5_core]
   ? kernfs_remove_by_name_ns+0xc3/0xf0
   device_release_driver_internal+0x3b2/0x560
   unbind_store+0xce/0xf0

Move the reporter's lifecycle to the eswitch devlink port
(un)register paths and store the handle on mlx5_devlink_port. The
reporter is logically tied to the eswitch port, and its diagnose
callback only uses mlx5_eswitch_rep, so pass the rep directly as the
reporter priv.

Fixes: cf14af140a5a ("net/mlx5e: Add vnic devlink health reporter to representors")
Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Cosmin Ratiu <cratiu@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  | 52 +------------------
 .../net/ethernet/mellanox/mlx5/core/en_rep.h  |  1 -
 .../mellanox/mlx5/core/esw/devlink_port.c     | 39 ++++++++++++++
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  1 +
 4 files changed, 41 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index c8b76d301c92..783c76953e95 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -56,7 +56,6 @@
 #include "lib/vxlan.h"
 #define CREATE_TRACE_POINTS
 #include "diag/en_rep_tracepoint.h"
-#include "diag/reporter_vnic.h"
 #include "en_accel/ipsec.h"
 #include "en/tc/int_port.h"
 #include "en/ptp.h"
@@ -1425,51 +1424,6 @@ static unsigned int mlx5e_ul_rep_stats_grps_num(struct mlx5e_priv *priv)
 	return ARRAY_SIZE(mlx5e_ul_rep_stats_grps);
 }
 
-static int
-mlx5e_rep_vnic_reporter_diagnose(struct devlink_health_reporter *reporter,
-				 struct devlink_fmsg *fmsg,
-				 struct netlink_ext_ack *extack)
-{
-	struct mlx5e_rep_priv *rpriv = devlink_health_reporter_priv(reporter);
-	struct mlx5_eswitch_rep *rep = rpriv->rep;
-
-	mlx5_reporter_vnic_diagnose_counters(rep->esw->dev, fmsg, rep->vport,
-					     true);
-	return 0;
-}
-
-static const struct devlink_health_reporter_ops mlx5_rep_vnic_reporter_ops = {
-	.name = "vnic",
-	.diagnose = mlx5e_rep_vnic_reporter_diagnose,
-};
-
-static void mlx5e_rep_vnic_reporter_create(struct mlx5e_priv *priv,
-					   struct devlink_port *dl_port)
-{
-	struct mlx5e_rep_priv *rpriv = priv->ppriv;
-	struct devlink_health_reporter *reporter;
-
-	reporter = devl_port_health_reporter_create(dl_port,
-						    &mlx5_rep_vnic_reporter_ops,
-						    rpriv);
-	if (IS_ERR(reporter)) {
-		mlx5_core_err(priv->mdev,
-			      "Failed to create representor vnic reporter, err = %pe\n",
-			      reporter);
-		return;
-	}
-
-	rpriv->rep_vnic_reporter = reporter;
-}
-
-static void mlx5e_rep_vnic_reporter_destroy(struct mlx5e_priv *priv)
-{
-	struct mlx5e_rep_priv *rpriv = priv->ppriv;
-
-	if (!IS_ERR_OR_NULL(rpriv->rep_vnic_reporter))
-		devl_health_reporter_destroy(rpriv->rep_vnic_reporter);
-}
-
 static const struct mlx5e_profile mlx5e_rep_profile = {
 	.init			= mlx5e_init_rep,
 	.cleanup		= mlx5e_cleanup_rep,
@@ -1593,10 +1547,8 @@ mlx5e_vport_vf_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 
 	dl_port = mlx5_esw_offloads_devlink_port(dev->priv.eswitch,
 						 rpriv->rep->vport);
-	if (!IS_ERR(dl_port)) {
+	if (!IS_ERR(dl_port))
 		SET_NETDEV_DEVLINK_PORT(netdev, dl_port);
-		mlx5e_rep_vnic_reporter_create(priv, dl_port);
-	}
 
 	err = register_netdev(netdev);
 	if (err) {
@@ -1609,7 +1561,6 @@ mlx5e_vport_vf_rep_load(struct mlx5_core_dev *dev, struct mlx5_eswitch_rep *rep)
 	return 0;
 
 err_detach_netdev:
-	mlx5e_rep_vnic_reporter_destroy(priv);
 	mlx5e_detach_netdev(netdev_priv(netdev));
 err_cleanup_profile:
 	priv->profile->cleanup(priv);
@@ -1667,7 +1618,6 @@ mlx5e_vport_rep_unload(struct mlx5_eswitch_rep *rep)
 	}
 
 	unregister_netdev(netdev);
-	mlx5e_rep_vnic_reporter_destroy(priv);
 	mlx5e_detach_netdev(priv);
 	priv->profile->cleanup(priv);
 	mlx5e_destroy_netdev(netdev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
index 70640fa1ad7b..bcd7b4e814d0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.h
@@ -118,7 +118,6 @@ struct mlx5e_rep_priv {
 	struct rtnl_link_stats64 prev_vf_vport_stats;
 	struct mlx5_flow_handle *send_to_vport_meta_rule;
 	struct rhashtable tc_ht;
-	struct devlink_health_reporter *rep_vnic_reporter;
 };
 
 static inline
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c
index 6e50311faa27..46f110170fb3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c
@@ -4,6 +4,27 @@
 #include <linux/mlx5/driver.h>
 #include "eswitch.h"
 #include "devlink.h"
+#include "mlx5_core.h"
+#include "diag/reporter_vnic.h"
+
+static int
+mlx5_esw_rep_vnic_reporter_diagnose(struct devlink_health_reporter *reporter,
+				    struct devlink_fmsg *fmsg,
+				    struct netlink_ext_ack *extack)
+{
+	struct mlx5_eswitch_rep *rep = devlink_health_reporter_priv(reporter);
+
+	mlx5_reporter_vnic_diagnose_counters(rep->esw->dev, fmsg, rep->vport,
+					     true);
+
+	return 0;
+}
+
+static const struct
+devlink_health_reporter_ops mlx5_esw_rep_vnic_reporter_ops = {
+	.name = "vnic",
+	.diagnose = mlx5_esw_rep_vnic_reporter_diagnose,
+};
 
 static void
 mlx5_esw_get_port_parent_id(struct mlx5_core_dev *dev, struct netdev_phys_item_id *ppid)
@@ -217,9 +238,11 @@ static void mlx5_esw_devlink_port_res_unregister(struct devlink_port *dl_port)
 
 int mlx5_esw_offloads_devlink_port_register(struct mlx5_eswitch *esw, struct mlx5_vport *vport)
 {
+	struct devlink_health_reporter *reporter;
 	struct mlx5_core_dev *dev = esw->dev;
 	const struct devlink_port_ops *ops;
 	struct mlx5_devlink_port *dl_port;
+	struct mlx5_eswitch_rep *rep;
 	u16 vport_num = vport->vport;
 	unsigned int dl_port_index;
 	struct devlink *devlink;
@@ -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);
+	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:
@@ -266,6 +299,12 @@ void mlx5_esw_offloads_devlink_port_unregister(struct mlx5_vport *vport)
 	if (!vport->dl_port)
 		return;
 	dl_port = vport->dl_port;
+
+	if (dl_port->vnic_reporter) {
+		devl_health_reporter_destroy(dl_port->vnic_reporter);
+		dl_port->vnic_reporter = NULL;
+	}
+
 	mlx5_esw_devlink_port_res_unregister(&dl_port->dl_port);
 
 	mlx5_esw_qos_vport_update_parent(vport, NULL, NULL);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index fea72b1dedab..e8817d45763a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -189,6 +189,7 @@ struct mlx5_vport;
 struct mlx5_devlink_port {
 	struct devlink_port dl_port;
 	struct mlx5_vport *vport;
+	struct devlink_health_reporter *vnic_reporter;
 };
 
 static inline void mlx5_devlink_port_init(struct mlx5_devlink_port *dl_port,

base-commit: 3bd438a58e910db5dc369aa25dfed1fc95f1b596
-- 
2.44.0

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

[Simon Horman]

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:

...