| Message ID | 20180807020813.GA185137@ban.mtv.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ath10k: antenna bitmask support? | expand |
Am 07.08.2018 um 04:08 schrieb Brian Norris: > Hi all, > > I'm looking at ancient changes like this: > > commit 5572a95b4b5768187652a346356e39e7542ca6e0 > Author: Ben Greear <greearb@candelatech.com> > Date: Mon Nov 24 16:22:10 2014 +0200 > > ath10k: apply chainmask settings to vdev on creation > > since I see that some firmwares seem to crash a lot if you apply certain > chainmask settings (e.g., 0x2). > > Is it really expected that you can set gaps in the chainmask or not? I > ask because I've been trying to use this for doing some antenna > configuration verification (e.g., disable all but one antenna and see > what happens), and this works as expected on APs I have running IPQ8064 > (QCA988X?), but it crashes the firmware on IPQ4019. > > I similarly wonder about this opinionated statement in > ath10k_check_chain_mask(): > > /* It is not clear that allowing gaps in chainmask > * is helpful. Probably it will not do what user > * is hoping for, so warn in that case. > */ > > It seems like we should reject unexpected values (-EINVAL) if they're > really not supported. But then, I've found differences depending on the > chipset it would seem, so I guess I'd have to figure out which chipsets > (or firmwares?) support which features... > > On a related note, if we *do* support "gaps" in these bitmasks, wouldn't > that mean we're handling 0x6, 0xa, etc., incorrectly in > get_nss_from_chainmask()? My prototype patch (untested so far) would be: > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index 836e0a47b94a..3b557d3dc036 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -19,6 +19,7 @@ > #include "mac.h" > > #include <net/mac80211.h> > +#include <linux/bitops.h> > #include <linux/etherdevice.h> > #include <linux/acpi.h> > > @@ -4905,13 +4906,7 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed) > > static u32 get_nss_from_chainmask(u16 chain_mask) > { > - if ((chain_mask & 0xf) == 0xf) > - return 4; > - else if ((chain_mask & 0x7) == 0x7) > - return 3; > - else if ((chain_mask & 0x3) == 0x3) > - return 2; > - return 1; > + return max_t(u32, hweight16(chain_mask & 0xf), 1); > } > > static int ath10k_mac_set_txbf_conf(struct ath10k_vif *arvif) > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k from what i know, gaps will not work. so valid masks are only like 0x1, 0x3, 0x7, 0xf. so same behaviour as for ath9k
Hi Sebastian, On Mon, Aug 6, 2018 at 9:11 PM Sebastian Gottschall <s.gottschall@dd-wrt.com> wrote: > > Am 07.08.2018 um 04:08 schrieb Brian Norris: ... > > Is it really expected that you can set gaps in the chainmask or not? I > > ask because I've been trying to use this for doing some antenna > > configuration verification (e.g., disable all but one antenna and see > > what happens), and this works as expected on APs I have running IPQ8064 > > (QCA988X?), but it crashes the firmware on IPQ4019. ... > from what i know, gaps will not work. so valid masks are only like 0x1, > 0x3, 0x7, 0xf. so same behaviour as for ath9k Thanks for your response. The thing is, masks like 0x2 and 0x4 *do* appear to work for IPQ8064, as I noted above. Let me elaborate. I tested with a conductively-wired setup, where antennas are wired directly from an AP to a client (with reasonable attenuation), only 2 of the 3 AP antennas are connected, and the client supports reporting signal strength on a per-antenna basis. If I set the AP's mask to 0x1, I see strong signal only on the client's antenna 1; if set to 0x2, I see strong signal only on the client's antenna 2; and if I set it to 0x4, I see only a very weak signal (presumably over the air, even without any antenna). In other words, I think this clearly works for some chipsets. I just wonder if anybody knows anything about why it does or doesn't work on a give chipset. I also acknowledge that while the firmware may work properly, ath10k may not always account for this properly (hence, proposals like the diff in the previous email). I see several occasions where ath10k does a simple integer greater/less-than comparison between 1 and cfg_{tx,rx}_chainmask, which seems wrong. But my question is more geared toward firmware and hardware support; fixing drivers is relatively easy ;) Regards, Brian
Hello Brian Am 07.08.2018 um 07:01 schrieb Brian Norris: > Thanks for your response. The thing is, masks like 0x2 and 0x4 *do* > appear to work for IPQ8064, as I noted above. Let me elaborate. > > I tested with a conductively-wired setup, where antennas are wired > directly from an AP to a client (with reasonable attenuation), only 2 > of the 3 AP antennas are connected, and the client supports reporting > signal strength on a per-antenna basis. If I set the AP's mask to 0x1, > I see strong signal only on the client's antenna 1; if set to 0x2, I > see strong signal only on the client's antenna 2; and if I set it to > 0x4, I see only a very weak signal (presumably over the air, even > without any antenna). > > In other words, I think this clearly works for some chipsets. I just > wonder if anybody knows anything about why it does or doesn't work on > a give chipset. i dont think so. even if you see a signal on it i'm pretty sure that the rate control algorithm within the firmware will simply fail and gets out of control due the way the rate control algorithm works and handles the chain controls. depending on the rate the rate control algorithm will select chains to transmit. so i assume if you set it to 0x2, all rates which are supported by 1x1 only, will not work anymore. so 1x1 clients simply wont work anymore. and then there are several other cases like vht160, which always uses chain 1 and 2 even if 4x4 is selected. so if you configure such a setup i expect nothing more than a crash in the firmware on 9984 chipsets for instance but of course these are assumptions from the firmware code i know and no proof. Sebastian > > I also acknowledge that while the firmware may work properly, ath10k > may not always account for this properly (hence, proposals like the > diff in the previous email). I see several occasions where ath10k does > a simple integer greater/less-than comparison between 1 and > cfg_{tx,rx}_chainmask, which seems wrong. But my question is more > geared toward firmware and hardware support; fixing drivers is > relatively easy ;) > > Regards, > Brian >
On 08/06/2018 07:08 PM, Brian Norris wrote: > Hi all, > > I'm looking at ancient changes like this: > > commit 5572a95b4b5768187652a346356e39e7542ca6e0 > Author: Ben Greear <greearb@candelatech.com> > Date: Mon Nov 24 16:22:10 2014 +0200 > > ath10k: apply chainmask settings to vdev on creation > > since I see that some firmwares seem to crash a lot if you apply certain > chainmask settings (e.g., 0x2). > > Is it really expected that you can set gaps in the chainmask or not? I > ask because I've been trying to use this for doing some antenna > configuration verification (e.g., disable all but one antenna and see > what happens), and this works as expected on APs I have running IPQ8064 > (QCA988X?), but it crashes the firmware on IPQ4019. I don't think you can even directly set the chainmask in the firmware, and from my previous looking at the firmware, it is not designed to support gaps. If you want to determine if an antenna is working, you can look at the per-chain RSSI stats when receiving frames using all available antenna (ie, 3x3 rates for a 3x3 NIC). Thanks, Ben
Hi Ben, On Tue, Aug 07, 2018 at 06:19:22AM -0700, Ben Greear wrote: > On 08/06/2018 07:08 PM, Brian Norris wrote: > > Hi all, > > > > I'm looking at ancient changes like this: > > > > commit 5572a95b4b5768187652a346356e39e7542ca6e0 > > Author: Ben Greear <greearb@candelatech.com> > > Date: Mon Nov 24 16:22:10 2014 +0200 > > > > ath10k: apply chainmask settings to vdev on creation > > > > since I see that some firmwares seem to crash a lot if you apply certain > > chainmask settings (e.g., 0x2). > > > > Is it really expected that you can set gaps in the chainmask or not? I > > ask because I've been trying to use this for doing some antenna > > configuration verification (e.g., disable all but one antenna and see > > what happens), and this works as expected on APs I have running IPQ8064 > > (QCA988X?), but it crashes the firmware on IPQ4019. > > I don't think you can even directly set the chainmask in the firmware, Well, I guess the WMI interface is a bit strange then: it appears to take a raw mask -- e.g.: ret = ath10k_wmi_pdev_set_param(ar, ar->wmi.pdev_param->tx_chain_mask, tx_ant); but I guess that doesn't mean it *really* accepts a mask? You have to presuppose what values it will accept? > and from my previous looking at the firmware, it is not designed to support > gaps. OK, thanks for the info. > If you want to determine if an antenna is working, you can look at the per-chain > RSSI stats when receiving frames using all available antenna (ie, 3x3 rates for a 3x3 NIC). That's not quite the same as verifying each antenna separately, but it might be good enough. I suppose this wasn't really considered when some of the test frameworks I'm using were written, seeing as per-chain signal reports weren't even available in ath10k until relatively recently: commit 8241253d03fe9098e98315a4d66027ae31ab65c5 Author: Norik Dzhandzhapanyan <norikd@gmail.com> Date: Mon Jun 12 18:03:34 2017 +0300 ath10k: add per chain RSSI reporting Thanks, Brian
Hi Sebastian, On Tue, Aug 07, 2018 at 09:22:31AM +0200, Sebastian Gottschall wrote: > Am 07.08.2018 um 07:01 schrieb Brian Norris: > > Thanks for your response. The thing is, masks like 0x2 and 0x4 *do* > > appear to work for IPQ8064, as I noted above. Let me elaborate. > > > > I tested with a conductively-wired setup, where antennas are wired > > directly from an AP to a client (with reasonable attenuation), only 2 > > of the 3 AP antennas are connected, and the client supports reporting > > signal strength on a per-antenna basis. If I set the AP's mask to 0x1, > > I see strong signal only on the client's antenna 1; if set to 0x2, I > > see strong signal only on the client's antenna 2; and if I set it to > > 0x4, I see only a very weak signal (presumably over the air, even > > without any antenna). > > > > In other words, I think this clearly works for some chipsets. I just > > wonder if anybody knows anything about why it does or doesn't work on > > a give chipset. > i dont think so. even if you see a signal on it i'm pretty sure that the > rate control algorithm within the firmware > will simply fail and gets out of control due the way the rate control > algorithm works and handles the chain controls. > depending on the rate the rate control algorithm will select chains to > transmit. so i assume if you set it to 0x2, all rates > which are supported by 1x1 only, will not work anymore. so 1x1 clients > simply wont work anymore. > and then there are several other cases like vht160, which always uses chain > 1 and 2 even if 4x4 is selected. so if you configure > such a setup i expect nothing more than a crash in the firmware on 9984 > chipsets for instance > but of course these are assumptions from the firmware code i know and no > proof. Thank you for the much more detailed information. For the record, I was testing a 2x2 client with some moderate test traffic, and stuff appeared to work, settling on the appropriate NSS=1 or NSS=2 rates according to how many antennas I configured. I didn't gauge throughput or anything like that, nor did I try a 1x1 client. So it seems like to some extent things "may" work, but there are a lot of known firmware deficiencies. In some cases, this is enough for certain debugging or verification scenarios but likely not good for any sort of "real" use case. I'm torn between whether we should just reject these configurations outright, or leave the status quo: log a warning but don't reject. I guess I'll personally just leave things along and note these limitations for myself. Regards, Brian
On 08/07/2018 10:52 AM, Brian Norris wrote: > Hi Ben, > > On Tue, Aug 07, 2018 at 06:19:22AM -0700, Ben Greear wrote: >> On 08/06/2018 07:08 PM, Brian Norris wrote: >>> Hi all, >>> >>> I'm looking at ancient changes like this: >>> >>> commit 5572a95b4b5768187652a346356e39e7542ca6e0 >>> Author: Ben Greear <greearb@candelatech.com> >>> Date: Mon Nov 24 16:22:10 2014 +0200 >>> >>> ath10k: apply chainmask settings to vdev on creation >>> >>> since I see that some firmwares seem to crash a lot if you apply certain >>> chainmask settings (e.g., 0x2). >>> >>> Is it really expected that you can set gaps in the chainmask or not? I >>> ask because I've been trying to use this for doing some antenna >>> configuration verification (e.g., disable all but one antenna and see >>> what happens), and this works as expected on APs I have running IPQ8064 >>> (QCA988X?), but it crashes the firmware on IPQ4019. >> >> I don't think you can even directly set the chainmask in the firmware, > > Well, I guess the WMI interface is a bit strange then: it appears to > take a raw mask -- e.g.: > > ret = ath10k_wmi_pdev_set_param(ar, ar->wmi.pdev_param->tx_chain_mask, > tx_ant); > > but I guess that doesn't mean it *really* accepts a mask? You have to > presuppose what values it will accept? Sorry, you are correct. I just looked at my 10.1 firmware, and it does indeed propagate the tx chainmask, and it has no special settings to ignore gaps as far as I can tell. In fact, it has special register handling for gaps that would appear to be designed to allow things like 0x5, 0x6 masks to work as hoped. I would guess then that 0x5 mask should mean NSS is 2 instead of 1. But, especially as you report some extra crashes in your attempts, you may need some way to whitelist firmware before allowing the user to set this. I don't have time to poke at it now, but if you want to test the -ct firmware for 988X hardware, and if you see any crashes or weird behaviour, please let me know and I'll work on it when I have time. Thanks, Ben > >> and from my previous looking at the firmware, it is not designed to support >> gaps. > > OK, thanks for the info. > >> If you want to determine if an antenna is working, you can look at the per-chain >> RSSI stats when receiving frames using all available antenna (ie, 3x3 rates for a 3x3 NIC). > > That's not quite the same as verifying each antenna separately, but it > might be good enough. I suppose this wasn't really considered when some > of the test frameworks I'm using were written, seeing as per-chain > signal reports weren't even available in ath10k until relatively > recently: > > commit 8241253d03fe9098e98315a4d66027ae31ab65c5 > Author: Norik Dzhandzhapanyan <norikd@gmail.com> > Date: Mon Jun 12 18:03:34 2017 +0300 > > ath10k: add per chain RSSI reporting > > Thanks, > Brian >
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 836e0a47b94a..3b557d3dc036 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -19,6 +19,7 @@ #include "mac.h" #include <net/mac80211.h> +#include <linux/bitops.h> #include <linux/etherdevice.h> #include <linux/acpi.h> @@ -4905,13 +4906,7 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed) static u32 get_nss_from_chainmask(u16 chain_mask) { - if ((chain_mask & 0xf) == 0xf) - return 4; - else if ((chain_mask & 0x7) == 0x7) - return 3; - else if ((chain_mask & 0x3) == 0x3) - return 2; - return 1; + return max_t(u32, hweight16(chain_mask & 0xf), 1); } static int ath10k_mac_set_txbf_conf(struct ath10k_vif *arvif)
Hi all, I'm looking at ancient changes like this: commit 5572a95b4b5768187652a346356e39e7542ca6e0 Author: Ben Greear <greearb@candelatech.com> Date: Mon Nov 24 16:22:10 2014 +0200 ath10k: apply chainmask settings to vdev on creation since I see that some firmwares seem to crash a lot if you apply certain chainmask settings (e.g., 0x2). Is it really expected that you can set gaps in the chainmask or not? I ask because I've been trying to use this for doing some antenna configuration verification (e.g., disable all but one antenna and see what happens), and this works as expected on APs I have running IPQ8064 (QCA988X?), but it crashes the firmware on IPQ4019. I similarly wonder about this opinionated statement in ath10k_check_chain_mask(): /* It is not clear that allowing gaps in chainmask * is helpful. Probably it will not do what user * is hoping for, so warn in that case. */ It seems like we should reject unexpected values (-EINVAL) if they're really not supported. But then, I've found differences depending on the chipset it would seem, so I guess I'd have to figure out which chipsets (or firmwares?) support which features... On a related note, if we *do* support "gaps" in these bitmasks, wouldn't that mean we're handling 0x6, 0xa, etc., incorrectly in get_nss_from_chainmask()? My prototype patch (untested so far) would be: