Re: ath12k: handling of HE and EHT capabilities

[Johannes Berg]

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.

>     struct ieee80211_sta_ht_cap {
>         u16 cap; /* use IEEE80211_HT_CAP_ */
>         bool ht_supported;
>         u8 ampdu_factor;
>         u8 ampdu_density;
>         struct ieee80211_mcs_info mcs;
>     };
> 
>     struct ieee80211_sta_vht_cap {
>         bool vht_supported;
>         u32 cap; /* use IEEE80211_VHT_CAP_ */
>         struct ieee80211_vht_mcs_info vht_mcs;
>     };
> 
> The structs for HT and VHT use `u16` and `u32` data types for the `cap`
> variable, matching what `iw` does. That part is consistent.

Careful. There are different structs used in different places, notably
HT/VHT and HE/EHT differ.

For HT and VHT, look at the start of nl80211_send_band_rateinfo(), which
sends themas individual attributes, defined in enum nl80211_band_attr,
and the values that are u16 (NL80211_BAND_ATTR_HT_CAPA) or u32
(NL80211_BAND_ATTR_VHT_CAPA) are in host byte order, though both are
actually documented misleadingly ("as in [V]HT information IE" is just
all around wrong.)

For HE/EHT, you have it in nl80211_send_iftype_data() since it's per
interface type, and all the individual values are just as they appear in
the spec, regardless of their size.

Note that spec is generally in little endian, but sometimes has strange
field lengths like MAC capabilities being 6 bytes in HE:

>     struct ieee80211_he_cap_elem {
>         u8 mac_cap_info[6];
>         u8 phy_cap_info[11];
>     } __packed;
> 
>     struct ieee80211_he_6ghz_capa {
>         /* uses IEEE80211_HE_6GHZ_CAP_* below */
>         __le16 capa; }
>     __packed;
> 
> However, for HE the types differ from the `iw` implementation. Here, `u8`
> arrays are used instead of `u16` for MAC and PHY capabilities. The 6 GHz
> capabilities use `u16`, which is also different.

That doesn't really matter, they're just a set of 6 or 11 bytes, and
e.g. the HE MAC capabilities are treated by the kernel as a set of 6
bytes, but by iw as a set of 3 __le16, which results in the same
interpretation, or at least should.

>     struct ieee80211_eht_cap_elem_fixed {
>         u8 mac_cap_info[2];
>         u8 phy_cap_info[9];
>     } __packed;
> 
> For EHT, `u8` arrays are also used for both MAC and PHY caps, instead of
> `u32` for the PHY caps as in the `iw` implementation.

Same thing here.

> The current `ath12k` implementation always uses `u32` values, which does
> not work on big‑endian platforms:

Yeah, that seems problematic and not really fitting for something that's
6, 11, 2 or 9 bytes long?

> 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?

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.

johannes