[PATCH net-next RFC 0/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[PATCH net-next RFC 0/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

This patchsets aims to introduce an entity, that allows to pin
devlink configuration objects (params, resources, etc) on for things
shared among multiple PFs.

As the shared entity that kind of float above the actual PFs does not
have explicit a PCI function, use faux device to back the devlink
instance. Expose the relationship between PF devlink instances and
this new shared instance by nested devlink attributes.

Example:

$ devlink dev
pci/0000:08:00.0:                   <--- PF0
  nested_devlink:
    auxiliary/mlx5_core.eth.0
faux/mlx5_core_83013c12b77faa1a30000c82a1045c91:  <--- shared devlink for chip that PF0 and PF1 are part of
  nested_devlink:
    pci/0000:08:00.0
    pci/0000:08:00.1
auxiliary/mlx5_core.eth.0           <--- ethernet auxiliary device of PF0
pci/0000:08:00.1:                   <--- PF1
  nested_devlink:
    auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1           <--- ethernet auxiliary device of PF1

The first patch is a little adjustment to recently introduced faux driver.
The second patch introduces the shared devlink instance.
The last patch introduces example devlink param for the shared instance.

Jiri Pirko (3):
  faux: extend the creation function for module namespace
  net/mlx5: Introduce shared devlink instance for PFs on same chip
  net/mlx5: Introduce enable_sriov param for shared devlink

 drivers/base/faux.c                           |  20 +-
 .../net/ethernet/mellanox/mlx5/core/Makefile  |   5 +-
 .../mellanox/mlx5/core/lib/nv_param.c         | 236 ++++++++++++++++++
 .../mellanox/mlx5/core/lib/nv_param.h         |  14 ++
 .../net/ethernet/mellanox/mlx5/core/main.c    |  18 ++
 .../ethernet/mellanox/mlx5/core/sh_devlink.c  | 164 ++++++++++++
 .../ethernet/mellanox/mlx5/core/sh_devlink.h  |  11 +
 include/linux/device/faux.h                   |   6 +-
 include/linux/mlx5/driver.h                   |   6 +
 include/linux/module.h                        |   2 +-
 10 files changed, 469 insertions(+), 13 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.h
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h

-- 
2.48.1

[PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

It is hard for the faux user to avoid potential name conflicts, as it is
only in control of faux devices it creates. Therefore extend the faux
device creation function by module parameter, embed the module name into
the device name in format "modulename_permodulename" and allow module to
control it's namespace.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/base/faux.c         | 20 ++++++++++++--------
 include/linux/device/faux.h |  6 ++++--
 include/linux/module.h      |  2 +-
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 531e9d789ee0..b1fcac6b0946 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -85,8 +85,9 @@ static void faux_device_release(struct device *dev)
  * faux_device_create_with_groups - Create and register with the driver
  *		core a faux device and populate the device with an initial
  *		set of sysfs attributes.
- * @name:	The name of the device we are adding, must be unique for
- *		all faux devices.
+ * @module:	Pointer to module this device is associated.
+ * @name:	The name of the device we are adding, must be unique for all
+ *		faux devices within a single module.
  * @parent:	Pointer to a potential parent struct device.  If set to
  *		NULL, the device will be created in the "root" of the faux
  *		device tree in sysfs.
@@ -108,7 +109,8 @@ static void faux_device_release(struct device *dev)
  * * NULL if an error happened with creating the device
  * * pointer to a valid struct faux_device that is registered with sysfs
  */
-struct faux_device *faux_device_create_with_groups(const char *name,
+struct faux_device *faux_device_create_with_groups(const struct module *module,
+						   const char *name,
 						   struct device *parent,
 						   const struct faux_device_ops *faux_ops,
 						   const struct attribute_group **groups)
@@ -137,12 +139,12 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 		dev->parent = &faux_bus_root;
 	dev->bus = &faux_bus_type;
 	dev->groups = groups;
-	dev_set_name(dev, "%s", name);
+	dev_set_name(dev, "%s_%s", module_name(module), name);
 
 	ret = device_add(dev);
 	if (ret) {
 		pr_err("%s: device_add for faux device '%s' failed with %d\n",
-		       __func__, name, ret);
+		       __func__, dev_name(dev), ret);
 		put_device(dev);
 		return NULL;
 	}
@@ -153,8 +155,9 @@ EXPORT_SYMBOL_GPL(faux_device_create_with_groups);
 
 /**
  * faux_device_create - create and register with the driver core a faux device
+ * @module:	Pointer to module this device is associated.
  * @name:	The name of the device we are adding, must be unique for all
- *		faux devices.
+ *		faux devices within a single module.
  * @parent:	Pointer to a potential parent struct device.  If set to
  *		NULL, the device will be created in the "root" of the faux
  *		device tree in sysfs.
@@ -174,11 +177,12 @@ EXPORT_SYMBOL_GPL(faux_device_create_with_groups);
  * * NULL if an error happened with creating the device
  * * pointer to a valid struct faux_device that is registered with sysfs
  */
-struct faux_device *faux_device_create(const char *name,
+struct faux_device *faux_device_create(const struct module *module,
+				       const char *name,
 				       struct device *parent,
 				       const struct faux_device_ops *faux_ops)
 {
-	return faux_device_create_with_groups(name, parent, faux_ops, NULL);
+	return faux_device_create_with_groups(module, name, parent, faux_ops, NULL);
 }
 EXPORT_SYMBOL_GPL(faux_device_create);
 
diff --git a/include/linux/device/faux.h b/include/linux/device/faux.h
index 9f43c0e46aa4..b1393a34b4f9 100644
--- a/include/linux/device/faux.h
+++ b/include/linux/device/faux.h
@@ -47,10 +47,12 @@ struct faux_device_ops {
 	void (*remove)(struct faux_device *faux_dev);
 };
 
-struct faux_device *faux_device_create(const char *name,
+struct faux_device *faux_device_create(const struct module *module,
+				       const char *name,
 				       struct device *parent,
 				       const struct faux_device_ops *faux_ops);
-struct faux_device *faux_device_create_with_groups(const char *name,
+struct faux_device *faux_device_create_with_groups(const struct module *module,
+						   const char *name,
 						   struct device *parent,
 						   const struct faux_device_ops *faux_ops,
 						   const struct attribute_group **groups);
diff --git a/include/linux/module.h b/include/linux/module.h
index 30e5b19bafa9..8d1a7e65d2e4 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -744,7 +744,7 @@ static inline void __module_get(struct module *module)
 /* This is a #define so the string doesn't get put in every .o file */
 #define module_name(mod)			\
 ({						\
-	struct module *__mod = (mod);		\
+	const struct module *__mod = (mod);	\
 	__mod ? __mod->name : "kernel";		\
 })
 
-- 
2.48.1

[PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Multiple PFS may reside on the same physical chip, running a single
firmware. Some of the resources and configurations may be shared among
these PFs. Currently, there is not good object to pin the configuration
knobs on.

Introduce a shared devlink, instantiated upon probe of the first PF,
removed during remove of the last PF. Back this shared devlink instance
by faux device, as there is no PCI device related to it.

Make the PF devlink instances nested in this shared devlink instance.

Example:

$ devlink dev
pci/0000:08:00.0:
  nested_devlink:
    auxiliary/mlx5_core.eth.0
faux/mlx5_core_83013c12b77faa1a30000c82a1045c91:
  nested_devlink:
    pci/0000:08:00.0
    pci/0000:08:00.1
auxiliary/mlx5_core.eth.0
pci/0000:08:00.1:
  nested_devlink:
    auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/Makefile  |   4 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    |  18 +++
 .../ethernet/mellanox/mlx5/core/sh_devlink.c  | 150 ++++++++++++++++++
 .../ethernet/mellanox/mlx5/core/sh_devlink.h  |  10 ++
 include/linux/mlx5/driver.h                   |   5 +
 5 files changed, 185 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 568bbe5f83f5..510850b6e6e2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -16,8 +16,8 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 		transobj.o vport.o sriov.o fs_cmd.o fs_core.o pci_irq.o \
 		fs_counters.o fs_ft_pool.o rl.o lag/debugfs.o lag/lag.o dev.o events.o wq.o lib/gid.o \
 		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o diag/fs_tracepoint.o \
-		diag/fw_tracer.o diag/crdump.o devlink.o diag/rsc_dump.o diag/reporter_vnic.o \
-		fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
+		diag/fw_tracer.o diag/crdump.o devlink.o sh_devlink.o diag/rsc_dump.o \
+		diag/reporter_vnic.o fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
 
 #
 # Netdev basic
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 710633d5fdbe..e1217a8bf4db 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -74,6 +74,7 @@
 #include "mlx5_irq.h"
 #include "hwmon.h"
 #include "lag/lag.h"
+#include "sh_devlink.h"
 
 MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
 MODULE_DESCRIPTION("Mellanox 5th generation network adapters (ConnectX series) core driver");
@@ -1554,10 +1555,17 @@ int mlx5_init_one(struct mlx5_core_dev *dev)
 	int err;
 
 	devl_lock(devlink);
+	if (dev->shd) {
+		err = devl_nested_devlink_set(priv_to_devlink(dev->shd),
+					      devlink);
+		if (err)
+			goto unlock;
+	}
 	devl_register(devlink);
 	err = mlx5_init_one_devl_locked(dev);
 	if (err)
 		devl_unregister(devlink);
+unlock:
 	devl_unlock(devlink);
 	return err;
 }
@@ -1998,6 +2006,13 @@ static int probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto pci_init_err;
 	}
 
+	err = mlx5_shd_init(dev);
+	if (err) {
+		mlx5_core_err(dev, "mlx5_shd_init failed with error code %d\n",
+			      err);
+		goto shd_init_err;
+	}
+
 	err = mlx5_init_one(dev);
 	if (err) {
 		mlx5_core_err(dev, "mlx5_init_one failed with error code %d\n",
@@ -2009,6 +2024,8 @@ static int probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 
 err_init_one:
+	mlx5_shd_uninit(dev);
+shd_init_err:
 	mlx5_pci_close(dev);
 pci_init_err:
 	mlx5_mdev_uninit(dev);
@@ -2030,6 +2047,7 @@ static void remove_one(struct pci_dev *pdev)
 	mlx5_drain_health_wq(dev);
 	mlx5_sriov_disable(pdev, false);
 	mlx5_uninit_one(dev);
+	mlx5_shd_uninit(dev);
 	mlx5_pci_close(dev);
 	mlx5_mdev_uninit(dev);
 	mlx5_adev_idx_free(dev->priv.adev_idx);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
new file mode 100644
index 000000000000..671a3442525b
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
+
+#include <linux/device/faux.h>
+#include <linux/mlx5/driver.h>
+#include <linux/mlx5/vport.h>
+
+#include "sh_devlink.h"
+
+static LIST_HEAD(shd_list);
+static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
+
+/* This structure represents a shared devlink instance,
+ * there is one created for PF group of the same chip.
+ */
+struct mlx5_shd {
+	/* Node in shd list */
+	struct list_head list;
+	/* Serial number of the chip */
+	const char *sn;
+	/* List of per-PF dev instances. */
+	struct list_head dev_list;
+	/* Related faux device */
+	struct faux_device *faux_dev;
+};
+
+static const struct devlink_ops mlx5_shd_ops = {
+};
+
+static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
+{
+	struct devlink *devlink;
+	struct mlx5_shd *shd;
+
+	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd), &faux_dev->dev);
+	if (!devlink)
+		return -ENOMEM;
+	shd = devlink_priv(devlink);
+	faux_device_set_drvdata(faux_dev, shd);
+
+	devl_lock(devlink);
+	devl_register(devlink);
+	devl_unlock(devlink);
+	return 0;
+}
+
+static void mlx5_shd_faux_remove(struct faux_device *faux_dev)
+{
+	struct mlx5_shd *shd = faux_device_get_drvdata(faux_dev);
+	struct devlink *devlink = priv_to_devlink(shd);
+
+	devl_lock(devlink);
+	devl_unregister(devlink);
+	devl_unlock(devlink);
+	devlink_free(devlink);
+}
+
+static const struct faux_device_ops mlx5_shd_faux_ops = {
+	.probe = mlx5_shd_faux_probe,
+	.remove = mlx5_shd_faux_remove,
+};
+
+static struct mlx5_shd *mlx5_shd_create(const char *sn)
+{
+	struct faux_device *faux_dev;
+	struct mlx5_shd *shd;
+
+	faux_dev = faux_device_create(THIS_MODULE, sn, NULL, &mlx5_shd_faux_ops);
+	if (!faux_dev)
+		return NULL;
+	shd = faux_device_get_drvdata(faux_dev);
+	if (!shd)
+		return NULL;
+	list_add_tail(&shd->list, &shd_list);
+	shd->sn = sn;
+	INIT_LIST_HEAD(&shd->dev_list);
+	shd->faux_dev = faux_dev;
+	return shd;
+}
+
+static void mlx5_shd_destroy(struct mlx5_shd *shd)
+{
+	list_del(&shd->list);
+	kfree(shd->sn);
+	faux_device_destroy(shd->faux_dev);
+}
+
+int mlx5_shd_init(struct mlx5_core_dev *dev)
+{
+	u8 *vpd_data __free(kfree) = NULL;
+	struct pci_dev *pdev = dev->pdev;
+	unsigned int vpd_size, kw_len;
+	struct mlx5_shd *shd;
+	const char *sn;
+	char *end;
+	int start;
+	int err;
+
+	if (!mlx5_core_is_pf(dev))
+		return 0;
+
+	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
+	if (IS_ERR(vpd_data)) {
+		err = PTR_ERR(vpd_data);
+		return err == -ENODEV ? 0 : err;
+	}
+	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3", &kw_len);
+	if (start < 0) {
+		/* Fall-back to SN for older devices. */
+		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+						     PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
+		if (start < 0)
+			return -ENOENT;
+	}
+	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
+	if (!sn)
+		return -ENOMEM;
+	end = strchrnul(sn, ' ');
+	*end = '\0';
+
+	guard(mutex)(&shd_mutex);
+	list_for_each_entry(shd, &shd_list, list) {
+		if (!strcmp(shd->sn, sn)) {
+			kfree(sn);
+			goto found;
+		}
+	}
+	shd = mlx5_shd_create(sn);
+	if (!shd) {
+		kfree(sn);
+		return -ENOMEM;
+	}
+found:
+	list_add_tail(&dev->shd_list, &shd->dev_list);
+	dev->shd = shd;
+	return 0;
+}
+
+void mlx5_shd_uninit(struct mlx5_core_dev *dev)
+{
+	struct mlx5_shd *shd = dev->shd;
+
+	if (!dev->shd)
+		return;
+
+	guard(mutex)(&shd_mutex);
+	list_del(&dev->shd_list);
+	if (list_empty(&shd->dev_list))
+		mlx5_shd_destroy(shd);
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
new file mode 100644
index 000000000000..67df03e3c72e
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
+
+#ifndef __MLX5_SH_DEVLINK_H__
+#define __MLX5_SH_DEVLINK_H__
+
+int mlx5_shd_init(struct mlx5_core_dev *dev);
+void mlx5_shd_uninit(struct mlx5_core_dev *dev);
+
+#endif /* __MLX5_SH_DEVLINK_H__ */
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 46bd7550adf8..78f1f034568f 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -721,6 +721,8 @@ enum mlx5_wc_state {
 	MLX5_WC_STATE_SUPPORTED,
 };
 
+struct mlx5_shd;
+
 struct mlx5_core_dev {
 	struct device *device;
 	enum mlx5_coredev_type coredev_type;
@@ -783,6 +785,9 @@ struct mlx5_core_dev {
 	enum mlx5_wc_state wc_state;
 	/* sync write combining state */
 	struct mutex wc_state_lock;
+	/* node in shared devlink list */
+	struct list_head shd_list;
+	struct mlx5_shd *shd;
 };
 
 struct mlx5_db {
-- 
2.48.1

[PATCH net-next RFC 3/3] net/mlx5: Introduce enable_sriov param for shared devlink

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

As some of the mlx5 devices only support enabling/disabling SR-IOV
per-chip and not per-PF, introduce this param attached to shared
devlink instance.

Example:
$ devlink dev param show faux/mlx5_core_83013c12b77faa1a30000c82a1045c91
faux/mlx5_core_83013c12b77faa1a30000c82a1045c91:
  name enable_sriov type generic
    values:
      cmode permanent value false
$ devlink dev param set faux/mlx5_core_83013c12b77faa1a30000c82a1045c91 name enable_sriov cmode permanent value true

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/Makefile  |   3 +-
 .../mellanox/mlx5/core/lib/nv_param.c         | 236 ++++++++++++++++++
 .../mellanox/mlx5/core/lib/nv_param.h         |  14 ++
 .../ethernet/mellanox/mlx5/core/sh_devlink.c  |  16 +-
 .../ethernet/mellanox/mlx5/core/sh_devlink.h  |   1 +
 include/linux/mlx5/driver.h                   |   1 +
 6 files changed, 269 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.h

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 510850b6e6e2..238c212ad0fd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -17,7 +17,8 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 		fs_counters.o fs_ft_pool.o rl.o lag/debugfs.o lag/lag.o dev.o events.o wq.o lib/gid.o \
 		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o diag/fs_tracepoint.o \
 		diag/fw_tracer.o diag/crdump.o devlink.o sh_devlink.o diag/rsc_dump.o \
-		diag/reporter_vnic.o fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
+		diag/reporter_vnic.o fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o \
+		lib/nv_param.o
 
 #
 # Netdev basic
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
new file mode 100644
index 000000000000..04682bec8fa5
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
+
+#include "nv_param.h"
+#include "sh_devlink.h"
+#include "mlx5_core.h"
+
+enum {
+	MLX5_CLASS_0_CTRL_ID_NV_GLOBAL_PCI_CONF	       = 0x80,
+	MLX5_CLASS_0_CTRL_ID_NV_GLOBAL_PCI_CAP		= 0x81,
+};
+
+struct mlx5_ifc_configuration_item_type_class_global_bits {
+	u8	 type_class[0x8];
+	u8	 parameter_index[0x18];
+};
+
+union mlx5_ifc_config_item_type_auto_bits {
+	struct mlx5_ifc_configuration_item_type_class_global_bits
+				configuration_item_type_class_global;
+	u8 reserved_at_0[0x40];
+};
+
+struct mlx5_ifc_config_item_bits {
+	u8	 valid[0x2];
+	u8	 priority[0x2];
+	u8	 header_type[0x2];
+	u8	 ovr_en[0x1];
+	u8	 rd_en[0x1];
+	u8	 access_mode[0x2];
+	u8	 reserved_at_a[0x1];
+	u8	 writer_id[0x5];
+	u8	 version[0x4];
+	u8	 reserved_at_14[0x2];
+	u8	 host_id_valid[0x1];
+	u8	 length[0x9];
+
+	union mlx5_ifc_config_item_type_auto_bits type;
+
+	u8	 reserved_at_40[0x10];
+	u8	 crc16[0x10];
+};
+
+struct mlx5_ifc_mnvda_reg_bits {
+	struct mlx5_ifc_config_item_bits configuration_item_header;
+
+	u8	 configuration_item_data[64][0x20];
+};
+
+struct mlx5_ifc_nv_global_pci_conf_bits {
+	u8	 sriov_valid[0x1];
+	u8	 reserved_at_1[0x10];
+	u8	 per_pf_total_vf[0x1];
+	u8	 reserved_at_12[0xe];
+
+	u8	 sriov_en[0x1];
+	u8	 reserved_at_21[0xf];
+	u8	 total_vfs[0x10];
+
+	u8	 reserved_at_40[0x20];
+};
+
+struct mlx5_ifc_nv_global_pci_cap_bits {
+	u8	 max_vfs_per_pf_valid[0x1];
+	u8	 reserved_at_1[0x13];
+	u8	 per_pf_total_vf_supported[0x1];
+	u8	 reserved_at_15[0xb];
+
+	u8	 sriov_support[0x1];
+	u8	 reserved_at_21[0xf];
+	u8	 max_vfs_per_pf[0x10];
+
+	u8	 reserved_at_40[0x60];
+};
+
+#define MNVDA_HDR_SZ \
+	(MLX5_ST_SZ_BYTES(mnvda_reg) - MLX5_BYTE_OFF(mnvda_reg, configuration_item_data))
+
+#define MLX5_SET_CONFIG_ITEM_TYPE(_cls_name, _mnvda_ptr, _field, _val) \
+	MLX5_SET(mnvda_reg, _mnvda_ptr, \
+		 configuration_item_header.type.configuration_item_type_class_##_cls_name._field, \
+		 _val)
+
+#define MLX5_SET_CONFIG_HDR_LEN(_mnvda_ptr, _cls_name) \
+	MLX5_SET(mnvda_reg, _mnvda_ptr, configuration_item_header.length, \
+		 MLX5_ST_SZ_BYTES(_cls_name))
+
+#define MLX5_GET_CONFIG_HDR_LEN(_mnvda_ptr) \
+	MLX5_GET(mnvda_reg, _mnvda_ptr, configuration_item_header.length)
+
+static int mlx5_nv_param_read(struct mlx5_core_dev *dev, void *mnvda, size_t len)
+{
+	u32 param_idx, type_class;
+	u32 header_len;
+	void *cls_ptr;
+	int err;
+
+	if (WARN_ON(len > MLX5_ST_SZ_BYTES(mnvda_reg)) || len < MNVDA_HDR_SZ)
+		return -EINVAL; /* A caller bug */
+
+	err = mlx5_core_access_reg(dev, mnvda, len, mnvda, len, MLX5_REG_MNVDA, 0, 0);
+	if (!err)
+		return 0;
+
+	cls_ptr = MLX5_ADDR_OF(mnvda_reg, mnvda,
+			       configuration_item_header.type.configuration_item_type_class_global);
+
+	type_class = MLX5_GET(configuration_item_type_class_global, cls_ptr, type_class);
+	param_idx = MLX5_GET(configuration_item_type_class_global, cls_ptr, parameter_index);
+	header_len = MLX5_GET_CONFIG_HDR_LEN(mnvda);
+
+	mlx5_core_warn(dev, "Failed to read mnvda reg: type_class 0x%x, param_idx 0x%x, header_len %u, err %d\n",
+		       type_class, param_idx, header_len, err);
+
+	/* Let devlink skip this one if it fails, kernel log will have the failure */
+	return -EOPNOTSUPP;
+}
+
+static int mlx5_nv_param_write(struct mlx5_core_dev *dev, void *mnvda, size_t len)
+{
+	if (WARN_ON(len > MLX5_ST_SZ_BYTES(mnvda_reg)) || len < MNVDA_HDR_SZ)
+		return -EINVAL;
+
+	if (WARN_ON(MLX5_GET_CONFIG_HDR_LEN(mnvda) == 0))
+		return -EINVAL;
+
+	return mlx5_core_access_reg(dev, mnvda, len, mnvda, len, MLX5_REG_MNVDA, 0, 1);
+}
+
+static int
+mlx5_nv_param_read_global_pci_conf(struct mlx5_core_dev *dev, void *mnvda, size_t len)
+{
+	MLX5_SET_CONFIG_ITEM_TYPE(global, mnvda, type_class, 0);
+	MLX5_SET_CONFIG_ITEM_TYPE(global, mnvda, parameter_index,
+				  MLX5_CLASS_0_CTRL_ID_NV_GLOBAL_PCI_CONF);
+	MLX5_SET_CONFIG_HDR_LEN(mnvda, nv_global_pci_conf);
+
+	return mlx5_nv_param_read(dev, mnvda, len);
+}
+
+static int
+mlx5_nv_param_read_global_pci_cap(struct mlx5_core_dev *dev, void *mnvda, size_t len)
+{
+	MLX5_SET_CONFIG_ITEM_TYPE(global, mnvda, type_class, 0);
+	MLX5_SET_CONFIG_ITEM_TYPE(global, mnvda, parameter_index,
+				  MLX5_CLASS_0_CTRL_ID_NV_GLOBAL_PCI_CAP);
+	MLX5_SET_CONFIG_HDR_LEN(mnvda, nv_global_pci_cap);
+
+	return mlx5_nv_param_read(dev, mnvda, len);
+}
+
+static int mlx5_shd_enable_sriov_get(struct devlink *devlink, u32 id,
+				     struct devlink_param_gset_ctx *ctx)
+{
+	struct mlx5_core_dev *dev = mlx5_shd_dev(devlink_priv(devlink));
+	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
+	void *cap, *data;
+	int err;
+
+	err = mlx5_nv_param_read_global_pci_cap(dev, mnvda, sizeof(mnvda));
+	if (err)
+		return err;
+
+	cap = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	if (!MLX5_GET(nv_global_pci_cap, cap, sriov_support))
+		return -EOPNOTSUPP;
+
+	memset(mnvda, 0, sizeof(mnvda));
+	err = mlx5_nv_param_read_global_pci_conf(dev, mnvda, sizeof(mnvda));
+	if (err)
+		return err;
+
+	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	ctx->val.vbool = MLX5_GET(nv_global_pci_conf, data, sriov_en);
+	return 0;
+}
+
+static int mlx5_shd_enable_sriov_set(struct devlink *devlink, u32 id,
+				     struct devlink_param_gset_ctx *ctx,
+				     struct netlink_ext_ack *extack)
+{
+	struct mlx5_core_dev *dev = mlx5_shd_dev(devlink_priv(devlink));
+	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
+	void *cap, *data;
+	int err;
+
+	err = mlx5_nv_param_read_global_pci_cap(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to read global PCI capability");
+		return err;
+	}
+
+	cap = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+
+	if (!MLX5_GET(nv_global_pci_cap, cap, sriov_support)) {
+		NL_SET_ERR_MSG_MOD(extack, "Not configurable on this device");
+		return -EOPNOTSUPP;
+	}
+
+	memset(mnvda, 0, sizeof(mnvda));
+	err = mlx5_nv_param_read_global_pci_conf(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to read global PCI configuration");
+		return err;
+	}
+
+	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	MLX5_SET(nv_global_pci_conf, data, sriov_valid, 1);
+	MLX5_SET(nv_global_pci_conf, data, sriov_en, ctx->val.vbool);
+
+	err = mlx5_nv_param_write(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Unable to write global PCI configuration");
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct devlink_param mlx5_shd_nv_param_devlink_params[] = {
+	DEVLINK_PARAM_GENERIC(ENABLE_SRIOV, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      mlx5_shd_enable_sriov_get,
+			      mlx5_shd_enable_sriov_set, NULL),
+};
+
+int mlx5_shd_nv_param_register_dl_params(struct devlink *devlink)
+{
+	return devl_params_register(devlink, mlx5_shd_nv_param_devlink_params,
+				    ARRAY_SIZE(mlx5_shd_nv_param_devlink_params));
+}
+
+void mlx5_shd_nv_param_unregister_dl_params(struct devlink *devlink)
+{
+	devl_params_unregister(devlink, mlx5_shd_nv_param_devlink_params,
+			       ARRAY_SIZE(mlx5_shd_nv_param_devlink_params));
+}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.h
new file mode 100644
index 000000000000..785edfe9bf15
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
+/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
+
+#ifndef __MLX5_NV_PARAM_H
+#define __MLX5_NV_PARAM_H
+
+#include <linux/mlx5/driver.h>
+#include "devlink.h"
+
+int mlx5_shd_nv_param_register_dl_params(struct devlink *devlink);
+void mlx5_shd_nv_param_unregister_dl_params(struct devlink *devlink);
+
+#endif
+
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
index 671a3442525b..1c730071f00f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
@@ -2,9 +2,10 @@
 /* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
 
 #include <linux/device/faux.h>
+#include <linux/mlx5/device.h>
 #include <linux/mlx5/driver.h>
 #include <linux/mlx5/vport.h>
-
+#include "lib/nv_param.h"
 #include "sh_devlink.h"
 
 static LIST_HEAD(shd_list);
@@ -24,6 +25,12 @@ struct mlx5_shd {
 	struct faux_device *faux_dev;
 };
 
+struct mlx5_core_dev *mlx5_shd_dev(struct mlx5_shd *shd)
+{
+	return list_first_entry(&shd->dev_list,
+				struct mlx5_core_dev, shd_list);
+}
+
 static const struct devlink_ops mlx5_shd_ops = {
 };
 
@@ -31,6 +38,7 @@ static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
 {
 	struct devlink *devlink;
 	struct mlx5_shd *shd;
+	int err;
 
 	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd), &faux_dev->dev);
 	if (!devlink)
@@ -39,6 +47,11 @@ static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
 	faux_device_set_drvdata(faux_dev, shd);
 
 	devl_lock(devlink);
+	err = mlx5_shd_nv_param_register_dl_params(devlink);
+	if (err) {
+		devl_unlock(devlink);
+		return err;
+	}
 	devl_register(devlink);
 	devl_unlock(devlink);
 	return 0;
@@ -51,6 +64,7 @@ static void mlx5_shd_faux_remove(struct faux_device *faux_dev)
 
 	devl_lock(devlink);
 	devl_unregister(devlink);
+	mlx5_shd_nv_param_unregister_dl_params(devlink);
 	devl_unlock(devlink);
 	devlink_free(devlink);
 }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
index 67df03e3c72e..837b8a3ca54e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
@@ -4,6 +4,7 @@
 #ifndef __MLX5_SH_DEVLINK_H__
 #define __MLX5_SH_DEVLINK_H__
 
+struct mlx5_core_dev *mlx5_shd_dev(struct mlx5_shd *shd);
 int mlx5_shd_init(struct mlx5_core_dev *dev);
 void mlx5_shd_uninit(struct mlx5_core_dev *dev);
 
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 78f1f034568f..044a45abe614 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -135,6 +135,7 @@ enum {
 	MLX5_REG_MTCAP		 = 0x9009,
 	MLX5_REG_MTMP		 = 0x900A,
 	MLX5_REG_MCIA		 = 0x9014,
+	MLX5_REG_MNVDA		 = 0x9024,
 	MLX5_REG_MFRL		 = 0x9028,
 	MLX5_REG_MLCR		 = 0x902b,
 	MLX5_REG_MRTC		 = 0x902d,
-- 
2.48.1

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Greg KH]

On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -744,7 +744,7 @@ static inline void __module_get(struct module *module)
>  /* This is a #define so the string doesn't get put in every .o file */
>  #define module_name(mod)			\
>  ({						\
> -	struct module *__mod = (mod);		\
> +	const struct module *__mod = (mod);	\
>  	__mod ? __mod->name : "kernel";		\
>  })

This feels like it should be a separate change, right?  Doesn't have to
do with this patch.

greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Jiri Pirko]

Tue, Mar 18, 2025 at 02:46:56PM +0100, gregkh@linuxfoundation.org wrote:
>On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -744,7 +744,7 @@ static inline void __module_get(struct module *module)
>>  /* This is a #define so the string doesn't get put in every .o file */
>>  #define module_name(mod)			\
>>  ({						\
>> -	struct module *__mod = (mod);		\
>> +	const struct module *__mod = (mod);	\
>>  	__mod ? __mod->name : "kernel";		\
>>  })
>
>This feels like it should be a separate change, right?  Doesn't have to
>do with this patch.

True. Will split.

>
>greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Greg KH]

On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> It is hard for the faux user to avoid potential name conflicts, as it is
> only in control of faux devices it creates. Therefore extend the faux
> device creation function by module parameter, embed the module name into
> the device name in format "modulename_permodulename" and allow module to
> control it's namespace.

Do you have an example of how this will change the current names we have
in the system to this new way?  What is going to break if those names
change?

I say this as the perf devices seem to have "issues" with their names
and locations in sysfs as userspace tools use them today, and in a
straight port to faux it is ok, but if the device name changes, that is
going to have problems.

Why can't you handle this "namespace" issue yourself in the caller to
the api?  Why must the faux code handle it for you?  We don't do this
for platform devices, why is this any different?

thanks,

greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Jiri Pirko]

Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote:
>On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> It is hard for the faux user to avoid potential name conflicts, as it is
>> only in control of faux devices it creates. Therefore extend the faux
>> device creation function by module parameter, embed the module name into
>> the device name in format "modulename_permodulename" and allow module to
>> control it's namespace.
>
>Do you have an example of how this will change the current names we have
>in the system to this new way?  What is going to break if those names
>change?

I was under impression, that since there are no in-tree users of faux
yet (at least I don't see them in net-next tree), there is no breakage.

>
>I say this as the perf devices seem to have "issues" with their names
>and locations in sysfs as userspace tools use them today, and in a
>straight port to faux it is ok, but if the device name changes, that is
>going to have problems.

Got it. I didn't consider that.


>
>Why can't you handle this "namespace" issue yourself in the caller to
>the api?  Why must the faux code handle it for you?  We don't do this
>for platform devices, why is this any different?

Well, I wanted to avoid alloc&printf names in driver, since
dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups()
don't.

Perhaps "const char *name" could be formatted as well for
faux_device_create()/faux_device_create_with_groups(). My laziness
wanted to avoid that :) Would that make sense to you?

>
>thanks,
>
>greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Greg KH]

On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote:
> Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote:
> >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> It is hard for the faux user to avoid potential name conflicts, as it is
> >> only in control of faux devices it creates. Therefore extend the faux
> >> device creation function by module parameter, embed the module name into
> >> the device name in format "modulename_permodulename" and allow module to
> >> control it's namespace.
> >
> >Do you have an example of how this will change the current names we have
> >in the system to this new way?  What is going to break if those names
> >change?
> 
> I was under impression, that since there are no in-tree users of faux
> yet (at least I don't see them in net-next tree), there is no breakage.

Look at linux-next please.

> >Why can't you handle this "namespace" issue yourself in the caller to
> >the api?  Why must the faux code handle it for you?  We don't do this
> >for platform devices, why is this any different?
> 
> Well, I wanted to avoid alloc&printf names in driver, since
> dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups()
> don't.

If you want to do something complex, do it in your driver :)

> Perhaps "const char *name" could be formatted as well for
> faux_device_create()/faux_device_create_with_groups(). My laziness
> wanted to avoid that :) Would that make sense to you?

I wouldn't object to that, making it a vararg?  How would the rust
binding handle that?

thanks,

greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Jiri Pirko]

Tue, Mar 18, 2025 at 05:04:37PM +0100, gregkh@linuxfoundation.org wrote:
>On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote:
>> Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote:
>> >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.com>
>> >> 
>> >> It is hard for the faux user to avoid potential name conflicts, as it is
>> >> only in control of faux devices it creates. Therefore extend the faux
>> >> device creation function by module parameter, embed the module name into
>> >> the device name in format "modulename_permodulename" and allow module to
>> >> control it's namespace.
>> >
>> >Do you have an example of how this will change the current names we have
>> >in the system to this new way?  What is going to break if those names
>> >change?
>> 
>> I was under impression, that since there are no in-tree users of faux
>> yet (at least I don't see them in net-next tree), there is no breakage.
>
>Look at linux-next please.

Sure, but it's still next. Next might break (uapi) as long it's next,
right?


>
>> >Why can't you handle this "namespace" issue yourself in the caller to
>> >the api?  Why must the faux code handle it for you?  We don't do this
>> >for platform devices, why is this any different?
>> 
>> Well, I wanted to avoid alloc&printf names in driver, since
>> dev_set_name() accepts vararg and faux_device_create()/faux_device_create_with_groups()
>> don't.
>
>If you want to do something complex, do it in your driver :)

Yeah, I don't really want to do anything complex, that's why I wanted to
take leverage of dev_set_name()


>
>> Perhaps "const char *name" could be formatted as well for
>> faux_device_create()/faux_device_create_with_groups(). My laziness
>> wanted to avoid that :) Would that make sense to you?
>
>I wouldn't object to that, making it a vararg?  How would the rust
>binding handle that?

Why should I care about rust? I got the impression the deal is that
rust bindings are taken care of by rust people. Did that change and
we need to keep rust in mind for all internal API? That sounds scarry
to me :(


>
>thanks,
>
>greg k-h

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Greg KH]

On Tue, Mar 18, 2025 at 05:51:34PM +0100, Jiri Pirko wrote:
> Tue, Mar 18, 2025 at 05:04:37PM +0100, gregkh@linuxfoundation.org wrote:
> >On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote:
> >> Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote:
> >> >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
> >> >> From: Jiri Pirko <jiri@nvidia.com>
> >> >> 
> >> >> It is hard for the faux user to avoid potential name conflicts, as it is
> >> >> only in control of faux devices it creates. Therefore extend the faux
> >> >> device creation function by module parameter, embed the module name into
> >> >> the device name in format "modulename_permodulename" and allow module to
> >> >> control it's namespace.
> >> >
> >> >Do you have an example of how this will change the current names we have
> >> >in the system to this new way?  What is going to break if those names
> >> >change?
> >> 
> >> I was under impression, that since there are no in-tree users of faux
> >> yet (at least I don't see them in net-next tree), there is no breakage.
> >
> >Look at linux-next please.
> 
> Sure, but it's still next. Next might break (uapi) as long it's next,
> right?

The point is that these conversions are thinking that their name is
stable.  This change is going to mean that those patches that have been
accepted into different trees are going to change.

> >> Perhaps "const char *name" could be formatted as well for
> >> faux_device_create()/faux_device_create_with_groups(). My laziness
> >> wanted to avoid that :) Would that make sense to you?
> >
> >I wouldn't object to that, making it a vararg?  How would the rust
> >binding handle that?
> 
> Why should I care about rust? I got the impression the deal is that
> rust bindings are taken care of by rust people. Did that change and
> we need to keep rust in mind for all internal API? That sounds scarry
> to me :(

I was just asking if you knew, that's all.

thanks,

greg k-h

RE: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Tuesday, March 18, 2025 5:47 AM
> To: netdev@vger.kernel.org
> Cc: davem@davemloft.net; Dumazet, Eric <edumazet@google.com>;
> kuba@kernel.org; pabeni@redhat.com; saeedm@nvidia.com; leon@kernel.org;
> tariqt@nvidia.com; andrew+netdev@lunn.ch; dakr@kernel.org;
> rafael@kernel.org; gregkh@linuxfoundation.org; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; cratiu@nvidia.com; Keller, Jacob E
> <jacob.e.keller@intel.com>; Knitter, Konrad <konrad.knitter@intel.com>;
> cjubran@nvidia.com
> Subject: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for
> PFs on same chip
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Multiple PFS may reside on the same physical chip, running a single
> firmware. Some of the resources and configurations may be shared among
> these PFs. Currently, there is not good object to pin the configuration
> knobs on.
> 
> Introduce a shared devlink, instantiated upon probe of the first PF,
> removed during remove of the last PF. Back this shared devlink instance
> by faux device, as there is no PCI device related to it.
> 
> Make the PF devlink instances nested in this shared devlink instance.
> 
> Example:
> 
> $ devlink dev
> pci/0000:08:00.0:
>   nested_devlink:
>     auxiliary/mlx5_core.eth.0
> faux/mlx5_core_83013c12b77faa1a30000c82a1045c91:
>   nested_devlink:
>     pci/0000:08:00.0
>     pci/0000:08:00.1
> auxiliary/mlx5_core.eth.0
> pci/0000:08:00.1:
>   nested_devlink:
>     auxiliary/mlx5_core.eth.1
> auxiliary/mlx5_core.eth.1
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/Makefile  |   4 +-
>  .../net/ethernet/mellanox/mlx5/core/main.c    |  18 +++
>  .../ethernet/mellanox/mlx5/core/sh_devlink.c  | 150 ++++++++++++++++++
>  .../ethernet/mellanox/mlx5/core/sh_devlink.h  |  10 ++
>  include/linux/mlx5/driver.h                   |   5 +
>  5 files changed, 185 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>  create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> index 568bbe5f83f5..510850b6e6e2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> @@ -16,8 +16,8 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o
> pagealloc.o \
>  		transobj.o vport.o sriov.o fs_cmd.o fs_core.o pci_irq.o \
>  		fs_counters.o fs_ft_pool.o rl.o lag/debugfs.o lag/lag.o dev.o
> events.o wq.o lib/gid.o \
>  		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o
> diag/fs_tracepoint.o \
> -		diag/fw_tracer.o diag/crdump.o devlink.o diag/rsc_dump.o
> diag/reporter_vnic.o \
> -		fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
> +		diag/fw_tracer.o diag/crdump.o devlink.o sh_devlink.o
> diag/rsc_dump.o \
> +		diag/reporter_vnic.o fw_reset.o qos.o lib/tout.o lib/aso.o wc.o
> fs_pool.o
> 
>  #
>  # Netdev basic
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 710633d5fdbe..e1217a8bf4db 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -74,6 +74,7 @@
>  #include "mlx5_irq.h"
>  #include "hwmon.h"
>  #include "lag/lag.h"
> +#include "sh_devlink.h"
> 
>  MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
>  MODULE_DESCRIPTION("Mellanox 5th generation network adapters (ConnectX
> series) core driver");
> @@ -1554,10 +1555,17 @@ int mlx5_init_one(struct mlx5_core_dev *dev)
>  	int err;
> 
>  	devl_lock(devlink);
> +	if (dev->shd) {
> +		err = devl_nested_devlink_set(priv_to_devlink(dev->shd),
> +					      devlink);
> +		if (err)
> +			goto unlock;
> +	}
>  	devl_register(devlink);
>  	err = mlx5_init_one_devl_locked(dev);
>  	if (err)
>  		devl_unregister(devlink);
> +unlock:
>  	devl_unlock(devlink);
>  	return err;
>  }
> @@ -1998,6 +2006,13 @@ static int probe_one(struct pci_dev *pdev, const struct
> pci_device_id *id)
>  		goto pci_init_err;
>  	}
> 
> +	err = mlx5_shd_init(dev);
> +	if (err) {
> +		mlx5_core_err(dev, "mlx5_shd_init failed with error code %d\n",
> +			      err);
> +		goto shd_init_err;
> +	}
> +
>  	err = mlx5_init_one(dev);
>  	if (err) {
>  		mlx5_core_err(dev, "mlx5_init_one failed with error code %d\n",
> @@ -2009,6 +2024,8 @@ static int probe_one(struct pci_dev *pdev, const struct
> pci_device_id *id)
>  	return 0;
> 
>  err_init_one:
> +	mlx5_shd_uninit(dev);
> +shd_init_err:
>  	mlx5_pci_close(dev);
>  pci_init_err:
>  	mlx5_mdev_uninit(dev);
> @@ -2030,6 +2047,7 @@ static void remove_one(struct pci_dev *pdev)
>  	mlx5_drain_health_wq(dev);
>  	mlx5_sriov_disable(pdev, false);
>  	mlx5_uninit_one(dev);
> +	mlx5_shd_uninit(dev);
>  	mlx5_pci_close(dev);
>  	mlx5_mdev_uninit(dev);
>  	mlx5_adev_idx_free(dev->priv.adev_idx);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
> b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
> new file mode 100644
> index 000000000000..671a3442525b
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
> +
> +#include <linux/device/faux.h>
> +#include <linux/mlx5/driver.h>
> +#include <linux/mlx5/vport.h>
> +
> +#include "sh_devlink.h"
> +
> +static LIST_HEAD(shd_list);
> +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
> +
> +/* This structure represents a shared devlink instance,
> + * there is one created for PF group of the same chip.
> + */
> +struct mlx5_shd {
> +	/* Node in shd list */
> +	struct list_head list;
> +	/* Serial number of the chip */
> +	const char *sn;
> +	/* List of per-PF dev instances. */
> +	struct list_head dev_list;
> +	/* Related faux device */
> +	struct faux_device *faux_dev;
> +};
> +

For ice, the equivalent of this would essentially replace ice_adapter I imagine.

> +static const struct devlink_ops mlx5_shd_ops = {
> +};
> +
> +static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
> +{
> +	struct devlink *devlink;
> +	struct mlx5_shd *shd;
> +
> +	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd),
> &faux_dev->dev);
> +	if (!devlink)
> +		return -ENOMEM;
> +	shd = devlink_priv(devlink);
> +	faux_device_set_drvdata(faux_dev, shd);
> +
> +	devl_lock(devlink);
> +	devl_register(devlink);
> +	devl_unlock(devlink);
> +	return 0;
> +}
> +
> +static void mlx5_shd_faux_remove(struct faux_device *faux_dev)
> +{
> +	struct mlx5_shd *shd = faux_device_get_drvdata(faux_dev);
> +	struct devlink *devlink = priv_to_devlink(shd);
> +
> +	devl_lock(devlink);
> +	devl_unregister(devlink);
> +	devl_unlock(devlink);
> +	devlink_free(devlink);
> +}
> +
> +static const struct faux_device_ops mlx5_shd_faux_ops = {
> +	.probe = mlx5_shd_faux_probe,
> +	.remove = mlx5_shd_faux_remove,
> +};
> +
> +static struct mlx5_shd *mlx5_shd_create(const char *sn)
> +{
> +	struct faux_device *faux_dev;
> +	struct mlx5_shd *shd;
> +
> +	faux_dev = faux_device_create(THIS_MODULE, sn, NULL,
> &mlx5_shd_faux_ops);
> +	if (!faux_dev)
> +		return NULL;
> +	shd = faux_device_get_drvdata(faux_dev);
> +	if (!shd)
> +		return NULL;
> +	list_add_tail(&shd->list, &shd_list);
> +	shd->sn = sn;
> +	INIT_LIST_HEAD(&shd->dev_list);
> +	shd->faux_dev = faux_dev;
> +	return shd;
> +}
> +
> +static void mlx5_shd_destroy(struct mlx5_shd *shd)
> +{
> +	list_del(&shd->list);
> +	kfree(shd->sn);
> +	faux_device_destroy(shd->faux_dev);
> +}
> +
> +int mlx5_shd_init(struct mlx5_core_dev *dev)
> +{
> +	u8 *vpd_data __free(kfree) = NULL;
> +	struct pci_dev *pdev = dev->pdev;
> +	unsigned int vpd_size, kw_len;
> +	struct mlx5_shd *shd;
> +	const char *sn;
> +	char *end;
> +	int start;
> +	int err;
> +
> +	if (!mlx5_core_is_pf(dev))
> +		return 0;
> +
> +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
> +	if (IS_ERR(vpd_data)) {
> +		err = PTR_ERR(vpd_data);
> +		return err == -ENODEV ? 0 : err;
> +	}
> +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
> &kw_len);
> +	if (start < 0) {
> +		/* Fall-back to SN for older devices. */
> +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
> +
> PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
> +		if (start < 0)
> +			return -ENOENT;
> +	}
> +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
> +	if (!sn)
> +		return -ENOMEM;
> +	end = strchrnul(sn, ' ');
> +	*end = '\0';
> +
> +	guard(mutex)(&shd_mutex);
> +	list_for_each_entry(shd, &shd_list, list) {
> +		if (!strcmp(shd->sn, sn)) {
> +			kfree(sn);
> +			goto found;
> +		}
> +	}
> +	shd = mlx5_shd_create(sn);
> +	if (!shd) {
> +		kfree(sn);
> +		return -ENOMEM;
> +	}

How is the faux device kept in memory? I guess its reference counted somewhere? But I don't see that reference being incremented in the list_for_each.

> +found:
> +	list_add_tail(&dev->shd_list, &shd->dev_list);
> +	dev->shd = shd;
> +	return 0;
> +}
> +
> +void mlx5_shd_uninit(struct mlx5_core_dev *dev)
> +{
> +	struct mlx5_shd *shd = dev->shd;
> +
> +	if (!dev->shd)
> +		return;
> +
> +	guard(mutex)(&shd_mutex);
> +	list_del(&dev->shd_list);
> +	if (list_empty(&shd->dev_list))
> +		mlx5_shd_destroy(shd);
> +}
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
> b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
> new file mode 100644
> index 000000000000..67df03e3c72e
> --- /dev/null
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
> +
> +#ifndef __MLX5_SH_DEVLINK_H__
> +#define __MLX5_SH_DEVLINK_H__
> +
> +int mlx5_shd_init(struct mlx5_core_dev *dev);
> +void mlx5_shd_uninit(struct mlx5_core_dev *dev);
> +
> +#endif /* __MLX5_SH_DEVLINK_H__ */
> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> index 46bd7550adf8..78f1f034568f 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -721,6 +721,8 @@ enum mlx5_wc_state {
>  	MLX5_WC_STATE_SUPPORTED,
>  };
> 
> +struct mlx5_shd;
> +
>  struct mlx5_core_dev {
>  	struct device *device;
>  	enum mlx5_coredev_type coredev_type;
> @@ -783,6 +785,9 @@ struct mlx5_core_dev {
>  	enum mlx5_wc_state wc_state;
>  	/* sync write combining state */
>  	struct mutex wc_state_lock;
> +	/* node in shared devlink list */
> +	struct list_head shd_list;
> +	struct mlx5_shd *shd;
>  };
> 
>  struct mlx5_db {
> --
> 2.48.1

Re: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Przemek Kitszel]

On 3/18/25 23:05, Keller, Jacob E wrote:
> 
> 
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>

>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>> @@ -0,0 +1,150 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
>> +
>> +#include <linux/device/faux.h>
>> +#include <linux/mlx5/driver.h>
>> +#include <linux/mlx5/vport.h>
>> +
>> +#include "sh_devlink.h"
>> +
>> +static LIST_HEAD(shd_list);
>> +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */

I essentially agree that faux_device could be used as-is, without any
devlink changes, works for me.
That does not remove the need to invent the name at some point ;)

we have resolved this in similar manner, that's fine, given my
understanding that you cannot let faux to dispatch for you, like:
faux_get_instance(serial_number_equivalent)

>> +
>> +/* This structure represents a shared devlink instance,
>> + * there is one created for PF group of the same chip.
>> + */
>> +struct mlx5_shd {
>> +	/* Node in shd list */
>> +	struct list_head list;
>> +	/* Serial number of the chip */
>> +	const char *sn;
>> +	/* List of per-PF dev instances. */
>> +	struct list_head dev_list;
>> +	/* Related faux device */
>> +	struct faux_device *faux_dev;
>> +};
>> +
> 
> For ice, the equivalent of this would essentially replace ice_adapter I imagine.

or "ice_adapter will be the ice equivalent"

> 
>> +static const struct devlink_ops mlx5_shd_ops = {

please double check if there is no crash for:
$ devlink dev info the/faux/thing

>> +};
>> +
>> +static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
>> +{
>> +	struct devlink *devlink;
>> +	struct mlx5_shd *shd;
>> +
>> +	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd),

sizeof(*shd)

I like that you reuse devlink_alloc(), with allocation of priv data,
that suits also our needs

>> &faux_dev->dev);
>> +	if (!devlink)
>> +		return -ENOMEM;
>> +	shd = devlink_priv(devlink);
>> +	faux_device_set_drvdata(faux_dev, shd);
>> +
>> +	devl_lock(devlink);
>> +	devl_register(devlink);
>> +	devl_unlock(devlink);
>> +	return 0;
>> +}

