Re: [PATCH net-next V7 02/14] devlink: introduce shared devlink instance for PFs on same chip

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 28 Jan 2026 13:25:32 +0200 Tariq Toukan wrote:
> 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 no good object to pin the configuration
> knobs on.
> 
> Introduce a shared devlink instance, instantiated upon probe of the
> first PF and removed during remove of the last PF. The shared devlink
> instance is backed by a faux device, as there is no PCI device related
> to it. The implementation uses reference counting to manage the
> lifecycle: each PF that probes calls devlink_shd_get() to get or create
> the shared instance, and calls devlink_shd_put() when it removes. The
> shared instance is automatically destroyed when the last PF removes.

> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index cb839e0435a1..c453faec8ebf 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1644,6 +1644,12 @@ void devlink_register(struct devlink *devlink);
>  void devlink_unregister(struct devlink *devlink);
>  void devlink_free(struct devlink *devlink);
>  
> +struct devlink *devlink_shd_get(const char *id,
> +				const struct devlink_ops *ops,
> +				size_t priv_size);
> +void devlink_shd_put(struct devlink *devlink);
> +void *devlink_shd_get_priv(struct devlink *devlink);

Would Cosmin or someone else be willing to take on co-maintainership 
of this API (including reviews of other drivers using it)?
We could add a maintainers entry with:

K:	devlink_shd_

So y'all get CCed.

> +#include <linux/device/faux.h>
> +#include <net/devlink.h>

> +/* This structure represents a shared devlink instance,
> + * there is one created per identifier (e.g., serial number).
> + */
> +struct devlink_shd {
> +	struct list_head list; /* Node in shd list */
> +	const char *id; /* Identifier string (e.g., serial number) */

Why does this have to be a string? The identifier should be irrelevant,
and if something like serial number exists it can be reported in dev
info for the shared instance?

> +	struct faux_device *faux_dev; /* Related faux device */
> +	refcount_t refcount; /* Reference count */
> +	char priv[] __aligned(NETDEV_ALIGN); /* Driver private data */

size member annotated with __counted_by() is missing here

> +};

> +static struct devlink_shd *devlink_shd_create(const char *id,
> +					      const struct devlink_ops *ops,
> +					      size_t priv_size)
> +{
> +	struct faux_device *faux_dev;
> +	struct devlink_shd *shd;
> +	struct devlink *devlink;
> +
> +	/* Create faux device - probe will be called synchronously */
> +	faux_dev = faux_device_create(id, NULL, NULL);
> +	if (!faux_dev)
> +		return NULL;
> +
> +	devlink = devlink_alloc(ops, sizeof(struct devlink_shd) + priv_size,
> +				&faux_dev->dev);
> +	if (!devlink)
> +		goto err_devlink_alloc;

error labels should be named after the target not the source in new code

> +	shd = devlink_priv(devlink);
> +
> +	shd->id = kstrdup(id, GFP_KERNEL);
> +	if (!shd->id)
> +		goto err_kstrdup_id;
> +	shd->faux_dev = faux_dev;
> +	refcount_set(&shd->refcount, 1);
> +
> +	devl_lock(devlink);
> +	devl_register(devlink);
> +	devl_unlock(devlink);
> +
> +	list_add_tail(&shd->list, &shd_list);
> +
> +	return shd;
> +
> +err_kstrdup_id:
> +	devlink_free(devlink);
> +
> +err_devlink_alloc:
> +	faux_device_destroy(faux_dev);
> +	return NULL;
> +}

> +struct devlink *devlink_shd_get(const char *id,
> +				const struct devlink_ops *ops,
> +				size_t priv_size)
> +{
> +	struct devlink_shd *shd;
> +
> +	if (WARN_ON(!id || !ops))
> +		return NULL;

Seems a little too defensive to check input attrs against NULL.
Let the kernel crash if someone is foolish enough..

> +	mutex_lock(&shd_mutex);
> +
> +	shd = devlink_shd_lookup(id);
> +	if (!shd)
> +		shd = devlink_shd_create(id, ops, priv_size);
> +	else
> +		refcount_inc(&shd->refcount);
> +
> +	mutex_unlock(&shd_mutex);
> +	return shd ? priv_to_devlink(shd) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(devlink_shd_get);
> +
> +/**
> + * devlink_shd_put - Release a reference on a shared devlink instance
> + * @devlink: Shared devlink instance
> + *
> + * Release a reference on a shared devlink instance obtained via
> + * devlink_shd_get().
> + */
> +void devlink_shd_put(struct devlink *devlink)
> +{
> +	struct devlink_shd *shd;
> +
> +	if (WARN_ON(!devlink))
> +		return;

ditto

> +	mutex_lock(&shd_mutex);
> +	shd = devlink_priv(devlink);
> +	if (refcount_dec_and_test(&shd->refcount))
> +		devlink_shd_destroy(shd);
> +	mutex_unlock(&shd_mutex);
> +}
> +EXPORT_SYMBOL_GPL(devlink_shd_put);