[PATCH net-next] tools: ynl-gen: fix nested policy attribute type

[PATCH net-next] tools: ynl-gen: fix nested policy attribute type

[Arkadiusz Kubalewski]

When nested multi-attribute is used in yaml spec, generated type in
the netlink policy is NLA_NEST, which is wrong as there is no such type.
Fix be adding `ed` sufix for policy generated for 'nest' type attribute
when the attribute is parsed as TypeMultiAttr class.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 tools/net/ynl/ynl-gen-c.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 9adcd785db0b..0b5e0802a9a7 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -498,6 +498,16 @@ class TypeArrayNest(Type):
         else:
             raise Exception(f"Sub-type {self.attr['sub-type']} not supported yet")
 
+    def attr_policy(self, cw):
+        t = self.attr['type']
+        if (t == 'nest'):
+            policy = c_upper(f'nla-{t}ed')
+        else:
+            policy = c_upper(f'nla-{t}')
+
+        spec = self._attr_policy(policy)
+        cw.p(f"\t[{self.enum_name}] = {spec},")
+
     def _attr_typol(self):
         return f'.type = YNL_PT_NEST, .nest = &{self.nested_render_name}_nest, '
 
-- 
2.37.3

[PATCH net-next] tools: ynl-gen: fix NLA_POLICY_MAX on enums with value

[Arkadiusz Kubalewski]

Policy defines max as length of enum list, if enum attribute is
defined with a value, generated netlink policy max value is wrong.
Calculate proper max value for policy where enums that are given
explicit value.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 tools/net/ynl/ynl-gen-c.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 4c12c6f8968e..9adcd785db0b 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -279,7 +279,16 @@ class TypeScalar(Type):
         elif 'enum' in self.attr:
             enum = self.family.consts[self.attr['enum']]
             cnt = len(enum['entries'])
-            return f"NLA_POLICY_MAX({policy}, {cnt - 1})"
+            values = [0] * cnt
+            for i in range(cnt):
+                if 'value' in enum['entries'][i]:
+                    values[i] = enum['entries'][i]['value']
+                else:
+                    if i == 0:
+                        values[i] = 0;
+                    else:
+                        values[i] = values[i - 1] + 1
+            return f"NLA_POLICY_MAX({policy}, {max(values)})"
         return super()._attr_policy(policy)
 
     def _attr_typol(self):
-- 
2.37.3

[PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Arkadiusz Kubalewski]

Including ynl generated uapi header files into source kerneldocs
(rst files in Documentation/) produces warnings during documentation
builds (i.e. make htmldocs)

Prevent warnings by generating also description for enums where rander_max
was selected.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 include/uapi/linux/netdev.h       |  1 +
 tools/include/uapi/linux/netdev.h |  1 +
 tools/net/ynl/ynl-gen-c.py        | 11 ++++++++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
index 639524b59930..d78f7ae95092 100644
--- a/include/uapi/linux/netdev.h
+++ b/include/uapi/linux/netdev.h
@@ -24,6 +24,7 @@
  *   XDP buffer support in the driver napi callback.
  * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev implements
  *   non-linear XDP buffer support in ndo_xdp_xmit callback.