[...]

>> +int mlx5_shd_init(struct mlx5_core_dev *dev)
>> +{
>> +	u8 *vpd_data __free(kfree) = NULL;

so bad that netdev mainainers discourage __free() :(
perhaps I should propose higher abstraction wrapper for it
on April 1st

>> +	struct pci_dev *pdev = dev->pdev;
>> +	unsigned int vpd_size, kw_len;
>> +	struct mlx5_shd *shd;
>> +	const char *sn;

I would extract name retrieval, perhaps mlx5_shd_get_name()?

>> +	char *end;
>> +	int start;
>> +	int err;
>> +
>> +	if (!mlx5_core_is_pf(dev))
>> +		return 0;
>> +
>> +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
>> +	if (IS_ERR(vpd_data)) {
>> +		err = PTR_ERR(vpd_data);
>> +		return err == -ENODEV ? 0 : err;

what? that means the shared devlink instance is something you will
work properly without?

>> +	}
>> +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
>> &kw_len);
>> +	if (start < 0) {
>> +		/* Fall-back to SN for older devices. */
>> +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
>> +
>> PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
>> +		if (start < 0)
>> +			return -ENOENT;
>> +	}
>> +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
>> +	if (!sn)
>> +		return -ENOMEM;
>> +	end = strchrnul(sn, ' ');
>> +	*end = '\0';
>> +
>> +	guard(mutex)(&shd_mutex);

guard()() is a no-no too, per "discouraged by netdev maintainers",
and here I'm on board with discouraging ;)

>> +	list_for_each_entry(shd, &shd_list, list) {
>> +		if (!strcmp(shd->sn, sn)) {
>> +			kfree(sn);
>> +			goto found;
>> +		}
>> +	}
>> +	shd = mlx5_shd_create(sn);
>> +	if (!shd) {
>> +		kfree(sn);
>> +		return -ENOMEM;
>> +	}
> 
> How is the faux device kept in memory? I guess its reference counted somewhere? 

get_device()/put_device() with faxu_dev->dev as argument

But I don't see that reference being incremented in the list_for_each.

Jiri keeps "the counter" as the implicit observation of shd list size :)
which is protected by mutex

