[PATCH 1/2] wireless: set correct mandatory rate flags

[PATCH 1/2] wireless: set correct mandatory rate flags

[Richard Schütz]

According to IEEE Std 802.11-2016 (16.2.3.4 Long PHY SIGNAL field) all of
the following rates are mandatory for a HR/DSSS PHY: 1 Mb/s, 2 Mb/s,
5.5 Mb/s and 11 Mb/s. Set IEEE80211_RATE_MANDATORY_B flag for all of these
instead of just 1 Mb/s to correctly reflect this.

Signed-off-by: Richard Schütz <rschuetz@uni-koblenz.de>
---
 net/wireless/util.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index bcb1284c3415..c69b5c31caf8 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -157,32 +157,28 @@ static void set_mandatory_flags_band(struct ieee80211_supported_band *sband)
 	case NL80211_BAND_2GHZ:
 		want = 7;
 		for (i = 0; i < sband->n_bitrates; i++) {
-			if (sband->bitrates[i].bitrate == 10) {
+			if (sband->bitrates[i].bitrate == 10 ||
+			    sband->bitrates[i].bitrate == 20 ||
+			    sband->bitrates[i].bitrate == 55 ||
+			    sband->bitrates[i].bitrate == 110) {
 				sband->bitrates[i].flags |=
 					IEEE80211_RATE_MANDATORY_B |
 					IEEE80211_RATE_MANDATORY_G;
 				want--;
+			} else {
+				sband->bitrates[i].flags |=
+					IEEE80211_RATE_ERP_G;
 			}
 
-			if (sband->bitrates[i].bitrate == 20 ||
-			    sband->bitrates[i].bitrate == 55 ||
-			    sband->bitrates[i].bitrate == 110 ||
-			    sband->bitrates[i].bitrate == 60 ||
+			if (sband->bitrates[i].bitrate == 60 ||
 			    sband->bitrates[i].bitrate == 120 ||
 			    sband->bitrates[i].bitrate == 240) {
 				sband->bitrates[i].flags |=
 					IEEE80211_RATE_MANDATORY_G;
 				want--;
 			}
-
-			if (sband->bitrates[i].bitrate != 10 &&
-			    sband->bitrates[i].bitrate != 20 &&
-			    sband->bitrates[i].bitrate != 55 &&
-			    sband->bitrates[i].bitrate != 110)
-				sband->bitrates[i].flags |=
-					IEEE80211_RATE_ERP_G;
 		}
-		WARN_ON(want != 0 && want != 3 && want != 6);
+		WARN_ON(want != 0 && want != 3);
 		break;
 	case NL80211_BAND_60GHZ:
 		/* check for mandatory HT MCS 1..4 */
-- 
2.14.1

[PATCH 2/2] wireless: return correct mandatory rates

[Richard Schütz]

Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B for
comparison to get all mandatory rates in 2.4 GHz band. It is safe to do so
because ERP mandatory rates are a superset of HR/DSSS mandatory rates. Also
force IEEE80211_RATE_MANDATORY_A for 10 MHz and 5 MHz channels as they use
"half-clocked" respectively "quarter-clocked" operation of the OFDM rates
(IEEE Std 802.11-2016, 17.1.1).

Signed-off-by: Richard Schütz <rschuetz@uni-koblenz.de>
---
 net/wireless/util.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index c69b5c31caf8..8cac453302f7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -48,15 +48,12 @@ u32 ieee80211_mandatory_rates(struct ieee80211_supported_band *sband,
 	if (WARN_ON(!sband))
 		return 1;
 
-	if (sband->band == NL80211_BAND_2GHZ) {
-		if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
-		    scan_width == NL80211_BSS_CHAN_WIDTH_10)
-			mandatory_flag = IEEE80211_RATE_MANDATORY_G;
-		else
-			mandatory_flag = IEEE80211_RATE_MANDATORY_B;
-	} else {
+	if (sband->band == NL80211_BAND_2GHZ &&
+	    scan_width != NL80211_BSS_CHAN_WIDTH_5 &&
+	    scan_width != NL80211_BSS_CHAN_WIDTH_10)
+		mandatory_flag = IEEE80211_RATE_MANDATORY_G;
+	else
 		mandatory_flag = IEEE80211_RATE_MANDATORY_A;
-	}
 
 	bitrates = sband->bitrates;
 	for (i = 0; i < sband->n_bitrates; i++)
-- 
2.14.1

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Johannes Berg]

On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
> for comparison to get all mandatory rates in 2.4 GHz band. It is safe
> to do so because ERP mandatory rates are a superset of HR/DSSS
> mandatory rates.

This I don't understand - what "comparison" are you talking about?

> Also force IEEE80211_RATE_MANDATORY_A for 10 MHz and 5 MHz channels
> as they use "half-clocked" respectively "quarter-clocked" operation
> of the OFDM rates (IEEE Std 802.11-2016, 17.1.1).

I don't think this is correct - the way the flags are used, anything on
2.4 GHz would never bother to check the MANDATORY_A flag.

johannes

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Richard Schütz]

