[PATCH] ath10k: use configured nss instead of max nss.

[PATCH] ath10k: use configured nss instead of max nss.

[greearb]

From: Ben Greear <greearb@candelatech.com>

When re-associating a station, the nss was set back to
maximum value even if user had configured small number
of tx chains.  So, pay attention to user's config in
this case as well.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 855c71c..c5d31cc 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
 	u32 legacy = 0x00ff;
 	u8 ht = 0xff, i;
 	u16 vht = 0x3ff;
+	u16 nrf = ar->num_rf_chains;
+
+	if (ar->cfg_tx_chainmask)
+		nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
 
 	switch (band) {
 	case IEEE80211_BAND_2GHZ:
@@ -4101,11 +4105,11 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
 	if (mask->control[band].legacy != legacy)
 		return false;
 
-	for (i = 0; i < ar->num_rf_chains; i++)
+	for (i = 0; i < nrf; i++)
 		if (mask->control[band].ht_mcs[i] != ht)
 			return false;
 
-	for (i = 0; i < ar->num_rf_chains; i++)
+	for (i = 0; i < nrf; i++)
 		if (mask->control[band].vht_mcs[i] != vht)
 			return false;
 
@@ -4356,6 +4360,9 @@ static int ath10k_set_bitrate_mask(struct ieee80211_hw *hw,
 	u8 fixed_nss = ar->num_rf_chains;
 	u8 force_sgi;
 
+	if (ar->cfg_tx_chainmask)
+		fixed_nss = get_nss_from_chainmask(ar->cfg_tx_chainmask);
+
 	force_sgi = mask->control[band].gi;
 	if (force_sgi == NL80211_TXRATE_FORCE_LGI)
 		return -EINVAL;
-- 
1.7.11.7

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Michal Kazior]

On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> When re-associating a station, the nss was set back to
> maximum value even if user had configured small number
> of tx chains.  So, pay attention to user's config in
> this case as well.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/mac.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 855c71c..c5d31cc 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>         u32 legacy = 0x00ff;
>         u8 ht = 0xff, i;
>         u16 vht = 0x3ff;
> +       u16 nrf = ar->num_rf_chains;
> +
> +       if (ar->cfg_tx_chainmask)
> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);

Oh, so you do update the peer nss value here.

I think it might be a good idea to convey the limitation of tx/rx
chainmask to the user: you can't change the tx/rx chainmask on the fly
easily (while connected/have associated stations). Or do you plan to
schedule peer reassoc in __ath10k_set_antenna() in a follow up
patch(es) later?


Michał

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Ben Greear]

On 09/23/2014 01:59 AM, Michal Kazior wrote:
> On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> When re-associating a station, the nss was set back to
>> maximum value even if user had configured small number
>> of tx chains.  So, pay attention to user's config in
>> this case as well.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/mac.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index 855c71c..c5d31cc 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>>         u32 legacy = 0x00ff;
>>         u8 ht = 0xff, i;
>>         u16 vht = 0x3ff;
>> +       u16 nrf = ar->num_rf_chains;
>> +
>> +       if (ar->cfg_tx_chainmask)
>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
> 
> Oh, so you do update the peer nss value here.

Yeah, I found this was needed in further testing yesterday...

> I think it might be a good idea to convey the limitation of tx/rx
> chainmask to the user: you can't change the tx/rx chainmask on the fly
> easily (while connected/have associated stations). Or do you plan to
> schedule peer reassoc in __ath10k_set_antenna() in a follow up
> patch(es) later?

My user-space tools assume you have to re-connect (requested from user-space)
after changing the chainmask.  I was under the impression that this
is how ath9k works, though I could be wrong.

I would rather keep this requirement in user-space...

I am not sure where to document this...just a comment in the code?

Thanks,
Ben


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

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Michal Kazior]

On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote:
> On 09/23/2014 01:59 AM, Michal Kazior wrote:
>> On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
>>> From: Ben Greear <greearb@candelatech.com>
[...]
>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>>>         u32 legacy = 0x00ff;
>>>         u8 ht = 0xff, i;
>>>         u16 vht = 0x3ff;
>>> +       u16 nrf = ar->num_rf_chains;
>>> +
>>> +       if (ar->cfg_tx_chainmask)
>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
[...]
>> I think it might be a good idea to convey the limitation of tx/rx
>> chainmask to the user: you can't change the tx/rx chainmask on the fly
>> easily (while connected/have associated stations). Or do you plan to
>> schedule peer reassoc in __ath10k_set_antenna() in a follow up
>> patch(es) later?
>
> My user-space tools assume you have to re-connect (requested from user-space)
> after changing the chainmask.  I was under the impression that this
> is how ath9k works, though I could be wrong.

ath9k doesn't have firmware and it doesn't have a blackbox rate
control so maybe it doesn't need to worry about this as much as ath10k
needs to.

But then this is not something you normally use except some very
specific use cases so I think it might not be worth the hassle to
implement full-blown reconfig after chainmask is updated.


> I would rather keep this requirement in user-space...
>
> I am not sure where to document this...just a comment in the code?

Hmm.. Perhaps in ath10k_set_antenna? Or maybe we should just put it up
on the wiki?

But it's still probably a good idea to comment the quoted code chunk
above explaining why nrf is overriden by chainmask (i.e. due to
firmware rate control issues, right?).


Michał

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Ben Greear]