Re: [PATCH net-next RFC 1/3] faux: extend the creation function for module namespace

[Jiri Pirko]

Tue, Mar 18, 2025 at 06:27:17PM +0100, gregkh@linuxfoundation.org wrote:
>On Tue, Mar 18, 2025 at 05:51:34PM +0100, Jiri Pirko wrote:
>> Tue, Mar 18, 2025 at 05:04:37PM +0100, gregkh@linuxfoundation.org wrote:
>> >On Tue, Mar 18, 2025 at 04:26:05PM +0100, Jiri Pirko wrote:
>> >> Tue, Mar 18, 2025 at 03:36:34PM +0100, gregkh@linuxfoundation.org wrote:
>> >> >On Tue, Mar 18, 2025 at 01:47:04PM +0100, Jiri Pirko wrote:
>> >> >> From: Jiri Pirko <jiri@nvidia.com>
>> >> >> 
>> >> >> It is hard for the faux user to avoid potential name conflicts, as it is
>> >> >> only in control of faux devices it creates. Therefore extend the faux
>> >> >> device creation function by module parameter, embed the module name into
>> >> >> the device name in format "modulename_permodulename" and allow module to
>> >> >> control it's namespace.
>> >> >
>> >> >Do you have an example of how this will change the current names we have
>> >> >in the system to this new way?  What is going to break if those names
>> >> >change?
>> >> 
>> >> I was under impression, that since there are no in-tree users of faux
>> >> yet (at least I don't see them in net-next tree), there is no breakage.
>> >
>> >Look at linux-next please.
>> 
>> Sure, but it's still next. Next might break (uapi) as long it's next,
>> right?
>
>The point is that these conversions are thinking that their name is
>stable.  This change is going to mean that those patches that have been
>accepted into different trees are going to change.

Correct. I was under impression this could be handled in within "next".
If not, my bad.


>
>> >> Perhaps "const char *name" could be formatted as well for
>> >> faux_device_create()/faux_device_create_with_groups(). My laziness
>> >> wanted to avoid that :) Would that make sense to you?
>> >
>> >I wouldn't object to that, making it a vararg?  How would the rust
>> >binding handle that?
>> 
>> Why should I care about rust? I got the impression the deal is that
>> rust bindings are taken care of by rust people. Did that change and
>> we need to keep rust in mind for all internal API? That sounds scarry
>> to me :(
>
>I was just asking if you knew, that's all.

Okay, I misunderstood. I have near 0 knowledge of Rust :)


