diff mbox

mac80211: always pick a basic rate to tx RTS/CTS for pre-HT rates

Message ID 1365156384-6699-1-git-send-email-karl.beldan@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Karl Beldan April 5, 2013, 10:06 a.m. UTC 
From: Karl Beldan <karl.beldan@rivierawaves.com>

When the 1st rate control entry is a pre-HT rate we want to set
rts_cts_rate_idx "as the fastest basic rate that is not faster than the
data rate"(code comments).
But in case some bss allowed rate indexes are lower than the lowest bss
basic rate, if the rate control selects a rate among the formers for its
1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
rate index.
This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
this situation.

Note that the code assumes that lowest indexes == lowest bitrates.

Signed-off-by: Karl Beldan <karl.beldan@rivierawaves.com>
---
 net/mac80211/tx.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Johannes Berg April 9, 2013, 10:29 a.m. UTC | #1 
On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> When the 1st rate control entry is a pre-HT rate we want to set
> rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> data rate"(code comments).
> But in case some bss allowed rate indexes are lower than the lowest bss
> basic rate, if the rate control selects a rate among the formers for its
> 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> rate index.
> This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> this situation.

I guess it's a good thing you're looking at this code. However, I'm not
sure what you're doing here is correct. In this case, the PHY mandatory
rates should be used. See 9.7.6.5.2 "Selection of a rate or MCS":

        To allow the transmitting STA to calculate the contents of the
        Duration/ID field, a STA responding to a received frame
        transmits its control response frame at a primary rate, or at an
        alternate rate, or at an MCS, as specified by the following
        rules:

              * If a CTS or ACK control response frame is carried in a
                non-HT PPDU, the primary rate is defined to be the
                highest rate in the BSSBasicRateSet parameter that is
                less than or equal to the rate (or non-HT reference
                rate; see 9.7.9) of the previous frame. If no rate in
                the BSSBasicRateSet parameter meets these conditions,
                the primary rate is defined to be the highest mandatory
                rate of the attached PHY that is less than or equal to
                the rate (or non-HT reference rate; see 9.7.9) of the
                previous frame. The STA may select an alternate rate
                according to the rules in 9.7.6.5.4. The STA shall
                transmit the non-HT PPDU CTS or ACK control response
                frame at either the primary rate or the alternate rate,
                if one exists.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Beldan April 9, 2013, 10:55 a.m. UTC | #2 
On Tue, Apr 09, 2013 at 12:29:16PM +0200, Johannes Berg wrote:
> On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > When the 1st rate control entry is a pre-HT rate we want to set
> > rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> > data rate"(code comments).
> > But in case some bss allowed rate indexes are lower than the lowest bss
> > basic rate, if the rate control selects a rate among the formers for its
> > 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> > rate index.
> > This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> > this situation.
> 
> I guess it's a good thing you're looking at this code. However, I'm not
> sure what you're doing here is correct. In this case, the PHY mandatory
> rates should be used. See 9.7.6.5.2 "Selection of a rate or MCS":
> 
Thanks for looking at this.

You are quoting the chapter for "control _response_ frames" which does
not apply here (even CTS-to-self are not control response frames).
 
Karl
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg April 9, 2013, 11:05 a.m. UTC | #3 
On Tue, 2013-04-09 at 12:55 +0200, Karl Beldan wrote:
> On Tue, Apr 09, 2013 at 12:29:16PM +0200, Johannes Berg wrote:
> > On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> > > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > > 
> > > When the 1st rate control entry is a pre-HT rate we want to set
> > > rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> > > data rate"(code comments).
> > > But in case some bss allowed rate indexes are lower than the lowest bss
> > > basic rate, if the rate control selects a rate among the formers for its
> > > 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> > > rate index.
> > > This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> > > this situation.
> > 
> > I guess it's a good thing you're looking at this code. However, I'm not
> > sure what you're doing here is correct. In this case, the PHY mandatory
> > rates should be used. See 9.7.6.5.2 "Selection of a rate or MCS":
> > 
> Thanks for looking at this.
> 
> You are quoting the chapter for "control _response_ frames" which does
> not apply here (even CTS-to-self are not control response frames).

Oh, oops, confused. But then you look at 9.7.6.2 "Rate selection for
control frames that initiate a TXOP", which just mandates that you use
any rate that the receiver supports, so why bother doing basic rates
etc. at all?

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Beldan April 9, 2013, 11:10 a.m. UTC | #4 
On Tue, Apr 09, 2013 at 01:05:12PM +0200, Johannes Berg wrote:
> On Tue, 2013-04-09 at 12:55 +0200, Karl Beldan wrote:
> > On Tue, Apr 09, 2013 at 12:29:16PM +0200, Johannes Berg wrote:
> > > On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> > > > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > > > 
> > > > When the 1st rate control entry is a pre-HT rate we want to set
> > > > rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> > > > data rate"(code comments).
> > > > But in case some bss allowed rate indexes are lower than the lowest bss
> > > > basic rate, if the rate control selects a rate among the formers for its
> > > > 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> > > > rate index.
> > > > This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> > > > this situation.
> > > 
> > > I guess it's a good thing you're looking at this code. However, I'm not
> > > sure what you're doing here is correct. In this case, the PHY mandatory
> > > rates should be used. See 9.7.6.5.2 "Selection of a rate or MCS":
> > > 
> > Thanks for looking at this.
> > 
> > You are quoting the chapter for "control _response_ frames" which does
> > not apply here (even CTS-to-self are not control response frames).
> 
> Oh, oops, confused. But then you look at 9.7.6.2 "Rate selection for
> control frames that initiate a TXOP", which just mandates that you use
> any rate that the receiver supports, so why bother doing basic rates
> etc. at all?
> 

