ath10k: antenna bitmask support?

ath10k: antenna bitmask support?

[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

Re: ath10k: antenna bitmask support?

[Sebastian Gottschall]

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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Brian Norris]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Sebastian Gottschall]

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
>


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Brian Norris]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Brian Norris]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: ath10k: antenna bitmask support?

[Ben Greear]

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
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k