[PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Karl Beldan]

From: Karl Beldan <karl.beldan@rivierawaves.com>

Fixes: 5253ffb8 ("mac80211: always pick a basic rate to tx RTS/CTS for pre-HT rates")
Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/rate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 8fdadfd..6081329 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -448,7 +448,7 @@ static void rate_fixup_ratelist(struct ieee80211_vif *vif,
 	 */
 	if (!(rates[0].flags & IEEE80211_TX_RC_MCS)) {
 		u32 basic_rates = vif->bss_conf.basic_rates;
-		s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;
+		s8 baserate = basic_rates ? ffs(basic_rates) - 1 : 0;
 
 		rate = &sband->bitrates[rates[0].idx];
 
-- 
2.0.1

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Johannes Berg]

On Tue, 2014-10-07 at 15:53 +0200, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>

Is that really trivial? It seems to have some impact on the code, but I
can't right now say exactly what the impact is. Can you describe it and
say whether I should add it to mac80211 or mac80211-next? For "trivial"
I'd probably say mac80211-next, but this might be more important than
that?

johannes

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Karl Beldan]

On Thu, Oct 09, 2014 at 11:06:26AM +0200, Johannes Berg wrote:
> On Tue, 2014-10-07 at 15:53 +0200, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> Is that really trivial? It seems to have some impact on the code, but I
> can't right now say exactly what the impact is. Can you describe it and
> say whether I should add it to mac80211 or mac80211-next? For "trivial"
> I'd probably say mac80211-next, but this might be more important than
> that?
> 
The typo is clearly showing but the faulty behavior clearly demands more
detail indeed.

It affects non-(V)HT rates and can lead to selecting an rts_cts rate
that is not a basic rate or way superior to the reference rate (ATM
rates[0] used for the 1st attempt of the protected frame data).
E.g, assuming the drivers register growing (bitrate) sorted
ieee80211_rate tables, having :
- rates[0].idx == d'2 and basic_rates == b'10100
will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
- rates[0].idx == d'2 and basic_rates == b'10001 
will select rts_cts idx b'10000
The first is not a basic rate and the second is > rates[0].

I hope it clarifies things enough.
 
Karl

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Johannes Berg]

On Thu, 2014-10-09 at 16:06 +0200, Karl Beldan wrote:
> On Thu, Oct 09, 2014 at 11:06:26AM +0200, Johannes Berg wrote:
> > On Tue, 2014-10-07 at 15:53 +0200, Karl Beldan wrote:
> > > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > Is that really trivial? It seems to have some impact on the code, but I
> > can't right now say exactly what the impact is. Can you describe it and
> > say whether I should add it to mac80211 or mac80211-next? For "trivial"
> > I'd probably say mac80211-next, but this might be more important than
> > that?
> > 
> The typo is clearly showing but the faulty behavior clearly demands more
> detail indeed.
> 
> It affects non-(V)HT rates and can lead to selecting an rts_cts rate
> that is not a basic rate or way superior to the reference rate (ATM
> rates[0] used for the 1st attempt of the protected frame data).
> E.g, assuming the drivers register growing (bitrate) sorted
> ieee80211_rate tables, having :
> - rates[0].idx == d'2 and basic_rates == b'10100
> will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
> - rates[0].idx == d'2 and basic_rates == b'10001 
> will select rts_cts idx b'10000
> The first is not a basic rate and the second is > rates[0].
> 
> I hope it clarifies things enough.

Well, I'm still not sure which tree I should put it in, I guess?

johannes

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Karl Beldan]

On Thu, Oct 09, 2014 at 04:15:03PM +0200, Johannes Berg wrote:
> On Thu, 2014-10-09 at 16:06 +0200, Karl Beldan wrote:
> > On Thu, Oct 09, 2014 at 11:06:26AM +0200, Johannes Berg wrote:
> > > On Tue, 2014-10-07 at 15:53 +0200, Karl Beldan wrote:
> > > > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > > 
> > > Is that really trivial? It seems to have some impact on the code, but I
> > > can't right now say exactly what the impact is. Can you describe it and
> > > say whether I should add it to mac80211 or mac80211-next? For "trivial"
> > > I'd probably say mac80211-next, but this might be more important than
> > > that?
> > > 
> > The typo is clearly showing but the faulty behavior clearly demands more
> > detail indeed.
> > 
> > It affects non-(V)HT rates and can lead to selecting an rts_cts rate
> > that is not a basic rate or way superior to the reference rate (ATM
> > rates[0] used for the 1st attempt of the protected frame data).
> > E.g, assuming the drivers register growing (bitrate) sorted
> > ieee80211_rate tables, having :
> > - rates[0].idx == d'2 and basic_rates == b'10100
> > will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
> > - rates[0].idx == d'2 and basic_rates == b'10001 
> > will select rts_cts idx b'10000
> > The first is not a basic rate and the second is > rates[0].
> > 
> > I hope it clarifies things enough.
> 
> Well, I'm still not sure which tree I should put it in, I guess?
> 

