Re: [PATCH net-next v4 6/6] netlink: specs: rt-route: describe seg6-local attrs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch adds the `seg6-local-flv-ops` enum to
  Documentation/netlink/specs/rt-route.yaml with `unspec` as its first…

--

netlink: specs: rt-route: describe seg6-local attrs

This commit adds two enums to the rt-route spec: the SEG6 local actions and
the seg6-local flavor operations. It also changes the bpf, counters and
flavors attributes in seg6-local from binary to nested attribute sets
(seg6-local-bpf, seg6-local-cnt and seg6-local-flv).

> diff --git a/Documentation/netlink/specs/rt-route.yaml b/Documentation/netlink/specs/rt-route.yaml
> index 82e59af56ecdb..48d1659f20f34 100644
> --- a/Documentation/netlink/specs/rt-route.yaml
> +++ b/Documentation/netlink/specs/rt-route.yaml
> @@ -116,6 +116,45 @@ definitions:

[ ... ]

> +  -
> +    name: seg6-local-flv-ops
> +    type: enum
> +    name-prefix: seg6-local-flv-op-
> +    enum-name:
> +    entries:
> +      -
> +        name: unspec
> +      -
> +        name: psp
> +      -
> +        name: usp
> +      -
> +        name: usd
> +      -
> +        name: next-csid

[ ... ]

> @@ -618,6 +661,50 @@ attribute-sets:

[ ... ]

> +  -
> +    name: seg6-local-flv
> +    name-prefix: seg6-local-flv-
> +    attributes:
> +      -
> +        name: operation
> +        type: u32
> +        enum: seg6-local-flv-ops
> +        enum-as-flags: true

[Severity: Low]
With enum-as-flags set, should unspec still be the first entry of
seg6-local-flv-ops?

YNL turns each entry's value into a bit position in this mode:

tools/net/ynl/pyynl/lib/nlspec.py:EnumEntry.user_value() {
        if self.enum_set['type'] == 'flags' or as_flags:
            return 1 << self.value
}

The flag values come out as unspec=0x1, psp=0x2, usp=0x4, usd=0x8 and
next-csid=0x10.

The psp through next-csid bits line up with the kernel's
SEG6_F_LOCAL_FLV_OP(flvname), which is BIT(SEG6_LOCAL_FLV_OP_##flvname). Bit
0 (UNSPEC), however, is not a real flavor operation. The supported masks in
net/ipv6/seg6_local.c contain only NEXT_CSID and PSP:

#define SEG6_LOCAL_END_FLV_SUPP_OPS	(SEG6_F_LOCAL_FLV_NEXT_CSID | \
					 SEG6_LOCAL_FLV8986_SUPP_OPS)
#define SEG6_LOCAL_END_X_FLV_SUPP_OPS	SEG6_F_LOCAL_FLV_NEXT_CSID

parse_nla_flavors() rejects every bit outside these masks:

	if (rc < 0 || (fops & ~supp_fops)) {
		NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)");
		return -EOPNOTSUPP;
	}

As a result, the spec offers an "unspec" flag that the kernel never reports
in a dump. When it is sent in a request, the kernel always rejects it with
-EOPNOTSUPP.

devlink.yaml's reload-action enum is also used with enum-as-flags, and it
handles this by leaving out unspec and setting value: 1 on its first entry.
Could this spec do the same? Dropping unspec and giving psp value: 1 would
keep every real bit in place.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-ynl_rt_encap-v4-0-4ed2ccf7f949%40kylinos.cn