Re: [net-next,08/10] devlink: introduce shared devlink instance for PFs on same chip

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "FYI"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
devlink: introduce shared devlink instance for PFs on same chip

This patch introduces a shared devlink instance for managing resources
shared among multiple PFs on the same physical chip. The implementation
uses reference counting to manage the lifecycle of these shared instances.

> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 45dec7067a8e..e97ee09af5d6 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1647,6 +1647,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);
> +

[ ... ]

> diff --git a/net/devlink/sh_dev.c b/net/devlink/sh_dev.c
> new file mode 100644
> index 000000000000..8bd8f4b506f5
> --- /dev/null
> +++ b/net/devlink/sh_dev.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. */
> +
> +#include <net/devlink.h>
> +
> +#include "devl_internal.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 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) */
> +	refcount_t refcount; /* Reference count */
> +	char priv[] __aligned(NETDEV_ALIGN); /* Driver private data */
> +};

This isn't a bug, but the flexible array member priv[] is missing a size
member annotated with __counted_by(). Jakub Kicinski noted this in the V7
review:

  https://lore.kernel.org/netdev/20260202194946.64555356@kernel.org/

  "size member annotated with __counted_by() is missing here"

You acknowledged this ("Will add.") but the annotation is still missing
in the current code.

> +
> +static struct devlink_shd *devlink_shd_lookup(const char *id)
> +{
> +	struct devlink_shd *shd;
> +
> +	list_for_each_entry(shd, &shd_list, list) {
> +		if (!strcmp(shd->id, id))
> +			return shd;
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct devlink_shd *devlink_shd_create(const char *id,
> +					      const struct devlink_ops *ops,
> +					      size_t priv_size)
> +{
> +	struct devlink_shd *shd;
> +	struct devlink *devlink;
> +
> +	devlink = devlink_alloc(ops, sizeof(struct devlink_shd) + priv_size,
> +				NULL);

Does devlink_alloc() set the correct dev_name for the shared instance?
Looking at the dev==NULL path in devlink_alloc_ns() (introduced in the
parent commit 80a67bcc7d4c), dev_name is set via:

net/devlink/core.c:devlink_alloc_ns() {
    ...
    if (!dev) {
        devlink->dev_name = kasprintf(GFP_KERNEL, "%u", devlink->index);
        ...
    }
    ...
    xa_alloc_cyclic(&devlinks, &devlink->index, devlink, ...)
}

The kasprintf() call happens before xa_alloc_cyclic() assigns the real
index. Since devlink was allocated with kvzalloc, devlink->index is 0 at
that point, so dev_name is always "0" regardless of the real assigned
index.

This means the shared devlink instance is registered and announced via
netlink with an incorrect identity (bus_name="devlink_index", dev_name="0"
instead of the actual index). The commit message example shows
'devlink_index/1: index 1', confirming the name should match the real
index.

> +	if (!devlink)
> +		return NULL;
> +	shd = devlink_priv(devlink);
> +
> +	shd->id = kstrdup(id, GFP_KERNEL);
> +	if (!shd->id)
> +		goto err_kstrdup_id;
> +	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:

This isn't a bug, but error labels should be named after the target not
the source in new code. Jakub Kicinski requested this in the V7 review:

  https://lore.kernel.org/netdev/20260202194946.64555356@kernel.org/

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

You acknowledged this ("Okay. Tried to be consistent with the rest of the
code. But as you wish.") but the current code still uses err_kstrdup_id
(source-based) instead of a target-based name like err_free_devlink.

> +	devlink_free(devlink);
> +	return NULL;
> +}
> +
> +static void devlink_shd_destroy(struct devlink_shd *shd)
> +{
> +	struct devlink *devlink = priv_to_devlink(shd);
> +
> +	list_del(&shd->list);
> +	devl_lock(devlink);
> +	devl_unregister(devlink);
> +	devl_unlock(devlink);
> +	kfree(shd->id);
> +	devlink_free(devlink);
> +}
> +
> +/**
> + * devlink_shd_get - Get or create a shared devlink instance
> + * @id: Identifier string (e.g., serial number) for the shared instance
> + * @ops: Devlink operations structure
> + * @priv_size: Size of private data structure
> + *
> + * Get an existing shared devlink instance identified by @id, or create
> + * a new one if it doesn't exist. The device is automatically added to
> + * the shared instance's device list. Return the devlink instance

The kernel-doc comment states "The device is automatically added to the
shared instance's device list" but the function does not add any device to
any device list. The function manages a shared devlink instance identified
by a string id with reference counting -- there is no device parameter and
no device list operation. Should this sentence be removed or corrected to
accurately describe the get-or-create-by-id semantics?

> + * with a reference held. The caller must call devlink_shd_put() when done.
> + *
> + * Return: Pointer to the shared devlink instance on success,
> + *         NULL on failure
> + */
> +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))

Jakub Kicinski suggested removing defensive NULL checks in the V7 review:

  https://lore.kernel.org/netdev/20260202194946.64555356@kernel.org/

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

You acknowledged this ("Okay.") but the current code still includes the
WARN_ON(!id || !ops) check.

> +		return NULL;
> +
> +	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))

Jakub Kicinski suggested removing this defensive check as well (referring
to his previous comment):

  https://lore.kernel.org/netdev/20260202194946.64555356@kernel.org/

  "ditto"

You acknowledged this ("Okay.") but the current code still includes the
WARN_ON(!devlink) check.

> +		return;
> +
> +	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);

[ ... ]