Am 08.09.2017 um 08:55 schrieb Johannes Berg:
> On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
>> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
>> for comparison to get all mandatory rates in 2.4 GHz band. It is safe
>> to do so because ERP mandatory rates are a superset of HR/DSSS
>> mandatory rates.
> This I don't understand - what "comparison" are you talking about?

Sorry, I meant the condition that checks for the presence of 
mandatory_flag at the bottom of the function.

>> Also force IEEE80211_RATE_MANDATORY_A for 10 MHz and 5 MHz channels
>> as they use "half-clocked" respectively "quarter-clocked" operation
>> of the OFDM rates (IEEE Std 802.11-2016, 17.1.1).
> I don't think this is correct - the way the flags are used, anything on
> 2.4 GHz would never bother to check the MANDATORY_A flag.

Do we actually allow 10 MHz and 5 MHz operation in the 2.4 GHz band? As 
far as I can tell that has only been specified for OFDM PHYs, which use 
the 5 GHz band and are covered by IEEE80211_RATE_MANDATORY_A, but I am 
not a hundred per cent sure about that. Cc'ing Simon Wunderlich who 
originally implemented checking of scan_width here.

The main intention of this patch series is to fix mandatory rates 
returned for normal operation in 2.4 GHz band. Currently only 1 Mb/s is 
returned here, which is wrong for both HR/DSSS and ERP PHYs.

-- 
Richard

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Richard Schütz]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

Am 08.09.2017 um 10:43 schrieb Richard Schütz:
> Am 08.09.2017 um 08:55 schrieb Johannes Berg:
>> On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
>>> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
>>> for comparison to get all mandatory rates in 2.4 GHz band. It is safe
>>> to do so because ERP mandatory rates are a superset of HR/DSSS
>>> mandatory rates.
>> This I don't understand - what "comparison" are you talking about?
> 
> Sorry, I meant the condition that checks for the presence of 
> mandatory_flag at the bottom of the function.
> 
>>> Also force IEEE80211_RATE_MANDATORY_A for 10 MHz and 5 MHz channels
>>> as they use "half-clocked" respectively "quarter-clocked" operation
>>> of the OFDM rates (IEEE Std 802.11-2016, 17.1.1).
>> I don't think this is correct - the way the flags are used, anything on
>> 2.4 GHz would never bother to check the MANDATORY_A flag.
> 
> Do we actually allow 10 MHz and 5 MHz operation in the 2.4 GHz band? As 
> far as I can tell that has only been specified for OFDM PHYs, which use 
> the 5 GHz band and are covered by IEEE80211_RATE_MANDATORY_A, but I am 
> not a hundred per cent sure about that. Cc'ing Simon Wunderlich who 
> originally implemented checking of scan_width here.

Looks like the old address is invalid now. New try.

> The main intention of this patch series is to fix mandatory rates 
> returned for normal operation in 2.4 GHz band. Currently only 1 Mb/s is 
> returned here, which is wrong for both HR/DSSS and ERP PHYs.

-- 
Richard


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5087 bytes --]

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

Hi,

