[PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Thomas Pedersen]

Some drivers (ath10k) report MCS 9 @ 20MHz, which
technically isn't allowed. To get more meaningful value
than 0 out of this however, just cap the bitrate for 20MHz
to MCS 8.

Signed-off-by: Thomas Pedersen <twp@qca.qualcomm.com>
---
 net/wireless/util.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/wireless/util.c b/net/wireless/util.c
index 0675f51..5fb0249 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1157,7 +1157,9 @@ static u32 cfg80211_calculate_bitrate_vht(struct rate_info *rate)
 		   58500000,
 		   65000000,
 		   78000000,
-		   0,
+		   /* some drivers report MCS 9 for 20MHz anyway. Clip to MCS 8
+		    * bitrate as it's closer than 0 */
+		   78000000,
 		},
 		{  13500000,
 		   27000000,
-- 
2.1.4

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Ben Greear]

On 09/06/2016 12:00 PM, Thomas Pedersen wrote:
> Some drivers (ath10k) report MCS 9 @ 20MHz, which
> technically isn't allowed. To get more meaningful value
> than 0 out of this however, just cap the bitrate for 20MHz
> to MCS 8.

If it is actually reporting MCS9, why lie about it?  Report it up
the stack as a proper value instead of hiding the issue?

Thanks,
Ben


>
> Signed-off-by: Thomas Pedersen <twp@qca.qualcomm.com>
> ---
>  net/wireless/util.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 0675f51..5fb0249 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -1157,7 +1157,9 @@ static u32 cfg80211_calculate_bitrate_vht(struct rate_info *rate)
>  		   58500000,
>  		   65000000,
>  		   78000000,
> -		   0,
> +		   /* some drivers report MCS 9 for 20MHz anyway. Clip to MCS 8
> +		    * bitrate as it's closer than 0 */
> +		   78000000,
>  		},
>  		{  13500000,
>  		   27000000,
>


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

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Pedersen, Thomas]

On 09/06/2016 12:07 PM, Ben Greear wrote:
> On 09/06/2016 12:00 PM, Thomas Pedersen wrote:
>> Some drivers (ath10k) report MCS 9 @ 20MHz, which
>> technically isn't allowed. To get more meaningful value
>> than 0 out of this however, just cap the bitrate for 20MHz
>> to MCS 8.
>
> If it is actually reporting MCS9, why lie about it?  Report it up
> the stack as a proper value instead of hiding the issue?

Good point, will send a v2 extrapolating the value to 86.5Mb/s.

--=20
thomas=

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Johannes Berg]

On Wed, 2016-09-07 at 18:20 +0000, Pedersen, Thomas wrote:
> On 09/06/2016 12:07 PM, Ben Greear wrote:
> > 
> > On 09/06/2016 12:00 PM, Thomas Pedersen wrote:
> > > 
> > > Some drivers (ath10k) report MCS 9 @ 20MHz, which
> > > technically isn't allowed. To get more meaningful value
> > > than 0 out of this however, just cap the bitrate for 20MHz
> > > to MCS 8.
> > 
> > If it is actually reporting MCS9, why lie about it?  Report it up
> > the stack as a proper value instead of hiding the issue?
> 
> Good point, will send a v2 extrapolating the value to 86.5Mb/s.

That makes no sense either, IMHO.

Are you saying that ath10k actually somehow manages to use an invalid
bitrate over the air?!

It seems more likely that it's actually just misreporting what it's
doing, and thus the issue should be fixed in ath10k.

johannes

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Ben Greear]

On 09/11/2016 11:43 PM, Johannes Berg wrote:
> On Wed, 2016-09-07 at 18:20 +0000, Pedersen, Thomas wrote:
>> On 09/06/2016 12:07 PM, Ben Greear wrote:
>>>
>>> On 09/06/2016 12:00 PM, Thomas Pedersen wrote:
>>>>
>>>> Some drivers (ath10k) report MCS 9 @ 20MHz, which
>>>> technically isn't allowed. To get more meaningful value
>>>> than 0 out of this however, just cap the bitrate for 20MHz
>>>> to MCS 8.
>>>
>>> If it is actually reporting MCS9, why lie about it?  Report it up
>>> the stack as a proper value instead of hiding the issue?
>>
>> Good point, will send a v2 extrapolating the value to 86.5Mb/s.
>
> That makes no sense either, IMHO.
>
> Are you saying that ath10k actually somehow manages to use an invalid
> bitrate over the air?!
>
> It seems more likely that it's actually just misreporting what it's
> doing, and thus the issue should be fixed in ath10k.

I saw ath10k report the same value in my testing.

Hard to know what the actual on-air encoding rate is.

I do know that some ath10k firmware would run 1Mbps encoding
management frames on 5Mhz, which is also not per spec, for what that is worth.