>
>thanks,
>
>greg k-h

Re: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Jiri Pirko]

Tue, Mar 18, 2025 at 11:05:23PM +0100, jacob.e.keller@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Tuesday, March 18, 2025 5:47 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; Dumazet, Eric <edumazet@google.com>;
>> kuba@kernel.org; pabeni@redhat.com; saeedm@nvidia.com; leon@kernel.org;
>> tariqt@nvidia.com; andrew+netdev@lunn.ch; dakr@kernel.org;
>> rafael@kernel.org; gregkh@linuxfoundation.org; Kitszel, Przemyslaw
>> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
>> <anthony.l.nguyen@intel.com>; cratiu@nvidia.com; Keller, Jacob E
>> <jacob.e.keller@intel.com>; Knitter, Konrad <konrad.knitter@intel.com>;
>> cjubran@nvidia.com
>> Subject: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for
>> PFs on same chip
>> 
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Multiple PFS may reside on the same physical chip, running a single
>> firmware. Some of the resources and configurations may be shared among
>> these PFs. Currently, there is not good object to pin the configuration
>> knobs on.
>> 
>> Introduce a shared devlink, instantiated upon probe of the first PF,
>> removed during remove of the last PF. Back this shared devlink instance
>> by faux device, as there is no PCI device related to it.
>> 
>> Make the PF devlink instances nested in this shared devlink instance.
>> 
>> Example:
>> 
>> $ devlink dev
>> pci/0000:08:00.0:
>>   nested_devlink:
>>     auxiliary/mlx5_core.eth.0
>> faux/mlx5_core_83013c12b77faa1a30000c82a1045c91:
>>   nested_devlink:
>>     pci/0000:08:00.0
>>     pci/0000:08:00.1
>> auxiliary/mlx5_core.eth.0
>> pci/0000:08:00.1:
>>   nested_devlink:
>>     auxiliary/mlx5_core.eth.1
>> auxiliary/mlx5_core.eth.1
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  .../net/ethernet/mellanox/mlx5/core/Makefile  |   4 +-
>>  .../net/ethernet/mellanox/mlx5/core/main.c    |  18 +++
>>  .../ethernet/mellanox/mlx5/core/sh_devlink.c  | 150 ++++++++++++++++++
>>  .../ethernet/mellanox/mlx5/core/sh_devlink.h  |  10 ++
>>  include/linux/mlx5/driver.h                   |   5 +
>>  5 files changed, 185 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>>  create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
>> b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
>> index 568bbe5f83f5..510850b6e6e2 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
>> @@ -16,8 +16,8 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o
>> pagealloc.o \
>>  		transobj.o vport.o sriov.o fs_cmd.o fs_core.o pci_irq.o \
>>  		fs_counters.o fs_ft_pool.o rl.o lag/debugfs.o lag/lag.o dev.o
>> events.o wq.o lib/gid.o \
>>  		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o
>> diag/fs_tracepoint.o \
>> -		diag/fw_tracer.o diag/crdump.o devlink.o diag/rsc_dump.o
>> diag/reporter_vnic.o \
>> -		fw_reset.o qos.o lib/tout.o lib/aso.o wc.o fs_pool.o
>> +		diag/fw_tracer.o diag/crdump.o devlink.o sh_devlink.o
>> diag/rsc_dump.o \
>> +		diag/reporter_vnic.o fw_reset.o qos.o lib/tout.o lib/aso.o wc.o
>> fs_pool.o
>> 
>>  #
>>  # Netdev basic
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> index 710633d5fdbe..e1217a8bf4db 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
>> @@ -74,6 +74,7 @@
>>  #include "mlx5_irq.h"
>>  #include "hwmon.h"
>>  #include "lag/lag.h"
>> +#include "sh_devlink.h"
>> 
>>  MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
>>  MODULE_DESCRIPTION("Mellanox 5th generation network adapters (ConnectX
>> series) core driver");
>> @@ -1554,10 +1555,17 @@ int mlx5_init_one(struct mlx5_core_dev *dev)
>>  	int err;
>> 
>>  	devl_lock(devlink);
>> +	if (dev->shd) {
>> +		err = devl_nested_devlink_set(priv_to_devlink(dev->shd),
>> +					      devlink);
>> +		if (err)
>> +			goto unlock;
>> +	}
>>  	devl_register(devlink);
>>  	err = mlx5_init_one_devl_locked(dev);
>>  	if (err)
>>  		devl_unregister(devlink);
>> +unlock:
>>  	devl_unlock(devlink);
>>  	return err;
>>  }
>> @@ -1998,6 +2006,13 @@ static int probe_one(struct pci_dev *pdev, const struct
>> pci_device_id *id)
>>  		goto pci_init_err;
>>  	}
>> 
>> +	err = mlx5_shd_init(dev);
>> +	if (err) {
>> +		mlx5_core_err(dev, "mlx5_shd_init failed with error code %d\n",
>> +			      err);
>> +		goto shd_init_err;
>> +	}
>> +
>>  	err = mlx5_init_one(dev);
>>  	if (err) {
>>  		mlx5_core_err(dev, "mlx5_init_one failed with error code %d\n",
>> @@ -2009,6 +2024,8 @@ static int probe_one(struct pci_dev *pdev, const struct
>> pci_device_id *id)
>>  	return 0;
>> 
>>  err_init_one:
>> +	mlx5_shd_uninit(dev);
>> +shd_init_err:
>>  	mlx5_pci_close(dev);
>>  pci_init_err:
>>  	mlx5_mdev_uninit(dev);
>> @@ -2030,6 +2047,7 @@ static void remove_one(struct pci_dev *pdev)
>>  	mlx5_drain_health_wq(dev);
>>  	mlx5_sriov_disable(pdev, false);
>>  	mlx5_uninit_one(dev);
>> +	mlx5_shd_uninit(dev);
>>  	mlx5_pci_close(dev);
>>  	mlx5_mdev_uninit(dev);
>>  	mlx5_adev_idx_free(dev->priv.adev_idx);
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>> new file mode 100644
>> index 000000000000..671a3442525b
>> --- /dev/null
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>> @@ -0,0 +1,150 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
>> +
>> +#include <linux/device/faux.h>
>> +#include <linux/mlx5/driver.h>
>> +#include <linux/mlx5/vport.h>
>> +
>> +#include "sh_devlink.h"
>> +
>> +static LIST_HEAD(shd_list);
>> +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
>> +
>> +/* This structure represents a shared devlink instance,
>> + * there is one created for PF group of the same chip.
>> + */
>> +struct mlx5_shd {
>> +	/* Node in shd list */
>> +	struct list_head list;
>> +	/* Serial number of the chip */
>> +	const char *sn;
>> +	/* List of per-PF dev instances. */
>> +	struct list_head dev_list;
>> +	/* Related faux device */
>> +	struct faux_device *faux_dev;
>> +};
>> +
>
>For ice, the equivalent of this would essentially replace ice_adapter I imagine.

Yep.


>
>> +static const struct devlink_ops mlx5_shd_ops = {
>> +};
>> +
>> +static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
>> +{
>> +	struct devlink *devlink;
>> +	struct mlx5_shd *shd;
>> +
>> +	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd),
>> &faux_dev->dev);
>> +	if (!devlink)
>> +		return -ENOMEM;
>> +	shd = devlink_priv(devlink);
>> +	faux_device_set_drvdata(faux_dev, shd);
>> +
>> +	devl_lock(devlink);
>> +	devl_register(devlink);
>> +	devl_unlock(devlink);
>> +	return 0;
>> +}
>> +
>> +static void mlx5_shd_faux_remove(struct faux_device *faux_dev)
>> +{
>> +	struct mlx5_shd *shd = faux_device_get_drvdata(faux_dev);
>> +	struct devlink *devlink = priv_to_devlink(shd);
>> +
>> +	devl_lock(devlink);
>> +	devl_unregister(devlink);
>> +	devl_unlock(devlink);
>> +	devlink_free(devlink);
>> +}
>> +
>> +static const struct faux_device_ops mlx5_shd_faux_ops = {
>> +	.probe = mlx5_shd_faux_probe,
>> +	.remove = mlx5_shd_faux_remove,
>> +};
>> +
>> +static struct mlx5_shd *mlx5_shd_create(const char *sn)
>> +{
>> +	struct faux_device *faux_dev;
>> +	struct mlx5_shd *shd;
>> +
>> +	faux_dev = faux_device_create(THIS_MODULE, sn, NULL,
>> &mlx5_shd_faux_ops);
>> +	if (!faux_dev)
>> +		return NULL;
>> +	shd = faux_device_get_drvdata(faux_dev);
>> +	if (!shd)
>> +		return NULL;
>> +	list_add_tail(&shd->list, &shd_list);
>> +	shd->sn = sn;
>> +	INIT_LIST_HEAD(&shd->dev_list);
>> +	shd->faux_dev = faux_dev;
>> +	return shd;
>> +}
>> +
>> +static void mlx5_shd_destroy(struct mlx5_shd *shd)
>> +{
>> +	list_del(&shd->list);
>> +	kfree(shd->sn);
>> +	faux_device_destroy(shd->faux_dev);
>> +}
>> +
>> +int mlx5_shd_init(struct mlx5_core_dev *dev)
>> +{
>> +	u8 *vpd_data __free(kfree) = NULL;
>> +	struct pci_dev *pdev = dev->pdev;
>> +	unsigned int vpd_size, kw_len;
>> +	struct mlx5_shd *shd;
>> +	const char *sn;
>> +	char *end;
>> +	int start;
>> +	int err;
>> +
>> +	if (!mlx5_core_is_pf(dev))
>> +		return 0;
>> +
>> +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
>> +	if (IS_ERR(vpd_data)) {
>> +		err = PTR_ERR(vpd_data);
>> +		return err == -ENODEV ? 0 : err;
>> +	}
>> +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
>> &kw_len);
>> +	if (start < 0) {
>> +		/* Fall-back to SN for older devices. */
>> +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
>> +
>> PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
>> +		if (start < 0)
>> +			return -ENOENT;
>> +	}
>> +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
>> +	if (!sn)
>> +		return -ENOMEM;
>> +	end = strchrnul(sn, ' ');
>> +	*end = '\0';
>> +
>> +	guard(mutex)(&shd_mutex);
>> +	list_for_each_entry(shd, &shd_list, list) {
>> +		if (!strcmp(shd->sn, sn)) {
>> +			kfree(sn);
>> +			goto found;
>> +		}
>> +	}
>> +	shd = mlx5_shd_create(sn);
>> +	if (!shd) {
>> +		kfree(sn);
>> +		return -ENOMEM;
>> +	}
>
>How is the faux device kept in memory? I guess its reference counted somewhere? But I don't see that reference being incremented in the list_for_each.

