Re: [PATCH net-next 1/6] devlink: extend devlink_param *set pointer

[Mateusz Polchlopek <mateusz.polchlopek@intel.com>]

On 4/24/2024 11:24 AM, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Wed, 24 Apr 2024 11:20:49 +0200
> 
>> On 4/24/24 11:05, Alexander Lobakin wrote:
>>> From: Tony Nguyen <anthony.l.nguyen@intel.com>
>>> Date: Mon, 22 Apr 2024 13:39:06 -0700
>>>
>>>> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>>
>>>> Extend devlink_param *set function pointer to take extack as a param.
>>>> Sometimes it is needed to pass information to the end user from set
>>>> function. It is more proper to use for that netlink instead of passing
>>>> message to dmesg.
>>>>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>
>>> [...]
>>>
>>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>> index d31769a116ce..35eb0f884386 100644
>>>> --- a/include/net/devlink.h
>>>> +++ b/include/net/devlink.h
>>>> @@ -483,7 +483,8 @@ struct devlink_param {
>>>>        int (*get)(struct devlink *devlink, u32 id,
>>>>               struct devlink_param_gset_ctx *ctx);
>>>>        int (*set)(struct devlink *devlink, u32 id,
>>>> -           struct devlink_param_gset_ctx *ctx);
>>>> +           struct devlink_param_gset_ctx *ctx,
>>>> +           struct netlink_ext_ack *extack);
>>>
>>> Sorry for the late comment. Can't we embed extack to
>>> devlink_param_gset_ctx instead? It would take much less lines.
>>
>> But then we will want to remove the extack param from .validate() too:
>>
>>>
>>>>        int (*validate)(struct devlink *devlink, u32 id,
>>>>                union devlink_param_value val,
>>>>                struct netlink_ext_ack *extack);
>>
>> right there.
> 
> We don't have &devlink_param_gset_ctx here, only the union.
> Extending this union with the extack requires converting it to a struct
> (which would have extack + this union), which is again a conversion of
> all the drivers : >

Makes sense. I also have to take a look on docs issue reported, so
Your's suggestion I will add in the next version too.

>> This would amount to roughly the same scope for changes, but would spare
>> us yet another round when someone would like to extend .get(), so I like
>> this idea.
> 
> Thanks,
> Olek