Thanks,
Ben

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

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Pedersen, Thomas]

T24gTW9uLCAyMDE2LTA5LTEyIGF0IDA4OjQzICswMjAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiBPbiBXZWQsIDIwMTYtMDktMDcgYXQgMTg6MjAgKzAwMDAsIFBlZGVyc2VuLCBUaG9tYXMgd3Jv
dGU6DQo+ID4gDQo+ID4gT24gMDkvMDYvMjAxNiAxMjowNyBQTSwgQmVuIEdyZWVhciB3cm90ZToN
Cj4gPiA+IA0KPiA+ID4gDQo+ID4gPiBPbiAwOS8wNi8yMDE2IDEyOjAwIFBNLCBUaG9tYXMgUGVk
ZXJzZW4gd3JvdGU6DQo+ID4gPiA+IA0KPiA+ID4gPiANCj4gPiA+ID4gU29tZSBkcml2ZXJzIChh
dGgxMGspIHJlcG9ydCBNQ1MgOSBAIDIwTUh6LCB3aGljaA0KPiA+ID4gPiB0ZWNobmljYWxseSBp
c24ndCBhbGxvd2VkLiBUbyBnZXQgbW9yZSBtZWFuaW5nZnVsIHZhbHVlDQo+ID4gPiA+IHRoYW4g
MCBvdXQgb2YgdGhpcyBob3dldmVyLCBqdXN0IGNhcCB0aGUgYml0cmF0ZSBmb3IgMjBNSHoNCj4g
PiA+ID4gdG8gTUNTIDguDQo+ID4gPiANCj4gPiA+IElmIGl0IGlzIGFjdHVhbGx5IHJlcG9ydGlu
ZyBNQ1M5LCB3aHkgbGllIGFib3V0IGl0P8KgwqBSZXBvcnQgaXQgdXANCj4gPiA+IHRoZSBzdGFj
ayBhcyBhIHByb3BlciB2YWx1ZSBpbnN0ZWFkIG9mIGhpZGluZyB0aGUgaXNzdWU/DQo+ID4gDQo+
ID4gR29vZCBwb2ludCwgd2lsbCBzZW5kIGEgdjIgZXh0cmFwb2xhdGluZyB0aGUgdmFsdWUgdG8g
ODYuNU1iL3MuDQo+IA0KPiBUaGF0IG1ha2VzIG5vIHNlbnNlIGVpdGhlciwgSU1ITy4NCj4gDQo+
IEFyZSB5b3Ugc2F5aW5nIHRoYXQgYXRoMTBrIGFjdHVhbGx5IHNvbWVob3cgbWFuYWdlcyB0byB1
c2UgYW4gaW52YWxpZA0KPiBiaXRyYXRlIG92ZXIgdGhlIGFpcj8hDQo+IA0KPiBJdCBzZWVtcyBt
b3JlIGxpa2VseSB0aGF0IGl0J3MgYWN0dWFsbHkganVzdCBtaXNyZXBvcnRpbmcgd2hhdCBpdCdz
DQo+IGRvaW5nLCBhbmQgdGh1cyB0aGUgaXNzdWUgc2hvdWxkIGJlIGZpeGVkIGluIGF0aDEway4N
Cg0KWWVhaCBzbyBhcHBhcmVudGx5IHRoZSBvdmVyaGVhZCBpbnZvbHZlZCBpbiAyNTYtUUFNIDUv
NiAoTUNTIDkpIHJlc3VsdHMNCmluIGxvd2VyIGVmZmVjdGl2ZSBiaXRyYXRlIHRoYW4ganVzdCB1
c2luZyBNQ1MgOCAodW5sZXNzIHlvdSdyZSB1c2luZyAzDQpzcGF0aWFsIHN0cmVhbXMpLiBTb3Vu
ZHMgbGlrZSBhIHJhdGUgY29udHJvbCBvciByZXBvcnRpbmcgYnVnIHRoZW4uDQpQbGVhc2UgZHJv
cCB0aGlzLg0KDQpUaGFua3MsDQpUaG9tYXM=

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Johannes Berg]

> Yeah so apparently the overhead involved in 256-QAM 5/6 (MCS 9)
> results in lower effective bitrate than just using MCS 8 (unless
> you're using 3 spatial streams).

Ah. I took a - very brief - look at why this one is invalid and
couldn't figure it out.

>  Sounds like a rate control or reporting bug then.
> Please drop this.
> 
Ok, thanks.

johannes

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Pedersen, Thomas]