This chapter precisely, however it reads:
{
"If a control frame other than a Basic BlockAckReq or Basic BlockAck is
carried in a non-HT PPDU, the transmitting STA shall transmit the frame
using one of the rates in the BSSBasicRateSet parameter or a rate from
the mandatory rate set of the attached PHY if the BSSBasicRateSet is
empty."
}
 
Karl
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg April 9, 2013, 11:16 a.m. UTC | #5 
On Tue, 2013-04-09 at 13:10 +0200, Karl Beldan wrote:

> > Oh, oops, confused. But then you look at 9.7.6.2 "Rate selection for
> > control frames that initiate a TXOP", which just mandates that you use
> > any rate that the receiver supports, so why bother doing basic rates
> > etc. at all?
> > 
> 
> This chapter precisely, however it reads:
> {
> "If a control frame other than a Basic BlockAckReq or Basic BlockAck is
> carried in a non-HT PPDU, the transmitting STA shall transmit the frame
> using one of the rates in the BSSBasicRateSet parameter or a rate from
> the mandatory rate set of the attached PHY if the BSSBasicRateSet is
> empty."
> }

Oh, right. I'll review the patch again I guess :-)

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Beldan April 9, 2013, 11:21 a.m. UTC | #6 
On Tue, Apr 09, 2013 at 01:16:55PM +0200, Johannes Berg wrote:
> On Tue, 2013-04-09 at 13:10 +0200, Karl Beldan wrote:
> 
> > > Oh, oops, confused. But then you look at 9.7.6.2 "Rate selection for
> > > control frames that initiate a TXOP", which just mandates that you use
> > > any rate that the receiver supports, so why bother doing basic rates
> > > etc. at all?
> > > 
> > 
> > This chapter precisely, however it reads:
> > {
> > "If a control frame other than a Basic BlockAckReq or Basic BlockAck is
> > carried in a non-HT PPDU, the transmitting STA shall transmit the frame
> > using one of the rates in the BSSBasicRateSet parameter or a rate from
> > the mandatory rate set of the attached PHY if the BSSBasicRateSet is
> > empty."
> > }
> 
> Oh, right. I'll review the patch again I guess :-)
> 

Ok, thanks.
 
Karl
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg April 11, 2013, 10:02 a.m. UTC | #7 
On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
> 
> When the 1st rate control entry is a pre-HT rate we want to set
> rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> data rate"(code comments).
> But in case some bss allowed rate indexes are lower than the lowest bss
> basic rate, if the rate control selects a rate among the formers for its
> 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> rate index.
> This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> this situation.

Ok after reviewing this again I applied it.


>  	if (!(info->control.rates[0].flags & IEEE80211_TX_RC_MCS)) {
> -		s8 baserate = 0;
> +		u32 basic_rates = tx->sdata->vif.bss_conf.basic_rates;
> +		s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;

Note that this also assumes that rate 0 is a mandatory rate, which
presumably will always be true.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karl Beldan April 11, 2013, 10:42 a.m. UTC | #8 
On Thu, Apr 11, 2013 at 12:02:31PM +0200, Johannes Berg wrote:
> On Fri, 2013-04-05 at 12:06 +0200, Karl Beldan wrote:
> > From: Karl Beldan <karl.beldan@rivierawaves.com>
> > 
> > When the 1st rate control entry is a pre-HT rate we want to set
> > rts_cts_rate_idx "as the fastest basic rate that is not faster than the
> > data rate"(code comments).
> > But in case some bss allowed rate indexes are lower than the lowest bss
> > basic rate, if the rate control selects a rate among the formers for its
> > 1st rate control entry, rts_cts_rate_idx remains 0 and is not a basic
> > rate index.
> > This commit sets rts_cts_rate_idx to the lowest bss basic rate index in
> > this situation.
> 
> Ok after reviewing this again I applied it.
> 
> 
> >  	if (!(info->control.rates[0].flags & IEEE80211_TX_RC_MCS)) {
> > -		s8 baserate = 0;
> > +		u32 basic_rates = tx->sdata->vif.bss_conf.basic_rates;
> > +		s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;
> 
> Note that this also assumes that rate 0 is a mandatory rate, which
> presumably will always be true.
> 
basic_rates == 0 should not happen so it's more a defensive fallback
in case there are no basic rates programmed (which should not happen) to
prevent out of bonds accesses via rts_cts_rate_idx.



 
Karl
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index aad0bf5..c93483f 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -712,19 +712,22 @@  ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	}
 
 	/*
-	 * set up the RTS/CTS rate as the fastest basic rate
-	 * that is not faster than the data rate
+	 * Set up the RTS/CTS rate as the fastest basic rate
+	 * that is not faster than the data rate unless there
+	 * is no basic rate slower than the data rate, in which
+	 * case we pick the slowest basic rate
 	 *
 	 * XXX: Should this check all retry rates?
 	 */
 	if (!(info->control.rates[0].flags & IEEE80211_TX_RC_MCS)) {
-		s8 baserate = 0;
+		u32 basic_rates = tx->sdata->vif.bss_conf.basic_rates;
+		s8 baserate = basic_rates ? ffs(basic_rates - 1) : 0;
 
 		rate = &sband->bitrates[info->control.rates[0].idx];
 
 		for (i = 0; i < sband->n_bitrates; i++) {
 			/* must be a basic rate */
-			if (!(tx->sdata->vif.bss_conf.basic_rates & BIT(i)))
+			if (!(basic_rates & BIT(i)))
 				continue;
 			/* must not be faster than the data rate */
 			if (sband->bitrates[i].bitrate > rate->bitrate)