Re: [PATCH net-next] devlink: unify devlink_shd_get_priv() into devlink_priv()

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 3/23/26 16:03, Jiri Pirko wrote:
> Mon, Mar 23, 2026 at 02:21:36PM +0100, przemyslaw.kitszel@intel.com wrote:
>> Unify access API to shared devlink priv data with normal devlink.
>>
>> Thanks to Jiri Piro, we now have ability to create shared devlink
>> instances [1]. Introduction series have added usege of those for mlx, but
>> without priv data attached to the shared devlink.
>>
>> Current API makes it possible to access shared devlink instance's priv
>> data:
>>
>> 	void *devlink_shd_get_priv(struct devlink *devlink);
>>
>> but it is easy to to forget (especially during rebase from "before shared
>> devlinks" era) and call:
>>
>> 	void *devlink_priv(struct devlink *devlink);
>>
>> which even has the same signature, so it's hard to catch the error.
>>
>> New proposed API unifies both calls into one, without any increase in the
>> observerd struct size. (Alternative could be to store additional pointer,
>> set during devlink_alloc).
> 
> You should use some AI corrector :) too many typos.

faux pas instead of faux bus, apologies for the most shameful of typos
("new machine"-kind of issue)

and thank you for looking

I have one more trivial related patch, will append to v2 tommorow

> 
>>
>> Unexport the less convenient API call.
>>
>> [1] commit 411ad0605875 ("Merge branch 'devlink-introduce-shared-devlink-instance-for-pfs-on-same-chip'")
>> [1] https://lore.kernel.org/all/20260312100407.551173-1-jiri@resnulli.us
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>> sorry that I have not mentioned it during [1] review, it got obvious
>> for me only after working with the code
>> ---
>> net/devlink/devl_internal.h |  4 ++++
>> net/devlink/core.c          |  3 +++
>> net/devlink/sh_dev.c        | 11 ++++++++---
>> 3 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> index 7dfb7cdd2d23..abbe308ee9bd 100644
>> --- a/net/devlink/devl_internal.h
>> +++ b/net/devlink/devl_internal.h
>> @@ -58,6 +58,7 @@ struct devlink {
>> 	struct mutex lock;
>> 	struct lock_class_key lock_key;
>> 	u8 reload_failed:1;
>> +	u8 is_shd:1;
>> 	refcount_t refcount;
>> 	struct rcu_work rwork;
>> 	struct devlink_rel *rel;
>> @@ -72,6 +73,9 @@ struct devlink *__devlink_alloc(const struct devlink_ops *ops, size_t priv_size,
>> 				struct net *net, struct device *dev,
>> 				const struct device_driver *dev_driver);
>>
>> +/* Get private data from shared devlink instance */
>> +void *devlink_shd_get_priv(struct devlink *devlink);
>> +
>> #define devl_warn(devlink, format, args...)				\
>> 	do {								\
>> 		if ((devlink)->dev)					\
>> diff --git a/net/devlink/core.c b/net/devlink/core.c
>> index eeb6a71f5f56..4c97e2e2c073 100644
>> --- a/net/devlink/core.c
>> +++ b/net/devlink/core.c
>> @@ -232,6 +232,9 @@ int devlink_rel_devlink_handle_put(struct sk_buff *msg, struct devlink *devlink,
>>
>> void *devlink_priv(struct devlink *devlink)
>> {
>> +	if (devlink->is_shd)
>> +		return devlink_shd_get_priv(devlink);
>> +
>> 	return &devlink->priv;
>> }
>> EXPORT_SYMBOL_GPL(devlink_priv);
>> diff --git a/net/devlink/sh_dev.c b/net/devlink/sh_dev.c
>> index 9b7456d64b57..450e3218759e 100644
>> --- a/net/devlink/sh_dev.c
>> +++ b/net/devlink/sh_dev.c
>> @@ -31,6 +31,11 @@ static struct devlink_shd *devlink_shd_lookup(const char *id)
>> 	return NULL;
>> }
>>
>> +static struct devlink_shd *devlink_to_shd(struct devlink *devlink)
>> +{
>> +	return &devlink->priv;
>> +}
> 
> Can you rather have unexported __devlink_priv() above existing
> devlink_priv()?

makes sense, will do

(this will not remove the need of devlink_shd_get_priv() in sh_dev.c,
as the struct devlink_shd is defined here)

> 
> 
>> +
>> static struct devlink_shd *devlink_shd_create(const char *id,
>> 					      const struct devlink_ops *ops,
>> 					      size_t priv_size,
>> @@ -43,13 +48,14 @@ static struct devlink_shd *devlink_shd_create(const char *id,
>> 				  &init_net, NULL, driver);
>> 	if (!devlink)
>> 		return NULL;
>> -	shd = devlink_priv(devlink);
>> +	shd = devlink_to_shd(devlink);
>>
>> 	shd->id = kstrdup(id, GFP_KERNEL);
>> 	if (!shd->id)
>> 		goto err_devlink_free;
>> 	shd->priv_size = priv_size;
>> 	refcount_set(&shd->refcount, 1);
>> +	devlink->is_shd = 1;
>>
>> 	devl_lock(devlink);
>> 	devl_register(devlink);
>> @@ -137,7 +143,7 @@ void devlink_shd_put(struct devlink *devlink)
>> 	struct devlink_shd *shd;
>>
>> 	mutex_lock(&shd_mutex);
>> -	shd = devlink_priv(devlink);
>> +	shd = devlink_to_shd(devlink);
>> 	if (refcount_dec_and_test(&shd->refcount))
>> 		devlink_shd_destroy(shd);
>> 	mutex_unlock(&shd_mutex);
>> @@ -159,4 +165,3 @@ void *devlink_shd_get_priv(struct devlink *devlink)
>>
>> 	return shd->priv;
>> }
>> -EXPORT_SYMBOL_GPL(devlink_shd_get_priv);
>> -- 
>> 2.39.3
>>