ath10k: implement more versatile set_bitrate_mask

Commit Message

Kalle Valo April 17, 2015, 7:06 a.m. UTC

Michal Kazior <[e-mail]> writes:

> Until now only a single fixed tx rate or nss was
> allowed to be set.
>
> The patch attempts to improve this by allowing
> most bitrate masks. The limitation is VHT MCS
> rates cannot be expressed separately using
> existing firmware interfaces and only the
> following VHT MCS ranges are supported: none, 0-7,
> 0-8, and 0-9.
>
> This keeps the old behaviour when requesting
> single tx rate or single nss. The new bitrate mask
> logic is only applied to other cases that would
> return -EINVAL until now.
>
> Depending on firmware revisions some combinations
> may crash firmware so use with care, please.
>
> This depends on "ath10k: don't use reassoc flag".
> Without it key cache would effectively be
> invalidated upon bitrate change leading to
> communication being no longer possible.
>
> Signed-off-by: Michal Kazior <[e-mail]>

To reduce support questions from the users it would be nice to give few
good examples how to use this with iw. And also it makes it easier to
test the patch. If you could send something I can add it to the commit
log.

> +static u16
> +ath10k_peer_assoc_h_vht_limit(u16 tx_mcs_set,
> +			      const u16 vht_mcs_limit[NL80211_VHT_NSS_MAX])
> +{
> +	int idx_limit;
> +	int nss;
> +	u16 mcs_map;
> +	u16 mcs;
> +
> +	for (nss = 0; nss < NL80211_VHT_NSS_MAX; nss++) {
> +		mcs_map = ath10k_mac_get_max_vht_mcs_map(tx_mcs_set, nss) &
> +			  vht_mcs_limit[nss];
> +
> +		if (mcs_map)
> +			idx_limit = fls(mcs_map) - 1;
> +		else
> +			idx_limit = -1;
> +
> +		switch (idx_limit) {
> +		case 0: /* fall through */
> +		case 1: /* fall through */
> +		case 2: /* fall through */
> +		case 3: /* fall through */
> +		case 4: /* fall through */
> +		case 5: /* fall through */
> +		case 6: /* fall through */
> +		default:
> +			/* see ath10k_mac_can_set_bitrate_mask() */
> +			WARN_ON(1);
> +			/* fall through */
> +		case -1:
> +			mcs = IEEE80211_VHT_MCS_NOT_SUPPORTED; break;
> +		case 7:
> +			mcs = IEEE80211_VHT_MCS_SUPPORT_0_7; break;
> +		case 8:
> +			mcs = IEEE80211_VHT_MCS_SUPPORT_0_8; break;
> +		case 9:
> +			mcs = IEEE80211_VHT_MCS_SUPPORT_0_9; break;
> +		}

I moved the breaks into their own lines, I think that just easier to
read. This is the diff:

Comments

Michal Kazior April 17, 2015, 7:30 a.m. UTC | #1

On 17 April 2015 at 09:06, Kalle Valo <[e-mail]> wrote:
> Michal Kazior <[e-mail]> writes:
>
>> Until now only a single fixed tx rate or nss was
>> allowed to be set.
>>
>> The patch attempts to improve this by allowing
>> most bitrate masks. The limitation is VHT MCS
>> rates cannot be expressed separately using
>> existing firmware interfaces and only the
>> following VHT MCS ranges are supported: none, 0-7,
>> 0-8, and 0-9.
>>
>> This keeps the old behaviour when requesting
>> single tx rate or single nss. The new bitrate mask
>> logic is only applied to other cases that would
>> return -EINVAL until now.
>>
>> Depending on firmware revisions some combinations
>> may crash firmware so use with care, please.
>>
>> This depends on "ath10k: don't use reassoc flag".
>> Without it key cache would effectively be
>> invalidated upon bitrate change leading to
>> communication being no longer possible.
>>
>> Signed-off-by: Michal Kazior <[e-mail]>
>
> To reduce support questions from the users it would be nice to give few
> good examples how to use this with iw. And also it makes it easier to
> test the patch. If you could send something I can add it to the commit
> log.

Should work:

  iw wlan0 set bitrates legacy 1 6 12 ht-mcs 1 2 3
  iw wlan0 set bitrates legacy-5 mcs-5 7 8 9
  iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9

Won't work:

  iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
  iw wlan0 set bitrates vht-mcs-5 2:7-9

(note the invalid VHT MCS ranges)

Generally the patch removes a lot of limitations from 'set bitrates'
ath10k had. Before it was possible to do only things as described in
51ab1a0a09a8 ("ath10k: add set_bitrate_mask callback").


[...]
>> +             default:
>> +                     /* see ath10k_mac_can_set_bitrate_mask() */
>> +                     WARN_ON(1);
>> +                     /* fall through */
>> +             case -1:
>> +                     mcs = IEEE80211_VHT_MCS_NOT_SUPPORTED; break;
>> +             case 7:
>> +                     mcs = IEEE80211_VHT_MCS_SUPPORT_0_7; break;
>> +             case 8:
>> +                     mcs = IEEE80211_VHT_MCS_SUPPORT_0_8; break;
>> +             case 9:
>> +                     mcs = IEEE80211_VHT_MCS_SUPPORT_0_9; break;
>> +             }
>
> I moved the breaks into their own lines, I think that just easier to
> read. This is the diff:
[snip]

I'm okay with this.


Micha?

Kalle Valo April 21, 2015, 3:04 p.m. UTC | #2

Michal Kazior <[e-mail]> writes:

>> To reduce support questions from the users it would be nice to give few
>> good examples how to use this with iw. And also it makes it easier to
>> test the patch. If you could send something I can add it to the commit
>> log.
>
> Should work:
>
>   iw wlan0 set bitrates legacy 1 6 12 ht-mcs 1 2 3
>   iw wlan0 set bitrates legacy-5 mcs-5 7 8 9
>   iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9
>
> Won't work:
>
>   iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
>   iw wlan0 set bitrates vht-mcs-5 2:7-9
>
> (note the invalid VHT MCS ranges)

Thanks, I added these to the commit log.

Kalle Valo April 22, 2015, 6:27 a.m. UTC | #3

Kalle Valo <[e-mail]> writes:

> Michal Kazior <[e-mail]> writes:
>
>>> To reduce support questions from the users it would be nice to give few
>>> good examples how to use this with iw. And also it makes it easier to
>>> test the patch. If you could send something I can add it to the commit
>>> log.
>>
>> Should work:
>>
>>   iw wlan0 set bitrates legacy 1 6 12 ht-mcs 1 2 3
>>   iw wlan0 set bitrates legacy-5 mcs-5 7 8 9
>>   iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9
>>
>> Won't work:
>>
>>   iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
>>   iw wlan0 set bitrates vht-mcs-5 2:7-9
>>
>> (note the invalid VHT MCS ranges)
>
> Thanks, I added these to the commit log.

Actually, I had some problems:

# iw wlan0 set bitrates legacy-2.4 1 6 12 ht-mcs-2.4 1 2 3
command failed: Invalid argument (-22)
# iw wlan0 set bitrates legacy-5 1 6 12 ht-mcs-5 1 2 3
command failed: Invalid argument (-22)
# iw wlan0 set bitrates legacy-5 vht-mcs-5 7 8 9
command failed: Invalid argument (-22)
# iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9
# 

The first one I understand as this CUS223 board only supports 5G. But
why did the second and third command fail?

Michal Kazior April 22, 2015, 7:33 a.m. UTC | #4

On 22 April 2015 at 08:27, Kalle Valo <[e-mail]> wrote:
> Kalle Valo <[e-mail]> writes:
>
>> Michal Kazior <[e-mail]> writes:
>>
>>>> To reduce support questions from the users it would be nice to give few
>>>> good examples how to use this with iw. And also it makes it easier to
>>>> test the patch. If you could send something I can add it to the commit
>>>> log.
>>>
>>> Should work:
>>>
>>>   iw wlan0 set bitrates legacy 1 6 12 ht-mcs 1 2 3
>>>   iw wlan0 set bitrates legacy-5 mcs-5 7 8 9

Oh, I just noticed I typo'ed: s/mcs-5/ht-mcs-5/.


>>>   iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9
>>>
>>> Won't work:
>>>
>>>   iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
>>>   iw wlan0 set bitrates vht-mcs-5 2:7-9
>>>
>>> (note the invalid VHT MCS ranges)
>>
>> Thanks, I added these to the commit log.
>
> Actually, I had some problems:
>
> # iw wlan0 set bitrates legacy-2.4 1 6 12 ht-mcs-2.4 1 2 3
> command failed: Invalid argument (-22)
> # iw wlan0 set bitrates legacy-5 1 6 12 ht-mcs-5 1 2 3
> command failed: Invalid argument (-22)

There's no 1mbps (CCK) on 5GHz. If you remove the "1" from legacy-5 it
should work.


> # iw wlan0 set bitrates legacy-5 vht-mcs-5 7 8 9
> command failed: Invalid argument (-22)

There's a couple of problems here:

 * The syntax for VHT MCS is different: vht-mcs-<2.4|5>
<NSS:MCSx,MCSy... | NSS:MCSx-MCSy>*
   You used syntax from HT MCS: ht-mcs-<2.4|5> <MCS index>*
   The correct way to express this would be:
      iw wlan0 set bitrates legacy-5 vht-mcs-5 1:7,8,9
   or
      iw wlan0 set bitrates legacy-5 vht-mcs-5 1:7-9
   assuming you wanted NSS=1 (VHT MCS don't imply NSS just like HT MCS do)

* As per commit log you can't use just any VHT MCS; you're limited to
none, 0-7, 0-8, 0-9. You can set a *single* VHT MCS as per
51ab1a0a09a8 ("ath10k: add set_bitrate_mask callback") though.


Micha?

Kalle Valo April 24, 2015, 2:31 p.m. UTC | #5

Michal Kazior <[e-mail]> writes:

> On 22 April 2015 at 08:27, Kalle Valo <[e-mail]> wrote:
>> Kalle Valo <[e-mail]> writes:
>>
>>> Michal Kazior <[e-mail]> writes:
>>>
>>>>> To reduce support questions from the users it would be nice to give few
>>>>> good examples how to use this with iw. And also it makes it easier to
>>>>> test the patch. If you could send something I can add it to the commit
>>>>> log.
>>>>
>>>> Should work:
>>>>
>>>>   iw wlan0 set bitrates legacy 1 6 12 ht-mcs 1 2 3
>>>>   iw wlan0 set bitrates legacy-5 mcs-5 7 8 9
>
> Oh, I just noticed I typo'ed: s/mcs-5/ht-mcs-5/.
>
>
>>>>   iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9
>>>>
>>>> Won't work:
>>>>
>>>>   iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
>>>>   iw wlan0 set bitrates vht-mcs-5 2:7-9
>>>>
>>>> (note the invalid VHT MCS ranges)
>>>
>>> Thanks, I added these to the commit log.
>>
>> Actually, I had some problems:
>>
>> # iw wlan0 set bitrates legacy-2.4 1 6 12 ht-mcs-2.4 1 2 3
>> command failed: Invalid argument (-22)
>> # iw wlan0 set bitrates legacy-5 1 6 12 ht-mcs-5 1 2 3
>> command failed: Invalid argument (-22)
>
> There's no 1mbps (CCK) on 5GHz. If you remove the "1" from legacy-5 it
> should work.

I fixed the commit now to:

----------------------------------------------------------------------
These work:

  iw wlan0 set bitrates legacy-5 6 12 ht-mcs-5 1 2 3
  iw wlan0 set bitrates legacy-5 ht-mcs-5 7 8 9
  iw wlan0 set bitrates legacy-5 24 ht-mcs-5 vht-mcs-5 1:0-9

These won't work:

  iw wlan0 set bitrates legacy-5 ht-mcs-5 vht-mcs-5 1:0-5
  iw wlan0 set bitrates vht-mcs-5 2:7-9

(note the invalid VHT MCS ranges)
----------------------------------------------------------------------

>> # iw wlan0 set bitrates legacy-5 vht-mcs-5 7 8 9
>> command failed: Invalid argument (-22)
>
> There's a couple of problems here:
>
>  * The syntax for VHT MCS is different: vht-mcs-<2.4|5>
> <NSS:MCSx,MCSy... | NSS:MCSx-MCSy>*
>    You used syntax from HT MCS: ht-mcs-<2.4|5> <MCS index>*
>    The correct way to express this would be:
>       iw wlan0 set bitrates legacy-5 vht-mcs-5 1:7,8,9
>    or
>       iw wlan0 set bitrates legacy-5 vht-mcs-5 1:7-9
>    assuming you wanted NSS=1 (VHT MCS don't imply NSS just like HT MCS do)
>
> * As per commit log you can't use just any VHT MCS; you're limited to
> none, 0-7, 0-8, 0-9. You can set a *single* VHT MCS as per
> 51ab1a0a09a8 ("ath10k: add set_bitrate_mask callback") though.

Thanks, I understand better now.

Patch

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index ace273f7ec73..069f399e4c25 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2277,13 +2277,17 @@  ath10k_peer_assoc_h_vht_limit(u16 tx_mcs_set,
                        WARN_ON(1);
                        /* fall through */
                case -1:
-                       mcs = IEEE80211_VHT_MCS_NOT_SUPPORTED; break;
+                       mcs = IEEE80211_VHT_MCS_NOT_SUPPORTED;
+                       break;
                case 7:
-                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_7; break;
+                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_7;
+                       break;
                case 8:
-                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_8; break;
+                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_8;
+                       break;
                case 9:
-                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_9; break;
+                       mcs = IEEE80211_VHT_MCS_SUPPORT_0_9;
+                       break;
                }
 
                tx_mcs_set &= ~(0x3 << (nss * 2));