ath10k: Fix reported HT MCS rates with NSS > 1
diff mbox

Message ID 20170511090930.18205-1-sven.eckelmann@openmesh.com
State Accepted
Commit c1dd8016ae02557e4f3fcf7339865924d9357f76
Delegated to: Kalle Valo
Headers show

Commit Message

Sven Eckelmann May 11, 2017, 9:09 a.m. UTC
The QCA4019 firmware 10.4-3.2.1-00050 reports only HT MCS rates between
0-9. But 802.11n MCS rates can be larger than that. For example a 2x2
device can send with up to MCS 15.

The firmware encodes the higher MCS rates using the NSS field. The actual
calculation is not documented by QCA but it seems like the NSS field can be
mapped for HT rates to following MCS offsets:

 * NSS 1: 0
 * NSS 2: 8
 * NSS 3: 16
 * NSS 4: 24

This offset therefore has to be added for HT rates before they are stored
in the rate_info struct.

Fixes: cec17c382140 ("ath10k: add per peer htt tx stats support for 10.4")
Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Arend van Spriel May 11, 2017, 9:39 a.m. UTC | #1
On 11-5-2017 11:09, Sven Eckelmann wrote:
> The QCA4019 firmware 10.4-3.2.1-00050 reports only HT MCS rates between
> 0-9. But 802.11n MCS rates can be larger than that. For example a 2x2
> device can send with up to MCS 15.
> 
> The firmware encodes the higher MCS rates using the NSS field. The actual
> calculation is not documented by QCA but it seems like the NSS field can be
> mapped for HT rates to following MCS offsets:
> 
>  * NSS 1: 0
>  * NSS 2: 8
>  * NSS 3: 16
>  * NSS 4: 24
> 
> This offset therefore has to be added for HT rates before they are stored
> in the rate_info struct.
> 
> Fixes: cec17c382140 ("ath10k: add per peer htt tx stats support for 10.4")
> Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
> ---
>  drivers/net/wireless/ath/ath10k/htt_rx.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index 84b6067ff6e7..6c0a821fe79d 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -2229,9 +2229,15 @@ ath10k_update_per_peer_tx_stats(struct ath10k *ar,
>  	txrate.mcs = ATH10K_HW_MCS_RATE(peer_stats->ratecode);
>  	sgi = ATH10K_HW_GI(peer_stats->flags);
>  
> -	if (((txrate.flags == WMI_RATE_PREAMBLE_HT) ||
> -	     (txrate.flags == WMI_RATE_PREAMBLE_VHT)) && txrate.mcs > 9) {
> -		ath10k_warn(ar, "Invalid mcs %hhd peer stats", txrate.mcs);
> +	if (txrate.flags == WMI_RATE_PREAMBLE_VHT && txrate.mcs > 9) {
> +		ath10k_warn(ar, "Invalid VHT mcs %hhd peer stats",  txrate.mcs);
> +		return;
> +	}

So you leave VHT as is. Did you check with 11ac device? I am wondering
if it needs the same change.

Regards,
Arend
Sven Eckelmann May 11, 2017, 10:08 a.m. UTC | #2
On Donnerstag, 11. Mai 2017 11:39:46 CEST Arend Van Spriel wrote:
[...]
> So you leave VHT as is. Did you check with 11ac device? I am wondering
> if it needs the same change.

VHT MCS rates are reported by drivers with NSS + MCS rates 0-9 [1] as 
separated values. So I would say that the code is correct to report them as 
separate values. But no, I haven't tested with an 802.11ac capable device 
which I can fully trust because I didn't have one in arms reach (and I would 
have to compile a new firmware for it). But the output for a second ath10k 
based device looks ok'ish:

    Station ac:86:74:61:6b:30 (on wlan1)
            inactive time:  700 ms
            rx bytes:       10741
            rx packets:     95
            tx bytes:       9886
            tx packets:     86
            tx retries:     0
            tx failed:      0
            rx drop misc:   1
            signal:         -22 dBm
            signal avg:     -21 dBm
            tx bitrate:     780.0 MBit/s VHT-MCS 8 80MHz short GI VHT-NSS 2
            rx bitrate:     780.0 MBit/s VHT-MCS 8 80MHz short GI VHT-NSS 2
            rx duration:    8172 us
            authorized:     yes
            authenticated:  yes
            associated:     yes
            preamble:       long
            WMM/WME:        yes
            MFP:            no
            TDLS peer:      no
            DTIM period:    2
            beacon interval:100
            short slot time:yes
            connected time: 106 seconds

Kind regards,
	Sven

[1] http://mcsindex.com/
Kalle Valo May 23, 2017, 3:28 p.m. UTC | #3
Sven Eckelmann <sven.eckelmann@openmesh.com> wrote:
> The QCA4019 firmware 10.4-3.2.1-00050 reports only HT MCS rates between
> 0-9. But 802.11n MCS rates can be larger than that. For example a 2x2
> device can send with up to MCS 15.
> 
> The firmware encodes the higher MCS rates using the NSS field. The actual
> calculation is not documented by QCA but it seems like the NSS field can be
> mapped for HT rates to following MCS offsets:
> 
>  * NSS 1: 0
>  * NSS 2: 8
>  * NSS 3: 16
>  * NSS 4: 24
> 
> This offset therefore has to be added for HT rates before they are stored
> in the rate_info struct.
> 
> Fixes: cec17c382140 ("ath10k: add per peer htt tx stats support for 10.4")
> Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>

Patch applied to ath-next branch of ath.git, thanks.

c1dd8016ae02 ath10k: fix reported HT MCS rates with NSS > 1
Sebastian Gottschall Nov. 5, 2017, 9:22 a.m. UTC | #4
the assumption made in this patch is obviously wrong (at least for more 
recent firmwares and 9984)
my log is flooded with messages like
[208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
[208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
[208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
[208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
[208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats


i tested this with the 10.4.3.5-0038 firmware which isnt official 
published but made from athwlan.bin i got from qca chipcode

Am 23.05.2017 um 17:28 schrieb Kalle Valo:
> Sven Eckelmann <sven.eckelmann@openmesh.com> wrote:
>> The QCA4019 firmware 10.4-3.2.1-00050 reports only HT MCS rates between
>> 0-9. But 802.11n MCS rates can be larger than that. For example a 2x2
>> device can send with up to MCS 15.
>>
>> The firmware encodes the higher MCS rates using the NSS field. The actual
>> calculation is not documented by QCA but it seems like the NSS field can be
>> mapped for HT rates to following MCS offsets:
>>
>>   * NSS 1: 0
>>   * NSS 2: 8
>>   * NSS 3: 16
>>   * NSS 4: 24
>>
>> This offset therefore has to be added for HT rates before they are stored
>> in the rate_info struct.
>>
>> Fixes: cec17c382140 ("ath10k: add per peer htt tx stats support for 10.4")
>> Signed-off-by: Sven Eckelmann <sven.eckelmann@openmesh.com>
> Patch applied to ath-next branch of ath.git, thanks.
>
> c1dd8016ae02 ath10k: fix reported HT MCS rates with NSS > 1
>
Sven Eckelmann Nov. 6, 2017, 8:23 a.m. UTC | #5
On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall wrote:
> the assumption made in this patch is obviously wrong (at least for more 
> recent firmwares and 9984)
> my log is flooded with messages like
> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> 
> 
> i tested this with the 10.4.3.5-0038 firmware which isnt official 
> published but made from athwlan.bin i got from qca chipcode

Hm, yes there is most likely something wrong. But not sure whether this patch 
is actually the culprit. It looks to me like this would also be reported the 
same way without the patch. cec17c382140 ("ath10k: add per peer htt tx stats 
support for 10.4") seems to be your problem, right?

This patch only splits WMI_RATE_PREAMBLE_HT & WMI_RATE_PREAMBLE_VHT. And for 
WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different approach. But 
the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is basically 
untouched.

Kind regards,
	Sven
Sebastian Gottschall Nov. 6, 2017, 8:28 a.m. UTC | #6
Am 06.11.2017 um 09:23 schrieb Sven Eckelmann:
> On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall wrote:
>> the assumption made in this patch is obviously wrong (at least for more
>> recent firmwares and 9984)
>> my log is flooded with messages like
>> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>
>>
>> i tested this with the 10.4.3.5-0038 firmware which isnt official
>> published but made from athwlan.bin i got from qca chipcode
> Hm, yes there is most likely something wrong. But not sure whether this patch
> is actually the culprit. It looks to me like this would also be reported the
> same way without the patch. cec17c382140 ("ath10k: add per peer htt tx stats
> support for 10.4") seems to be your problem, right?
>
> This patch only splits WMI_RATE_PREAMBLE_HT & WMI_RATE_PREAMBLE_VHT. And for
> WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different approach. But
> the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is basically
> untouched.

then a question follows up. is this check really neccessary?


Sebastian


>
> Kind regards,
> 	Sven
Sven Eckelmann Nov. 6, 2017, 9:02 a.m. UTC | #7
On Montag, 6. November 2017 09:28:42 CET Sebastian Gottschall wrote:
> Am 06.11.2017 um 09:23 schrieb Sven Eckelmann:
> > On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall wrote:
> >> the assumption made in this patch is obviously wrong (at least for more
> >> recent firmwares and 9984)
> >> my log is flooded with messages like
> >> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> >> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> >> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> >> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> >> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
[...]
> > This patch only splits WMI_RATE_PREAMBLE_HT & WMI_RATE_PREAMBLE_VHT. And for
> > WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different approach. But
> > the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is basically
> > untouched.
> 
> then a question follows up. is this check really neccessary?

Until we find out what the heck VHT MCS 15 should mean in this context - maybe.
But to the message - no, the message is most likely not necessary for each 
received "invalid" peer tx stat.

Kind regards.
	Sven
Peter Oh Nov. 6, 2017, 9:25 p.m. UTC | #8
On 11/06/2017 01:02 AM, Sven Eckelmann wrote:
> On Montag, 6. November 2017 09:28:42 CET Sebastian Gottschall wrote:
>> Am 06.11.2017 um 09:23 schrieb Sven Eckelmann:
>>> On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall wrote:
>>>> the assumption made in this patch is obviously wrong (at least for more
>>>> recent firmwares and 9984)
>>>> my log is flooded with messages like
>>>> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
> [...]
>>> This patch only splits WMI_RATE_PREAMBLE_HT & WMI_RATE_PREAMBLE_VHT. And for
>>> WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different approach. But
>>> the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is basically
>>> untouched.
>> then a question follows up. is this check really neccessary?
> Until we find out what the heck VHT MCS 15 should mean in this context - maybe.
> But to the message - no, the message is most likely not necessary for each
> received "invalid" peer tx stat.
This validation check expects peer tx stat packets from FW contain 
reasonable values and gives warning if values are different from 
expectation. The problem comes from the assumption that "it always 
contains reasonable stat value" which is wrong at least based on my 
results. For instance, the reason MCS == 15 is because all 4 bits of mcs 
potion set to 1, not because FW really sets it to 15 intentionally. when 
the mcs potion bits are set to all 1s, the other bits such as nss are 
also set to all 1s.
Hence it looks FW passed totally invalid stat info to me.
I don't have preference on whether it's better to split VHT and HT check 
or not, but it is more appropriate to change that warning level to debug 
level.
Kalle Valo Nov. 20, 2017, 12:10 p.m. UTC | #9
Peter Oh <peter.oh@bowerswilkins.com> writes:

> On 11/06/2017 01:02 AM, Sven Eckelmann wrote:
>> On Montag, 6. November 2017 09:28:42 CET Sebastian Gottschall wrote:
>>> Am 06.11.2017 um 09:23 schrieb Sven Eckelmann:
>>>> On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall wrote:
>>>>> the assumption made in this patch is obviously wrong (at least for more
>>>>> recent firmwares and 9984)
>>>>> my log is flooded with messages like
>>>>> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>>> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>>> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>>> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>>>>> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer stats
>> [...]
>>>> This patch only splits WMI_RATE_PREAMBLE_HT & WMI_RATE_PREAMBLE_VHT. And for
>>>> WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different approach. But
>>>> the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is basically
>>>> untouched.
>>> then a question follows up. is this check really neccessary?
>> Until we find out what the heck VHT MCS 15 should mean in this context - maybe.
>> But to the message - no, the message is most likely not necessary for each
>> received "invalid" peer tx stat.
>
> This validation check expects peer tx stat packets from FW contain
> reasonable values and gives warning if values are different from
> expectation. The problem comes from the assumption that "it always
> contains reasonable stat value" which is wrong at least based on my
> results. For instance, the reason MCS == 15 is because all 4 bits of
> mcs potion set to 1, not because FW really sets it to 15
> intentionally. when the mcs potion bits are set to all 1s, the other
> bits such as nss are also set to all 1s.
> Hence it looks FW passed totally invalid stat info to me.
>
> I don't have preference on whether it's better to split VHT and HT
> check or not, but it is more appropriate to change that warning level
> to debug level.

I think we should add a special case to not print the warning if mcs ==
15 until we figure out what it means.
Anilkumar Kolli Nov. 21, 2017, 6:13 a.m. UTC | #10
On 2017-11-20 17:40, Kalle Valo wrote:
> Peter Oh <peter.oh@bowerswilkins.com> writes:
> 
>> On 11/06/2017 01:02 AM, Sven Eckelmann wrote:
>>> On Montag, 6. November 2017 09:28:42 CET Sebastian Gottschall wrote:
>>>> Am 06.11.2017 um 09:23 schrieb Sven Eckelmann:
>>>>> On Sonntag, 5. November 2017 10:22:22 CET Sebastian Gottschall 
>>>>> wrote:
>>>>>> the assumption made in this patch is obviously wrong (at least for 
>>>>>> more
>>>>>> recent firmwares and 9984)
>>>>>> my log is flooded with messages like
>>>>>> [208802.803537] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer 
>>>>>> stats
>>>>>> [208805.108515] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer 
>>>>>> stats
>>>>>> [208821.747621] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer 
>>>>>> stats
>>>>>> [208822.516599] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer 
>>>>>> stats
>>>>>> [208841.257780] ath10k_pci 0001:03:00.0: Invalid VHT mcs 15 peer 
>>>>>> stats
>>> [...]
>>>>> This patch only splits WMI_RATE_PREAMBLE_HT & 
>>>>> WMI_RATE_PREAMBLE_VHT. And for
>>>>> WMI_RATE_PREAMBLE_HT (*not VHT*), it uses a slightly different 
>>>>> approach. But
>>>>> the WMI_RATE_PREAMBLE_VHT part (which you see in your logs) is 
>>>>> basically
>>>>> untouched.
>>>> then a question follows up. is this check really neccessary?
>>> Until we find out what the heck VHT MCS 15 should mean in this 
>>> context - maybe.
>>> But to the message - no, the message is most likely not necessary for 
>>> each
>>> received "invalid" peer tx stat.
>> 
>> This validation check expects peer tx stat packets from FW contain
>> reasonable values and gives warning if values are different from
>> expectation. The problem comes from the assumption that "it always
>> contains reasonable stat value" which is wrong at least based on my
>> results. For instance, the reason MCS == 15 is because all 4 bits of
>> mcs potion set to 1, not because FW really sets it to 15
>> intentionally. when the mcs potion bits are set to all 1s, the other
>> bits such as nss are also set to all 1s.
>> Hence it looks FW passed totally invalid stat info to me.
>> 
>> I don't have preference on whether it's better to split VHT and HT
>> check or not, but it is more appropriate to change that warning level
>> to debug level.
> 
> I think we should add a special case to not print the warning if mcs ==
> 15 until we figure out what it means.

Fix identified in Firmware and will push ASAP.

--
Anil.
Sven Eckelmann Dec. 5, 2017, 9:16 a.m. UTC | #11
On Dienstag, 21. November 2017 11:43:36 CET akolli@codeaurora.org wrote:
[...]
> > I think we should add a special case to not print the warning if mcs ==
> > 15 until we figure out what it means.
> 
> Fix identified in Firmware and will push ASAP.

Is it known when this will be released and for which firmware/HW versions? And 
will this also fix the legacy rate reports? At least in the moment, 
ath10k_htt_fetch_peer_stats seems to generate following on QCA4019 for some 
clients:

    [ 1627.720177] ath10k_ahb a800000.wifi: Invalid legacy rate 26 peer stats
    [ 1632.010290] ath10k_ahb a800000.wifi: Invalid legacy rate 26 peer stats

Kind regards,
	Sven

Patch
diff mbox

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 84b6067ff6e7..6c0a821fe79d 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -2229,9 +2229,15 @@  ath10k_update_per_peer_tx_stats(struct ath10k *ar,
 	txrate.mcs = ATH10K_HW_MCS_RATE(peer_stats->ratecode);
 	sgi = ATH10K_HW_GI(peer_stats->flags);
 
-	if (((txrate.flags == WMI_RATE_PREAMBLE_HT) ||
-	     (txrate.flags == WMI_RATE_PREAMBLE_VHT)) && txrate.mcs > 9) {
-		ath10k_warn(ar, "Invalid mcs %hhd peer stats", txrate.mcs);
+	if (txrate.flags == WMI_RATE_PREAMBLE_VHT && txrate.mcs > 9) {
+		ath10k_warn(ar, "Invalid VHT mcs %hhd peer stats",  txrate.mcs);
+		return;
+	}
+
+	if (txrate.flags == WMI_RATE_PREAMBLE_HT &&
+	    (txrate.mcs > 7 || txrate.nss < 1)) {
+		ath10k_warn(ar, "Invalid HT mcs %hhd nss %hhd peer stats",
+			    txrate.mcs, txrate.nss);
 		return;
 	}
 
@@ -2254,7 +2260,7 @@  ath10k_update_per_peer_tx_stats(struct ath10k *ar,
 		arsta->txrate.legacy = rate;
 	} else if (txrate.flags == WMI_RATE_PREAMBLE_HT) {
 		arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
-		arsta->txrate.mcs = txrate.mcs;
+		arsta->txrate.mcs = txrate.mcs + 8 * (txrate.nss - 1);
 	} else {
 		arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
 		arsta->txrate.mcs = txrate.mcs;