There is one faux created in mlx5_shd_create() for one shd instance.
Reference counting is done by &shd->dev_list. See mlx5_shd_uninit()
where mlx5_shd_destroy() is called.



>
>> +found:
>> +	list_add_tail(&dev->shd_list, &shd->dev_list);
>> +	dev->shd = shd;
>> +	return 0;
>> +}
>> +
>> +void mlx5_shd_uninit(struct mlx5_core_dev *dev)
>> +{
>> +	struct mlx5_shd *shd = dev->shd;
>> +
>> +	if (!dev->shd)
>> +		return;
>> +
>> +	guard(mutex)(&shd_mutex);
>> +	list_del(&dev->shd_list);
>> +	if (list_empty(&shd->dev_list))
>> +		mlx5_shd_destroy(shd);
>> +}
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
>> b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
>> new file mode 100644
>> index 000000000000..67df03e3c72e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
>> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
>> +
>> +#ifndef __MLX5_SH_DEVLINK_H__
>> +#define __MLX5_SH_DEVLINK_H__
>> +
>> +int mlx5_shd_init(struct mlx5_core_dev *dev);
>> +void mlx5_shd_uninit(struct mlx5_core_dev *dev);
>> +
>> +#endif /* __MLX5_SH_DEVLINK_H__ */
>> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
>> index 46bd7550adf8..78f1f034568f 100644
>> --- a/include/linux/mlx5/driver.h
>> +++ b/include/linux/mlx5/driver.h
>> @@ -721,6 +721,8 @@ enum mlx5_wc_state {
>>  	MLX5_WC_STATE_SUPPORTED,
>>  };
>> 
>> +struct mlx5_shd;
>> +
>>  struct mlx5_core_dev {
>>  	struct device *device;
>>  	enum mlx5_coredev_type coredev_type;
>> @@ -783,6 +785,9 @@ struct mlx5_core_dev {
>>  	enum mlx5_wc_state wc_state;
>>  	/* sync write combining state */
>>  	struct mutex wc_state_lock;
>> +	/* node in shared devlink list */
>> +	struct list_head shd_list;
>> +	struct mlx5_shd *shd;
>>  };
>> 
>>  struct mlx5_db {
>> --
>> 2.48.1
>

Re: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Jiri Pirko]

Wed, Mar 19, 2025 at 09:21:52AM +0100, przemyslaw.kitszel@intel.com wrote:
>On 3/18/25 23:05, Keller, Jacob E wrote:
>> 
>> 
>> > -----Original Message-----
>> > From: Jiri Pirko <jiri@resnulli.us>
>
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>> > @@ -0,0 +1,150 @@
>> > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> > +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
>> > +
>> > +#include <linux/device/faux.h>
>> > +#include <linux/mlx5/driver.h>
>> > +#include <linux/mlx5/vport.h>
>> > +
>> > +#include "sh_devlink.h"
>> > +
>> > +static LIST_HEAD(shd_list);
>> > +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
>
>I essentially agree that faux_device could be used as-is, without any
>devlink changes, works for me.
>That does not remove the need to invent the name at some point ;)