On Friday, September 8, 2017 10:53:37 AM CEST Richard Schütz wrote:
> Am 08.09.2017 um 10:43 schrieb Richard Schütz:
> > Am 08.09.2017 um 08:55 schrieb Johannes Berg:
> >> On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
> >>> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
> >>> for comparison to get all mandatory rates in 2.4 GHz band. It is safe
> >>> to do so because ERP mandatory rates are a superset of HR/DSSS
> >>> mandatory rates.
> >> 
> >> This I don't understand - what "comparison" are you talking about?
> > 
> > Sorry, I meant the condition that checks for the presence of
> > mandatory_flag at the bottom of the function.
> > 
> >>> Also force IEEE80211_RATE_MANDATORY_A for 10 MHz and 5 MHz channels
> >>> as they use "half-clocked" respectively "quarter-clocked" operation
> >>> of the OFDM rates (IEEE Std 802.11-2016, 17.1.1).
> >> 
> >> I don't think this is correct - the way the flags are used, anything on
> >> 2.4 GHz would never bother to check the MANDATORY_A flag.
> > 
> > Do we actually allow 10 MHz and 5 MHz operation in the 2.4 GHz band? As
> > far as I can tell that has only been specified for OFDM PHYs, which use
> > the 5 GHz band and are covered by IEEE80211_RATE_MANDATORY_A, but I am
> > not a hundred per cent sure about that. Cc'ing Simon Wunderlich who
> > originally implemented checking of scan_width here.
> 
> Looks like the old address is invalid now. New try.
> 

Yeah, officially only OFDM has the half/quarter clock stuff defined, not ERP (2.4 
GHz 11g) or DSSS, and also not HT.

However, technically, the Qualcomm/Atheros hardware (ath9k and ath5k) supports 
DSSS or HT on quarter and half rates just fine, also on 2.4 GHz.

I believe we currently support the 5/10 MHz on 2.4 GHz, although we shouldn't 
when we follow the standard strictly. The question is if we should follow the 
standard strictly - this feature is already quite limited, and people tend to 
use the ath9k/ath5k chanbw patch from OpenWRT/LEDE.

Cheers,
      Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Johannes Berg]

On Fri, 2017-09-08 at 10:43 +0200, Richard Schütz wrote:
> Am 08.09.2017 um 08:55 schrieb Johannes Berg:
> > On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
> > > Use IEEE80211_RATE_MANDATORY_G instead of
> > > IEEE80211_RATE_MANDATORY_B
> > > for comparison to get all mandatory rates in 2.4 GHz band. It is
> > > safe
> > > to do so because ERP mandatory rates are a superset of HR/DSSS
> > > mandatory rates.
> > 
> > This I don't understand - what "comparison" are you talking about?
> 
> Sorry, I meant the condition that checks for the presence of 
> mandatory_flag at the bottom of the function.

Ah, sorry, I got confused with the other patch.

> Do we actually allow 10 MHz and 5 MHz operation in the 2.4 GHz band?
> As  far as I can tell that has only been specified for OFDM PHYs,
> which use the 5 GHz band and are covered by
> IEEE80211_RATE_MANDATORY_A, but I am not a hundred per cent sure
> about that. Cc'ing Simon Wunderlich who originally implemented
> checking of scan_width here.

Clearly we do allow that, since the existing check is:

        if (sband->band == NL80211_BAND_2GHZ) {
                if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
                    scan_width == NL80211_BSS_CHAN_WIDTH_10)
                        mandatory_flag = IEEE80211_RATE_MANDATORY_G;

That wouldn't make any sense if we didn't have 5/10 MHz on 2.4 GHz.

> The main intention of this patch series is to fix mandatory rates 
> returned for normal operation in 2.4 GHz band. Currently only 1 Mb/s
> is returned here, which is wrong for both HR/DSSS and ERP PHYs.

The patch is still wrong wrt. 5/10 MHz though.

I think what you really wanted to do is the following:

 * rename RATE_MANDATORY_B to RATE_MANDATORY_HR_DSSS
 * combine RATE_MANDATORY_G/_A to RATE_MANDATORY_OFDM

Then, what you need to do, is change the checks in
ieee80211_mandatory_rates() to be

        if (sband->band == NL80211_BAND_2GHZ) {
                if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
                    scan_width == NL80211_BSS_CHAN_WIDTH_10)
                        mandatory_flag = IEEE80211_RATE_MANDATORY_OFDM;
                else
                        mandatory_flag = IEEE80211_RATE_MANDATORY_HR_DSSS;
        } else {
                mandatory_flag = IEEE80211_RATE_MANDATORY_OFDM;
        }