T24gVHVlLCAyMDE2LTA5LTEzIGF0IDIwOjAyICswMjAwLCBKb2hhbm5lcyBCZXJnIHdyb3RlOg0K
PiA+IFllYWggc28gYXBwYXJlbnRseSB0aGUgb3ZlcmhlYWQgaW52b2x2ZWQgaW4gMjU2LVFBTSA1
LzYgKE1DUyA5KQ0KPiA+IHJlc3VsdHMgaW4gbG93ZXIgZWZmZWN0aXZlIGJpdHJhdGUgdGhhbiBq
dXN0IHVzaW5nIE1DUyA4ICh1bmxlc3MNCj4gPiB5b3UncmUgdXNpbmcgMyBzcGF0aWFsIHN0cmVh
bXMpLg0KPiANCj4gQWguIEkgdG9vayBhIC0gdmVyeSBicmllZiAtIGxvb2sgYXQgd2h5IHRoaXMg
b25lIGlzIGludmFsaWQgYW5kDQo+IGNvdWxkbid0IGZpZ3VyZSBpdCBvdXQuDQoNCkkgZG9uJ3Qg
a25vdyB3aGF0IEknbSB0YWxraW5nIGFib3V0LiBUaGlzIHdhcyBhIG1pc2NvbW11bmljYXRpb24g
ZnJvbQ0KdGhlIHN5c3RlbXMgdGVhbS4gSSB0aGluayBpdCB3YXMgYmVjYXVzZSBFVk0gdGFyZ2V0
cyB3ZXJlIHRvbyBoaWdoLCBidXQNCkxEUEMgZml4ZXMgdGhhdCwgc2VlIGJlbG93Lg0KDQo+ID4g
IFNvdW5kcyBsaWtlIGEgcmF0ZSBjb250cm9sIG9yIHJlcG9ydGluZyBidWcgdGhlbi4NCj4gPiBQ
bGVhc2UgZHJvcCB0aGlzLg0KPiA+IA0KPiBPaywgdGhhbmtzLg0KDQpBY3R1YWxseSwgY2FuIHlv
dSBhcHBseSB0aGUgdjIgKGNmZzgwMjExOiBhZGQgYml0cmF0ZSBmb3IgMjBNSHogTUNTIDkpDQpv
ZiB0aGlzPyBTeXN0ZW1zIGd1eXMgY29uZmlybWVkIHRoZXkgdXNlIE1DUyA5IEAgMjBNSHogd2hl
biBMRFBDIGlzDQplbmFibGVkLiBBbHNvIGNvbmZpcm1lZCBiaXRyYXRlIHNob3VsZCBiZSBvay4N
Cg0KVGhhbmtzLA0KDQp0aG9tYXMNCg==

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Johannes Berg]

> Actually, can you apply the v2 (cfg80211: add bitrate for 20MHz MCS
> 9) of this? Systems guys confirmed they use MCS 9 @ 20MHz when LDPC
> is enabled. Also confirmed bitrate should be ok.

I don't really understand that. How can the bitrate be "OK" when the
spec explicitly says it cannot be used?

johannes

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Arend Van Spriel]

On 19-9-2016 11:00, Johannes Berg wrote:
> 
>> Actually, can you apply the v2 (cfg80211: add bitrate for 20MHz MCS
>> 9) of this? Systems guys confirmed they use MCS 9 @ 20MHz when LDPC
>> is enabled. Also confirmed bitrate should be ok.
> 
> I don't really understand that. How can the bitrate be "OK" when the
> spec explicitly says it cannot be used?

This conversation sounds a lot like these two threads [1] and [2]. Just
not sure if our vendor specific IEs cover this rate as well.

Regards,
Arend

[1]
http://mid.gmane.org/1457633336-27898-1-git-send-email-greearb@candelatech.com
[2] 1461244669-19871-1-git-send-email-kvalo@qca.qualcomm.com

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Ben Greear]

On 09/19/2016 02:00 AM, Johannes Berg wrote:
>
>> Actually, can you apply the v2 (cfg80211: add bitrate for 20MHz MCS
>> 9) of this? Systems guys confirmed they use MCS 9 @ 20MHz when LDPC
>> is enabled. Also confirmed bitrate should be ok.
>
> I don't really understand that. How can the bitrate be "OK" when the
> spec explicitly says it cannot be used?
>
> johannes
>

I think it is a moot point as far as this change goes:  Regardless of whether
the NIC should or not, it _does_.  So, mis-reporting it up the stack only hides
the issue and does not even give the user a clue that on-the-air encoding may be
slightly off-spec.

If the on-air encoding is an issue, then we need to hack the firmware to
disable this 'feature', but that is a completely separate issue.

Once this patch goes in, someone might consider properly reporting CCK
rx rates for 5Ghz band too:  ath10k can do this 'feature' as well, at least
in some firmware.  Probably can reproduce by sending off-channel mgt frames on
5Ghz when associated on 2.4, or something similar to this.  I was using ath9k as
sniffer when I found this long ago, so at least ath9k needs the change....

Thanks,
Ben

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

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Johannes Berg]

