Re: [PATCH 2/2] net: ynl: allow u8 sub-type for indexed array

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ynl: allow u8 sub-type for indexed array

This adds u8 to the list of sub-types accepted by AttrSet.new_attr() for
indexed-array attributes in tools/net/ynl/pyynl/ynl_gen_c.py, so that the
nl80211 chain-signal attributes can be described in the YAML spec.  Without
it the generator rejects the sub-type outright.

> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index cdc3646f2642c..7373a48d5516a 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -1158,7 +1158,7 @@ class AttrSet(SpecAttrSet):
>          elif elem['type'] == 'nest':
>              t = TypeNest(self.family, self, elem, value)
>          elif elem['type'] == 'indexed-array' and 'sub-type' in elem:
> -            if elem["sub-type"] in ['binary', 'nest', 'u32']:
> +            if elem["sub-type"] in ['binary', 'nest', 'u32', 'u8']:

[Severity: Medium]
Should this patch come before the spec change in the series?

The preceding commit cb06a9dc93de ("wifi: nl80211: add sta-info nested
attribute") already adds chain-signal and chain-signal-avg to
Documentation/netlink/specs/nl80211.yaml:

      -
        name: chain-signal
        type: indexed-array
        sub-type: u8

SpecAttrSet.__init__() in tools/net/ynl/pyynl/lib/nlspec.py calls
new_attr() for every attribute of every attribute set at spec parse time,
regardless of which ops or generation mode are requested:

    for elem in self.yaml['attributes']:
        ...
        attr = self.new_attr(elem, val)

so at the intermediate commit AttrSet.new_attr() takes the else branch and
raises 'new_attr: unsupported sub-type u8'.

nl80211 is part of GENS in tools/net/ynl/generated/Makefile, which globs all
specs and only filters out conntrack and nftables:

    SPECS_PATHS=$(wildcard $(SPECS_DIR)/*.yaml)
    GENS_UNSUP=conntrack nftables
    GENS=$(filter-out ${GENS_UNSUP},$(patsubst $(SPECS_DIR)/%.yaml,%,${SPECS_PATHS}))

Does that mean make -C tools/net/ynl and ynl-regen.sh fail for anyone who
lands on or bisects to cb06a9dc93de?  Reordering so the generator support
lands first, or squashing the two, would avoid that.

[Severity: Medium]
Does the scalar indexed-array path lose the entry index that
NL80211_STA_INFO_CHAIN_SIGNAL relies on?

Documentation/userspace-api/netlink/genetlink-legacy.rst describes the entry
attribute type as the index:

    The ``ENTRY`` nests are special and have the index of the entry as
    their type instead of normal attribute type.

nl80211_put_signal() keys each u8 on the chain number and skips chains that
are not in the mask, so the emitted index sequence can be sparse:

    for (i = 0; i < IEEE80211_MAX_CHAINS; i++) {
            if (!(mask & BIT(i)))
                    continue;
            if (nla_put_u8(msg, i, signal[i]))

Sparse masks do occur in practice, for example ath10k_htt_rx_h_signal()
sets each bit of status->chains independently based on per-chain RSSI
validity.

The generated parser for scalar sub-types in _multi_parse() stores entries
densely and drops ynl_attr_type(attr):

    elif aspec.sub_type in scalars:
        ri.cw.p(f"dst->{aspec.c_name}[i] = ynl_attr_get_{aspec.sub_type}(attr);")
    ri.cw.p('i++;')

So a message carrying chains 0 and 2 is reported as _count.chain_signal = 2
with the chain-2 value sitting at index 1, and TypeIndexedArray.attr_put()
would then re-serialize it as index 1:

    for (i = 0; i < {var}->_count.{c_name}; i++)
        ynl_attr_put_u8(nlh, i, {var}->{c_name}[i]);

YnlFamily._decode_array_attr() in tools/net/ynl/pyynl/lib/ynl.py has the
same loss since it appends item.as_scalar() to a plain list without
recording item.type.

The lossy path predates this patch, since only u32 indexed arrays were
allowed and the in-tree u32 user arp-ip-target is dense.  Is it worth
carrying the index alongside the value before routing nl80211 chain-signal
through this path, so per-chain values are not attributed to the wrong
chain?

>              else:
>                  raise Exception(f'new_attr: unsupported sub-type {elem["sub-type"]}')

[Severity: Medium]
One more thing on the spec added by the preceding commit of this series, in
Documentation/netlink/specs/nl80211.yaml.  The new eht-ru-alloc enum
declares the HE name prefix:

    name: eht-ru-alloc
    type: enum
    name-prefix: nl80211-rate-info-he-ru-alloc-

while include/uapi/linux/nl80211.h names these values with the EHT prefix:

    enum nl80211_eht_ru_alloc {
            NL80211_RATE_INFO_EHT_RU_ALLOC_26,
            NL80211_RATE_INFO_EHT_RU_ALLOC_52,
            NL80211_RATE_INFO_EHT_RU_ALLOC_52P26,

The same patch adds he-ru-alloc with that identical prefix:

    name: he-ru-alloc
    type: enum
    name-prefix: nl80211-rate-info-he-ru-alloc-

The generator builds the C enumerators straight from that prefix:

    self.value_pfx = yaml.get('name-prefix', f"{family.ident_name}-{yaml['name']}-")
    self.c_name = c_upper(self.enum_set.value_pfx + self.name)
    cw.p(entry.c_name + suffix)   /* render_uapi() */

so uapi generation from this spec emits
NL80211_RATE_INFO_HE_RU_ALLOC_26/52/106/242/484/2x996 twice, and the
EHT-only entries (52p26, 106p26, 484p242, 996p484, 996p484p242, 2x996p484,
3x996, 3x996p484, 4x996) come out under NL80211_RATE_INFO_HE_RU_ALLOC_*
names that do not exist in the UAPI header.  No in-tree build runs
--mode uapi for nl80211 today, so nothing breaks right now.

Should the eht-ru-alloc prefix be nl80211-rate-info-eht-ru-alloc- instead?