That would actually fix a bug in a way because right now the code
treats HR/DSSS rates (1, 2, 5.5, 11) for 2.4 GHz narrow-band operation
as mandatory, which seems wrong.

johannes

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Richard Schütz]

Am 08.09.2017 um 11:03 schrieb Johannes Berg:
> On Fri, 2017-09-08 at 10:43 +0200, Richard Schütz wrote:
>> Am 08.09.2017 um 08:55 schrieb Johannes Berg:
>>> On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
>>>> Use IEEE80211_RATE_MANDATORY_G instead of
>>>> IEEE80211_RATE_MANDATORY_B
>>>> for comparison to get all mandatory rates in 2.4 GHz band. It is
>>>> safe
>>>> to do so because ERP mandatory rates are a superset of HR/DSSS
>>>> mandatory rates.
>>>
>>> This I don't understand - what "comparison" are you talking about?
>>
>> Sorry, I meant the condition that checks for the presence of
>> mandatory_flag at the bottom of the function.
> 
> Ah, sorry, I got confused with the other patch.
> 
>> Do we actually allow 10 MHz and 5 MHz operation in the 2.4 GHz band?
>> As  far as I can tell that has only been specified for OFDM PHYs,
>> which use the 5 GHz band and are covered by
>> IEEE80211_RATE_MANDATORY_A, but I am not a hundred per cent sure
>> about that. Cc'ing Simon Wunderlich who originally implemented
>> checking of scan_width here.
> 
> Clearly we do allow that, since the existing check is:
> 
>          if (sband->band == NL80211_BAND_2GHZ) {
>                  if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
>                      scan_width == NL80211_BSS_CHAN_WIDTH_10)
>                          mandatory_flag = IEEE80211_RATE_MANDATORY_G;
> 
> That wouldn't make any sense if we didn't have 5/10 MHz on 2.4 GHz.
> 
>> The main intention of this patch series is to fix mandatory rates
>> returned for normal operation in 2.4 GHz band. Currently only 1 Mb/s
>> is returned here, which is wrong for both HR/DSSS and ERP PHYs.
> 
> The patch is still wrong wrt. 5/10 MHz though.
> 
> I think what you really wanted to do is the following:
> 
>   * rename RATE_MANDATORY_B to RATE_MANDATORY_HR_DSSS
>   * combine RATE_MANDATORY_G/_A to RATE_MANDATORY_OFDM
> 
> Then, what you need to do, is change the checks in
> ieee80211_mandatory_rates() to be
> 
>          if (sband->band == NL80211_BAND_2GHZ) {
>                  if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
>                      scan_width == NL80211_BSS_CHAN_WIDTH_10)
>                          mandatory_flag = IEEE80211_RATE_MANDATORY_OFDM;
>                  else
>                          mandatory_flag = IEEE80211_RATE_MANDATORY_HR_DSSS;

This would leave us with 1, 2, 5.5 and 11 Mb/s for ERP PHYs again when 
it really should be 1, 2, 5.5, 6, 11, 12 and 24 Mb/s.

>          } else {
>                  mandatory_flag = IEEE80211_RATE_MANDATORY_OFDM;
>          }
> 
> That would actually fix a bug in a way because right now the code
> treats HR/DSSS rates (1, 2, 5.5, 11) for 2.4 GHz narrow-band operation
> as mandatory, which seems wrong.

-- 
Richard

Re: [PATCH 2/2] wireless: return correct mandatory rates

[Johannes Berg]

On Fri, 2017-09-08 at 12:10 +0200, Richard Schütz wrote:
> 
> This would leave us with 1, 2, 5.5 and 11 Mb/s for ERP PHYs again
> when it really should be 1, 2, 5.5, 6, 11, 12 and 24 Mb/s.
> 

Yes, you're right - I had thought about that but forgot. The places
that check it would have to be amended to check both HR_DSSS and OFDM
where they want ERP PHY operation.

johannes

