| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <wdkd7yelgosii7bklmahxf5t6xnn2vydnwiiruiwqpyue722dj@yjnkcdctzeav>
Date: Tue, 3 Feb 2026 10:44:16 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Tariq Toukan <tariqt@...dia.com>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Donald Hunter <donald.hunter@...il.com>,
Jonathan Corbet <corbet@....net>, Saeed Mahameed <saeedm@...dia.com>,
Leon Romanovsky <leon@...nel.org>, Mark Bloch <mbloch@...dia.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, linux-rdma@...r.kernel.org,
Gal Pressman <gal@...dia.com>, Moshe Shemesh <moshe@...dia.com>,
Carolina Jubran <cjubran@...dia.com>, Cosmin Ratiu <cratiu@...dia.com>, Jiri Pirko <jiri@...dia.com>,
Randy Dunlap <rdunlap@...radead.org>, Simon Horman <horms@...nel.org>,
Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH net-next V7 02/14] devlink: introduce shared devlink
instance for PFs on same chip
Tue, Feb 03, 2026 at 04:49:46AM +0100, kuba@...nel.org wrote:
>On Wed, 28 Jan 2026 13:25:32 +0200 Tariq Toukan wrote:
>> From: Jiri Pirko <jiri@...dia.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?
String gives drivers flexibility to use anything. Perhaps I'm missing
your point. Are you againts free-form or just string and buf+buf_len
would be fine?
>
>> + 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
Will add.
>
>> +};
>
>> +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
Okay. Tried to be consistent with the rest of the code. But as you wish.
>
>> + 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..
Okay.
>
>> + 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
Okay.
>
>> + 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);
Powered by blists - more mailing lists