openwrt / iwinfo

[MIRROR] Library for accessing wireless device drivers
https://git.openwrt.org/?p=project/iwinfo.git;
GNU General Public License v2.0
5 stars 13 forks source link

iwinfo: add basic IEEE 802.11be support #10

Open cmonroe opened 2 months ago

cmonroe commented 2 months ago

Add basic support for 802.11be / WiFi 7 to iwinfo.

cmonroe commented 2 months ago

Hi @jow-

Thanks for looking this over! I think I got all of your requested changes in the most recent push if you don't mind checking them. I tested that 20, 80, 160 and 320MHz bandwidths are still reported correctly via the CLI and ubus. In order for assoclist to display 320MHz correctly via ubus I've updated rpcd #5 as well.

Two questions, mostly for my own understanding if you have the time to share:

  1. Is adding the is_eht flag in the middle of iwinfo_rate_entry OK because all of the defined bitfields end up in the same byte?
  2. Does adding a new element not require an ABI change? The rpcd PR mentioned above will make rpcd reliant upon presence of mhz_hi and I wanted to make sure this is OK.
jow- commented 2 months ago

Hi,

Is adding the is_eht flag in the middle of iwinfo_rate_entry OK because all of the defined bitfields end up in the same byte?

Yes. The is_eht field is a single-bit bitfield member (due to the :1 suffix). Adjacent bitfield struct members with the same base type are packed into the same value, so a

struct {
  uint8_t foo:1;
  uint8_t bar:1;
  uint8_t baz:1;
}

... ends up being a struct with one byte holding three bits, while in contrast a

struct {
  uint8_t foo:1;
  uint8_t bar:1;
  bool something_else;
  uint8_t baz:1;
}

... ends up being three bytes large (first a byte holding two bits, then a byte holding a boolean 1 or 0 value, then another byte holding one bit).

Does adding a new element not require an ABI change?

It depends, in this particular case the structure size and layout did not change, so existing code built against an older libiwinfo loading a newer one will continue to work. It might not be able to make sense of the new values (e.g. wrongly report "64 MHz" for a 320 wide channel) but it will not crash trying to access nonexistent/invalid memory when the structures returned by iwinfo are larger than what the program expects.

The reason why we need to be somewhat careful with the ABI here is that some programs (e.g. LuCI) dlopen() libiwinfo at runtime, so the usual ABI handling mechanics in packages do not apply.

Here's my local testcase for the structure size:

@j7:~$ cat /tmp/structsize.c 
#include <stdint.h>
#include <stdio.h>

struct iwinfo_rate_entry_old {
    uint32_t rate;
    int8_t mcs;
    uint8_t is_40mhz:1;
    uint8_t is_short_gi:1;
    uint8_t is_ht:1;
    uint8_t is_vht:1;
    uint8_t is_he:1;
    uint8_t he_gi;
    uint8_t he_dcm;
    uint8_t mhz;
    uint8_t nss;
};

struct iwinfo_rate_entry_new {
    uint32_t rate;
    int8_t mcs;
    uint8_t is_40mhz:1;
    uint8_t is_short_gi:1;
    uint8_t is_ht:1;
    uint8_t is_vht:1;
    uint8_t is_he:1;
    uint8_t is_eht:1;
    uint8_t he_gi;
    uint8_t he_dcm;
    uint8_t mhz_lo;
    uint8_t nss;
        uint8_t mhz_hi;
        uint8_t eht_gi;
};

int main(int argc, char **argv) {
    printf("%zd, %zd\n",
        sizeof(struct iwinfo_rate_entry_old),
        sizeof(struct iwinfo_rate_entry_new));

    return 0;
}
jow@j7:~$ gcc -o /tmp/structsize /tmp/structsize.c && /tmp/structsize
12, 12
jow@j7:~$

The reason why it was possible in this case is that the original struct had two unused alignment padding bytes at the end.

cmonroe commented 2 months ago

Thanks for the super clear info, it is very helpful :)

The ABI change part makes a lot of sense now.. I was thinking about the problem backwards before your explanation.

Please let me know if any other changes are needed and thanks again for your help!