On 09/24/2014 12:09 AM, Michal Kazior wrote:
> On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote:
>> On 09/23/2014 01:59 AM, Michal Kazior wrote:
>>> On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
>>>> From: Ben Greear <greearb@candelatech.com>
> [...]
>>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>>>>         u32 legacy = 0x00ff;
>>>>         u8 ht = 0xff, i;
>>>>         u16 vht = 0x3ff;
>>>> +       u16 nrf = ar->num_rf_chains;
>>>> +
>>>> +       if (ar->cfg_tx_chainmask)
>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
> [...]
>>> I think it might be a good idea to convey the limitation of tx/rx
>>> chainmask to the user: you can't change the tx/rx chainmask on the fly
>>> easily (while connected/have associated stations). Or do you plan to
>>> schedule peer reassoc in __ath10k_set_antenna() in a follow up
>>> patch(es) later?
>>
>> My user-space tools assume you have to re-connect (requested from user-space)
>> after changing the chainmask.  I was under the impression that this
>> is how ath9k works, though I could be wrong.
> 
> ath9k doesn't have firmware and it doesn't have a blackbox rate
> control so maybe it doesn't need to worry about this as much as ath10k
> needs to.
> 
> But then this is not something you normally use except some very
> specific use cases so I think it might not be worth the hassle to
> implement full-blown reconfig after chainmask is updated.

See this in net/mac80211/cfg.c:

static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant)
{
	struct ieee80211_local *local = wiphy_priv(wiphy);

	if (local->started)
		return -EOPNOTSUPP;

	return drv_set_antenna(local, tx_ant, rx_ant);
}


It ensures there are no active vdevs when this change is made, so
you should not have to worry about block-ack or anything similar.


> But it's still probably a good idea to comment the quoted code chunk
> above explaining why nrf is overriden by chainmask (i.e. due to
> firmware rate control issues, right?).

I am not certain it is a bug in the firmware.  The driver should not configure
nss incorrectly as it was doing previous to my recent patches.

Thanks,
Ben

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

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Michal Kazior]

On 24 September 2014 18:09, Ben Greear <greearb@candelatech.com> wrote:
> On 09/24/2014 12:09 AM, Michal Kazior wrote:
>> On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote:
>>> On 09/23/2014 01:59 AM, Michal Kazior wrote:
>>>> On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
>>>>> From: Ben Greear <greearb@candelatech.com>
>> [...]
>>>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>>>>>         u32 legacy = 0x00ff;
>>>>>         u8 ht = 0xff, i;
>>>>>         u16 vht = 0x3ff;
>>>>> +       u16 nrf = ar->num_rf_chains;
>>>>> +
>>>>> +       if (ar->cfg_tx_chainmask)
>>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>> [...]
>>>> I think it might be a good idea to convey the limitation of tx/rx
>>>> chainmask to the user: you can't change the tx/rx chainmask on the fly
>>>> easily (while connected/have associated stations). Or do you plan to
>>>> schedule peer reassoc in __ath10k_set_antenna() in a follow up
>>>> patch(es) later?
[...]
> See this in net/mac80211/cfg.c:
>
> static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant)
> {
>         struct ieee80211_local *local = wiphy_priv(wiphy);
>
>         if (local->started)
>                 return -EOPNOTSUPP;

Ah, thanks! So my argument is invalid :)


>> But it's still probably a good idea to comment the quoted code chunk
>> above explaining why nrf is overriden by chainmask (i.e. due to
>> firmware rate control issues, right?).
>
> I am not certain it is a bug in the firmware.  The driver should not configure
> nss incorrectly as it was doing previous to my recent patches.

Since firmware does rate control it just seems redundant for the
driver to update both chainmask and nss (the first implies the other).
But maybe that's just me.


Michał

Re: [PATCH] ath10k: use configured nss instead of max nss.

[Ben Greear]

On 09/24/2014 11:16 PM, Michal Kazior wrote:
> On 24 September 2014 18:09, Ben Greear <greearb@candelatech.com> wrote:
>> On 09/24/2014 12:09 AM, Michal Kazior wrote:
>>> On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote:
>>>> On 09/23/2014 01:59 AM, Michal Kazior wrote:
>>>>> On 23 September 2014 01:00,  <greearb@candelatech.com> wrote:
>>>>>> From: Ben Greear <greearb@candelatech.com>
>>> [...]
>>>>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar,
>>>>>>          u32 legacy = 0x00ff;
>>>>>>          u8 ht = 0xff, i;
>>>>>>          u16 vht = 0x3ff;
>>>>>> +       u16 nrf = ar->num_rf_chains;
>>>>>> +
>>>>>> +       if (ar->cfg_tx_chainmask)
>>>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>> [...]
>>>>> I think it might be a good idea to convey the limitation of tx/rx
>>>>> chainmask to the user: you can't change the tx/rx chainmask on the fly
>>>>> easily (while connected/have associated stations). Or do you plan to
>>>>> schedule peer reassoc in __ath10k_set_antenna() in a follow up
>>>>> patch(es) later?
> [...]
>> See this in net/mac80211/cfg.c:
>>
>> static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant)
>> {
>>          struct ieee80211_local *local = wiphy_priv(wiphy);
>>
>>          if (local->started)
>>                  return -EOPNOTSUPP;
>
> Ah, thanks! So my argument is invalid :)
>
>
>>> But it's still probably a good idea to comment the quoted code chunk
>>> above explaining why nrf is overriden by chainmask (i.e. due to
>>> firmware rate control issues, right?).
>>
>> I am not certain it is a bug in the firmware.  The driver should not configure
>> nss incorrectly as it was doing previous to my recent patches.
>
> Since firmware does rate control it just seems redundant for the
> driver to update both chainmask and nss (the first implies the other).
> But maybe that's just me.

nss is really a per-station issue, where the chainmask is a per-radio issue.

Think of an AP with 2x2 and 3x3 stations connected, for instance.

Thanks,
Ben

>
>
> Michał
>

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