[PATCH v2 2/2] wireless: return correct mandatory rates

[Richard Schütz]

Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B to get
all mandatory rates in 2.4 GHz band. It is safe to do so because ERP
mandatory rates are a superset of HR/DSSS mandatory rates. Also limit to
OFDM rates for 10 MHz and 5 MHz channels as originally intended by commit
74608aca4d82e.

Signed-off-by: Richard Schütz <rschuetz@uni-koblenz.de>
---
 net/wireless/util.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index c69b5c31caf8..386070e0035a 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -51,16 +51,17 @@ u32 ieee80211_mandatory_rates(struct ieee80211_supported_band *sband,
 	if (sband->band == NL80211_BAND_2GHZ) {
 		if (scan_width == NL80211_BSS_CHAN_WIDTH_5 ||
 		    scan_width == NL80211_BSS_CHAN_WIDTH_10)
-			mandatory_flag = IEEE80211_RATE_MANDATORY_G;
+			mandatory_flag = IEEE80211_RATE_MANDATORY_G |
+					 IEEE80211_RATE_ERP_G;
 		else
-			mandatory_flag = IEEE80211_RATE_MANDATORY_B;
+			mandatory_flag = IEEE80211_RATE_MANDATORY_G;
 	} else {
 		mandatory_flag = IEEE80211_RATE_MANDATORY_A;
 	}
 
 	bitrates = sband->bitrates;
 	for (i = 0; i < sband->n_bitrates; i++)
-		if (bitrates[i].flags & mandatory_flag)
+		if ((bitrates[i].flags & mandatory_flag) == mandatory_flag)
 			mandatory_rates |= BIT(i);
 	return mandatory_rates;
 }
-- 
2.14.1

Re: [PATCH v2 2/2] wireless: return correct mandatory rates

[Johannes Berg]

On Fri, 2017-09-08 at 18:07 +0200, Richard Schütz wrote:
> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
> to get
> all mandatory rates in 2.4 GHz band. It is safe to do so because ERP
> mandatory rates are a superset of HR/DSSS mandatory rates. Also limit
> to
> OFDM rates for 10 MHz and 5 MHz channels as originally intended by
> commit
> 74608aca4d82e.

I don't understand this. Why should all ERP (Clause 18, 11g) rates be
mandatory for the users of this?

The way I see it, this would make us assume that all 2.4 GHz clients
support ERP in IBSS, which may not be true?

johannes

Re: [PATCH v2 2/2] wireless: return correct mandatory rates

[Richard Schütz]

Am 21.09.2017 um 15:52 schrieb Johannes Berg:
> On Fri, 2017-09-08 at 18:07 +0200, Richard Schütz wrote:
>> Use IEEE80211_RATE_MANDATORY_G instead of IEEE80211_RATE_MANDATORY_B
>> to get
>> all mandatory rates in 2.4 GHz band. It is safe to do so because ERP
>> mandatory rates are a superset of HR/DSSS mandatory rates. Also limit
>> to
>> OFDM rates for 10 MHz and 5 MHz channels as originally intended by
>> commit
>> 74608aca4d82e.
> 
> I don't understand this. Why should all ERP (Clause 18, 11g) rates be
> mandatory for the users of this?
> 
> The way I see it, this would make us assume that all 2.4 GHz clients
> support ERP in IBSS, which may not be true?

I disagree. On a HR/DSSS PHY this would still only return HR/DSSS rates 
because there should not by any ERP rates present in sband->bitrates in 
the first place.

The reason for suggesting this change is that the basic rate set for a 
mesh STA is initialized with this function if not explicitly configured 
beforehand.

IEEE Std 802.11-2016 (10.7.4 Basic rate set, basic HT-MCS set, and basic 
VHT-MCS and NSS set for mesh STA) states: A mesh STA shall not establish 
a mesh peering with a mesh STA using a different BSSBasicRateSet (see 
14.2.7 and 14.2.8). The SME of a Mesh STA should use the mandatory PHY 
rates as the default BSSBasicRateSet in the MLME-START.request primitive 
to reduce the risk that a candidate peer mesh STA utilizes a different 
BSSBasicRateSet.

