Re: [PATCH net-next 01/14] devlink: define enum for attr types of dynamic attributes

[Saeed Mahameed <saeed@kernel.org>]

On 06 Mar 12:05, Simon Horman wrote:
>On Thu, Feb 27, 2025 at 06:12:14PM -0800, Saeed Mahameed wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Devlink param and health reporter fmsg use attributes with dynamic type
>> which is determined according to a different type. Currently used values
>> are NLA_*. The problem is, they are not part of UAPI. They may change
>> which would cause a break.
>>
>> To make this future safe, introduce a enum that shadows NLA_* values in
>> it and is part of UAPI.
>>
>> Also, this allows to possibly carry types that are unrelated to NLA_*
>> values.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>> ---
>>  Documentation/netlink/specs/devlink.yaml | 23 ++++++++++++++++++
>>  include/uapi/linux/devlink.h             | 17 ++++++++++++++
>>  net/devlink/health.c                     | 17 ++++++++++++--
>>  net/devlink/param.c                      | 30 ++++++++++++------------
>>  4 files changed, 70 insertions(+), 17 deletions(-)
>>
>> diff --git a/Documentation/netlink/specs/devlink.yaml b/Documentation/netlink/specs/devlink.yaml
>> index 09fbb4c03fc8..e99fc51856c5 100644
>> --- a/Documentation/netlink/specs/devlink.yaml
>> +++ b/Documentation/netlink/specs/devlink.yaml
>> @@ -202,6 +202,29 @@ definitions:
>>          name: exception
>>        -
>>          name: control
>> +  -
>> +    type: enum
>> +    name: dyn_attr_type
>> +    entries:
>> +      -
>> +        name: u8
>> +        value: 1
>> +      -
>> +        name: u16
>> +      -
>> +        name: u32
>> +      -
>> +        name: u64
>> +      -
>> +        name: string
>> +      -
>> +        name: flag
>> +      -
>> +        name: nul_string
>> +        value: 10
>> +      -
>> +        name: binary
>> +  -
^^^^^^^ extra empty type.. 

>
>Hi Saeed,
>
>Thanks for these patches.
>
>Unfortunately the above seems to cause ynl-regen to blow up.

Thanks Simon, good catch, there was an extra empty type added by mistake,
see above, will remove in V2.

>I don't know why, but I see:
>
>  $ ./tools/net/ynl/ynl-regen.sh
>  Traceback (most recent call last):
>    File "/home/nipa/net-next/wt-1/tools/net/ynl/pyynl/ynl_gen_c.py", line 3074, in <module>
>      main()
>      ~~~~^^
>    File "/home/nipa/net-next/wt-1/tools/net/ynl/pyynl/ynl_gen_c.py", line 2783, in main
>      parsed = Family(args.spec, exclude_ops)
>    File "/home/nipa/net-next/wt-1/tools/net/ynl/pyynl/ynl_gen_c.py", line 954, in __init__
>      super().__init__(file_name, exclude_ops=exclude_ops)
>      ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>    File "/home/nipa/net-next/wt-1/tools/net/ynl/pyynl/lib/nlspec.py", line 462, in __init__
>      jsonschema.validate(self.yaml, schema)
>      ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
>    File "/usr/lib/python3.13/site-packages/jsonschema/validators.py", line 1307, in validate
>      raise error
>  jsonschema.exceptions.ValidationError: None is not of type 'object'
>
>  ...
>
>Perhaps I need a newer version of jsonschema?
>
>
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 9401aa343673..8cdd60eb3c43 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -385,6 +385,23 @@ enum devlink_linecard_state {
>>  	DEVLINK_LINECARD_STATE_MAX = __DEVLINK_LINECARD_STATE_MAX - 1
>>  };
>>
>> +/**
>> + * enum devlink_dyn_attr_type - Dynamic attribute type type.
>
>Perhaps this relates to auto-generation.
>But I think the Kernel doc should also document the enum values:
>DEVLINK_DYN_ATTR_TYPE_U8, ...
>
>> + */
>> +enum devlink_dyn_attr_type {
>> +	/* Following values relate to the internal NLA_* values */
>> +	DEVLINK_DYN_ATTR_TYPE_U8 = 1,
>> +	DEVLINK_DYN_ATTR_TYPE_U16,
>> +	DEVLINK_DYN_ATTR_TYPE_U32,
>> +	DEVLINK_DYN_ATTR_TYPE_U64,
>> +	DEVLINK_DYN_ATTR_TYPE_STRING,
>> +	DEVLINK_DYN_ATTR_TYPE_FLAG,
>> +	DEVLINK_DYN_ATTR_TYPE_NUL_STRING = 10,
>> +	DEVLINK_DYN_ATTR_TYPE_BINARY,
>> +	__DEVLINK_DYN_ATTR_TYPE_CUSTOM_BASE = 0x80,
>> +	/* Any possible custom types, unrelated to NLA_* values go below */
>> +};
>> +
>>  enum devlink_attr {
>>  	/* don't change the order or add anything between, this is ABI! */
>>  	DEVLINK_ATTR_UNSPEC,
>
>...