What name? Name of faux device? Sure. In case of ice it's probably PCI DSN

>
>we have resolved this in similar manner, that's fine, given my
>understanding that you cannot let faux to dispatch for you, like:
>faux_get_instance(serial_number_equivalent)

Not sure what you are asking TBH.


>
>> > +
>> > +/* This structure represents a shared devlink instance,
>> > + * there is one created for PF group of the same chip.
>> > + */
>> > +struct mlx5_shd {
>> > +	/* Node in shd list */
>> > +	struct list_head list;
>> > +	/* Serial number of the chip */
>> > +	const char *sn;
>> > +	/* List of per-PF dev instances. */
>> > +	struct list_head dev_list;
>> > +	/* Related faux device */
>> > +	struct faux_device *faux_dev;
>> > +};
>> > +
>> 
>> For ice, the equivalent of this would essentially replace ice_adapter I imagine.
>
>or "ice_adapter will be the ice equivalent"

Oh yes, that makes sense.


>
>> 
>> > +static const struct devlink_ops mlx5_shd_ops = {
>
>please double check if there is no crash for:
>$ devlink dev info the/faux/thing

Will do, but why do you think so?


>
>> > +};
>> > +
>> > +static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
>> > +{
>> > +	struct devlink *devlink;
>> > +	struct mlx5_shd *shd;
>> > +
>> > +	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd),
>
>sizeof(*shd)