Sorry - needed some time to think through this thread again.

> I think it is a moot point as far as this change goes:  Regardless of
> whether the NIC should or not, it _does_.  So, mis-reporting it up
> the stack only hides the issue and does not even give the user a clue
> that on-the-air encoding may be slightly off-spec.

Arguably, reporting something that *seems* sane, like this patch does,
will do more to hide it than reporting 0 which is clearly bogus, no?

I realize you were replying to whether or not the *driver* should
"misreport" it, but the same argument applies the other way here, imho.

> If the on-air encoding is an issue, then we need to hack the firmware
> to disable this 'feature', but that is a completely separate issue.

I guess it trusts rate control enough to try, and if it cannot be
received by a spec-abiding receiver, it won't use it due to the
failures ... not such a big deal I guess, even though it's odd.

Does /anyone/ know why the spec disallowed it? Perhaps those "system"
people you refer to?

> Once this patch goes in, someone might consider properly reporting
> CCK rx rates for 5Ghz band too:  ath10k can do this 'feature' as
> well, at least in some firmware.  Probably can reproduce by sending
> off-channel mgt frames on 5Ghz when associated on 2.4, or something
> similar to this.  I was using ath9k as sniffer when I found this long
> ago, so at least ath9k needs the change....

I don't see how this is related? It's not advertising those rates, so
it probably also can't use/report them as far as mac80211 and the rest
of the stack are concerned.

johannes

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Ben Greear]

On 10/04/2016 02:32 AM, Johannes Berg wrote:
> Sorry - needed some time to think through this thread again.
>
>> I think it is a moot point as far as this change goes:  Regardless of
>> whether the NIC should or not, it _does_.  So, mis-reporting it up
>> the stack only hides the issue and does not even give the user a clue
>> that on-the-air encoding may be slightly off-spec.
>
> Arguably, reporting something that *seems* sane, like this patch does,
> will do more to hide it than reporting 0 which is clearly bogus, no?
>
> I realize you were replying to whether or not the *driver* should
> "misreport" it, but the same argument applies the other way here, imho.

If the firmware/NIC is putting it on air at a particular encoding, then
I think the stack should report it exactly as it is on air if possible.

Returning zero is not much of a clue to the user.  Adding a WARN_ON_ONCE()
might be more useful, but no one can fix this by modifying the driver or stack, so the warning
will hit, confuse people, and then still never get fixed since it is a firmware
issue.

And, since someone added specific code to the firmware to use this rate
in certain cases, then maybe it is actually a good thing to do.

>> If the on-air encoding is an issue, then we need to hack the firmware
>> to disable this 'feature', but that is a completely separate issue.
>
> I guess it trusts rate control enough to try, and if it cannot be
> received by a spec-abiding receiver, it won't use it due to the
> failures ... not such a big deal I guess, even though it's odd.
>
> Does /anyone/ know why the spec disallowed it? Perhaps those "system"
> people you refer to?

I didn't talk to any system people, but maybe Thomas did.  I can confirm
that the firmware appears to do this on purpose and not just to be buggy
on accident though.

>> Once this patch goes in, someone might consider properly reporting
>> CCK rx rates for 5Ghz band too:  ath10k can do this 'feature' as
>> well, at least in some firmware.  Probably can reproduce by sending
>> off-channel mgt frames on 5Ghz when associated on 2.4, or something
>> similar to this.  I was using ath9k as sniffer when I found this long
>> ago, so at least ath9k needs the change....
>
> I don't see how this is related? It's not advertising those rates, so
> it probably also can't use/report them as far as mac80211 and the rest
> of the stack are concerned.

I can guarantee you that at least some ath10k firmware has a bug that will
send CCK encodings on 5Ghz.  This is a bug in the firmware, but the packets
will go on air, and they can be decoded by ath9k (and ath10k)
and passed up the stack.

Stock ath9k driver will ignore them (or maybe lie and say rate is 6Mbps, I cannot
recall at this point) currently because it thinks the rate is
bad (since it sort of is).  But, if you relax ath9k, then it can actually
sniff these frames and give you a clue as to what is really wrong.

It reminded me of hiding the rate by setting it to zero, since in both cases,
overly strict decisions based on what is *supposed* to happen caused me
to have to dig closely and modify code in order to understand what is
actually happening on-air.

Basically, if the NIC can decode a frame, and checksums pass and so forth,
then it seems it should pass it up the stack.  Possibly with WARN_ONCE to
give use a better clue that there is an issue.

Thanks,
Ben

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

Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8

[Johannes Berg]

> If the firmware/NIC is putting it on air at a particular encoding,
> then I think the stack should report it exactly as it is on air if
> possible.

It already does. We're only debating what bitrate to report :P

Anyway, I don't have the latest patch anymore - somebody please resend
it.

johannes