Re: ath12k: handling of HE and EHT capabilities

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 2026-03-12 at 11:53 +0100, Alexander Wilhelm wrote:
> On Thu, Mar 12, 2026 at 10:37:46AM +0100, Johannes Berg wrote:
> > Hi,
> > > For example, I use the `iw` tool to display the capabilities and their
> > > descriptions. The code for that has the following function prototypes:
> > > 
> > >     * void print_ht_capability(__u16 cap);
> > >     * void print_vht_info(__u32 capa, const __u8 *mcs);
> > >     * static void __print_he_capa(const __u16 *mac_cap,
> > >                                   const __u16 *phy_cap,
> > >                                   const __u16 *mcs_set, size_t mcs_len,
> > >                                   const __u8 *ppet, int ppet_len,
> > >                                   bool indent);
> > >     * static void __print_eht_capa(int band,
> > >                                    const __u8 *mac_cap,
> > >                                    const __u32 *phy_cap,
> > >                                    const __u8 *mcs_set, size_t mcs_len,
> > >                                    const __u8 *ppet, size_t ppet_len,
> > >                                    const __u16 *he_phy_cap,
> > >                                    bool indent);
> > 
> > This is perhaps a bit unfortunate, but note that the HE and EHT __u16
> > and __u32 here are really little endian pointers, and the functions do
> > byte-order conversion.
> 
> I don’t see this in the function. For example, the MAC capabilities are a
> `u16 *` in CPU endianness, which is simply memcpy’d from the parsed
> `NL80211_BAND_IFTYPE_ATTR_HE_CAP_MAC`. Later, they are treated as `u16 *`,
> as shown in the following code:
> 
>     printf("%s\t\tHE MAC Capabilities (0x", pre);
>     for (i = 0; i < 3; i++)
>         printf("%04x", mac_cap[i]);
>     printf("):\n");
> 
> Here is the result on little‑ vs. big‑endian platforms:
> 
>     Little endian:
>     HE MAC Capabilities (0x081a010d030f):
> 
>     Big endian:
>     HE MAC Capabilities (0x0b00189a4010):

Oh, OK, so _that_ print is definitely wrong in iw. But the individual
prints are converted:

        #define PRINT_HE_CAP(_var, _idx, _bit, _str) \
        do { \
                if (le16toh(_var[_idx]) & BIT(_bit)) \
                        printf("%s\t\t\t" _str "\n", pre); \
        } while (0)

> For the PHY capabilities, they are also a `u16 *`, but they are treated as
> a `u8 *`. However, later in the description printing, `PRINT_HE_CAP` does
> not take endianness into account.

PRINT_HE_CAP *does* convert, there's le16toh() there. Same in
PRINT_HE_CAP_MASK.

It should convert, because it's from a u8[6] kernel API that just
carries the values as they are in the spec.

> > > I want to address and fix this issue. However, I cannot apply the “never
> > > break the userspace” rule here, as it seems, it is already broken.
> > 
> > I don't think it's broken, why do you say so?

Well, I see now that I missed the

        printf("%s\t\tHE MAC Capabilities (0x", pre);
        for (i = 0; i < 3; i++)
                printf("%04x", le16toh(mac_cap[i]));

and

        printf("%s\t\tEHT MAC Capabilities (0x", pre);
        for (i = 0; i < 2; i++)
                printf("%02x", mac_cap[i]);

parts, those are definitely broken in iw on big endian platforms. We
should fix those in iw. The actual individual prints seem fine though.

> Well, if `ath12k` uses `u32` in CPU‑native order, that’s a bug, and I can’t
> get `ieee80211_hw` registered. If I use `__le32` in little-endian order
> instead, I end up with incorrect capabilities and mismatched descriptions
> shown by the `iw` tool (but I can get the driver running). So neither
> approach seems to be a 100% solution at first glance. Did I misinterpret
> the rule?

The *descriptions* should be fine I think? Just the first line with the
hex would be messed up.

> > What's (clearly) broken is how ath12k puts the data into the HE/EHT
> > structs that the kernel expects, but per your dmesg:
> > 
> > >     ath12k_pci 0001:01:00.0: ieee80211 registration failed: -22
> > >     ath12k_pci 0001:01:00.0: failed register the radio with mac80211: -22
> > 
> > it seems that even mac80211 doesn't like the capabilities, so the byte
> > order issue already exists there.
> > 
> > It seems to me the issue is that ath12k_band_cap is in u32, converted,
> > but then memcpy()d.
> 
> The `ath12k` driver uses `u32` arrays in CPU‑native order for this, so the
> swap is effectively happening.

Yeah but the swap is wrong, since HE/EHT capabilities are just byte
arrays in spec byte order in cfg80211/nl80211.

>  Later, in `ieee80211_register_hw`, the
> values are compared at the bit level, and that’s where it fails. I
> understand that technically `__le32` could be used in `ath12k`, meaning no
> swap, but since `u8` arrays are used in `cfg80211`, that might actually be
> the better approach.

Sure, could use u8 in ath12k too, dunno, up to the maintainer. At least
if it was __le32 you could still memcpy() from it since no swap
happened, and wouldn't change the code structure that much.

johannes