Sure.


>
>I like that you reuse devlink_alloc(), with allocation of priv data,
>that suits also our needs
>
>> > &faux_dev->dev);
>> > +	if (!devlink)
>> > +		return -ENOMEM;
>> > +	shd = devlink_priv(devlink);
>> > +	faux_device_set_drvdata(faux_dev, shd);
>> > +
>> > +	devl_lock(devlink);
>> > +	devl_register(devlink);
>> > +	devl_unlock(devlink);
>> > +	return 0;
>> > +}
>
>[...]
>
>> > +int mlx5_shd_init(struct mlx5_core_dev *dev)
>> > +{
>> > +	u8 *vpd_data __free(kfree) = NULL;
>
>so bad that netdev mainainers discourage __free() :(
>perhaps I should propose higher abstraction wrapper for it
>on April 1st
>
>> > +	struct pci_dev *pdev = dev->pdev;
>> > +	unsigned int vpd_size, kw_len;
>> > +	struct mlx5_shd *shd;
>> > +	const char *sn;
>
>I would extract name retrieval, perhaps mlx5_shd_get_name()?

I had that, did not really make the code nicer :)


>
>> > +	char *end;
>> > +	int start;
>> > +	int err;
>> > +
>> > +	if (!mlx5_core_is_pf(dev))
>> > +		return 0;
>> > +
>> > +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
>> > +	if (IS_ERR(vpd_data)) {
>> > +		err = PTR_ERR(vpd_data);
>> > +		return err == -ENODEV ? 0 : err;
>
>what? that means the shared devlink instance is something you will
>work properly without?

Not sure. This is something that should not happen for any existing
device.


>
>> > +	}
>> > +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
>> > &kw_len);
>> > +	if (start < 0) {
>> > +		/* Fall-back to SN for older devices. */
>> > +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
>> > +
>> > PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
>> > +		if (start < 0)
>> > +			return -ENOENT;
>> > +	}
>> > +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
>> > +	if (!sn)
>> > +		return -ENOMEM;
>> > +	end = strchrnul(sn, ' ');
>> > +	*end = '\0';
>> > +
>> > +	guard(mutex)(&shd_mutex);
>
>guard()() is a no-no too, per "discouraged by netdev maintainers",
>and here I'm on board with discouraging ;)

That's a fight with windmills. It will happen sooner than later anyway.
It is just too conventient. I don't understand why netdev has to have
special treat comparing to the rest of the kernel all the time...


>
>> > +	list_for_each_entry(shd, &shd_list, list) {
>> > +		if (!strcmp(shd->sn, sn)) {
>> > +			kfree(sn);
>> > +			goto found;
>> > +		}
>> > +	}
>> > +	shd = mlx5_shd_create(sn);
>> > +	if (!shd) {
>> > +		kfree(sn);
>> > +		return -ENOMEM;
>> > +	}
>> 
>> How is the faux device kept in memory? I guess its reference counted
>> somewhere?
>
>get_device()/put_device() with faxu_dev->dev as argument
>
>But I don't see that reference being incremented in the list_for_each.
>
>Jiri keeps "the counter" as the implicit observation of shd list size :)
>which is protected by mutex

Yep. Why isn't that enough? I need the list anyway. Plus, I'm using the
list to reference count shd, not the fauxdev. Fauxdev is explicitly
create/destroyed by calling appropriate function. I belive that is the
correct way (maybe the only way?) to instantiate/deinstantiate faux.

Re: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Przemek Kitszel]

On 3/19/25 12:56, Jiri Pirko wrote:
> Wed, Mar 19, 2025 at 09:21:52AM +0100, przemyslaw.kitszel@intel.com wrote:
>> On 3/18/25 23:05, Keller, Jacob E wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jiri Pirko <jiri@resnulli.us>
>>
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>>>> @@ -0,0 +1,150 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>>> +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
>>>> +
>>>> +#include <linux/device/faux.h>
>>>> +#include <linux/mlx5/driver.h>
>>>> +#include <linux/mlx5/vport.h>
>>>> +
>>>> +#include "sh_devlink.h"
>>>> +
>>>> +static LIST_HEAD(shd_list);
>>>> +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
>>
>> I essentially agree that faux_device could be used as-is, without any
>> devlink changes, works for me.
>> That does not remove the need to invent the name at some point ;)
> 
> What name? Name of faux device? Sure. In case of ice it's probably PCI DSN

yes x2

> 
>>
>> we have resolved this in similar manner, that's fine, given my
>> understanding that you cannot let faux to dispatch for you, like:
>> faux_get_instance(serial_number_equivalent)
> 
> Not sure what you are asking TBH.

not asking, just noticing, with the hope that Greg will notice
inconvenience for drivers and perhaps think of something even better

anyway faux dev let's us resolve problems in rather elegant way,
compared to what we have to do before (like this series is not even
touching devlink/)

