Re: [PATCH net-next 07/14] devlink: Implement port params registration

Przemek Kitszel
2025-03-20T08:16:13Z

On 2/28/25 16:21, Jiri Pirko wrote:
> Fri, Feb 28, 2025 at 02:23:00PM +0100, przemyslaw.kitszel@intel.com wrote:
>> On 2/28/25 13:28, Jiri Pirko wrote:
>>> Fri, Feb 28, 2025 at 12:58:38PM +0100, przemyslaw.kitszel@intel.com wrote:
>>>> On 2/28/25 03:12, Saeed Mahameed wrote:
>>>>> From: Saeed Mahameed <saeedm@nvidia.com>
>>>>>
>>>>> Port params infrastructure is incomplete and needs a bit of plumbing to
>>>>> support port params commands from netlink.
>>>>>
>>>>> Introduce port params registration API, very similar to current devlink
>>>>> params API, add the params xarray to devlink_port structure and
>>>>> decouple devlink params registration routines from the devlink
>>>>> structure.
>>>>>
>>>>> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>>> ---
>>>>> include/net/devlink.h | 14 ++++
>>>>> net/devlink/param.c | 150 ++++++++++++++++++++++++++++++++++--------
>>>>> net/devlink/port.c | 3 +
>>>>> 3 files changed, 140 insertions(+), 27 deletions(-)
>>>> For me devlink and devlink-port should be really the same, to the point
>>>> that the only difference is `bool is_port` flag inside of the
>>>> struct devlink. Then you could put special logic if really desired (to
>>>> exclude something for port).
>>>
>>> Why? Why other devlink objects shouldn't be the same as well. Then we
>>> can have one union. Does not make sense to me. The only think dev and
>>> port is sharing would be params. What else? Totally different beast.
>>
>> Instead of focusing on differences try to find similarities.
>>
>> health reporters per port and "toplevel",
>> just by grepping:
>> devlink_nl_sb_pool_fill()
>> devlink_nl_sb_port_pool_fill(),
>
> Sharedbuffer is separate story.
>
>>
>> devlink_region_create()
>> devlink_port_region_create()
>
> Okay, regions I missed.
>
>>
>> and there is no reason to assume that someone will not want to
>> extend ports to have devlink resources or other thing
>
> But looks at differences. They are huge.
>
> But perhaps I'm missing the point. What you want to achieve? Just to
> reduce API? That is always a tradeoff. I don't think the pros top the
> cons here.

not necessarily "reduce API", rather reuse the impl
but it is not a request for this series, just a comment

by itself this series is an improvement, and I didn't found anything
wrong in the code