| Message ID | 20170511090930.18205-1-sven.eckelmann@openmesh.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c1dd8016ae02557e4f3fcf7339865924d9357f76 |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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/
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
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 >
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
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
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
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.
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.
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.
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
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;
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(-)