Re: [PATCH net-next 1/2] devlink, mlx5: add init/fini ops for shared devlink

[Jiri Pirko <jiri@resnulli.us>]

Tue, Apr 28, 2026 at 11:09:11AM +0200, przemyslaw.kitszel@intel.com wrote:
>Add .shd_init() and .shd_fini() ops, that will be called for the first
>devlink_shd_get() (to initialize driver' priv data) and on the last
>devlink_shd_put() (to allow for the cleanup). Both ops are optional.
>
>.shd_init() could return an error, which will stop creation of shd
>instance. The initializer also gets an additional, optional param,
>that driver could use for any needs.
>
>If any of the callbacks will need to get devlink instance, it could
>be accessed by shd_priv_to_devlink().
>
>Both callbacks are called with devl_lock held and devlink registered.
>
>Next commit will make use of the callbacks, another one will make use also
>of the non-null additional param (outside of this series).
>
>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>---
>first discussed at:
>https://lore.kernel.org/netdev/20260325063143.261806-3-przemyslaw.kitszel@intel.com
>
>Sashiko suggested to convert devlink_shd_create() to return ERR_PTR(),
>and propagate that up to the driver. It think it will just make code more
>verbose for not much benefit. And drivers could just store err if they
>want in the passed @init_param.
>
>---
> include/net/devlink.h                         | 26 +++++++++++++
> .../ethernet/mellanox/mlx5/core/sh_devlink.c  |  2 +-
> net/devlink/sh_dev.c                          | 39 ++++++++++++++++++-
> 3 files changed, 64 insertions(+), 3 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index bcd31de1f890..5d3a1337bfa1 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1586,6 +1586,30 @@ struct devlink_ops {
> 				    struct devlink_rate *parent,
> 				    void *priv_child, void *priv_parent,
> 				    struct netlink_ext_ack *extack);
>+
>+	/**
>+	 * shd_init: Shared devlink instance initializer
>+	 * @priv: shd_devlink' priv
>+	 * @init_param: additional param to pass to driver callback
>+	 *
>+	 * Called once when the shared instance is first created (by the first
>+	 * devlink_shd_get() call).
>+	 * Should initialize the driver's private data embedded in the shared
>+	 * devlink. May be NULL.
>+	 *
>+	 * Return: 0 on success, negative to prevent shared instance usage.
>+	 */
>+	int (*shd_init)(void *priv, void *init_param);

1. "param" has specific meaning in devlink context
2. You don't use the arg in driver

Care to drop it?

Otherwise, this looks fine to me. Thanks! (small nitpick below)


>+	/**
>+	 * shd_fini: Shared devlink instance finalizer
>+	 * @priv: shd_devlink' priv
>+	 *
>+	 * Called once when the last reference is dropped and the shared
>+	 * instance is destroyed. Should clean up the driver's private data.
>+	 * May be NULL.
>+	 */
>+	void (*shd_fini)(void *priv);
>+
> 	/**
> 	 * selftests_check() - queries if selftest is supported
> 	 * @devlink: devlink instance
>@@ -1651,9 +1675,11 @@ void devlink_free(struct devlink *devlink);
> struct devlink *devlink_shd_get(const char *id,
> 				const struct devlink_ops *ops,
> 				size_t priv_size,
>+				void *init_param,
> 				const struct device_driver *driver);
> void devlink_shd_put(struct devlink *devlink);
> void *devlink_shd_get_priv(struct devlink *devlink);
>+struct devlink *shd_priv_to_devlink(void *priv);
> 
> /**
>  * struct devlink_port_ops - Port operations
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>index b925364765ac..1b8b1ce7e72d 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/sh_devlink.c
>@@ -43,7 +43,7 @@ int mlx5_shd_init(struct mlx5_core_dev *dev)
> 	*end = '\0';
> 
> 	/* Get or create shared devlink instance */
>-	devlink = devlink_shd_get(sn, &mlx5_shd_ops, 0, pdev->dev.driver);
>+	devlink = devlink_shd_get(sn, &mlx5_shd_ops, 0, NULL, pdev->dev.driver);
> 	kfree(sn);
> 	if (!devlink)
> 		return -ENOMEM;
>diff --git a/net/devlink/sh_dev.c b/net/devlink/sh_dev.c
>index 85acce97e788..048a2a6adc9e 100644
>--- a/net/devlink/sh_dev.c
>+++ b/net/devlink/sh_dev.c
>@@ -34,6 +34,7 @@ static struct devlink_shd *devlink_shd_lookup(const char *id)
> static struct devlink_shd *devlink_shd_create(const char *id,
> 					      const struct devlink_ops *ops,
> 					      size_t priv_size,
>+					      void *init_param,
> 					      const struct device_driver *driver)
> {
> 	struct devlink_shd *shd;
>@@ -49,16 +50,30 @@ static struct devlink_shd *devlink_shd_create(const char *id,
> 	if (!shd->id)
> 		goto err_devlink_free;
> 	shd->priv_size = priv_size;
>-	refcount_set(&shd->refcount, 1);
> 
> 	devl_lock(devlink);
> 	devl_register(devlink);
>+
>+	if (ops->shd_init) {
>+		int err;
>+
>+		err = ops->shd_init(shd->priv, init_param);
>+		if (err)
>+			goto err_unregister;
>+	}
>+
> 	devl_unlock(devlink);
> 
>+	refcount_set(&shd->refcount, 1);
> 	list_add_tail(&shd->list, &shd_list);
> 
> 	return shd;
> 
>+err_unregister:
>+	devl_unregister(devlink);
>+	devl_unlock(devlink);
>+	kfree(shd->id);
>+
> err_devlink_free:
> 	devlink_free(devlink);
> 	return NULL;
>@@ -69,7 +84,12 @@ static void devlink_shd_destroy(struct devlink_shd *shd)
> 	struct devlink *devlink = priv_to_devlink(shd);
> 
> 	list_del(&shd->list);
>+

Not sure why to add empty line here.


> 	devl_lock(devlink);
>+
>+	if (devlink->ops->shd_fini)
>+		devlink->ops->shd_fini(shd->priv);
>+
> 	devl_unregister(devlink);
> 	devl_unlock(devlink);
> 	kfree(shd->id);
>@@ -81,6 +101,7 @@ static void devlink_shd_destroy(struct devlink_shd *shd)
>  * @id: Identifier string (e.g., serial number) for the shared instance
>  * @ops: Devlink operations structure
>  * @priv_size: Size of private data structure
>+ * @init_param: Passed to .shd_init() callback alongside driver's priv
>  * @driver: Driver associated with the shared devlink instance
>  *
>  * Get an existing shared devlink instance identified by @id, or create
>@@ -96,16 +117,17 @@ static void devlink_shd_destroy(struct devlink_shd *shd)
> struct devlink *devlink_shd_get(const char *id,
> 				const struct devlink_ops *ops,
> 				size_t priv_size,
>+				void *init_param,
> 				const struct device_driver *driver)
> {
> 	struct devlink *devlink;
> 	struct devlink_shd *shd;
> 
> 	mutex_lock(&shd_mutex);
> 
> 	shd = devlink_shd_lookup(id);
> 	if (!shd) {
>-		shd = devlink_shd_create(id, ops, priv_size, driver);
>+		shd = devlink_shd_create(id, ops, priv_size, init_param, driver);
> 		goto unlock;
> 	}
> 
>@@ -159,3 +181,16 @@ void *devlink_shd_get_priv(struct devlink *devlink)
> 	return shd->priv;
> }
> EXPORT_SYMBOL_GPL(devlink_shd_get_priv);
>+
>+/** shd_priv_to_devlink - Get devlink instance from shd_devlink's priv
>+ * @priv: Driver's priv data
>+ *
>+ * Return: pointer to shared devlink instance the @priv belongs to.
>+ */
>+struct devlink *shd_priv_to_devlink(void *priv)
>+{
>+	struct devlink_shd *shd = container_of(priv, struct devlink_shd, priv);
>+
>+	return priv_to_devlink(shd);
>+}
>+EXPORT_SYMBOL_GPL(shd_priv_to_devlink);
>-- 
>2.39.3
>