Selecting only HR/DSSS mandatory rates for a mesh STA default basic rate 
set on an ERP PHY violates this requirement in my opinion. 
wpa_supplicant explicitly configures all ERP mandatory rates for ERP 
mesh points in its default configuration, whereas iw relies on the 
kernel to do the selection. This currently leaves us with the 
unfortunate situation that you can not join such a mesh BSS created by 
wpa_supplicant using iw without further configuration and the other way 
round.

-- 
Richard

Re: [PATCH v2 2/2] wireless: return correct mandatory rates

[Johannes Berg]

On Fri, 2017-09-22 at 12:09 +0200, Richard Schütz wrote:

> > The way I see it, this would make us assume that all 2.4 GHz
> > clients support ERP in IBSS, which may not be true?
> 
> I disagree. On a HR/DSSS PHY this would still only return HR/DSSS
> rates because there should not by any ERP rates present in sband-
> >bitrates in the first place.

Sure, but that means that if you're an ERP PHY (which all users of this
code are likely to be), you'd assume that *everyone else* you're
talking to also is - and that's not necessarily desired.

> The reason for suggesting this change is that the basic rate set for
> a mesh STA is initialized with this function if not explicitly
> configured beforehand.

Exactly! So if you mark all the rates as basic that are mandatory for
you, you can never peer mesh with a HR/DSSS PHY. Now, that's probably
unlikely to happen - but for IBSS I'd argue it's reasonable, and the
same would happen there afaict.

> IEEE Std 802.11-2016 (10.7.4 Basic rate set, basic HT-MCS set, and
> basic VHT-MCS and NSS set for mesh STA) states: A mesh STA shall not
> establish a mesh peering with a mesh STA using a different
> BSSBasicRateSet (see 14.2.7 and 14.2.8). The SME of a Mesh STA should
> use the mandatory PHY rates as the default BSSBasicRateSet in the
> MLME-START.request primitive to reduce the risk that a candidate peer
> mesh STA utilizes a different BSSBasicRateSet.

"Should" isn't "shall"

> Selecting only HR/DSSS mandatory rates for a mesh STA default basic
> rate set on an ERP PHY violates this requirement in my opinion. 

It does seem to violate the "should", but arguably that's just a
configuration choice.

> wpa_supplicant explicitly configures all ERP mandatory rates for ERP 
> mesh points in its default configuration, whereas iw relies on the 
> kernel to do the selection. This currently leaves us with the 
> unfortunate situation that you can not join such a mesh BSS created
> by wpa_supplicant using iw without further configuration and the
> other way round.

So now we've finally gotten to the bottom of why you're doing all this?

Anyway, I disagree with the patch. I can get behind a patch changing
the mesh code, but not in a way that affects the IBSS like this.

johannes

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Johannes Berg]

On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
> According to IEEE Std 802.11-2016 (16.2.3.4 Long PHY SIGNAL field)
> all of
> the following rates are mandatory for a HR/DSSS PHY: 1 Mb/s, 2 Mb/s,
> 5.5 Mb/s and 11 Mb/s. Set IEEE80211_RATE_MANDATORY_B flag for all of
> these instead of just 1 Mb/s to correctly reflect this.

Hmm, I guess technically you're correct, since 11b added what's now
Clause 16 (was Clause 18 at the time), and that has all of them
mandatory? But perhaps this was actually intended for Clause 15
compatibility?

johannes

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Richard Schütz]

Am 08.09.2017 um 08:54 schrieb Johannes Berg:
> On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
>> According to IEEE Std 802.11-2016 (16.2.3.4 Long PHY SIGNAL field)
>> all of
>> the following rates are mandatory for a HR/DSSS PHY: 1 Mb/s, 2 Mb/s,
>> 5.5 Mb/s and 11 Mb/s. Set IEEE80211_RATE_MANDATORY_B flag for all of
>> these instead of just 1 Mb/s to correctly reflect this.
> 
> Hmm, I guess technically you're correct, since 11b added what's now
> Clause 16 (was Clause 18 at the time), and that has all of them
> mandatory? But perhaps this was actually intended for Clause 15
> compatibility?

Compatibility in what way?

-- 
Richard

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Johannes Berg]

