GNOME Bugzilla – Bug 779771
nmcli dev wifi list always shows the RATE as 54 Mbit/s for .11n APs
Last modified: 2017-04-10 11:43:32 UTC
Created attachment 347504 [details] [review] Possible fix The output of the command nmcli dev wifi list always shows the RATE for .11n APs as 54 Mbit/s even though the AP is operating at .11n rates.
Hi, thanks for the patch. It didn't apply on master and had some style issues. So, I pushed the branch for review to https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/wifi-ap-max-rate-bgo779771 Do you have a link to some reference manuals for the numbers and format of ICs? in nm_wifi_ap_update_from_properties(), nm_wifi_ap_set_max_bitrate() is set from "Rates" and "IEs". How do they go together? I think if Rates is present, the "IEs" property should be ignored? get_max_rate() may read unaligned integers. Should use unaligned_read_ne16(), unaligned_read_ne32(). get_max_rate() doesn't check that the remaining elem_len is large enough. What is the documentation for the content of this binary blob? get_max_rate() loops over the IEs and may set max_rate multiple times. I guess, you want to keep the max there? max_rate = NM_MAX (max_rate, get_max_ht_rate (*bytes, bytes+3)); In get_max_vht_rate(), mcs can only be one of 7,8,9. Why do the get_max_vht*() handle any additional values? how get_max_ht_rate() determines mcs seems wrong. Why is mcs initialized as -1? (note that the variable is unsigned). Also, you may shift Can you link to documentation for this format?
Thanks for the changes. The documentation I used is listed below. I sort of cobble together a few different sites to get the info that I needed. My understand of the issues you listed are as follows: Yes, the point is the get the maximum data rate supported by the AP. I think the Rates code does the same thing. The rates from the IEs take precedence over the rates from Rates(which are only .11abg rates). The rates from the IEs are 11n and 11ac data rates, so the bigger the better. The vht rates correspond to .11ac and the standard makes it mandatory to support mcs rates 0-7. Thus, are options are 0-7,8, and 9. We'll default to 7. I used -1 to initialize mcs because mcs0 is a valid rate so the first iteration of the loop would enable mcs0 as a rate. Ooops, bad coding on my part. :( I used these links to figure out the format: ht capabilities: https://mrncciew.com/2014/10/19/cwap-ht-capabilities-ie/ vht capabilities: http://chimera.labs.oreilly.com/books/1234000001739/ch03.html 11n rates: https://en.wikipedia.org/wiki/IEEE_802.11n-2009 11ac rates: https://en.wikipedia.org/wiki/IEEE_802.11ac
/* Find the maximum supported mcs rate */ mcs = -1; for (i = 0; i <= 15; i++) { for (j = 0; j <= 7; j++) { if (*supported_mcs_set & (1 << j)) mcs++; } supported_mcs_set++; } This looks wrong, only the first 77 bits should be taken into account. For example, iw does: for (mcs_bit = 0; mcs_bit <= 76; mcs_bit++) { unsigned int mcs_octet = mcs_bit/8; unsigned int MCS_RATE_BIT = 1 << mcs_bit % 8; bool mcs_rate_idx_set; mcs_rate_idx_set = !!(mcs[mcs_octet] & MCS_RATE_BIT); Also, the code counts the number of bits set, while it should find the rate associated to each MCS with bit set and return the maximum. +get_max_rate_ht (const guint8 *bytes, guint len, guint32 *out_maxrate) ... + if (len != 28) + return FALSE; 28 is the total length of TLV, here it should be 26. +get_max_rate_vht (const guint8 *bytes, guint len, guint32 *out_maxrate) ... + if (len != 16) + return FALSE; I think this should be 12.
fixed the length checks and rebased+repushed branch th/wifi-ap-max-rate-bgo779771 I agree, it needs more work as I think the parsing is not correct... (patches welcome, but preferably on top of th/wifi-ap-max-rate-bgo779771).
I've adjusted the parsing to only go through the first 77 bits of the supported mcs rate field. Basically, I modified to use the same method the iw code uses. Testing on my infrastructure looks good: # nmcli dev wifi list * SSID MODE CHAN RATE SIGNAL BARS SECURITY wfa11 Infra 1 195 Mbit/s 100 **** WPA1 ccopen Infra 1 195 Mbit/s 100 **** JRA-1250-PSK Infra 149 270 Mbit/s 100 **** WPA2 LT_Guest Infra 153 54 Mbit/s 100 **** wfa17 Infra 11 54 Mbit/s 97 **** WEP wfa21 Infra 11 130 Mbit/s 94 **** WPA1 WPA2 WEPtest Infra 11 54 Mbit/s 94 **** WEP wfa19 Infra 48 270 Mbit/s 89 **** WPA2 802.1X wfa18 Infra 48 54 Mbit/s 89 **** WPA1 802.1X atwpa2-psk-aes Infra 48 270 Mbit/s 87 **** WPA2 wfa21 Infra 48 270 Mbit/s 87 **** WPA1 WPA2 nps_aes Infra 48 270 Mbit/s 87 **** WPA2 802.1X WEPtest Infra 48 54 Mbit/s 87 **** WEP hostapd5 Infra 48 270 Mbit/s 85 **** WPA2 802.1X wfa17 Infra 48 54 Mbit/s 85 **** WEP -- Infra 11 130 Mbit/s 84 **** WPA2 wfa7 Infra 36 54 Mbit/s 84 **** WPA1 test Infra 5 13 Mbit/s 82 **** wfa11 Infra 36 405 Mbit/s 82 **** WPA1 ccopen Infra 36 405 Mbit/s 82 **** wfa10 Infra 36 405 Mbit/s 82 **** WPA2 802.1X TestAP136 Infra 6 130 Mbit/s 80 *** wfa12 Infra 36 405 Mbit/s 80 *** WPA1 802.1X DR Infra 8 54 Mbit/s 77 *** WPA2 test Infra 48 13 Mbit/s 74 *** ca_test Infra 157 270 Mbit/s 67 *** WPA2 wfa9 Infra 1 54 Mbit/s 64 *** WPA2 reg_test0 Infra 165 270 Mbit/s 64 *** wfa10 Infra 157 405 Mbit/s 62 *** WPA2 802.1X reg_test1 Infra 5 130 Mbit/s 60 *** WPA2 ccopen Infra 104 270 Mbit/s 59 *** wfa20 Infra 104 270 Mbit/s 59 *** WPA1 WPA2 802.1X wfa18 Infra 104 54 Mbit/s 59 *** WPA1 802.1X wfa12 Infra 157 405 Mbit/s 59 *** WPA1 802.1X wfa2 Infra 157 54 Mbit/s 59 *** WEP chariot Infra 1 130 Mbit/s 57 *** WPA2 802.1X PE15n Infra 1 54 Mbit/s 57 *** WPA2 802.1X I'll attach the patch I made to th/wifi-ap-max-rate-bgo779771
Created attachment 348817 [details] [review] Patch for parsing supported mcs set
(In reply to James Kalb from comment #6) > Created attachment 348817 [details] [review] [review] > Patch for parsing supported mcs set Thanks. I added your patch to branch: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/wifi-ap-max-rate-bgo779771 and pushed a couple of commits to fix how the max rate is computed. For VHT, I think we can use the "highest rate" field of IE instead of manually decoding the MCS, but I couldn't test it. Once this is reviewed, probably we should squash all commits into a single one because there are too many "fix-previous-patch" commits making the history hard to follow.
My original implementation was to use the "highest rate" field. However, that field is not a required field in the IE so it could be easily ignored and forgotten. Decoding the MCS was a surefire way to obtain the max rate. I think the "highest rate" field is a much better/simpler solution, but I'm unsure how much it will be used in a real world setting. I say this because it wasn't set in the infrastructure I've been testing against. That may or may not be reflective of normal situations.
(In reply to James Kalb from comment #8) > My original implementation was to use the "highest rate" field. However, > that field is not a required field in the IE so it could be easily ignored > and forgotten. Decoding the MCS was a surefire way to obtain the max rate. > I think the "highest rate" field is a much better/simpler solution, but I'm > unsure how much it will be used in a real world setting. I say this because > it wasn't set in the infrastructure I've been testing against. Makes sense, I reverted the patch. Now the branch looks good to me.
Merged to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=608ac31e87673f4e82ce5abca195551f6f48d571 Thanks!