+ * @NETDEV_XDP_ACT_MASK: valid values mask
  */
 enum netdev_xdp_act {
 	NETDEV_XDP_ACT_BASIC = 1,
diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
index 639524b59930..d78f7ae95092 100644
--- a/tools/include/uapi/linux/netdev.h
+++ b/tools/include/uapi/linux/netdev.h
@@ -24,6 +24,7 @@
  *   XDP buffer support in the driver napi callback.
  * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev implements
  *   non-linear XDP buffer support in ndo_xdp_xmit callback.
+ * @NETDEV_XDP_ACT_MASK: valid values mask
  */
 enum netdev_xdp_act {
 	NETDEV_XDP_ACT_BASIC = 1,
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 0b5e0802a9a7..0d396bf98c27 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -2011,6 +2011,7 @@ def render_uapi(family, cw):
         # Write kdoc for enum and flags (one day maybe also structs)
         if const['type'] == 'enum' or const['type'] == 'flags':
             enum = family.consts[const['name']]
+            name_pfx = const.get('name-prefix', f"{family.name}-{const['name']}-")
 
             if enum.has_doc():
                 cw.p('/**')
@@ -2022,10 +2023,18 @@ def render_uapi(family, cw):
                     if entry.has_doc():
                         doc = '@' + entry.c_name + ': ' + entry['doc']
                         cw.write_doc_line(doc)
+                if const.get('render-max', False):
+                    if const['type'] == 'flags':
+                        doc = '@' + c_upper(name_pfx + 'mask') + ': valid values mask'
+                        cw.write_doc_line(doc)
+                    else:
+                        doc = '@' + c_upper(name_pfx + 'max') + ': max valid value'
+                        cw.write_doc_line(doc)
+                        doc = '@__' + c_upper(name_pfx + 'max') + ': do not use'
+                        cw.write_doc_line(doc)
                 cw.p(' */')
 
             uapi_enum_start(family, cw, const, 'name')
-            name_pfx = const.get('name-prefix', f"{family.name}-{const['name']}-")
             for entry in enum.entries.values():
                 suffix = ','
                 if entry.value_change:
-- 
2.37.3

Re: [PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Jakub Kicinski]

On Wed, 14 Jun 2023 01:17:09 +0200 Arkadiusz Kubalewski wrote:
> Including ynl generated uapi header files into source kerneldocs
> (rst files in Documentation/) produces warnings during documentation
> builds (i.e. make htmldocs)
> 
> Prevent warnings by generating also description for enums where rander_max
> was selected.

Do you reckon that documenting the meta-values makes sense, or should
we throw a:

/* private: */

comment in front of them so that kdoc ignores them? Does user space
have any use for those? If we want to document them...

> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> index 639524b59930..d78f7ae95092 100644
> --- a/include/uapi/linux/netdev.h
> +++ b/include/uapi/linux/netdev.h
> @@ -24,6 +24,7 @@
>   *   XDP buffer support in the driver napi callback.
>   * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev implements
>   *   non-linear XDP buffer support in ndo_xdp_xmit callback.
> + * @NETDEV_XDP_ACT_MASK: valid values mask

... I think we need to include some sort of indication that the value
will change as the uAPI is extended. Unlike the other values which are
set in stone, so to speak. "mask of currently defines values" ? Dunno.

>   */
>  enum netdev_xdp_act {
>  	NETDEV_XDP_ACT_BASIC = 1,
> diff --git a/tools/include/uapi/linux/netdev.h b/tools/include/uapi/linux/netdev.h
> index 639524b59930..d78f7ae95092 100644
> --- a/tools/include/uapi/linux/netdev.h
> +++ b/tools/include/uapi/linux/netdev.h
> @@ -24,6 +24,7 @@
>   *   XDP buffer support in the driver napi callback.
>   * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev implements
>   *   non-linear XDP buffer support in ndo_xdp_xmit callback.
> + * @NETDEV_XDP_ACT_MASK: valid values mask
>   */
>  enum netdev_xdp_act {
>  	NETDEV_XDP_ACT_BASIC = 1,
> diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
> index 0b5e0802a9a7..0d396bf98c27 100755
> --- a/tools/net/ynl/ynl-gen-c.py
> +++ b/tools/net/ynl/ynl-gen-c.py
> @@ -2011,6 +2011,7 @@ def render_uapi(family, cw):
>          # Write kdoc for enum and flags (one day maybe also structs)
>          if const['type'] == 'enum' or const['type'] == 'flags':
>              enum = family.consts[const['name']]
> +            name_pfx = const.get('name-prefix', f"{family.name}-{const['name']}-")
>  
>              if enum.has_doc():
>                  cw.p('/**')
> @@ -2022,10 +2023,18 @@ def render_uapi(family, cw):
>                      if entry.has_doc():
>                          doc = '@' + entry.c_name + ': ' + entry['doc']
>                          cw.write_doc_line(doc)
> +                if const.get('render-max', False):
> +                    if const['type'] == 'flags':
> +                        doc = '@' + c_upper(name_pfx + 'mask') + ': valid values mask'
> +                        cw.write_doc_line(doc)
> +                    else:
> +                        doc = '@' + c_upper(name_pfx + 'max') + ': max valid value'
> +                        cw.write_doc_line(doc)
> +                        doc = '@__' + c_upper(name_pfx + 'max') + ': do not use'

This one is definitely a candidate for /* private: */

> +                        cw.write_doc_line(doc)
>                  cw.p(' */')
>  
>              uapi_enum_start(family, cw, const, 'name')
> -            name_pfx = const.get('name-prefix', f"{family.name}-{const['name']}-")
>              for entry in enum.entries.values():
>                  suffix = ','
>                  if entry.value_change:

-- 
pw-bot: cr

RE: [PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, June 14, 2023 2:59 AM
>
>On Wed, 14 Jun 2023 01:17:09 +0200 Arkadiusz Kubalewski wrote:
>> Including ynl generated uapi header files into source kerneldocs
>> (rst files in Documentation/) produces warnings during documentation
>> builds (i.e. make htmldocs)
>>
>> Prevent warnings by generating also description for enums where
>> rander_max was selected.
>
>Do you reckon that documenting the meta-values makes sense, or should
>we throw a:
>
>/* private: */
>

Most probably it doesn't..
Tried this:
/*
 [ other values description ]
 * private:
 * @__<NAME>_MAX
 */
and this:
/*
 [ other values description ]
 * private: @__<NAME>_MAX
 */

Both are not working as we would expect.

Do you mean to have double comments for enums? like:
/*
 [ other values description ]
 */
/*
 * private:
 * @__<NAME>_MAX
 */
 
>comment in front of them so that kdoc ignores them? Does user space
>have any use for those? If we want to document them...
>

Hmm, do you recall where I can find proper format of such ignore enum comment
for kdoc generation?
Or maybe we need to also submit patch to some kdoc build process to actually
change the current behavior?

>> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
>> index 639524b59930..d78f7ae95092 100644
>> --- a/include/uapi/linux/netdev.h
>> +++ b/include/uapi/linux/netdev.h
>> @@ -24,6 +24,7 @@
>>   *   XDP buffer support in the driver napi callback.
>>   * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev implements
>>   *   non-linear XDP buffer support in ndo_xdp_xmit callback.
>> + * @NETDEV_XDP_ACT_MASK: valid values mask
>
>... I think we need to include some sort of indication that the value
>will change as the uAPI is extended. Unlike the other values which are
>set in stone, so to speak. "mask of currently defines values" ? Dunno.
>

Sure can fix this.

>>   */
>>  enum netdev_xdp_act {
>>  	NETDEV_XDP_ACT_BASIC = 1,
>> diff --git a/tools/include/uapi/linux/netdev.h
>>b/tools/include/uapi/linux/netdev.h
>> index 639524b59930..d78f7ae95092 100644
>> --- a/tools/include/uapi/linux/netdev.h
>> +++ b/tools/include/uapi/linux/netdev.h
>> @@ -24,6 +24,7 @@
>>   *   XDP buffer support in the driver napi callback.
>>   * @NETDEV_XDP_ACT_NDO_XMIT_SG: This feature informs if netdev
>>implements
>>   *   non-linear XDP buffer support in ndo_xdp_xmit callback.
>> + * @NETDEV_XDP_ACT_MASK: valid values mask
>>   */
>>  enum netdev_xdp_act {
>>  	NETDEV_XDP_ACT_BASIC = 1,
>> diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
>> index 0b5e0802a9a7..0d396bf98c27 100755
>> --- a/tools/net/ynl/ynl-gen-c.py
>> +++ b/tools/net/ynl/ynl-gen-c.py
>> @@ -2011,6 +2011,7 @@ def render_uapi(family, cw):
>>          # Write kdoc for enum and flags (one day maybe also structs)
>>          if const['type'] == 'enum' or const['type'] == 'flags':
>>              enum = family.consts[const['name']]
>> +            name_pfx = const.get('name-prefix', f"{family.name}-
>>{const['name']}-")
>>
>>              if enum.has_doc():
>>                  cw.p('/**')
>> @@ -2022,10 +2023,18 @@ def render_uapi(family, cw):
>>                      if entry.has_doc():
>>                          doc = '@' + entry.c_name + ': ' + entry['doc']
>>                          cw.write_doc_line(doc)
>> +                if const.get('render-max', False):
>> +                    if const['type'] == 'flags':
>> +                        doc = '@' + c_upper(name_pfx + 'mask') + ':
>>valid values mask'
>> +                        cw.write_doc_line(doc)
>> +                    else:
>> +                        doc = '@' + c_upper(name_pfx + 'max') + ': max
>>valid value'
>> +                        cw.write_doc_line(doc)
>> +                        doc = '@__' + c_upper(name_pfx + 'max') + ': do
>>not use'
>
>This one is definitely a candidate for /* private: */

Yep, makes sense, trying to find some way...

Thank you,
Arkadiusz

>
>> +                        cw.write_doc_line(doc)
>>                  cw.p(' */')
>>
>>              uapi_enum_start(family, cw, const, 'name')
>> -            name_pfx = const.get('name-prefix', f"{family.name}-
>{const['name']}-")
>>              for entry in enum.entries.values():
>>                  suffix = ','
>>                  if entry.value_change:
>
>--
>pw-bot: cr

Re: [PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Jakub Kicinski]

On Wed, 14 Jun 2023 12:48:14 +0000 Kubalewski, Arkadiusz wrote:
> >From: Jakub Kicinski <kuba@kernel.org>
> >Sent: Wednesday, June 14, 2023 2:59 AM
> >
> >On Wed, 14 Jun 2023 01:17:09 +0200 Arkadiusz Kubalewski wrote:  
> >> Including ynl generated uapi header files into source kerneldocs
> >> (rst files in Documentation/) produces warnings during documentation
> >> builds (i.e. make htmldocs)
> >>
> >> Prevent warnings by generating also description for enums where
> >> rander_max was selected.  
> >
> >Do you reckon that documenting the meta-values makes sense, or should
> >we throw a:
> >
> >/* private: */
> >  
> 
> Most probably it doesn't..
> Tried this:
> /*
>  [ other values description ]
>  * private:
>  * @__<NAME>_MAX
>  */
> and this:
> /*
>  [ other values description ]
>  * private: @__<NAME>_MAX
>  */
> 
> Both are not working as we would expect.
> 
> Do you mean to have double comments for enums? like:
> /*
>  [ other values description ]
>  */
> /*
>  * private:
>  * @__<NAME>_MAX
>  */
>
> >comment in front of them so that kdoc ignores them? Does user space
> >have any use for those? If we want to document them...
> 
> Hmm, do you recall where I can find proper format of such ignore enum comment
> for kdoc generation?
> Or maybe we need to also submit patch to some kdoc build process to actually
> change the current behavior?

It's explained in the kdoc documentation :(
https://docs.kernel.org/doc-guide/kernel-doc.html#members

RE: [PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, June 14, 2023 7:39 PM
>
>On Wed, 14 Jun 2023 12:48:14 +0000 Kubalewski, Arkadiusz wrote:
>> >From: Jakub Kicinski <kuba@kernel.org>
>> >Sent: Wednesday, June 14, 2023 2:59 AM
>> >
>> >On Wed, 14 Jun 2023 01:17:09 +0200 Arkadiusz Kubalewski wrote:
>> >> Including ynl generated uapi header files into source kerneldocs
>> >> (rst files in Documentation/) produces warnings during documentation
>> >> builds (i.e. make htmldocs)
>> >>
>> >> Prevent warnings by generating also description for enums where
>> >> rander_max was selected.
>> >
>> >Do you reckon that documenting the meta-values makes sense, or should
>> >we throw a:
>> >
>> >/* private: */
>> >
>>
>> Most probably it doesn't..
>> Tried this:
>> /*
>>  [ other values description ]
>>  * private:
>>  * @__<NAME>_MAX
>>  */
>> and this:
>> /*
>>  [ other values description ]
>>  * private: @__<NAME>_MAX
>>  */
>>
>> Both are not working as we would expect.
>>
>> Do you mean to have double comments for enums? like:
>> /*
>>  [ other values description ]
>>  */
>> /*
>>  * private:
>>  * @__<NAME>_MAX
>>  */
>>
>> >comment in front of them so that kdoc ignores them? Does user space
>> >have any use for those? If we want to document them...
>>
>> Hmm, do you recall where I can find proper format of such ignore enum
>comment
>> for kdoc generation?
>> Or maybe we need to also submit patch to some kdoc build process to
>actually
>> change the current behavior?
>
>It's explained in the kdoc documentation :(
>https://docs.kernel.org/doc-guide/kernel-doc.html#members


Thanks for pointing this, but it doesn't work :/

I tried described format but still ./scripts/kernel-doc warns about it.
Same as 'make htmldocs' does, as it uses ./scripts/kernel-doc

Also, if the enum is not described in the header, the docs produced by
the 'make htmldocs' would list the enum with the comment "undescribed".

It seems we need fixing:
- prevent warning from ./scripts/kernel-doc, so enums marked as "private:"
  would not warn
- generate __<ENUM_NAME>_MAX while marking them as "/* private: */"
- add some kind of "pattern exclude" directive/mechanics for generating
  docs with sphinx

Does it make sense?
TBH, not yet sure if all above are possible..

Thank you!
Arkadiusz

Re: [PATCH net-next] tools: ynl-gen: generate docs for <name>_max/_mask enums

[Jakub Kicinski]

On Wed, 14 Jun 2023 22:11:38 +0000 Kubalewski, Arkadiusz wrote:
> Thanks for pointing this, but it doesn't work :/
> 
> I tried described format but still ./scripts/kernel-doc warns about it.
> Same as 'make htmldocs' does, as it uses ./scripts/kernel-doc
> 
> Also, if the enum is not described in the header, the docs produced by
> the 'make htmldocs' would list the enum with the comment "undescribed".

Oh, you're right :S Looks like private: does not work for enums.

> It seems we need fixing:
> - prevent warning from ./scripts/kernel-doc, so enums marked as "private:"
>   would not warn
> - generate __<ENUM_NAME>_MAX while marking them as "/* private: */"
> - add some kind of "pattern exclude" directive/mechanics for generating
>   docs with sphinx
> 
> Does it make sense?
> TBH, not yet sure if all above are possible..

Let's ask Jon, and wait for him to chime in, I don't think these
warnings should be a blocker for new families.

Jon, we have some "meta" entries in the uAPI enums in netlink 
to mark the number of attributes, eg:

enum {
	NETDEV_A_DEV_IFINDEX = 1,
	NETDEV_A_DEV_PAD,
	NETDEV_A_DEV_XDP_FEATURES,
/* private: */
	__NETDEV_A_DEV_MAX, // this
	NETDEV_A_DEV_MAX = (__NETDEV_A_DEV_MAX - 1) // and this
};

Also masks of all flags like:

enum netdev_xdp_act {
	NETDEV_XDP_ACT_BASIC = 1,
	NETDEV_XDP_ACT_REDIRECT = 2,
	NETDEV_XDP_ACT_NDO_XMIT = 4,
	NETDEV_XDP_ACT_XSK_ZEROCOPY = 8,
	NETDEV_XDP_ACT_HW_OFFLOAD = 16,
	NETDEV_XDP_ACT_RX_SG = 32,
	NETDEV_XDP_ACT_NDO_XMIT_SG = 64,
/* private: */
	NETDEV_XDP_ACT_MASK = 127, // this
};

which user space should not care about.

I was hoping we can mark them as /* private: */ but that doesn't
work, when we add kdocs without documenting those - there's a warning.
Is this a known problem? Is it worth fixing?
Do we need to fix both kernel-doc and sphinx or just the former?

Re: [PATCH net-next] tools: ynl-gen: fix nested policy attribute type

[Jakub Kicinski]

On Wed, 14 Jun 2023 01:17:07 +0200 Arkadiusz Kubalewski wrote:
> When nested multi-attribute is used in yaml spec, generated type in
> the netlink policy is NLA_NEST, which is wrong as there is no such type.
> Fix be adding `ed` sufix for policy generated for 'nest' type attribute
> when the attribute is parsed as TypeMultiAttr class.

I CCed you on my changes which address the same issue, they have
already been merged:

https://lore.kernel.org/all/20230612155920.1787579-1-kuba@kernel.org/

I think that covers the first two patches. What am I missing? :S

RE: [PATCH net-next] tools: ynl-gen: fix nested policy attribute type

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, June 14, 2023 2:51 AM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
>chuck.lever@oracle.com
>Subject: Re: [PATCH net-next] tools: ynl-gen: fix nested policy attribute
>type
>
>On Wed, 14 Jun 2023 01:17:07 +0200 Arkadiusz Kubalewski wrote:
>> When nested multi-attribute is used in yaml spec, generated type in
>> the netlink policy is NLA_NEST, which is wrong as there is no such type.
>> Fix be adding `ed` sufix for policy generated for 'nest' type attribute
>> when the attribute is parsed as TypeMultiAttr class.
>
>I CCed you on my changes which address the same issue, they have
>already been merged:
>
>https://lore.kernel.org/all/20230612155920.1787579-1-kuba@kernel.org/
>
>I think that covers the first two patches. What am I missing? :S

Yep, sorry I missed them.
Did rebase and it seems your changes are great, everything works,
please drop this patch.

Thank you!
Arkadiusz