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 03:44:54PM +0200, przemyslaw.kitszel@intel.com wrote:
>On 4/28/26 13:10, Jiri Pirko wrote:
>> 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?
>
>I have a user for it, but it will be a separate series
>(I have already 15 patches there), will post RFC to link here
>to the user, will that work?

Add it when/if you need it, no? I still believe there might be a better
way instead of this.


>
>my intention was to not tie touching mlx code with big series for intel
>
>> 
>> Otherwise, this looks fine to me. Thanks! (small nitpick below)
>
>ack for the nit