ath9k's rate control code is so convoluted that it really make it
difficult to spot bugs and fix them. Here's a bunch of cleanups for
ath9k rate control code, and a few for mac80211. I just tested ath9k.
Will have to test others later.
Luis R. Rodriguez (15):
ath9k: fix oops by downgrading assert in rc.c
ath9k: cleanup try count for MRR in rate control
ath9k: remove unused min rate calculation code
ath9k: remove unused stepdown when looking for the next rate
ath9k: remove pointless wrapper ath_rc_rate_getidx()
ath9k: rename ath_rc_get_nextlowervalid_txrate()
ath9k: remove unused ath_rc_isvalid_txmask()
ath9k: remove ATH9K_MODE_11B
ath9k: remap ATH9K_MODE_*
ath9k: rename ath_rc_ratefind_ht() to ath_rc_get_highest_rix()
ath9k: remove unnecessary IEEE80211_TX_CTL_NO_ACK checks
mac80211: make minstrel/pid RC use ieee80211_is_data(fc)
iwlwifi: use ieee80211_is_data(fc)
mac80211: move management / no-ack frame rate decision to mac80211
ath9k: remove rate control wraper
drivers/net/wireless/ath/ath9k/ath9k.h | 1 -
drivers/net/wireless/ath/ath9k/debug.h | 1 +
drivers/net/wireless/ath/ath9k/hw.c | 1 -
drivers/net/wireless/ath/ath9k/hw.h | 17 ++--
drivers/net/wireless/ath/ath9k/main.c | 3 +-
drivers/net/wireless/ath/ath9k/rc.c | 190 ++++++++++------------------
drivers/net/wireless/iwlwifi/iwl-3945-rs.c | 21 +---
drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 10 --
drivers/net/wireless/iwlwifi/iwl-sta.c | 5 +-
net/mac80211/rate.c | 29 ++++-
net/mac80211/rc80211_minstrel.c | 24 ----
net/mac80211/rc80211_pid_algo.c | 15 ---
12 files changed, 111 insertions(+), 206 deletions(-)
This is just calling another helper, so just use the other
helper directly. This should make it clear that when do not
find the next rate we stick to the current one.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 21 ++++-----------------
1 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 557f054..6ccd565 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -820,19 +820,6 @@ static void ath_rc_rate_set_rtscts(struct ath_softc *sc,
tx_info->control.rts_cts_rate_idx = cix;
}
-static u8 ath_rc_rate_getidx(struct ath_softc *sc,
- struct ath_rate_priv *ath_rc_priv,
- const struct ath_rate_table *rate_table,
- u8 rix)
-{
- u8 nextindex = 0;
- if (ath_rc_get_nextlowervalid_txrate(rate_table,
- ath_rc_priv, rix, &nextindex))
- return nextindex;
- else
- return rix;
-}
-
static void ath_rc_ratefind(struct ath_softc *sc,
struct ath_rate_priv *ath_rc_priv,
struct ieee80211_tx_rate_control *txrc,
@@ -873,8 +860,8 @@ static void ath_rc_ratefind(struct ath_softc *sc,
/* Get the next tried/allowed rate. No RTS for the next series
* after the probe rate
*/
- nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix);
+ ath_rc_get_nextlowervalid_txrate(rate_table, ath_rc_priv,
+ rix, &nrix);
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
try_per_rate, nrix, 0);
@@ -891,8 +878,8 @@ static void ath_rc_ratefind(struct ath_softc *sc,
if (i + 1 == 4)
try_per_rate = 4;
- nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix);
+ ath_rc_get_nextlowervalid_txrate(rate_table, ath_rc_priv,
+ rix, &nrix);
/* All other rates in the series have RTS enabled */
ath_rc_rate_set_series(rate_table, &rates[i], txrc,
try_per_rate, nrix, 1);
--
1.6.0.6
This has no functional change and just cleans up the code
to be more legible and removes a useless variable for
Multi Rate Retry.
For regular frames we use 2 retries for MRR segments [0-2].
For the last MRR segment [3] we use 4.
MRR[0] = 2
MRR[1] = 2
MRR[2] = 2
MRR[3] = 4
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/ath9k.h | 1 -
drivers/net/wireless/ath/ath9k/main.c | 3 ++-
drivers/net/wireless/ath/ath9k/rc.c | 29 ++++++++++++++++++++---------
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 9cd523f..ca14642 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -169,7 +169,6 @@ void ath_descdma_cleanup(struct ath_softc *sc, struct ath_descdma *dd,
#define WME_NUM_TID 16
#define ATH_TXBUF 512
#define ATH_TXMAXTRY 13
-#define ATH_11N_TXMAXTRY 10
#define ATH_MGT_TXMAXTRY 4
#define WME_BA_BMP_SIZE 64
#define WME_MAX_BA WME_BA_BMP_SIZE
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index f7baa40..2270166 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1573,7 +1573,8 @@ void ath_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
hw->max_rates = 4;
hw->channel_change_time = 5000;
hw->max_listen_interval = 10;
- hw->max_rate_tries = ATH_11N_TXMAXTRY;
+ /* Hardware supports 10 but we use 4 */
+ hw->max_rate_tries = 4;
hw->sta_data_size = sizeof(struct ath_node);
hw->vif_data_size = sizeof(struct ath_vif);
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index abad86b..4f619ae 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -861,9 +861,21 @@ static void ath_rc_ratefind(struct ath_softc *sc,
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
struct ieee80211_supported_band *sband = txrc->sband;
__le16 fc = hdr->frame_control;
- u8 try_per_rate = 0, i = 0, rix, nrix;
+ u8 try_per_rate, i = 0, rix, nrix;
int is_probe = 0;
+ /*
+ * For Multi Rate Retry we use a different number of
+ * retry attempt counts. This ends up looking like this:
+ *
+ * MRR[0] = 2
+ * MRR[1] = 2
+ * MRR[2] = 2
+ * MRR[3] = 4
+ *
+ */
+ try_per_rate = sc->hw->max_rate_tries;
+
rate_table = sc->cur_rate_table;
rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
nrix = rix;
@@ -874,7 +886,6 @@ static void ath_rc_ratefind(struct ath_softc *sc,
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
1, nrix, 0);
- try_per_rate = (ATH_11N_TXMAXTRY/4);
/* Get the next tried/allowed rate. No RTS for the next series
* after the probe rate
*/
@@ -885,7 +896,6 @@ static void ath_rc_ratefind(struct ath_softc *sc,
tx_info->flags |= IEEE80211_TX_CTL_RATE_CTRL_PROBE;
} else {
- try_per_rate = (ATH_11N_TXMAXTRY/4);
/* Set the choosen rate. No RTS for first series entry. */
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
try_per_rate, nrix, 0);
@@ -893,18 +903,19 @@ static void ath_rc_ratefind(struct ath_softc *sc,
/* Fill in the other rates for multirate retry */
for ( ; i < 4; i++) {
- u8 try_num;
u8 min_rate;
- try_num = ((i + 1) == 4) ?
- ATH_11N_TXMAXTRY - (try_per_rate * i) : try_per_rate ;
+ /* Use twice the number of tries for the last MRR segment. */
+ if (i + 1 == 4)
+ try_per_rate = 4;
+
min_rate = (((i + 1) == 4) && 0);
nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
rate_table, nrix, 1, min_rate);
/* All other rates in the series have RTS enabled */
ath_rc_rate_set_series(rate_table, &rates[i], txrc,
- try_num, nrix, 1);
+ try_per_rate, nrix, 1);
}
/*
@@ -1549,7 +1560,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
/*
* If underrun error is seen assume it as an excessive retry only
* if prefetch trigger level have reached the max (0x3f for 5416)
- * Adjust the long retry as if the frame was tried ATH_11N_TXMAXTRY
+ * Adjust the long retry as if the frame was tried hw->max_rate_tries
* times. This affects how ratectrl updates PER for the failed rate.
*/
if (tx_info_priv->tx.ts_flags &
@@ -1564,7 +1575,7 @@ static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
tx_status = 1;
ath_rc_tx_status(sc, ath_rc_priv, tx_info, final_ts_idx, tx_status,
- (is_underrun) ? ATH_11N_TXMAXTRY :
+ (is_underrun) ? sc->hw->max_rate_tries :
tx_info_priv->tx.ts_longretry);
/* Check if aggregation has to be enabled for this tid */
--
1.6.0.6
On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
> All rate control algorithms agree to send management and no-ack
> frames at the lowest rate. They also agree to do this when sta
> and the private rate control data is NULL. We move this to mac80211
> and simplify the rate control algorithm code.
We previously thought this would constrain our rate control algorithms
more than we would like for future experimentation with higher multicast
rates. Has that changed?
johannes
On Fri, Jun 05, 2009 at 12:52:52AM -0700, Vasanth Thiagarajan wrote:
> On Fri, Jun 05, 2009 at 01:17:16PM +0530, Luis Rodriguez wrote:
> > On Thu, Jun 4, 2009 at 11:30 PM, Vasanthakumar
> > Thiagarajan<[email protected]> wrote:
> > >
> > >> +
> > >> + /*
> > >> + * Fine tuning for when no decent rate was found, the
> > >> + * lowest should *not* be used under normal circumstances.
> > >> + */
> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > >> + "disabling MRR\n");
> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> > >> + rates[1].idx = -1;
> > >> + }
> > >
> > > I think we can still fill other rates (1..3) with the lowest rate
> > > index as we dont differentiate the situation where the lowest rate
> > > is chosen truely by the algorithm from this particular case.
> >
> > I thought about that as well, but does it really make sense for us to
> > use MRR with the same lowest rate? That's why I just used one segment.
> > Thoughts?
>
> or we can try for max_retry (4) times. In that case the rate indices of
> other rates (just not 1) should be made -1 or this segment should be
> moved just below the rate find.
So the count should have already been set up to 4, and the next segment [1]
is set to -1. Please let me know if there is anything else you see needs
change.
Luis
On Fri, Jun 5, 2009 at 12:42 AM, Jouni Malinen<[email protected]> wrote:
> On Thu, 2009-06-04 at 23:28 -0700, Johannes Berg wrote:
>> On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
>> > All rate control algorithms agree to send management and no-ack
>> > frames at the lowest rate. They also agree to do this when sta
>> > and the private rate control data is NULL. We move this to mac80211
>> > and simplify the rate control algorithm code.
>>
>> We previously thought this would constrain our rate control algorithms
>> more than we would like for future experimentation with higher multicast
>> rates. Has that changed?
>
> I hope not. I don't like the idea of forcing the lowest rate in
> mac80211; we should allow rate control algorithms to be improved to do
> more intelligent selection of rate at least for multicast/broadcast
> frames. That might also be of use for some (likely post-association)
> unicast management frames since their use is going to increase in the
> future.
How about a generic call for now then and have each RC use it for now.
Luis
We check for this condition early on in our mac80211 get_rate()
callback ath_get_rate(), so remove this check later down the path.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 16e41b1..344b996 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -783,7 +783,6 @@ static void ath_rc_rate_set_rtscts(struct ath_softc *sc,
* just CTS. Note that this is only done for OFDM/HT unicast frames.
*/
if ((sc->sc_flags & SC_OP_PROTECT_ENABLE) &&
- !(tx_info->flags & IEEE80211_TX_CTL_NO_ACK) &&
(rate_table->info[rix].phy == WLAN_RC_PHY_OFDM ||
WLAN_RC_PHY_HT(rate_table->info[rix].phy))) {
rates[0].flags |= IEEE80211_TX_RC_USE_CTS_PROTECT;
@@ -890,9 +889,8 @@ static void ath_rc_ratefind(struct ath_softc *sc,
*
* FIXME: Fix duration
*/
- if (!(tx_info->flags & IEEE80211_TX_CTL_NO_ACK) &&
- (ieee80211_has_morefrags(fc) ||
- (le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG))) {
+ if (ieee80211_has_morefrags(fc) ||
+ (le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_FRAG)) {
rates[1].count = rates[2].count = rates[3].count = 0;
rates[1].idx = rates[2].idx = rates[3].idx = 0;
rates[0].count = ATH_TXMAXTRY;
--
1.6.0.6
This is not used, and when we need to get the lowest rate
we should simply use mac80211's own rate_lowest_index(sband, sta).
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 33 +++++++++------------------------
1 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 4f619ae..f62361c 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -823,28 +823,17 @@ static void ath_rc_rate_set_rtscts(struct ath_softc *sc,
static u8 ath_rc_rate_getidx(struct ath_softc *sc,
struct ath_rate_priv *ath_rc_priv,
const struct ath_rate_table *rate_table,
- u8 rix, u16 stepdown,
- u16 min_rate)
+ u8 rix, u16 stepdown)
{
u32 j;
u8 nextindex = 0;
- if (min_rate) {
- for (j = RATE_TABLE_SIZE; j > 0; j--) {
- if (ath_rc_get_nextlowervalid_txrate(rate_table,
- ath_rc_priv, rix, &nextindex))
- rix = nextindex;
- else
- break;
- }
- } else {
- for (j = stepdown; j > 0; j--) {
- if (ath_rc_get_nextlowervalid_txrate(rate_table,
- ath_rc_priv, rix, &nextindex))
- rix = nextindex;
- else
- break;
- }
+ for (j = stepdown; j > 0; j--) {
+ if (ath_rc_get_nextlowervalid_txrate(rate_table,
+ ath_rc_priv, rix, &nextindex))
+ rix = nextindex;
+ else
+ break;
}
return rix;
}
@@ -890,7 +879,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
* after the probe rate
*/
nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix, 1, 0);
+ rate_table, nrix, 1);
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
try_per_rate, nrix, 0);
@@ -903,16 +892,12 @@ static void ath_rc_ratefind(struct ath_softc *sc,
/* Fill in the other rates for multirate retry */
for ( ; i < 4; i++) {
- u8 min_rate;
-
/* Use twice the number of tries for the last MRR segment. */
if (i + 1 == 4)
try_per_rate = 4;
- min_rate = (((i + 1) == 4) && 0);
-
nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix, 1, min_rate);
+ rate_table, nrix, 1);
/* All other rates in the series have RTS enabled */
ath_rc_rate_set_series(rate_table, &rates[i], txrc,
try_per_rate, nrix, 1);
--
1.6.0.6
On Thu, Jun 4, 2009 at 11:30 PM, Vasanthakumar
Thiagarajan<[email protected]> wrote:
>
>> +
>> + /*
>> + * Fine tuning for when no decent rate was found, the
>> + * lowest should *not* be used under normal circumstances.
>> + */
>> + if (rix == ath_rc_priv->valid_rate_index[0]) {
>> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
>> + "disabling MRR\n");
>> + rates[0].idx = rate_lowest_index(sband, sta);
>> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
>> + rates[1].idx = -1;
>> + }
>
> I think we can still fill other rates (1..3) with the lowest rate
> index as we dont differentiate the situation where the lowest rate
> is chosen truely by the algorithm from this particular case.
I thought about that as well, but does it really make sense for us to
use MRR with the same lowest rate? That's why I just used one segment.
Thoughts?
Luis
On Mon, Jun 08, 2009 at 05:26:41PM -0700, Luis R. Rodriguez wrote:
> On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> Thiagarajan<[email protected]> wrote:
> >
> > ________________________________________
> >
> >> > >> + /*
> >> > >> + * Fine tuning for when no decent rate was found, the
> >> > >> + * lowest should *not* be used under normal circumstances.
> >> > >> + */
> >> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> >> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> >> > >> + "disabling MRR\n");
> >> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> >> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> >> > >> + rates[1].idx = -1;
> >> > >> + }
> >> > >
> >> > > I think we can still fill other rates (1..3) with the lowest rate
> >> > > index as we dont differentiate the situation where the lowest rate
> >> > > is chosen truely by the algorithm from this particular case.
> >> >
> >> > I thought about that as well, but does it really make sense for us to
> >> > use MRR with the same lowest rate? That's why I just used one segment.
> >> > Thoughts?
> >>
> >> or we can try for max_retry (4) times. In that case the rate indices of
> >> other rates (just not 1) should be made -1 or this segment should
> >> moved just below the rate find.
> >
> > and the next segment [1]
> > is set to -1. Please let me know if there is anything else you see needs
> > change.
> >
> > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > just below ath_rc_ratefind_ht(), like the following diff.
> >
> >
> > rate_table = sc->cur_rate_table;
> > rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > +
> > + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > + "disabling MRR\n");
> > +
> > + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > + 4, rix, 0);
>
> The above sets the rate[0].idx to rix
>
> > + rates[0].idx = rate_lowest_index(sband, sta);
>
> and then here we set it to rate_lowest_index(sband, sta) comes up
> with. They should be the same, but this just goes to show we need to
> clean this better.
I just found this is wrong too.. the rate table used to find
rix is different than the rate table for the mode. ie, in 5ghz
when associated to an 11n station this could not include any
legacy rates.
Luis
All rate control algorithms agree to send management and no-ack
frames at the lowest rate. They also agree to do this when sta
and the private rate control data is NULL. We move this to mac80211
and simplify the rate control algorithm code.
Cc: Zhu Yi <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: [email protected]
Cc: Gabor Juhos <[email protected]>
Cc: Felix Fietkau <[email protected]>
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 15 --------------
drivers/net/wireless/iwlwifi/iwl-3945-rs.c | 20 +------------------
drivers/net/wireless/iwlwifi/iwl-agn-rs.c | 10 ---------
net/mac80211/rate.c | 29 +++++++++++++++++++++++++++-
net/mac80211/rc80211_minstrel.c | 23 ----------------------
net/mac80211/rc80211_pid_algo.c | 14 -------------
6 files changed, 29 insertions(+), 82 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 344b996..2180180 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -1538,23 +1538,8 @@ exit:
static void ath_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
struct ieee80211_tx_rate_control *txrc)
{
- struct ieee80211_supported_band *sband = txrc->sband;
- struct sk_buff *skb = txrc->skb;
- struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
struct ath_softc *sc = priv;
struct ath_rate_priv *ath_rc_priv = priv_sta;
- __le16 fc = hdr->frame_control;
-
- /* lowest rate for management and NO_ACK frames */
- if (!ieee80211_is_data(fc) ||
- tx_info->flags & IEEE80211_TX_CTL_NO_ACK || !sta) {
- tx_info->control.rates[0].idx = rate_lowest_index(sband, sta);
- tx_info->control.rates[0].count =
- (tx_info->flags & IEEE80211_TX_CTL_NO_ACK) ?
- 1 : ATH_MGT_TXMAXTRY;
- return;
- }
/* Find tx rate for unicast frames */
ath_rc_ratefind(sc, ath_rc_priv, txrc, sta);
diff --git a/drivers/net/wireless/iwlwifi/iwl-3945-rs.c b/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
index bd2f709..7b6567b 100644
--- a/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
@@ -673,7 +673,6 @@ static void rs_get_rate(void *priv_r, struct ieee80211_sta *sta,
s8 scale_action = 0;
unsigned long flags;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- __le16 fc;
u16 rate_mask = 0;
s8 max_rate_idx = -1;
struct iwl_priv *priv = (struct iwl_priv *)priv_r;
@@ -681,24 +680,7 @@ static void rs_get_rate(void *priv_r, struct ieee80211_sta *sta,
IWL_DEBUG_RATE(priv, "enter\n");
- if (sta)
- rate_mask = sta->supp_rates[sband->band];
-
- /* Send management frames and NO_ACK data using lowest rate. */
- fc = hdr->frame_control;
- if (!ieee80211_is_data(fc) || info->flags & IEEE80211_TX_CTL_NO_ACK ||
- !sta || !priv_sta) {
- IWL_DEBUG_RATE(priv, "leave: No STA priv data to update!\n");
- if (!rate_mask)
- info->control.rates[0].idx =
- rate_lowest_index(sband, NULL);
- else
- info->control.rates[0].idx =
- rate_lowest_index(sband, sta);
- if (info->flags & IEEE80211_TX_CTL_NO_ACK)
- info->control.rates[0].count = 1;
- return;
- }
+ rate_mask = sta->supp_rates[sband->band];
/* get user max rate if set */
max_rate_idx = txrc->max_rate_idx;
diff --git a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
index ff20e50..0052e8c 100644
--- a/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-agn-rs.c
@@ -2012,7 +2012,6 @@ static void rs_rate_scale_perform(struct iwl_priv *priv,
{
struct ieee80211_hw *hw = priv->hw;
struct ieee80211_conf *conf = &hw->conf;
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
int low = IWL_RATE_INVALID;
int high = IWL_RATE_INVALID;
@@ -2039,15 +2038,6 @@ static void rs_rate_scale_perform(struct iwl_priv *priv,
IWL_DEBUG_RATE(priv, "rate scale calculate new rate for skb\n");
- /* Send management frames and NO_ACK data using lowest rate. */
- /* TODO: this could probably be improved.. */
- if (!ieee80211_is_data(hdr->frame_control) ||
- info->flags & IEEE80211_TX_CTL_NO_ACK)
- return;
-
- if (!sta || !lq_sta)
- return;
-
lq_sta->supp_rates = sta->supp_rates[lq_sta->band];
tid = rs_tl_add_packet(lq_sta, hdr);
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 4641f00..2795f70 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -198,11 +198,24 @@ static void rate_control_release(struct kref *kref)
kfree(ctrl_ref);
}
+static bool rc_no_data_or_no_ack(struct ieee80211_tx_rate_control *txrc)
+{
+ struct sk_buff *skb = txrc->skb;
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+ __le16 fc;
+
+ fc = hdr->frame_control;
+
+ return ((info->flags & IEEE80211_TX_CTL_NO_ACK) || !ieee80211_is_data(fc));
+}
+
void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
struct sta_info *sta,
struct ieee80211_tx_rate_control *txrc)
{
struct rate_control_ref *ref = sdata->local->rate_ctrl;
+ struct ieee80211_hw *hw = &sdata->local->hw;
void *priv_sta = NULL;
struct ieee80211_sta *ista = NULL;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
@@ -222,7 +235,21 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
if (sta && sdata->force_unicast_rateidx > -1) {
info->control.rates[0].idx = sdata->force_unicast_rateidx;
} else {
- ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
+ /*
+ * Lets not bother rate control algos with this as they
+ * all agree to use the lowest rate to send management frames
+ * and NO_ACK data with the respective hw retries. This also
+ * guarantees we don't call the rate control get_rate()
+ * callback with a NULL sta or NULL private RC data.
+ */
+ if (!ista || !priv_sta || rc_no_data_or_no_ack(txrc)) {
+ info->control.rates[0].idx =
+ rate_lowest_index(txrc->sband, ista);
+ info->control.rates[0].count =
+ (info->flags & IEEE80211_TX_CTL_NO_ACK) ?
+ 1 : hw->max_rate_tries;
+ } else
+ ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);
info->flags |= IEEE80211_TX_INTFL_RCALGO;
}
diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 0e1db92..40c1ba4 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -70,19 +70,6 @@ rix_to_ndx(struct minstrel_sta_info *mi, int rix)
return i;
}
-static inline bool
-use_low_rate(struct sk_buff *skb)
-{
- struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
- struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- __le16 fc;
-
- fc = hdr->frame_control;
-
- return ((info->flags & IEEE80211_TX_CTL_NO_ACK) || !ieee80211_is_data(fc));
-}
-
-
static void
minstrel_update_stats(struct minstrel_priv *mp, struct minstrel_sta_info *mi)
{
@@ -228,7 +215,6 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
void *priv_sta, struct ieee80211_tx_rate_control *txrc)
{
struct sk_buff *skb = txrc->skb;
- struct ieee80211_supported_band *sband = txrc->sband;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct minstrel_sta_info *mi = priv_sta;
struct minstrel_priv *mp = priv;
@@ -241,15 +227,6 @@ minstrel_get_rate(void *priv, struct ieee80211_sta *sta,
int mrr_ndx[3];
int sample_rate;
- if (!sta || !mi || use_low_rate(skb)) {
- ar[0].idx = rate_lowest_index(sband, sta);
- if (info->flags & IEEE80211_TX_CTL_NO_ACK)
- ar[0].count = 1;
- else
- ar[0].count = mp->max_retry;
- return;
- }
-
mrr = mp->has_mrr && !txrc->rts && !txrc->bss_conf->use_cts_prot;
if (time_after(jiffies, mi->stats_update + (mp->update_interval *
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index ac6dad6..1dad177 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -274,13 +274,10 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
void *priv_sta,
struct ieee80211_tx_rate_control *txrc)
{
- struct sk_buff *skb = txrc->skb;
struct ieee80211_supported_band *sband = txrc->sband;
- struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct rc_pid_sta_info *spinfo = priv_sta;
int rateidx;
- __le16 fc;
if (txrc->rts)
info->control.rates[0].count =
@@ -289,17 +286,6 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
info->control.rates[0].count =
txrc->hw->conf.short_frame_max_tx_count;
- /* Send management frames and NO_ACK data using lowest rate. */
- fc = le16_to_cpu(hdr->frame_control);
- if (!sta || !spinfo || !ieee80211_is_data(fc) ||
- info->flags & IEEE80211_TX_CTL_NO_ACK) {
- info->control.rates[0].idx = rate_lowest_index(sband, sta);
- if (info->flags & IEEE80211_TX_CTL_NO_ACK)
- info->control.rates[0].count = 1;
-
- return;
- }
-
rateidx = spinfo->txrate_idx;
if (rateidx >= sband->n_bitrates)
--
1.6.0.6
On Thu, Jun 4, 2009 at 10:41 PM, Luis R. Rodriguez
<[email protected]> wrote:
> ath9k's rate control code is so convoluted that it really make it
> difficult to spot bugs and fix them. Here's a bunch of cleanups for
> ath9k rate control code, and a few for mac80211. I just tested ath9k.
> Will have to test others later.
Here's the all-in-one file patch:
http://bombadil.infradead.org/~mcgrof/patches/ath9k/2009-06-04/rate-all.patch
Testing appreciated.
Luis
This is not used, remove this.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 21 ++++++++-------------
1 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index f62361c..557f054 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -823,19 +823,14 @@ static void ath_rc_rate_set_rtscts(struct ath_softc *sc,
static u8 ath_rc_rate_getidx(struct ath_softc *sc,
struct ath_rate_priv *ath_rc_priv,
const struct ath_rate_table *rate_table,
- u8 rix, u16 stepdown)
+ u8 rix)
{
- u32 j;
u8 nextindex = 0;
-
- for (j = stepdown; j > 0; j--) {
- if (ath_rc_get_nextlowervalid_txrate(rate_table,
- ath_rc_priv, rix, &nextindex))
- rix = nextindex;
- else
- break;
- }
- return rix;
+ if (ath_rc_get_nextlowervalid_txrate(rate_table,
+ ath_rc_priv, rix, &nextindex))
+ return nextindex;
+ else
+ return rix;
}
static void ath_rc_ratefind(struct ath_softc *sc,
@@ -879,7 +874,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
* after the probe rate
*/
nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix, 1);
+ rate_table, nrix);
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
try_per_rate, nrix, 0);
@@ -897,7 +892,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
try_per_rate = 4;
nrix = ath_rc_rate_getidx(sc, ath_rc_priv,
- rate_table, nrix, 1);
+ rate_table, nrix);
/* All other rates in the series have RTS enabled */
ath_rc_rate_set_series(rate_table, &rates[i], txrc,
try_per_rate, nrix, 1);
--
1.6.0.6
> +
> + /*
> + * Fine tuning for when no decent rate was found, the
> + * lowest should *not* be used under normal circumstances.
> + */
> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> + "disabling MRR\n");
> + rates[0].idx = rate_lowest_index(sband, sta);
> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> + rates[1].idx = -1;
> + }
I think we can still fill other rates (1..3) with the lowest rate
index as we dont differentiate the situation where the lowest rate
is chosen truely by the algorithm from this particular case.
Vasanth
On Tue, Jun 09, 2009 at 05:56:41AM +0530, Luis R. Rodriguez wrote:
> On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> Thiagarajan<[email protected]> wrote:
> >
> > ________________________________________
> >
> >> > >> + /*
> >> > >> + * Fine tuning for when no decent rate was found, the
> >> > >> + * lowest should *not* be used under normal circumstances.
> >> > >> + */
> >> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> >> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> >> > >> + "disabling MRR\n");
> >> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> >> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> >> > >> + rates[1].idx = -1;
> >> > >> + }
> >> > >
> >> > > I think we can still fill other rates (1..3) with the lowest rate
> >> > > index as we dont differentiate the situation where the lowest rate
> >> > > is chosen truely by the algorithm from this particular case.
> >> >
> >> > I thought about that as well, but does it really make sense for us to
> >> > use MRR with the same lowest rate? That's why I just used one segment.
> >> > Thoughts?
> >>
> >> or we can try for max_retry (4) times. In that case the rate indices of
> >> other rates (just not 1) should be made -1 or this segment should
> >> moved just below the rate find.
> >
> > and the next segment [1]
> > is set to -1. Please let me know if there is anything else you see needs
> > change.
> >
> > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > just below ath_rc_ratefind_ht(), like the following diff.
> >
> >
> > rate_table = sc->cur_rate_table;
> > rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > +
> > + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > + "disabling MRR\n");
> > +
> > + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > + 4, rix, 0);
>
> The above sets the rate[0].idx to rix
>
> > + rates[0].idx = rate_lowest_index(sband, sta);
>
> and then here we set it to rate_lowest_index(sband, sta) comes up
> with. They should be the same, but this just goes to show we need to
> clean this better.
yeah, set rix to lowest rate index before passing rix to
ath_rc_rate_set_series().
>
> ath_rc_rate_set_series() is also doing some flag checks which I'm not
> so sure we need to do all the time so I'd like to avoid it.
We need preamble and RTS_CTS bits in rate flags.
> there's also that is_probe check and the flags that sets.
This should be ignored for lowest rate index as there is no way
that a new rate has been probed in this case.
Vasanth
On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
Thiagarajan<[email protected]> wrote:
>
> ________________________________________
>
>> > >> + /*
>> > >> + * Fine tuning for when no decent rate was found, the
>> > >> + * lowest should *not* be used under normal circumstances.
>> > >> + */
>> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
>> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
>> > >> + "disabling MRR\n");
>> > >> + rates[0].idx = rate_lowest_index(sband, sta);
>> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
>> > >> + rates[1].idx = -1;
>> > >> + }
>> > >
>> > > I think we can still fill other rates (1..3) with the lowest rate
>> > > index as we dont differentiate the situation where the lowest rate
>> > > is chosen truely by the algorithm from this particular case.
>> >
>> > I thought about that as well, but does it really make sense for us to
>> > use MRR with the same lowest rate? That's why I just used one segment.
>> > Thoughts?
>>
>> or we can try for max_retry (4) times. In that case the rate indices of
>> other rates (just not 1) should be made -1 or this segment should
>> moved just below the rate find.
>
> and the next segment [1]
> is set to -1. Please let me know if there is anything else you see needs
> change.
>
> Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> just below ath_rc_ratefind_ht(), like the following diff.
>
>
> rate_table = sc->cur_rate_table;
> rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> +
> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> + "disabling MRR\n");
> +
> + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> + 4, rix, 0);
The above sets the rate[0].idx to rix
> + rates[0].idx = rate_lowest_index(sband, sta);
and then here we set it to rate_lowest_index(sband, sta) comes up
with. They should be the same, but this just goes to show we need to
clean this better.
ath_rc_rate_set_series() is also doing some flag checks which I'm not
so sure we need to do all the time so I'd like to avoid it. And
there's also that is_probe check and the flags that sets.
> + goto find_ctrl_rateix;
This seems like a good idea but in fact I also think this is useless,
not sure why mac80211 can't figure this out for us. More cleanup.
> + }
> +
> nrix = rix;
>
> if (is_probe) {
> @@ -933,6 +944,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
> rates[0].count = ATH_TXMAXTRY;
> }
>
> +find_ctrl_rateix:
> /* Setup RTS/CTS */
> ath_rc_rate_set_rtscts(sc, rate_table, tx_info);
> }
I'll respin again.
Luis
On Tue, Jun 09, 2009 at 06:28:21AM +0530, Luis Rodriguez wrote:
> On Mon, Jun 08, 2009 at 05:26:41PM -0700, Luis R. Rodriguez wrote:
> > On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> > Thiagarajan<[email protected]> wrote:
> > >
> > > ________________________________________
> > >
> > >> > >> + /*
> > >> > >> + * Fine tuning for when no decent rate was found, the
> > >> > >> + * lowest should *not* be used under normal circumstances.
> > >> > >> + */
> > >> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > >> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > >> > >> + "disabling MRR\n");
> > >> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> > >> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> > >> > >> + rates[1].idx = -1;
> > >> > >> + }
> > >> > >
> > >> > > I think we can still fill other rates (1..3) with the lowest rate
> > >> > > index as we dont differentiate the situation where the lowest rate
> > >> > > is chosen truely by the algorithm from this particular case.
> > >> >
> > >> > I thought about that as well, but does it really make sense for us to
> > >> > use MRR with the same lowest rate? That's why I just used one segment.
> > >> > Thoughts?
> > >>
> > >> or we can try for max_retry (4) times. In that case the rate indices of
> > >> other rates (just not 1) should be made -1 or this segment should
> > >> moved just below the rate find.
> > >
> > > and the next segment [1]
> > > is set to -1. Please let me know if there is anything else you see needs
> > > change.
> > >
> > > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > > just below ath_rc_ratefind_ht(), like the following diff.
> > >
> > >
> > > rate_table = sc->cur_rate_table;
> > > rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > > +
> > > + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > > + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > > + "disabling MRR\n");
> > > +
> > > + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > > + 4, rix, 0);
> >
> > The above sets the rate[0].idx to rix
> >
> > > + rates[0].idx = rate_lowest_index(sband, sta);
> >
> > and then here we set it to rate_lowest_index(sband, sta) comes up
> > with. They should be the same, but this just goes to show we need to
> > clean this better.
>
> I just found this is wrong too.. the rate table used to find
> rix is different than the rate table for the mode. ie, in 5ghz
> when associated to an 11n station this could not include any
> legacy rates.
This is wrong. All basci rates still be used in 11n mode.
Vasanth
On Thu, Jun 4, 2009 at 11:24 PM, Johannes Berg
<[email protected]> wrote:
> On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
>
>> - u16 fc;
>> + __le16 fc;
>>
>> if (txrc->rts)
>> info->control.rates[0].count =
>> @@ -291,8 +291,7 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
>>
>> /* Send management frames and NO_ACK data using lowest rate. */
>> fc = le16_to_cpu(hdr->frame_control);
>> - if (!sta || !spinfo ||
>> - (fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
>> + if (!sta || !spinfo || !ieee80211_is_data(fc) ||
>
> you might want to run sparse on this entire patchset.
I did, PID was not on my kconfig though, so its the only thing I
didn't test compile.
Luis
On Tue, Jun 09, 2009 at 10:54:00AM +0530, Vasanth Thiagarajan wrote:
> On Tue, Jun 09, 2009 at 06:28:21AM +0530, Luis Rodriguez wrote:
> > On Mon, Jun 08, 2009 at 05:26:41PM -0700, Luis R. Rodriguez wrote:
> > > On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> > > Thiagarajan<[email protected]> wrote:
> > > >
> > > > ________________________________________
> > > >
> > > >> > >> + /*
> > > >> > >> + * Fine tuning for when no decent rate was found, the
> > > >> > >> + * lowest should *not* be used under normal circumstances.
> > > >> > >> + */
> > > >> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > > >> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > > >> > >> + "disabling MRR\n");
> > > >> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> > > >> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> > > >> > >> + rates[1].idx = -1;
> > > >> > >> + }
> > > >> > >
> > > >> > > I think we can still fill other rates (1..3) with the lowest rate
> > > >> > > index as we dont differentiate the situation where the lowest rate
> > > >> > > is chosen truely by the algorithm from this particular case.
> > > >> >
> > > >> > I thought about that as well, but does it really make sense for us to
> > > >> > use MRR with the same lowest rate? That's why I just used one segment.
> > > >> > Thoughts?
> > > >>
> > > >> or we can try for max_retry (4) times. In that case the rate indices of
> > > >> other rates (just not 1) should be made -1 or this segment should
> > > >> moved just below the rate find.
> > > >
> > > > and the next segment [1]
> > > > is set to -1. Please let me know if there is anything else you see needs
> > > > change.
> > > >
> > > > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > > > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > > > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > > > just below ath_rc_ratefind_ht(), like the following diff.
> > > >
> > > >
> > > > rate_table = sc->cur_rate_table;
> > > > rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > > > +
> > > > + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > > > + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > > > + "disabling MRR\n");
> > > > +
> > > > + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > > > + 4, rix, 0);
> > >
> > > The above sets the rate[0].idx to rix
> > >
> > > > + rates[0].idx = rate_lowest_index(sband, sta);
> > >
> > > and then here we set it to rate_lowest_index(sband, sta) comes up
> > > with. They should be the same, but this just goes to show we need to
> > > clean this better.
> >
> > I just found this is wrong too.. the rate table used to find
> > rix is different than the rate table for the mode. ie, in 5ghz
> > when associated to an 11n station this could not include any
> > legacy rates.
>
> This is wrong. All basci rates still be used in 11n mode.
Never mind, i think this is only true in 11ng mode
On Thu, 2009-06-04 at 23:28 -0700, Johannes Berg wrote:
> On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
> > All rate control algorithms agree to send management and no-ack
> > frames at the lowest rate. They also agree to do this when sta
> > and the private rate control data is NULL. We move this to mac80211
> > and simplify the rate control algorithm code.
>
> We previously thought this would constrain our rate control algorithms
> more than we would like for future experimentation with higher multicast
> rates. Has that changed?
I hope not. I don't like the idea of forcing the lowest rate in
mac80211; we should allow rate control algorithms to be improved to do
more intelligent selection of rate at least for multicast/broadcast
frames. That might also be of use for some (likely post-association)
unicast management frames since their use is going to increase in the
future.
- Jouni
Cc: Felix Fietkau <[email protected]>
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
net/mac80211/rc80211_minstrel.c | 7 +++----
net/mac80211/rc80211_pid_algo.c | 5 ++---
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 0a11515..0e1db92 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -75,12 +75,11 @@ use_low_rate(struct sk_buff *skb)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- u16 fc;
+ __le16 fc;
- fc = le16_to_cpu(hdr->frame_control);
+ fc = hdr->frame_control;
- return ((info->flags & IEEE80211_TX_CTL_NO_ACK) ||
- (fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA);
+ return ((info->flags & IEEE80211_TX_CTL_NO_ACK) || !ieee80211_is_data(fc));
}
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index a0bef76..ac6dad6 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -280,7 +280,7 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct rc_pid_sta_info *spinfo = priv_sta;
int rateidx;
- u16 fc;
+ __le16 fc;
if (txrc->rts)
info->control.rates[0].count =
@@ -291,8 +291,7 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
/* Send management frames and NO_ACK data using lowest rate. */
fc = le16_to_cpu(hdr->frame_control);
- if (!sta || !spinfo ||
- (fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
+ if (!sta || !spinfo || !ieee80211_is_data(fc) ||
info->flags & IEEE80211_TX_CTL_NO_ACK) {
info->control.rates[0].idx = rate_lowest_index(sband, sta);
if (info->flags & IEEE80211_TX_CTL_NO_ACK)
--
1.6.0.6
Cc: Zhu Yi <[email protected]>
Cc: Reinette Chatre <[email protected]>
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/iwlwifi/iwl-3945-rs.c | 7 +++----
drivers/net/wireless/iwlwifi/iwl-sta.c | 5 ++---
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-3945-rs.c b/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
index 5eb538d..bd2f709 100644
--- a/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
+++ b/drivers/net/wireless/iwlwifi/iwl-3945-rs.c
@@ -673,7 +673,7 @@ static void rs_get_rate(void *priv_r, struct ieee80211_sta *sta,
s8 scale_action = 0;
unsigned long flags;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
- u16 fc;
+ __le16 fc;
u16 rate_mask = 0;
s8 max_rate_idx = -1;
struct iwl_priv *priv = (struct iwl_priv *)priv_r;
@@ -685,9 +685,8 @@ static void rs_get_rate(void *priv_r, struct ieee80211_sta *sta,
rate_mask = sta->supp_rates[sband->band];
/* Send management frames and NO_ACK data using lowest rate. */
- fc = le16_to_cpu(hdr->frame_control);
- if ((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
- info->flags & IEEE80211_TX_CTL_NO_ACK ||
+ fc = hdr->frame_control;
+ if (!ieee80211_is_data(fc) || info->flags & IEEE80211_TX_CTL_NO_ACK ||
!sta || !priv_sta) {
IWL_DEBUG_RATE(priv, "leave: No STA priv data to update!\n");
if (!rate_mask)
diff --git a/drivers/net/wireless/iwlwifi/iwl-sta.c b/drivers/net/wireless/iwlwifi/iwl-sta.c
index 2addf73..afa1633 100644
--- a/drivers/net/wireless/iwlwifi/iwl-sta.c
+++ b/drivers/net/wireless/iwlwifi/iwl-sta.c
@@ -1044,11 +1044,10 @@ EXPORT_SYMBOL(iwl_rxon_add_station);
int iwl_get_sta_id(struct iwl_priv *priv, struct ieee80211_hdr *hdr)
{
int sta_id;
- u16 fc = le16_to_cpu(hdr->frame_control);
+ __le16 fc = hdr->frame_control;
/* If this frame is broadcast or management, use broadcast station id */
- if (((fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA) ||
- is_multicast_ether_addr(hdr->addr1))
+ if (!ieee80211_is_data(fc) || is_multicast_ether_addr(hdr->addr1))
return priv->hw_params.bcast_sta_id;
switch (priv->iw_mode) {
--
1.6.0.6
What this does is get us our next lower rate so call it that,
ath_rc_get_lower_rix().
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 6ccd565..eaa22e4 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -501,9 +501,9 @@ static int ath_rc_valid_phyrate(u32 phy, u32 capflag, int ignore_cw)
}
static inline int
-ath_rc_get_nextlowervalid_txrate(const struct ath_rate_table *rate_table,
- struct ath_rate_priv *ath_rc_priv,
- u8 cur_valid_txrate, u8 *next_idx)
+ath_rc_get_lower_rix(const struct ath_rate_table *rate_table,
+ struct ath_rate_priv *ath_rc_priv,
+ u8 cur_valid_txrate, u8 *next_idx)
{
int8_t i;
@@ -860,8 +860,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
/* Get the next tried/allowed rate. No RTS for the next series
* after the probe rate
*/
- ath_rc_get_nextlowervalid_txrate(rate_table, ath_rc_priv,
- rix, &nrix);
+ ath_rc_get_lower_rix(rate_table, ath_rc_priv, rix, &nrix);
ath_rc_rate_set_series(rate_table, &rates[i++], txrc,
try_per_rate, nrix, 0);
@@ -878,8 +877,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
if (i + 1 == 4)
try_per_rate = 4;
- ath_rc_get_nextlowervalid_txrate(rate_table, ath_rc_priv,
- rix, &nrix);
+ ath_rc_get_lower_rix(rate_table, ath_rc_priv, rix, &nrix);
/* All other rates in the series have RTS enabled */
ath_rc_rate_set_series(rate_table, &rates[i], txrc,
try_per_rate, nrix, 1);
@@ -1176,8 +1174,8 @@ static void ath_rc_update_ht(struct ath_softc *sc,
if (ath_rc_priv->state[tx_rate].per >= 55 && tx_rate > 0 &&
rate_table->info[tx_rate].ratekbps <=
rate_table->info[ath_rc_priv->rate_max_phy].ratekbps) {
- ath_rc_get_nextlowervalid_txrate(rate_table, ath_rc_priv,
- (u8)tx_rate, &ath_rc_priv->rate_max_phy);
+ ath_rc_get_lower_rix(rate_table, ath_rc_priv,
+ (u8)tx_rate, &ath_rc_priv->rate_max_phy);
/* Don't probe for a little while. */
ath_rc_priv->probe_time = now_msec;
--
1.6.0.6
This saves us 2733 bytes.
text data bss dec hex filename
252265 3628 1584 257477 3edc5 ath9k-has-b-rate.ko
249905 3628 1584 255117 3e48d ath9k.ko
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Siged-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/hw.c | 1 -
drivers/net/wireless/ath/ath9k/hw.h | 1 -
drivers/net/wireless/ath/ath9k/rc.c | 23 -----------------------
3 files changed, 0 insertions(+), 25 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 1579c94..de2c000 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -3294,7 +3294,6 @@ void ath9k_hw_fill_cap_info(struct ath_hw *ah)
}
if (eeval & AR5416_OPFLAGS_11G) {
- set_bit(ATH9K_MODE_11B, pCap->wireless_modes);
set_bit(ATH9K_MODE_11G, pCap->wireless_modes);
if (ah->config.ht_enable) {
if (!(eeval & AR5416_OPFLAGS_N_2G_HT20))
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index dd8508e..73d859e 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -113,7 +113,6 @@
enum wireless_mode {
ATH9K_MODE_11A = 0,
- ATH9K_MODE_11B = 2,
ATH9K_MODE_11G = 3,
ATH9K_MODE_11NA_HT20 = 6,
ATH9K_MODE_11NG_HT20 = 7,
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 5b986b8..7cea944 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -380,27 +380,6 @@ static const struct ath_rate_table ar5416_11g_ratetable = {
0, /* Phy rates allowed initially */
};
-static const struct ath_rate_table ar5416_11b_ratetable = {
- 4,
- {
- { VALID, VALID, WLAN_RC_PHY_CCK, 1000, /* 1 Mb */
- 900, 0x1b, 0x00, (0x80|2),
- 0, 0, 1, 0, 0 },
- { VALID, VALID, WLAN_RC_PHY_CCK, 2000, /* 2 Mb */
- 1800, 0x1a, 0x04, (0x80|4),
- 1, 1, 1, 1, 0 },
- { VALID, VALID, WLAN_RC_PHY_CCK, 5500, /* 5.5 Mb */
- 4300, 0x19, 0x04, (0x80|11),
- 1, 2, 2, 2, 0 },
- { VALID, VALID, WLAN_RC_PHY_CCK, 11000, /* 11 Mb */
- 7100, 0x18, 0x04, (0x80|22),
- 1, 4, 100, 3, 0 },
- },
- 100, /* probe interval */
- 100, /* rssi reduce interval */
- 0, /* Phy rates allowed initially */
-};
-
static inline int8_t median(int8_t a, int8_t b, int8_t c)
{
if (a >= b) {
@@ -1722,8 +1701,6 @@ static struct rate_control_ops ath_rate_ops = {
void ath_rate_attach(struct ath_softc *sc)
{
- sc->hw_rate_table[ATH9K_MODE_11B] =
- &ar5416_11b_ratetable;
sc->hw_rate_table[ATH9K_MODE_11A] =
&ar5416_11a_ratetable;
sc->hw_rate_table[ATH9K_MODE_11G] =
--
1.6.0.6
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index eaa22e4..5b986b8 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -454,13 +454,6 @@ static inline void ath_rc_set_valid_txmask(struct ath_rate_priv *ath_rc_priv,
ath_rc_priv->valid_rate_index[index] = valid_tx_rate ? 1 : 0;
}
-static inline int ath_rc_isvalid_txmask(struct ath_rate_priv *ath_rc_priv,
- u8 index)
-{
- ASSERT(index <= ath_rc_priv->rate_table_size);
- return ath_rc_priv->valid_rate_index[index];
-}
-
static inline
int ath_rc_get_nextvalid_txrate(const struct ath_rate_table *rate_table,
struct ath_rate_priv *ath_rc_priv,
--
1.6.0.6
After the cleanup we just use get_rate as a wrapper, skip
the wrapper.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 18 ++++--------------
1 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 2180180..3b9b819 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -792,11 +792,11 @@ static void ath_rc_rate_set_rtscts(struct ath_softc *sc,
tx_info->control.rts_cts_rate_idx = cix;
}
-static void ath_rc_ratefind(struct ath_softc *sc,
- struct ath_rate_priv *ath_rc_priv,
- struct ieee80211_tx_rate_control *txrc,
- struct ieee80211_sta *sta)
+static void ath_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
+ struct ieee80211_tx_rate_control *txrc)
{
+ struct ath_softc *sc = priv;
+ struct ath_rate_priv *ath_rc_priv = priv_sta;
const struct ath_rate_table *rate_table;
struct sk_buff *skb = txrc->skb;
struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
@@ -1535,16 +1535,6 @@ exit:
kfree(tx_info_priv);
}
-static void ath_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
- struct ieee80211_tx_rate_control *txrc)
-{
- struct ath_softc *sc = priv;
- struct ath_rate_priv *ath_rc_priv = priv_sta;
-
- /* Find tx rate for unicast frames */
- ath_rc_ratefind(sc, ath_rc_priv, txrc, sta);
-}
-
static void ath_rate_init(void *priv, struct ieee80211_supported_band *sband,
struct ieee80211_sta *sta, void *priv_sta)
{
--
1.6.0.6
Good morning all.
I wanna configured LinuxVoyage (Debian distro kernel 2.6.26) in 802.11a with Ath5k.
How i've to modify /etc/network/interfaces?
Thanks for your time.
_________________________________________________________________
News, entertainment and everything you care about at Live.com. Get it now!
http://www.live.com/getstarted.aspx
The purpose is to find the highest rate we can use.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/rc.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index 7cea944..16e41b1 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -601,10 +601,11 @@ static u8 ath_rc_setvalid_htrates(struct ath_rate_priv *ath_rc_priv,
return hi;
}
-static u8 ath_rc_ratefind_ht(struct ath_softc *sc,
- struct ath_rate_priv *ath_rc_priv,
- const struct ath_rate_table *rate_table,
- int *is_probing)
+/* Finds the highest rate index we can use */
+static u8 ath_rc_get_highest_rix(struct ath_softc *sc,
+ struct ath_rate_priv *ath_rc_priv,
+ const struct ath_rate_table *rate_table,
+ int *is_probing)
{
u32 dt, best_thruput, this_thruput, now_msec;
u8 rate, next_rate, best_rate, maxindex, minindex;
@@ -820,7 +821,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
try_per_rate = sc->hw->max_rate_tries;
rate_table = sc->cur_rate_table;
- rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
+ rix = ath_rc_get_highest_rix(sc, ath_rc_priv, rate_table, &is_probe);
nrix = rix;
if (is_probe) {
@@ -908,7 +909,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
"disabling MRR\n");
rates[0].idx = rate_lowest_index(sband, sta);
- /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
+ /* Disable MRR when ath_rc_get_highest_rix() found rate 0 */
rates[1].idx = -1;
}
}
--
1.6.0.6
There are a lot of gaps here.
Cc: Derek Smithies <[email protected]>
Cc: Chittajit Mitra <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/hw.h | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 73d859e..08eb5ae 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -113,14 +113,14 @@
enum wireless_mode {
ATH9K_MODE_11A = 0,
- ATH9K_MODE_11G = 3,
- ATH9K_MODE_11NA_HT20 = 6,
- ATH9K_MODE_11NG_HT20 = 7,
- ATH9K_MODE_11NA_HT40PLUS = 8,
- ATH9K_MODE_11NA_HT40MINUS = 9,
- ATH9K_MODE_11NG_HT40PLUS = 10,
- ATH9K_MODE_11NG_HT40MINUS = 11,
- ATH9K_MODE_MAX
+ ATH9K_MODE_11G,
+ ATH9K_MODE_11NA_HT20,
+ ATH9K_MODE_11NG_HT20,
+ ATH9K_MODE_11NA_HT40PLUS,
+ ATH9K_MODE_11NA_HT40MINUS,
+ ATH9K_MODE_11NG_HT40PLUS,
+ ATH9K_MODE_11NG_HT40MINUS,
+ ATH9K_MODE_MAX,
};
enum ath9k_hw_caps {
--
1.6.0.6
On Tue, Jun 09, 2009 at 10:54:00AM +0530, Vasanth Thiagarajan wrote:
> On Tue, Jun 09, 2009 at 06:28:21AM +0530, Luis Rodriguez wrote:
> > On Mon, Jun 08, 2009 at 05:26:41PM -0700, Luis R. Rodriguez wrote:
> > > On Fri, Jun 5, 2009 at 11:55 PM, Vasanth
> > > Thiagarajan<[email protected]> wrote:
> > > >
> > > > ________________________________________
> > > >
> > > >> > >> + /*
> > > >> > >> + * Fine tuning for when no decent rate was found, the
> > > >> > >> + * lowest should *not* be used under normal circumstances.
> > > >> > >> + */
> > > >> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > > >> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > > >> > >> + "disabling MRR\n");
> > > >> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> > > >> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> > > >> > >> + rates[1].idx = -1;
> > > >> > >> + }
> > > >> > >
> > > >> > > I think we can still fill other rates (1..3) with the lowest rate
> > > >> > > index as we dont differentiate the situation where the lowest rate
> > > >> > > is chosen truely by the algorithm from this particular case.
> > > >> >
> > > >> > I thought about that as well, but does it really make sense for us to
> > > >> > use MRR with the same lowest rate? That's why I just used one segment.
> > > >> > Thoughts?
> > > >>
> > > >> or we can try for max_retry (4) times. In that case the rate indices of
> > > >> other rates (just not 1) should be made -1 or this segment should
> > > >> moved just below the rate find.
> > > >
> > > > and the next segment [1]
> > > > is set to -1. Please let me know if there is anything else you see needs
> > > > change.
> > > >
> > > > Setting rate index of the rate series[1] is not enough as you are still filling the others rate
> > > > segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
> > > > set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
> > > > just below ath_rc_ratefind_ht(), like the following diff.
> > > >
> > > >
> > > > rate_table = sc->cur_rate_table;
> > > > rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
> > > > +
> > > > + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > > > + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > > > + "disabling MRR\n");
> > > > +
> > > > + ath_rc_rate_set_series(rate_table, &rates[0], txrc,
> > > > + 4, rix, 0);
> > >
> > > The above sets the rate[0].idx to rix
> > >
> > > > + rates[0].idx = rate_lowest_index(sband, sta);
> > >
> > > and then here we set it to rate_lowest_index(sband, sta) comes up
> > > with. They should be the same, but this just goes to show we need to
> > > clean this better.
> >
> > I just found this is wrong too.. the rate table used to find
> > rix is different than the rate table for the mode. ie, in 5ghz
> > when associated to an 11n station this could not include any
> > legacy rates.
>
> This is wrong. All basci rates still be used in 11n mode.
Never mind, this is only for 11ng.
>
> Vasanth
2009/6/5 Gábor Stefanik <[email protected]>:
> On Fri, Jun 5, 2009 at 7:41 AM, Luis R. Rodriguez<[email protected]> wrote:
>> index ba06e78..abad86b 100644
>> --- a/drivers/net/wireless/ath/ath9k/rc.c
>> +++ b/drivers/net/wireless/ath/ath9k/rc.c
>> @@ -741,10 +741,24 @@ static u8 ath_rc_ratefind_ht(struct ath_softc *sc,
>> if (rate > (ath_rc_priv->rate_table_size - 1))
>> rate = ath_rc_priv->rate_table_size - 1;
>>
>> - ASSERT((rate_table->info[rate].valid &&
>> - (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)) ||
>> - (rate_table->info[rate].valid_single_stream &&
>> - !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)));
>> + if (rate_table->info[rate].valid &&
>> + (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG))
>> + return rate;
>> +
>> + if (rate_table->info[rate].valid_single_stream &&
>> + !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG));
>> + return rate;
>> +
>> + /*
>> + * This should not happen, but we know it does for now... This
>> + * needs a proper fix but we're still not sure how this is caused.
>> + * Its not *critical* though so lets just warn when debug is enabled
>> + * for configuration changes.
>> + */
>> + if (sc->debug.debug_mask & ATH_DBG_RATE)
>> + WARN_ON(1);
>
> WARN_ON(sc->debug.debug_mask & ATH_DBG_RATE)
Will do, thanks.
Luis
On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
> - u16 fc;
> + __le16 fc;
>
> if (txrc->rts)
> info->control.rates[0].count =
> @@ -291,8 +291,7 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
>
> /* Send management frames and NO_ACK data using lowest rate. */
> fc = le16_to_cpu(hdr->frame_control);
> - if (!sta || !spinfo ||
> - (fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> + if (!sta || !spinfo || !ieee80211_is_data(fc) ||
you might want to run sparse on this entire patchset.
johannes
Unfortunately locking the driver rate control private area
doesn't resolve this issue and in fact causes some issues.
The root cause is yet undetermined. For now instead of crashing
lets just downgrades the ASSERT() and only show a trace when
we are debugging with ATH_DBG_CONFIG enabled. That attempt
and its results can be seen here:
http://marc.info/?l=linux-wireless&m=124399166108295&w=1
The ASSERT was happening because the rate control algorithm
figures it should find at least one valid dual stream or
single stream rate. *Something* is causing us to not find
such rate. This happens after association and only when HT
is enabled AFAICT. What we are doing by downgrading the assert
is informing the driver to use the lowest rate index. When
this happens lets disable multi rate retry and only try on
the lowest supported rate.
Traces of the ASSERT are available on this thread:
http://marc.info/?l=linux-wireless&m=124277331319024
At least one bug report (as side issue, mind you):
https://bugzilla.redhat.com/show_bug.cgi?id=503285
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/net/wireless/ath/ath9k/debug.h | 1 +
drivers/net/wireless/ath/ath9k/rc.c | 40 +++++++++++++++++++++++++++----
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index edda15b..9d72bc8 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -30,6 +30,7 @@ enum ATH_DEBUG {
ATH_DBG_CONFIG = 0x00000200,
ATH_DBG_FATAL = 0x00000400,
ATH_DBG_PS = 0x00000800,
+ ATH_DBG_RATE = 0x00001000,
ATH_DBG_ANY = 0xffffffff
};
diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index ba06e78..abad86b 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -741,10 +741,24 @@ static u8 ath_rc_ratefind_ht(struct ath_softc *sc,
if (rate > (ath_rc_priv->rate_table_size - 1))
rate = ath_rc_priv->rate_table_size - 1;
- ASSERT((rate_table->info[rate].valid &&
- (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)) ||
- (rate_table->info[rate].valid_single_stream &&
- !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)));
+ if (rate_table->info[rate].valid &&
+ (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG))
+ return rate;
+
+ if (rate_table->info[rate].valid_single_stream &&
+ !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG));
+ return rate;
+
+ /*
+ * This should not happen, but we know it does for now... This
+ * needs a proper fix but we're still not sure how this is caused.
+ * Its not *critical* though so lets just warn when debug is enabled
+ * for configuration changes.
+ */
+ if (sc->debug.debug_mask & ATH_DBG_RATE)
+ WARN_ON(1);
+
+ rate = ath_rc_priv->valid_rate_index[0];
return rate;
}
@@ -837,13 +851,15 @@ static u8 ath_rc_rate_getidx(struct ath_softc *sc,
static void ath_rc_ratefind(struct ath_softc *sc,
struct ath_rate_priv *ath_rc_priv,
- struct ieee80211_tx_rate_control *txrc)
+ struct ieee80211_tx_rate_control *txrc,
+ struct ieee80211_sta *sta)
{
const struct ath_rate_table *rate_table;
struct sk_buff *skb = txrc->skb;
struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
struct ieee80211_tx_rate *rates = tx_info->control.rates;
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ struct ieee80211_supported_band *sband = txrc->sband;
__le16 fc = hdr->frame_control;
u8 try_per_rate = 0, i = 0, rix, nrix;
int is_probe = 0;
@@ -935,6 +951,18 @@ static void ath_rc_ratefind(struct ath_softc *sc,
/* Setup RTS/CTS */
ath_rc_rate_set_rtscts(sc, rate_table, tx_info);
+
+ /*
+ * Fine tuning for when no decent rate was found, the
+ * lowest should *not* be used under normal circumstances.
+ */
+ if (rix == ath_rc_priv->valid_rate_index[0]) {
+ DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
+ "disabling MRR\n");
+ rates[0].idx = rate_lowest_index(sband, sta);
+ /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
+ rates[1].idx = -1;
+ }
}
static bool ath_rc_update_per(struct ath_softc *sc,
@@ -1582,7 +1610,7 @@ static void ath_get_rate(void *priv, struct ieee80211_sta *sta, void *priv_sta,
}
/* Find tx rate for unicast frames */
- ath_rc_ratefind(sc, ath_rc_priv, txrc);
+ ath_rc_ratefind(sc, ath_rc_priv, txrc, sta);
}
static void ath_rate_init(void *priv, struct ieee80211_supported_band *sband,
--
1.6.0.6
On Fri, Jun 05, 2009 at 01:17:16PM +0530, Luis Rodriguez wrote:
> On Thu, Jun 4, 2009 at 11:30 PM, Vasanthakumar
> Thiagarajan<[email protected]> wrote:
> >
> >> +
> >> + /*
> >> + * Fine tuning for when no decent rate was found, the
> >> + * lowest should *not* be used under normal circumstances.
> >> + */
> >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> >> + "disabling MRR\n");
> >> + rates[0].idx = rate_lowest_index(sband, sta);
> >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> >> + rates[1].idx = -1;
> >> + }
> >
> > I think we can still fill other rates (1..3) with the lowest rate
> > index as we dont differentiate the situation where the lowest rate
> > is chosen truely by the algorithm from this particular case.
>
> I thought about that as well, but does it really make sense for us to
> use MRR with the same lowest rate? That's why I just used one segment.
> Thoughts?
or we can try for max_retry (4) times. In that case the rate indices of
other rates (just not 1) should be made -1 or this segment should be
moved just below the rate find.
Vasanth
On Thu, 2009-06-04 at 23:34 -0700, Luis R. Rodriguez wrote:
> On Thu, Jun 4, 2009 at 11:24 PM, Johannes Berg
> <[email protected]> wrote:
> > On Fri, 2009-06-05 at 01:41 -0400, Luis R. Rodriguez wrote:
> >
> >> - u16 fc;
> >> + __le16 fc;
> >>
> >> if (txrc->rts)
> >> info->control.rates[0].count =
> >> @@ -291,8 +291,7 @@ rate_control_pid_get_rate(void *priv, struct ieee80211_sta *sta,
> >>
> >> /* Send management frames and NO_ACK data using lowest rate. */
> >> fc = le16_to_cpu(hdr->frame_control);
> >> - if (!sta || !spinfo ||
> >> - (fc & IEEE80211_FCTL_FTYPE) != IEEE80211_FTYPE_DATA ||
> >> + if (!sta || !spinfo || !ieee80211_is_data(fc) ||
> >
> > you might want to run sparse on this entire patchset.
>
> I did, PID was not on my kconfig though, so its the only thing I
> didn't test compile.
Ok. Curious I accidentally found the only bug then :)
johannes
On Fri, Jun 5, 2009 at 7:41 AM, Luis R. Rodriguez<[email protected]> wrote:
> Unfortunately locking the driver rate control private area
> doesn't resolve this issue and in fact causes some issues.
> The root cause is yet undetermined. For now instead of crashing
> lets just downgrades the ASSERT() and only show a trace when
> we are debugging with ATH_DBG_CONFIG enabled. That attempt
> and its results can be seen here:
>
> http://marc.info/?l=linux-wireless&m=124399166108295&w=1
>
> The ASSERT was happening because the rate control algorithm
> figures it should find at least one valid dual stream or
> single stream rate. *Something* is causing us to not find
> such rate. This happens after association and only when HT
> is enabled AFAICT. What we are doing by downgrading the assert
> is informing the driver to use the lowest rate index. When
> this happens lets disable multi rate retry and only try on
> the lowest supported rate.
>
> Traces of the ASSERT are available on this thread:
>
> http://marc.info/?l=linux-wireless&m=124277331319024
>
> At least one bug report (as side issue, mind you):
>
> https://bugzilla.redhat.com/show_bug.cgi?id=503285
>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> ?drivers/net/wireless/ath/ath9k/debug.h | ? ?1 +
> ?drivers/net/wireless/ath/ath9k/rc.c ? ?| ? 40 +++++++++++++++++++++++++++----
> ?2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
> index edda15b..9d72bc8 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.h
> +++ b/drivers/net/wireless/ath/ath9k/debug.h
> @@ -30,6 +30,7 @@ enum ATH_DEBUG {
> ? ? ? ?ATH_DBG_CONFIG ? ? ? ? ?= 0x00000200,
> ? ? ? ?ATH_DBG_FATAL ? ? ? ? ? = 0x00000400,
> ? ? ? ?ATH_DBG_PS ? ? ? ? ? ? ?= 0x00000800,
> + ? ? ? ATH_DBG_RATE ? ? ? ? ? ?= 0x00001000,
> ? ? ? ?ATH_DBG_ANY ? ? ? ? ? ? = 0xffffffff
> ?};
>
> diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
> index ba06e78..abad86b 100644
> --- a/drivers/net/wireless/ath/ath9k/rc.c
> +++ b/drivers/net/wireless/ath/ath9k/rc.c
> @@ -741,10 +741,24 @@ static u8 ath_rc_ratefind_ht(struct ath_softc *sc,
> ? ? ? ?if (rate > (ath_rc_priv->rate_table_size - 1))
> ? ? ? ? ? ? ? ?rate = ath_rc_priv->rate_table_size - 1;
>
> - ? ? ? ASSERT((rate_table->info[rate].valid &&
> - ? ? ? ? ? ? ? (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)) ||
> - ? ? ? ? ? ? ?(rate_table->info[rate].valid_single_stream &&
> - ? ? ? ? ? ? ? !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG)));
> + ? ? ? if (rate_table->info[rate].valid &&
> + ? ? ? ? ? (ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG))
> + ? ? ? ? ? ? ? return rate;
> +
> + ? ? ? if (rate_table->info[rate].valid_single_stream &&
> + ? ? ? ? ? !(ath_rc_priv->ht_cap & WLAN_RC_DS_FLAG));
> + ? ? ? ? ? ? ? return rate;
> +
> + ? ? ? /*
> + ? ? ? ?* This should not happen, but we know it does for now... This
> + ? ? ? ?* needs a proper fix but we're still not sure how this is caused.
> + ? ? ? ?* Its not *critical* though so lets just warn when debug is enabled
> + ? ? ? ?* for configuration changes.
> + ? ? ? ?*/
> + ? ? ? if (sc->debug.debug_mask & ATH_DBG_RATE)
> + ? ? ? ? ? ? ? WARN_ON(1);
WARN_ON(sc->debug.debug_mask & ATH_DBG_RATE)
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
________________________________________
> > >> + /*
> > >> + * Fine tuning for when no decent rate was found, the
> > >> + * lowest should *not* be used under normal circumstances.
> > >> + */
> > >> + if (rix == ath_rc_priv->valid_rate_index[0]) {
> > >> + DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
> > >> + "disabling MRR\n");
> > >> + rates[0].idx = rate_lowest_index(sband, sta);
> > >> + /* Disable MRR when ath_rc_ratefind_ht() found rate 0 */
> > >> + rates[1].idx = -1;
> > >> + }
> > >
> > > I think we can still fill other rates (1..3) with the lowest rate
> > > index as we dont differentiate the situation where the lowest rate
> > > is chosen truely by the algorithm from this particular case.
> >
> > I thought about that as well, but does it really make sense for us to
> > use MRR with the same lowest rate? That's why I just used one segment.
> > Thoughts?
>
> or we can try for max_retry (4) times. In that case the rate indices of
> other rates (just not 1) should be made -1 or this segment should
> moved just below the rate find.
and the next segment [1]
is set to -1. Please let me know if there is anything else you see needs
change.
Setting rate index of the rate series[1] is not enough as you are still filling the others rate
segments(2 and 3) by ath_rc_rate_getidx() in the for..loop, so other segments are also be
set to -1, but it looks hacky, one clean way of doing this can be, moving you code segment to
just below ath_rc_ratefind_ht(), like the following diff.
rate_table = sc->cur_rate_table;
rix = ath_rc_ratefind_ht(sc, ath_rc_priv, rate_table, &is_probe);
+
+ if (rix == ath_rc_priv->valid_rate_index[0]) {
+ DPRINTF(sc, ATH_DBG_RATE, "lowest rate being used, "
+ "disabling MRR\n");
+
+ ath_rc_rate_set_series(rate_table, &rates[0], txrc,
+ 4, rix, 0);
+ rates[0].idx = rate_lowest_index(sband, sta);
+ goto find_ctrl_rateix;
+ }
+
nrix = rix;
if (is_probe) {
@@ -933,6 +944,7 @@ static void ath_rc_ratefind(struct ath_softc *sc,
rates[0].count = ATH_TXMAXTRY;
}
+find_ctrl_rateix:
/* Setup RTS/CTS */
ath_rc_rate_set_rtscts(sc, rate_table, tx_info);
}
Vasanth