On Fri, 2017-09-08 at 10:43 +0200, Richard Schütz wrote:

> > Hmm, I guess technically you're correct, since 11b added what's now
> > Clause 16 (was Clause 18 at the time), and that has all of them
> > mandatory? But perhaps this was actually intended for Clause 15
> > compatibility?
> 
> Compatibility in what way?

Well, realistically there are only three users of this information:

 * ieee80211_mandatory_rates(), used for supported station rates if we
   don't know anything better in IBSS (and OCB) and for basic rates in
   mesh;
 * basic rates in IBSS (__cfg80211_join_ibss);
 * duration calculation in ieee80211_duration, but that's just a
   fallback

So I guess it's now pretty unlikely that anyone would have a pre-11b
device, so it would make sense to actually make this change.

johannes

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Johannes Berg]

On Thu, 2017-09-07 at 17:47 +0200, Richard Schütz wrote:
> According to IEEE Std 802.11-2016 (16.2.3.4 Long PHY SIGNAL field)
> all of
> the following rates are mandatory for a HR/DSSS PHY: 1 Mb/s, 2 Mb/s,
> 5.5 Mb/s and 11 Mb/s. Set IEEE80211_RATE_MANDATORY_B flag for all of
> these
> instead of just 1 Mb/s to correctly reflect this.

Applied, but I modified it to use a switch statement.

johannes

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2564 bytes --]

On 09/07/2017 05:47 PM, Richard Schütz wrote:
> According to IEEE Std 802.11-2016 (16.2.3.4 Long PHY SIGNAL field) all of
> the following rates are mandatory for a HR/DSSS PHY: 1 Mb/s, 2 Mb/s,
> 5.5 Mb/s and 11 Mb/s. Set IEEE80211_RATE_MANDATORY_B flag for all of these
> instead of just 1 Mb/s to correctly reflect this.
> 
> Signed-off-by: Richard Schütz <rschuetz@uni-koblenz.de>

We've noticed that this is breaking interoperability of 11s nodes in
OpenWrt: association is only possible when neither or both peers have this
patch. I have not tested interop with non-Linux 11s peers.

I propose to revert this for now (I assume it's too late for 4.15, but
hopefully the regression can be fixed in 4.15.1).

Regards,
Matthias