All I can say is that nor this faulty behavior nor the correspond fix
are likely to cause a crash (we always tx registered rates).
 
Karl

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Johannes Berg]

On Thu, 2014-10-09 at 16:45 +0200, Karl Beldan wrote:

> > > The typo is clearly showing but the faulty behavior clearly demands more
> > > detail indeed.
> > > 
> > > It affects non-(V)HT rates and can lead to selecting an rts_cts rate
> > > that is not a basic rate or way superior to the reference rate (ATM
> > > rates[0] used for the 1st attempt of the protected frame data).
> > > E.g, assuming the drivers register growing (bitrate) sorted
> > > ieee80211_rate tables, having :
> > > - rates[0].idx == d'2 and basic_rates == b'10100
> > > will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
> > > - rates[0].idx == d'2 and basic_rates == b'10001 
> > > will select rts_cts idx b'10000
> > > The first is not a basic rate and the second is > rates[0].
> > > 
> > > I hope it clarifies things enough.
> > 
> > Well, I'm still not sure which tree I should put it in, I guess?
> > 
> 
> All I can say is that nor this faulty behavior nor the correspond fix
> are likely to cause a crash (we always tx registered rates).

But it's still pretty misbehaving, no? That IMHO warrants fixing and
even cc stable.

johannes

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Karl Beldan]

On Thu, Oct 09, 2014 at 05:22:21PM +0200, Johannes Berg wrote:
> On Thu, 2014-10-09 at 16:45 +0200, Karl Beldan wrote:
> 
> > > > The typo is clearly showing but the faulty behavior clearly demands more
> > > > detail indeed.
> > > > 
> > > > It affects non-(V)HT rates and can lead to selecting an rts_cts rate
> > > > that is not a basic rate or way superior to the reference rate (ATM
> > > > rates[0] used for the 1st attempt of the protected frame data).
> > > > E.g, assuming the drivers register growing (bitrate) sorted
> > > > ieee80211_rate tables, having :
> > > > - rates[0].idx == d'2 and basic_rates == b'10100
> > > > will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
> > > > - rates[0].idx == d'2 and basic_rates == b'10001 
> > > > will select rts_cts idx b'10000
> > > > The first is not a basic rate and the second is > rates[0].
> > > > 
> > > > I hope it clarifies things enough.
> > > 
> > > Well, I'm still not sure which tree I should put it in, I guess?
> > > 
> > 
> > All I can say is that nor this faulty behavior nor the correspond fix
> > are likely to cause a crash (we always tx registered rates).
> 
> But it's still pretty misbehaving, no? That IMHO warrants fixing and
> even cc stable.
> 
> johannes

You'd like me to reword with these details in a v2 and cc stable or ?
 
Karl

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Johannes Berg]

On Thu, 2014-10-09 at 18:28 +0200, Karl Beldan wrote:

> > But it's still pretty misbehaving, no? That IMHO warrants fixing and
> > even cc stable.

> You'd like me to reword with these details in a v2 and cc stable or ?

Yes please, if you don't mind.

johannes

Re: [PATCH] mac80211/trivial: fix typo in starting baserate for rts_cts_rate_idx

[Karl Beldan]

On Thu, Oct 09, 2014 at 05:22:21PM +0200, Johannes Berg wrote:
> On Thu, 2014-10-09 at 16:45 +0200, Karl Beldan wrote:
> 
> > > > The typo is clearly showing but the faulty behavior clearly demands more
> > > > detail indeed.
> > > > 
> > > > It affects non-(V)HT rates and can lead to selecting an rts_cts rate
> > > > that is not a basic rate or way superior to the reference rate (ATM
> > > > rates[0] used for the 1st attempt of the protected frame data).
> > > > E.g, assuming the drivers register growing (bitrate) sorted
> > > > ieee80211_rate tables, having :
> > > > - rates[0].idx == d'2 and basic_rates == b'10100
> > > > will select rts_cts idx b'10011 & ~d'(BIT(2)-1), i.e. 1, likewise
> > > > - rates[0].idx == d'2 and basic_rates == b'10001 
> > > > will select rts_cts idx b'10000
> > > > The first is not a basic rate and the second is > rates[0].
> > > > 
> > > > I hope it clarifies things enough.
> > > 
> > > Well, I'm still not sure which tree I should put it in, I guess?
> > > 
> > 
> > All I can say is that nor this faulty behavior nor the correspond fix
> > are likely to cause a crash (we always tx registered rates).
> 
> But it's still pretty misbehaving, no? That IMHO warrants fixing and
> even cc stable.
> 
Also, for the record, ATM we have only one rts_cts rate. A way to handle
every retry rates is still missing, along with (V)HT rates.
 
Karl