> 
> 
>>
>>>> +
>>>> +/* This structure represents a shared devlink instance,
>>>> + * there is one created for PF group of the same chip.
>>>> + */
>>>> +struct mlx5_shd {
>>>> +	/* Node in shd list */
>>>> +	struct list_head list;
>>>> +	/* Serial number of the chip */
>>>> +	const char *sn;
>>>> +	/* List of per-PF dev instances. */
>>>> +	struct list_head dev_list;
>>>> +	/* Related faux device */
>>>> +	struct faux_device *faux_dev;
>>>> +};
>>>> +
>>>
>>> For ice, the equivalent of this would essentially replace ice_adapter I imagine.
>>
>> or "ice_adapter will be the ice equivalent"
> 
> Oh yes, that makes sense.
> 
> 
>>
>>>
>>>> +static const struct devlink_ops mlx5_shd_ops = {
>>
>> please double check if there is no crash for:
>> $ devlink dev info the/faux/thing
> 
> Will do, but why do you think so?

My local version of the ice equivalent crashes here, due to null ops
pointers, despite all the checks in devlink core trying to prevent that.

[...]

>>>> +	char *end;
>>>> +	int start;
>>>> +	int err;
>>>> +
>>>> +	if (!mlx5_core_is_pf(dev))
>>>> +		return 0;
>>>> +
>>>> +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
>>>> +	if (IS_ERR(vpd_data)) {
>>>> +		err = PTR_ERR(vpd_data);
>>>> +		return err == -ENODEV ? 0 : err;
>>
>> what? that means the shared devlink instance is something you will
>> work properly without?
> 
> Not sure. This is something that should not happen for any existing
> device.

then failing is a best option to notice that :)

> 
> 
>>
>>>> +	}
>>>> +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
>>>> &kw_len);
>>>> +	if (start < 0) {
>>>> +		/* Fall-back to SN for older devices. */
>>>> +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
>>>> +
>>>> PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
>>>> +		if (start < 0)
>>>> +			return -ENOENT;
>>>> +	}
>>>> +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
>>>> +	if (!sn)
>>>> +		return -ENOMEM;
>>>> +	end = strchrnul(sn, ' ');
>>>> +	*end = '\0';
>>>> +
>>>> +	guard(mutex)(&shd_mutex);
>>
>> guard()() is a no-no too, per "discouraged by netdev maintainers",
>> and here I'm on board with discouraging ;)
> 
> That's a fight with windmills. It will happen sooner than later anyway.
> It is just too conventient. I don't understand why netdev has to have
> special treat comparing to the rest of the kernel all the time...
> 
> 
>>
>>>> +	list_for_each_entry(shd, &shd_list, list) {
>>>> +		if (!strcmp(shd->sn, sn)) {
>>>> +			kfree(sn);
>>>> +			goto found;
>>>> +		}
>>>> +	}
>>>> +	shd = mlx5_shd_create(sn);
>>>> +	if (!shd) {
>>>> +		kfree(sn);
>>>> +		return -ENOMEM;
>>>> +	}
>>>
>>> How is the faux device kept in memory? I guess its reference counted
>>> somewhere?
>>
>> get_device()/put_device() with faxu_dev->dev as argument
>>
>> But I don't see that reference being incremented in the list_for_each.
>>
>> Jiri keeps "the counter" as the implicit observation of shd list size :)
>> which is protected by mutex
> 
> Yep. Why isn't that enough? I need the list anyway. Plus, I'm using the
> list to reference count shd, not the fauxdev. Fauxdev is explicitly
> create/destroyed by calling appropriate function. I belive that is the
> correct way (maybe the only way?) to instantiate/deinstantiate faux.

I've just explained my reasoning why your code is correct :)

The other correct design would be to have it in the faux/ to
dispatch/de-duplicate given a string or numeric ID

RE: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance for PFs on same chip

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, March 19, 2025 4:56 AM
> To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; gregkh@linuxfoundation.org;
> davem@davemloft.net; netdev@vger.kernel.org; Dumazet, Eric
> <edumazet@google.com>; kuba@kernel.org; pabeni@redhat.com;
> saeedm@nvidia.com; leon@kernel.org; tariqt@nvidia.com;
> andrew+netdev@lunn.ch; dakr@kernel.org; rafael@kernel.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; cratiu@nvidia.com; Knitter, Konrad
> <konrad.knitter@intel.com>; cjubran@nvidia.com
> Subject: Re: [PATCH net-next RFC 2/3] net/mlx5: Introduce shared devlink instance
> for PFs on same chip
> 
> Wed, Mar 19, 2025 at 09:21:52AM +0100, przemyslaw.kitszel@intel.com wrote:
> >On 3/18/25 23:05, Keller, Jacob E wrote:
> >>
> >>
> >> > -----Original Message-----
> >> > From: Jiri Pirko <jiri@resnulli.us>
> >
> >> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
> >> > @@ -0,0 +1,150 @@
> >> > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> >> > +/* Copyright (c) 2025, NVIDIA CORPORATION & AFFILIATES. All rights
> reserved. */
> >> > +
> >> > +#include <linux/device/faux.h>
> >> > +#include <linux/mlx5/driver.h>
> >> > +#include <linux/mlx5/vport.h>
> >> > +
> >> > +#include "sh_devlink.h"
> >> > +
> >> > +static LIST_HEAD(shd_list);
> >> > +static DEFINE_MUTEX(shd_mutex); /* Protects shd_list and shd->list */
> >
> >I essentially agree that faux_device could be used as-is, without any
> >devlink changes, works for me.
> >That does not remove the need to invent the name at some point ;)
> 
> What name? Name of faux device? Sure. In case of ice it's probably PCI DSN
> 
> >
> >we have resolved this in similar manner, that's fine, given my
> >understanding that you cannot let faux to dispatch for you, like:
> >faux_get_instance(serial_number_equivalent)
> 
> Not sure what you are asking TBH.
> 
> 
> >
> >> > +
> >> > +/* This structure represents a shared devlink instance,
> >> > + * there is one created for PF group of the same chip.
> >> > + */
> >> > +struct mlx5_shd {
> >> > +	/* Node in shd list */
> >> > +	struct list_head list;
> >> > +	/* Serial number of the chip */
> >> > +	const char *sn;
> >> > +	/* List of per-PF dev instances. */
> >> > +	struct list_head dev_list;
> >> > +	/* Related faux device */
> >> > +	struct faux_device *faux_dev;
> >> > +};
> >> > +
> >>
> >> For ice, the equivalent of this would essentially replace ice_adapter I imagine.
> >
> >or "ice_adapter will be the ice equivalent"
> 
> Oh yes, that makes sense.
> 
> 
> >
> >>
> >> > +static const struct devlink_ops mlx5_shd_ops = {
> >
> >please double check if there is no crash for:
> >$ devlink dev info the/faux/thing
> 
> Will do, but why do you think so?
> 
> 
> >
> >> > +};
> >> > +
> >> > +static int mlx5_shd_faux_probe(struct faux_device *faux_dev)
> >> > +{
> >> > +	struct devlink *devlink;
> >> > +	struct mlx5_shd *shd;
> >> > +
> >> > +	devlink = devlink_alloc(&mlx5_shd_ops, sizeof(struct mlx5_shd),
> >
> >sizeof(*shd)
> 
> Sure.
> 
> 
> >
> >I like that you reuse devlink_alloc(), with allocation of priv data,
> >that suits also our needs
> >
> >> > &faux_dev->dev);
> >> > +	if (!devlink)
> >> > +		return -ENOMEM;
> >> > +	shd = devlink_priv(devlink);
> >> > +	faux_device_set_drvdata(faux_dev, shd);
> >> > +
> >> > +	devl_lock(devlink);
> >> > +	devl_register(devlink);
> >> > +	devl_unlock(devlink);
> >> > +	return 0;
> >> > +}
> >
> >[...]
> >
> >> > +int mlx5_shd_init(struct mlx5_core_dev *dev)
> >> > +{
> >> > +	u8 *vpd_data __free(kfree) = NULL;
> >
> >so bad that netdev mainainers discourage __free() :(
> >perhaps I should propose higher abstraction wrapper for it
> >on April 1st
> >
> >> > +	struct pci_dev *pdev = dev->pdev;
> >> > +	unsigned int vpd_size, kw_len;
> >> > +	struct mlx5_shd *shd;
> >> > +	const char *sn;
> >
> >I would extract name retrieval, perhaps mlx5_shd_get_name()?
> 
> I had that, did not really make the code nicer :)
> 
> 
> >
> >> > +	char *end;
> >> > +	int start;
> >> > +	int err;
> >> > +
> >> > +	if (!mlx5_core_is_pf(dev))
> >> > +		return 0;
> >> > +
> >> > +	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
> >> > +	if (IS_ERR(vpd_data)) {
> >> > +		err = PTR_ERR(vpd_data);
> >> > +		return err == -ENODEV ? 0 : err;
> >
> >what? that means the shared devlink instance is something you will
> >work properly without?
> 
> Not sure. This is something that should not happen for any existing
> device.
> 
> 
> >
> >> > +	}
> >> > +	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size, "V3",
> >> > &kw_len);
> >> > +	if (start < 0) {
> >> > +		/* Fall-back to SN for older devices. */
> >> > +		start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
> >> > +
> >> > PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
> >> > +		if (start < 0)
> >> > +			return -ENOENT;
> >> > +	}
> >> > +	sn = kstrndup(vpd_data + start, kw_len, GFP_KERNEL);
> >> > +	if (!sn)
> >> > +		return -ENOMEM;
> >> > +	end = strchrnul(sn, ' ');
> >> > +	*end = '\0';
> >> > +
> >> > +	guard(mutex)(&shd_mutex);
> >
> >guard()() is a no-no too, per "discouraged by netdev maintainers",
> >and here I'm on board with discouraging ;)
> 
> That's a fight with windmills. It will happen sooner than later anyway.
> It is just too conventient. I don't understand why netdev has to have
> special treat comparing to the rest of the kernel all the time...
> 
> 
> >
> >> > +	list_for_each_entry(shd, &shd_list, list) {
> >> > +		if (!strcmp(shd->sn, sn)) {
> >> > +			kfree(sn);
> >> > +			goto found;
> >> > +		}
> >> > +	}
> >> > +	shd = mlx5_shd_create(sn);
> >> > +	if (!shd) {
> >> > +		kfree(sn);
> >> > +		return -ENOMEM;
> >> > +	}
> >>
> >> How is the faux device kept in memory? I guess its reference counted
> >> somewhere?
> >
> >get_device()/put_device() with faxu_dev->dev as argument
> >
> >But I don't see that reference being incremented in the list_for_each.
> >
> >Jiri keeps "the counter" as the implicit observation of shd list size :)
> >which is protected by mutex
> 
> Yep. Why isn't that enough? I need the list anyway. Plus, I'm using the
> list to reference count shd, not the fauxdev. Fauxdev is explicitly
> create/destroyed by calling appropriate function. I belive that is the
> correct way (maybe the only way?) to instantiate/deinstantiate faux.

Re-using the list as the count for how many users seems reasonable. I just didn't catch that's  how you did it.

Thanks,
Jake