> ---
>  net/wireless/util.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index bcb1284c3415..c69b5c31caf8 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -157,32 +157,28 @@ static void set_mandatory_flags_band(struct ieee80211_supported_band *sband)
>  	case NL80211_BAND_2GHZ:
>  		want = 7;
>  		for (i = 0; i < sband->n_bitrates; i++) {
> -			if (sband->bitrates[i].bitrate == 10) {
> +			if (sband->bitrates[i].bitrate == 10 ||
> +			    sband->bitrates[i].bitrate == 20 ||
> +			    sband->bitrates[i].bitrate == 55 ||
> +			    sband->bitrates[i].bitrate == 110) {
>  				sband->bitrates[i].flags |=
>  					IEEE80211_RATE_MANDATORY_B |
>  					IEEE80211_RATE_MANDATORY_G;
>  				want--;
> +			} else {
> +				sband->bitrates[i].flags |=
> +					IEEE80211_RATE_ERP_G;
>  			}
>  
> -			if (sband->bitrates[i].bitrate == 20 ||
> -			    sband->bitrates[i].bitrate == 55 ||
> -			    sband->bitrates[i].bitrate == 110 ||
> -			    sband->bitrates[i].bitrate == 60 ||
> +			if (sband->bitrates[i].bitrate == 60 ||
>  			    sband->bitrates[i].bitrate == 120 ||
>  			    sband->bitrates[i].bitrate == 240) {
>  				sband->bitrates[i].flags |=
>  					IEEE80211_RATE_MANDATORY_G;
>  				want--;
>  			}
> -
> -			if (sband->bitrates[i].bitrate != 10 &&
> -			    sband->bitrates[i].bitrate != 20 &&
> -			    sband->bitrates[i].bitrate != 55 &&
> -			    sband->bitrates[i].bitrate != 110)
> -				sband->bitrates[i].flags |=
> -					IEEE80211_RATE_ERP_G;
>  		}
> -		WARN_ON(want != 0 && want != 3 && want != 6);
> +		WARN_ON(want != 0 && want != 3);
>  		break;
>  	case NL80211_BAND_60GHZ:
>  		/* check for mandatory HT MCS 1..4 */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Johannes Berg]

On Fri, 2018-01-26 at 23:17 +0100, Matthias Schiffer wrote:
> 
> I propose to revert this for now (I assume it's too late for 4.15, but
> hopefully the regression can be fixed in 4.15.1).

I really don't think we should revert this, it fixes a real bug.

We can easily switch the default though, would something like this
help?

diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 51aa55618ef7..b12da6ef3c12 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -170,9 +170,28 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 		enum nl80211_bss_scan_width scan_width;
 		struct ieee80211_supported_band *sband =
 				rdev->wiphy.bands[setup->chandef.chan->band];
-		scan_width = cfg80211_chandef_to_scan_width(&setup->chandef);
-		setup->basic_rates = ieee80211_mandatory_rates(sband,
-							       scan_width);
+
+		if (setup->chandef.chan->band == NL80211_BAND_2GHZ) {
+			int i;
+
+			/*
+			 * Older versions selected the mandatory rates for
+			 * 2.4 GHz as well, but were broken in that only
+			 * 1 Mbps was regarded as a mandatory rate. Keep
+			 * using just 1 Mbps as the default basic rate for
+			 * mesh to be interoperable with older versions.
+			 */
+			for (i = 0; i < sband->n_bitrates; i++) {
+				if (sband->bitrates[i].bitrate == 10) {
+					setup->basic_rates = BIT(i);
+					break;
+				}
+			}
+		} else {
+			scan_width = cfg80211_chandef_to_scan_width(&setup->chandef);
+			setup->basic_rates = ieee80211_mandatory_rates(sband,
+								       scan_width);
+		}
 	}
 
 	err = cfg80211_chandef_dfs_required(&rdev->wiphy,

johannes

Re: [PATCH 1/2] wireless: set correct mandatory rate flags

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 1882 bytes --]

On 01/30/2018 08:43 AM, Johannes Berg wrote:
> On Fri, 2018-01-26 at 23:17 +0100, Matthias Schiffer wrote:
>>
>> I propose to revert this for now (I assume it's too late for 4.15, but
>> hopefully the regression can be fixed in 4.15.1).
> 
> I really don't think we should revert this, it fixes a real bug.
> 
> We can easily switch the default though, would something like this
> help?

Working perfectly.

Tested-by: Matthias Schiffer <mschiffer@universe-factory.net>


> 
> diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
> index 51aa55618ef7..b12da6ef3c12 100644
> --- a/net/wireless/mesh.c
> +++ b/net/wireless/mesh.c
> @@ -170,9 +170,28 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
>  		enum nl80211_bss_scan_width scan_width;
>  		struct ieee80211_supported_band *sband =
>  				rdev->wiphy.bands[setup->chandef.chan->band];
> -		scan_width = cfg80211_chandef_to_scan_width(&setup->chandef);
> -		setup->basic_rates = ieee80211_mandatory_rates(sband,
> -							       scan_width);
> +
> +		if (setup->chandef.chan->band == NL80211_BAND_2GHZ) {
> +			int i;
> +
> +			/*
> +			 * Older versions selected the mandatory rates for
> +			 * 2.4 GHz as well, but were broken in that only
> +			 * 1 Mbps was regarded as a mandatory rate. Keep
> +			 * using just 1 Mbps as the default basic rate for
> +			 * mesh to be interoperable with older versions.
> +			 */
> +			for (i = 0; i < sband->n_bitrates; i++) {
> +				if (sband->bitrates[i].bitrate == 10) {
> +					setup->basic_rates = BIT(i);
> +					break;
> +				}
> +			}
> +		} else {
> +			scan_width = cfg80211_chandef_to_scan_width(&setup->chandef);
> +			setup->basic_rates = ieee80211_mandatory_rates(sband,
> +								       scan_width);
> +		}
>  	}
>  
>  	err = cfg80211_chandef_dfs_required(&rdev->wiphy,
> 
> johannes
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]