From: Johannes Berg <[email protected]>
If the user rate mask results in no (basic) rates being usable,
clear it. Also, if we're already operating when it's set, reject
it instead.
Technically, selecting basic rates as the criterion is a bit too
restrictive, but calculating the usable rates over all stations
(e.g. in AP mode) is harder, and all stations must support the
basic rates. Similarly, in client mode, the basic rates will be
used anyway for control frames.
This fixes the "no supported rates (...) in rate_mask ..." warning
that occurs on TX when you've selected a rate mask that's not
compatible with the connection (e.g. an AP that enables only the
rates 36, 48, 54 and you've selected only 6, 9, 12.)
Reported-by: Kirtika Ruchandani <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/cfg.c | 18 +++++++++++++++++-
net/mac80211/mlme.c | 2 ++
net/mac80211/rate.c | 27 +++++++++++++++++++++++++++
net/mac80211/rate.h | 2 ++
4 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9c7490cb2243..8bc3d3669348 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3,7 +3,7 @@
*
* Copyright 2006-2010 Johannes Berg <[email protected]>
* Copyright 2013-2015 Intel Mobile Communications GmbH
- * Copyright (C) 2015-2016 Intel Deutschland GmbH
+ * Copyright (C) 2015-2017 Intel Deutschland GmbH
*
* This file is GPLv2 as found in COPYING.
*/
@@ -2042,6 +2042,7 @@ static int ieee80211_change_bss(struct wiphy *wiphy,
params->basic_rates_len,
&sdata->vif.bss_conf.basic_rates);
changed |= BSS_CHANGED_BASIC_RATES;
+ ieee80211_check_rate_mask(sdata);
}
if (params->ap_isolate >= 0) {
@@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct wiphy *wiphy,
return ret;
}
+ /*
+ * If active validate the setting and reject it if it doesn't leave
+ * at least one basic rate usable, since we really have to be able
+ * to send something, and if we're an AP we have to be able to do
+ * so at a basic rate so that all clients can receive it.
+ */
+ if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
+ sdata->vif.bss_conf.chandef.chan) {
+ u32 basic_rates = sdata->vif.bss_conf.basic_rates;
+ enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
+
+ if (!(mask->control[band].legacy & basic_rates))
+ return -EINVAL;
+ }
+
for (i = 0; i < NUM_NL80211_BANDS; i++) {
struct ieee80211_supported_band *sband = wiphy->bands[i];
int j;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 4b4d29edec09..24d69bcf71ad 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1908,6 +1908,8 @@ static void ieee80211_set_associated(struct ieee80211_sub_if_data *sdata,
sdata->u.mgd.associated = cbss;
memcpy(sdata->u.mgd.bssid, cbss->bssid, ETH_ALEN);
+ ieee80211_check_rate_mask(sdata);
+
sdata->u.mgd.flags |= IEEE80211_STA_RESET_SIGNAL_AVE;
if (sdata->vif.p2p ||
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 094c15645228..3bddd9bbb76f 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -2,6 +2,7 @@
* Copyright 2002-2005, Instant802 Networks, Inc.
* Copyright 2005-2006, Devicescape Software, Inc.
* Copyright (c) 2006 Jiri Benc <[email protected]>
+ * Copyright 2017 Intel Deutschland GmbH
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -241,6 +242,32 @@ static void rate_control_free(struct ieee80211_local *local,
kfree(ctrl_ref);
}
+void ieee80211_check_rate_mask(struct ieee80211_sub_if_data *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_supported_band *sband;
+ u32 user_mask, basic_rates = sdata->vif.bss_conf.basic_rates;
+ enum nl80211_band band;
+
+ if (WARN_ON(!sdata->vif.bss_conf.chandef.chan))
+ return;
+
+ if (WARN_ON_ONCE(!basic_rates))
+ return;
+
+ band = sdata->vif.bss_conf.chandef.chan->band;
+ user_mask = sdata->rc_rateidx_mask[band];
+ sband = local->hw.wiphy->bands[band];
+
+ if (user_mask & basic_rates)
+ return;
+
+ sdata_dbg(sdata,
+ "no overlap between basic rates (0x%x) and user mask (0x%x on band %d) - clearing the latter",
+ basic_rates, user_mask, band);
+ sdata->rc_rateidx_mask[band] = (1 << sband->n_bitrates) - 1;
+}
+
static bool rc_no_data_or_no_ack_use_min(struct ieee80211_tx_rate_control *txrc)
{
struct sk_buff *skb = txrc->skb;
diff --git a/net/mac80211/rate.h b/net/mac80211/rate.h
index d51a1cce4d4a..f7825ef5f871 100644
--- a/net/mac80211/rate.h
+++ b/net/mac80211/rate.h
@@ -110,6 +110,8 @@ static inline void rate_control_remove_sta_debugfs(struct sta_info *sta)
#endif
}
+void ieee80211_check_rate_mask(struct ieee80211_sub_if_data *sdata);
+
/* Get a reference to the rate control algorithm. If `name' is NULL, get the
* first available algorithm. */
int ieee80211_init_rate_ctrl_alg(struct ieee80211_local *local,
--
2.11.0
On 2017-03-08 21:20, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> If the user rate mask results in no (basic) rates being usable,
> clear it. Also, if we're already operating when it's set, reject
> it instead.
>
> Technically, selecting basic rates as the criterion is a bit too
> restrictive, but calculating the usable rates over all stations
> (e.g. in AP mode) is harder, and all stations must support the
> basic rates. Similarly, in client mode, the basic rates will be
> used anyway for control frames.
>
> This fixes the "no supported rates (...) in rate_mask ..." warning
> that occurs on TX when you've selected a rate mask that's not
> compatible with the connection (e.g. an AP that enables only the
> rates 36, 48, 54 and you've selected only 6, 9, 12.)
>
> Reported-by: Kirtika Ruchandani <[email protected]>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/cfg.c | 18 +++++++++++++++++-
> net/mac80211/mlme.c | 2 ++
> net/mac80211/rate.c | 27 +++++++++++++++++++++++++++
> net/mac80211/rate.h | 2 ++
> 4 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 9c7490cb2243..8bc3d3669348 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3,7 +3,7 @@
> *
> * Copyright 2006-2010 Johannes Berg <[email protected]>
> * Copyright 2013-2015 Intel Mobile Communications GmbH
> - * Copyright (C) 2015-2016 Intel Deutschland GmbH
> + * Copyright (C) 2015-2017 Intel Deutschland GmbH
> *
> * This file is GPLv2 as found in COPYING.
> */
> @@ -2042,6 +2042,7 @@ static int ieee80211_change_bss(struct wiphy
> *wiphy,
> params->basic_rates_len,
> &sdata->vif.bss_conf.basic_rates);
> changed |= BSS_CHANGED_BASIC_RATES;
> + ieee80211_check_rate_mask(sdata);
> }
>
> if (params->ap_isolate >= 0) {
> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
> wiphy *wiphy,
> return ret;
> }
>
> + /*
> + * If active validate the setting and reject it if it doesn't leave
> + * at least one basic rate usable, since we really have to be able
> + * to send something, and if we're an AP we have to be able to do
> + * so at a basic rate so that all clients can receive it.
> + */
> + if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
> + sdata->vif.bss_conf.chandef.chan) {
> + u32 basic_rates = sdata->vif.bss_conf.basic_rates;
> + enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
> +
> + if (!(mask->control[band].legacy & basic_rates))
> + return -EINVAL;
If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to
fixed in 54M and 54M is
not basic rate in AP's becaon as example of below, then the iw command
will fail.
Tag: Supported Rates 6(B), 9, 12(B), 18, 24(B), 36, 48, 54, [Mbit/sec]
Tag Number: Supported Rates (1)
Tag length: 8
Supported Rates: 6(B) (0x8c)
Supported Rates: 9 (0x12)
Supported Rates: 12(B) (0x98)
Supported Rates: 18 (0x24)
Supported Rates: 24(B) (0xb0)
Supported Rates: 36 (0x48)
Supported Rates: 48 (0x60)
Supported Rates: 54 (0x6c)
> + }
> +
> for (i = 0; i < NUM_NL80211_BANDS; i++) {
> struct ieee80211_supported_band *sband = wiphy->bands[i];
> int j;
...
On 11/12/2020 11:55 AM, Wen Gong wrote:
> On 2017-03-08 21:20, Johannes Berg wrote:
>> From: Johannes Berg <[email protected]>
>>
[...]
>> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
>> wiphy *wiphy,
>> return ret;
>> }
>>
>> + /*
>> + * If active validate the setting and reject it if it doesn't leave
>> + * at least one basic rate usable, since we really have to be able
>> + * to send something, and if we're an AP we have to be able to do
>> + * so at a basic rate so that all clients can receive it.
>> + */
>> + if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>> + sdata->vif.bss_conf.chandef.chan) {
>> + u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>> + enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
>> +
>> + if (!(mask->control[band].legacy & basic_rates))
>> + return -EINVAL;
> If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to
> fixed in 54M and 54M is
> not basic rate in AP's becaon as example of below, then the iw command
> will fail.
Which was the intent of this change, wasn't it? You want to allow anyway
or you want a clear error message as to why it fails?
Regards,
Arend
On 2020-11-12 20:49, Arend Van Spriel wrote:
> On 11/12/2020 11:55 AM, Wen Gong wrote:
>> On 2017-03-08 21:20, Johannes Berg wrote:
>>> From: Johannes Berg <[email protected]>
>>>
>
> [...]
>
>>> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
>>> wiphy *wiphy,
>>> return ret;
>>> }
>>>
>>> + /*
>>> + * If active validate the setting and reject it if it doesn't
>>> leave
>>> + * at least one basic rate usable, since we really have to be
>>> able
>>> + * to send something, and if we're an AP we have to be able to
>>> do
>>> + * so at a basic rate so that all clients can receive it.
>>> + */
>>> + if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>>> + sdata->vif.bss_conf.chandef.chan) {
>>> + u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>>> + enum nl80211_band band =
>>> sdata->vif.bss_conf.chandef.chan->band;
>>> +
>>> + if (!(mask->control[band].legacy & basic_rates))
>>> + return -EINVAL;
>> If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to
>> fixed in 54M and 54M is
>> not basic rate in AP's becaon as example of below, then the iw command
>> will fail.
>
> Which was the intent of this change, wasn't it? You want to allow
> anyway or you want a clear error message as to why it fails?
We need to set the tx rate to fixed at a single rate, e.g.,
54M/48M/36M... for a test case.
I do not want a clear error message, I want to the 54M rate pass/set
success to lower wlan driver.
Then lower wlan driver can handle it.
>
> Regards,
> Arend
On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
>
> > Which was the intent of this change, wasn't it?
Indeed. Permitting this leads to warnings later.
> We need to set the tx rate to fixed at a single rate, e.g.,
> 54M/48M/36M... for a test case.
> I do not want a clear error message, I want to the 54M rate pass/set
> success to lower wlan driver.
> Then lower wlan driver can handle it.
No, that will not happen. You should configure your test AP to actually
support 54M.
johannes
On Fri, 2020-11-13 at 16:14 +0800, Wen Gong wrote:
> On 2020-11-13 15:38, Johannes Berg wrote:
> > On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
> > > > Which was the intent of this change, wasn't it?
> >
> > Indeed. Permitting this leads to warnings later.
> >
> > > We need to set the tx rate to fixed at a single rate, e.g.,
> > > 54M/48M/36M... for a test case.
> > > I do not want a clear error message, I want to the 54M rate pass/set
> > > success to lower wlan driver.
> > > Then lower wlan driver can handle it.
> >
> > No, that will not happen. You should configure your test AP to actually
> > support 54M.
> Yes, the AP support 54M, but it is not basic rate, so
> ieee80211_set_bitrate_mask will reject 54M
> because fail for check (mask->control[band].legacy & basic_rates).
Ah. So this is what I said in the original commit message even:
> Technically, selecting basic rates as the criterion is a bit too
> restrictive, but calculating the usable rates over all stations
> (e.g. in AP mode) is harder, and all stations must support the
> basic rates. Similarly, in client mode, the basic rates will be
> used anyway for control frames.
I guess if we really want to redefine the user rate mask to not apply to
control frames, then we can relax this?
johannes
On 2020-11-13 15:38, Johannes Berg wrote:
> On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
>>
>> > Which was the intent of this change, wasn't it?
>
> Indeed. Permitting this leads to warnings later.
>
>> We need to set the tx rate to fixed at a single rate, e.g.,
>> 54M/48M/36M... for a test case.
>> I do not want a clear error message, I want to the 54M rate pass/set
>> success to lower wlan driver.
>> Then lower wlan driver can handle it.
>
> No, that will not happen. You should configure your test AP to actually
> support 54M.
Yes, the AP support 54M, but it is not basic rate, so
ieee80211_set_bitrate_mask will reject 54M
because fail for check (mask->control[band].legacy & basic_rates).
suppor rates of my AP:
Tag: Supported Rates 6(B), 9, 12(B), 18, 24(B), 36, 48, 54, [Mbit/sec]
Tag Number: Supported Rates (1)
Tag length: 8
Supported Rates: 6(B) (0x8c)
Supported Rates: 9 (0x12)
Supported Rates: 12(B) (0x98)
Supported Rates: 18 (0x24)
Supported Rates: 24(B) (0xb0)
Supported Rates: 36 (0x48)
Supported Rates: 48 (0x60)
Supported Rates: 54 (0x6c)
>
> johannes
On 2020-11-13 16:16, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:14 +0800, Wen Gong wrote:
>> On 2020-11-13 15:38, Johannes Berg wrote:
>> > On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
>> > > > Which was the intent of this change, wasn't it?
>> >
>> > Indeed. Permitting this leads to warnings later.
>> >
>> > > We need to set the tx rate to fixed at a single rate, e.g.,
>> > > 54M/48M/36M... for a test case.
>> > > I do not want a clear error message, I want to the 54M rate pass/set
>> > > success to lower wlan driver.
>> > > Then lower wlan driver can handle it.
>> >
>> > No, that will not happen. You should configure your test AP to actually
>> > support 54M.
>> Yes, the AP support 54M, but it is not basic rate, so
>> ieee80211_set_bitrate_mask will reject 54M
>> because fail for check (mask->control[band].legacy & basic_rates).
>
> Ah. So this is what I said in the original commit message even:
>
>> Technically, selecting basic rates as the criterion is a bit too
>> restrictive, but calculating the usable rates over all stations
>> (e.g. in AP mode) is harder, and all stations must support the
>> basic rates. Similarly, in client mode, the basic rates will be
>> used anyway for control frames.
>
> I guess if we really want to redefine the user rate mask to not apply
> to
> control frames, then we can relax this?
>
Yes, for AP mode, it is hard to calculate the usable rates over all
stations.
But for STATION mode, it can set 54M because AP support it, so it should
not reject it.
If add a check for nl80211_iftype of ieee80211_vif in
ieee80211_set_bitrate_mask, it can
solve this like this:
if (sdata->vif.type != NL80211_IFTYPE_STATION &&
!(mask->control[band].legacy & basic_rates))
> johannes
On Fri, 2020-11-13 at 16:35 +0800, Wen Gong wrote:
>
> > I guess if we really want to redefine the user rate mask to not apply
> > to
> > control frames, then we can relax this?
> >
> Yes, for AP mode, it is hard to calculate the usable rates over all
> stations.
> But for STATION mode, it can set 54M because AP support it, so it should
> not reject it.
> If add a check for nl80211_iftype of ieee80211_vif in
> ieee80211_set_bitrate_mask, it can
> solve this like this:
> if (sdata->vif.type != NL80211_IFTYPE_STATION &&
> !(mask->control[band].legacy & basic_rates))
That would forgo the check completely - we'd still need to check against
the *supported* rates.
johannes
On 2020-11-13 16:35, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:35 +0800, Wen Gong wrote:
>>
>> > I guess if we really want to redefine the user rate mask to not apply
>> > to
>> > control frames, then we can relax this?
>> >
>> Yes, for AP mode, it is hard to calculate the usable rates over all
>> stations.
>> But for STATION mode, it can set 54M because AP support it, so it
>> should
>> not reject it.
>> If add a check for nl80211_iftype of ieee80211_vif in
>> ieee80211_set_bitrate_mask, it can
>> solve this like this:
>> if (sdata->vif.type != NL80211_IFTYPE_STATION &&
>> !(mask->control[band].legacy & basic_rates))
>
>
> That would forgo the check completely - we'd still need to check
> against
> the *supported* rates.
>
yes.
It can add check with supp_rates[band] of ieee80211_sta for
NL80211_IFTYPE_STATION type.
for others, check with sdata->vif.bss_conf.basic_rates
> johannes
On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
> yes.
> It can add check with supp_rates[band] of ieee80211_sta for
> NL80211_IFTYPE_STATION type.
> for others, check with sdata->vif.bss_conf.basic_rates
Right.
Though, might need to check that only if there's no TDLS station or
something?
johannes
On 2020-11-13 16:51, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
>
>> yes.
>> It can add check with supp_rates[band] of ieee80211_sta for
>> NL80211_IFTYPE_STATION type.
>> for others, check with sdata->vif.bss_conf.basic_rates
>
> Right.
>
> Though, might need to check that only if there's no TDLS station or
> something?
>
yes, I think it should do like that for TDLS:
test_sta_flag(sta, WLAN_STA_TDLS_PEER)
> johannes
On Fri, 2020-11-13 at 17:09 +0800, Wen Gong wrote:
> On 2020-11-13 16:51, Johannes Berg wrote:
> > On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
> >
> > > yes.
> > > It can add check with supp_rates[band] of ieee80211_sta for
> > > NL80211_IFTYPE_STATION type.
> > > for others, check with sdata->vif.bss_conf.basic_rates
> >
> > Right.
> >
> > Though, might need to check that only if there's no TDLS station or
> > something?
> >
> yes, I think it should do like that for TDLS:
> test_sta_flag(sta, WLAN_STA_TDLS_PEER)
Yes but you have to iterate all the stations and check they belong to
the interface and all that I think?
johannes
> > Yes but you have to iterate all the stations and check they belong to
> > the interface and all that I think?
> >
> Maybe not need.
> if NL80211_IFTYPE_STATION and NON-WLAN_STA_TDLS_PEER, it has only 1
> station for the ieee80211_vif
> by my understand.
Well, yes, but there's no counter.
So you need to iterate to see if there are any additional TDLS peers (in
addition to the STA entry for the AP).
johannes
On 2020-11-13 17:10, Johannes Berg wrote:
> On Fri, 2020-11-13 at 17:09 +0800, Wen Gong wrote:
>> On 2020-11-13 16:51, Johannes Berg wrote:
>> > On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
>> >
>> > > yes.
>> > > It can add check with supp_rates[band] of ieee80211_sta for
>> > > NL80211_IFTYPE_STATION type.
>> > > for others, check with sdata->vif.bss_conf.basic_rates
>> >
>> > Right.
>> >
>> > Though, might need to check that only if there's no TDLS station or
>> > something?
>> >
>> yes, I think it should do like that for TDLS:
>> test_sta_flag(sta, WLAN_STA_TDLS_PEER)
>
> Yes but you have to iterate all the stations and check they belong to
> the interface and all that I think?
>
Maybe not need.
if NL80211_IFTYPE_STATION and NON-WLAN_STA_TDLS_PEER, it has only 1
station for the ieee80211_vif
by my understand.
> johannes