[PATCH 0/3] ath9k: rate fix for invalid frames

[PATCH 0/3] ath9k: rate fix for invalid frames

[Luis R. Rodriguez]

We've tried a few things to find the real root cause to
the ath9k rate control assert. This is it, turns out we
make some assumptions about scanning and nullfunc frames.
We fix those assumptions here.

I'll send the ath9k rate control cleanup stuff later
though, all of that is still valid.

I think the iwlwifi rate control work arounds could be removed
now though but not sure, I saw in the bug report the get_rate
warn on was found during disassoc or something like that which
I cannot see being triggerable by the conditions found in these
patches unless iwlwifi is getting frames through some other
uknown mean.

Luis R. Rodriguez (3):
  mac80211: fix - drop frames for sta with no valid rate
  mac80211: add debugfs counter for not valid birate frames
  ath9k: downgrade assert in rc.c for invalid rate

 drivers/net/wireless/ath/ath9k/rc.c |   16 ++++++++++---
 include/net/mac80211.h              |   11 +++++++++
 net/mac80211/debugfs.c              |    4 +++
 net/mac80211/ieee80211_i.h          |    2 +
 net/mac80211/tx.c                   |   39 +++++++++++++++++++++++++++++++++++
 5 files changed, 68 insertions(+), 4 deletions(-)

[PATCH 1/3] mac80211: fix - drop frames for sta with no valid rate

[Luis R. Rodriguez]

If we're associated and scanning mac80211 will allow through
nullfunc and probe request frames. When we're scanning on a
different band than the one we're associated on we should
not send nullfunc frames to the sta on that band as it would
be the incorrect band.

Lets catch the case where no valid rate is usable when associated
and discard those frames and warn only for the case the frame is
not a nullfunc or probe request as those are the only accounted
for frames mac80211 should allow through while scanning.

This fixes an oops which occured due to an assert in ath9k:

http://marc.info/?l=linux-wireless&m=124277331319024

The assert was happening because the rate control algorithm
figures it should find at least one valid dual stream or
single stream rate. Since we allow mac80211 to send get_rate
callback for drivers for a sta on invalid band no valid will
actually have been found and hence the assert.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 include/net/mac80211.h |   11 +++++++++++
 net/mac80211/tx.c      |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 17d61d1..9559efa 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2103,6 +2103,17 @@ rate_lowest_index(struct ieee80211_supported_band *sband,
 	return 0;
 }
 
+static inline
+bool rate_usable_index_exists(struct ieee80211_supported_band *sband,
+			      struct ieee80211_sta *sta)
+{
+	unsigned int i;
+
+	for (i = 0; i < sband->n_bitrates; i++)
+		if (rate_supported(sta, sband->band, i))
+			return true;
+	return false;
+}
 
 int ieee80211_rate_control_register(struct rate_control_ops *ops);
 void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1436f74..03f9a4e 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -511,6 +511,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	int i, len;
 	bool inval = false, rts = false, short_preamble = false;
 	struct ieee80211_tx_rate_control txrc;
+	u32 sta_flags;
 
 	memset(&txrc, 0, sizeof(txrc));
 
@@ -543,7 +544,44 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	     (tx->sta && test_sta_flags(tx->sta, WLAN_STA_SHORT_PREAMBLE))))
 		txrc.short_preamble = short_preamble = true;
 
+	sta_flags = tx->sta ? get_sta_flags(tx->sta) : 0;
+
+	/*
+	 * Lets not bother rate control if we're associated and cannot
+	 * talk to the sta. It makes little since for this to happen,
+	 * it should mean we're scanning on another band somehow some frames
+	 * got through the TX queue -- these should have been not been added
+	 * to our TX queue. There is one excemption to this, but we handle
+	 * these below.
+	 */
+
+	if (unlikely((tx->local->sw_scanning) &&
+	    (sta_flags & WLAN_STA_ASSOC) &&
+	    !rate_usable_index_exists(sband, &tx->sta->sta))) {
+		/*
+		 *  The only accounted for frames of this type in
+		 *  mac80211 are probe requests and null func frames,
+		 *  so just warn for other drop of frames. Drop the
+		 *  frames anyway as we have no usable bit rate.
+		 */
+		if (!ieee80211_is_probe_req(hdr->frame_control) &&
+		    !ieee80211_is_nullfunc(hdr->frame_control)) {
+#ifdef CONFIG_MAC80211_VERBOSE_DEBUG
+			printk(KERN_DEBUG "%s: dropped data frame -- no "
+			       "supported rate for station %pM on %c "
+			       "GHz band\n",
+			       tx->dev->name, hdr->addr1,
+			       tx->channel->band ? '5' : '2');
+			WARN_ON(1);
+#endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
+		}
+		return TX_DROP;
+	}
 
+	/*
+	 * If we're associated at this point with the sta we know we can at
+	 * least send the frame at the lowest bit rate.
+	 */
 	rate_control_get_rate(tx->sdata, tx->sta, &txrc);
 
 	if (unlikely(info->control.rates[0].idx < 0))
-- 
1.6.0.6

Re: [PATCH 1/3] mac80211: fix - drop frames for sta with no valid rate

[Johannes Berg]

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

On Tue, 2009-06-09 at 04:30 -0400, Luis R. Rodriguez wrote:
> If we're associated and scanning mac80211 will allow through
> nullfunc and probe request frames. When we're scanning on a
> different band than the one we're associated on we should
> not send nullfunc frames to the sta on that band as it would
> be the incorrect band.
> 
> Lets catch the case where no valid rate is usable when associated
> and discard those frames and warn only for the case the frame is
> not a nullfunc or probe request as those are the only accounted
> for frames mac80211 should allow through while scanning.
> 
> This fixes an oops which occured due to an assert in ath9k:
> 
> http://marc.info/?l=linux-wireless&m=124277331319024
> 
> The assert was happening because the rate control algorithm
> figures it should find at least one valid dual stream or
> single stream rate. Since we allow mac80211 to send get_rate
> callback for drivers for a sta on invalid band no valid will
> actually have been found and hence the assert.
> 
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  include/net/mac80211.h |   11 +++++++++++
>  net/mac80211/tx.c      |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 17d61d1..9559efa 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -2103,6 +2103,17 @@ rate_lowest_index(struct ieee80211_supported_band *sband,
>  	return 0;
>  }
>  
> +static inline
> +bool rate_usable_index_exists(struct ieee80211_supported_band *sband,
> +			      struct ieee80211_sta *sta)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < sband->n_bitrates; i++)
> +		if (rate_supported(sta, sband->band, i))
> +			return true;
> +	return false;
> +}
>  
>  int ieee80211_rate_control_register(struct rate_control_ops *ops);
>  void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 1436f74..03f9a4e 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -511,6 +511,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>  	int i, len;
>  	bool inval = false, rts = false, short_preamble = false;
>  	struct ieee80211_tx_rate_control txrc;
> +	u32 sta_flags;
>  
>  	memset(&txrc, 0, sizeof(txrc));
>  
> @@ -543,7 +544,44 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>  	     (tx->sta && test_sta_flags(tx->sta, WLAN_STA_SHORT_PREAMBLE))))
>  		txrc.short_preamble = short_preamble = true;
>  
> +	sta_flags = tx->sta ? get_sta_flags(tx->sta) : 0;
> +
> +	/*
> +	 * Lets not bother rate control if we're associated and cannot
> +	 * talk to the sta. It makes little since for this to happen,
> +	 * it should mean we're scanning on another band somehow some frames
> +	 * got through the TX queue -- these should have been not been added
> +	 * to our TX queue. There is one excemption to this, but we handle
> +	 * these below.
> +	 */
> +
> +	if (unlikely((tx->local->sw_scanning) &&
> +	    (sta_flags & WLAN_STA_ASSOC) &&
> +	    !rate_usable_index_exists(sband, &tx->sta->sta))) {
> +		/*
> +		 *  The only accounted for frames of this type in
> +		 *  mac80211 are probe requests and null func frames,
> +		 *  so just warn for other drop of frames. Drop the
> +		 *  frames anyway as we have no usable bit rate.
> +		 */

Can we not avoid this situation to start with by flushing the master
device queues before we go scan?

johannes

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

Re: [PATCH 1/3] mac80211: fix - drop frames for sta with no valid rate

[Luis R. Rodriguez]

On Tue, Jun 9, 2009 at 1:54 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Tue, 2009-06-09 at 04:30 -0400, Luis R. Rodriguez wrote:
>> If we're associated and scanning mac80211 will allow through
>> nullfunc and probe request frames. When we're scanning on a
>> different band than the one we're associated on we should
>> not send nullfunc frames to the sta on that band as it would
>> be the incorrect band.
>>
>> Lets catch the case where no valid rate is usable when associated
>> and discard those frames and warn only for the case the frame is
>> not a nullfunc or probe request as those are the only accounted
>> for frames mac80211 should allow through while scanning.
>>
>> This fixes an oops which occured due to an assert in ath9k:
>>
>> http://marc.info/?l=linux-wireless&m=124277331319024
>>
>> The assert was happening because the rate control algorithm
>> figures it should find at least one valid dual stream or
>> single stream rate. Since we allow mac80211 to send get_rate
>> callback for drivers for a sta on invalid band no valid will
>> actually have been found and hence the assert.
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  include/net/mac80211.h |   11 +++++++++++
>>  net/mac80211/tx.c      |   38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 49 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index 17d61d1..9559efa 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -2103,6 +2103,17 @@ rate_lowest_index(struct ieee80211_supported_band *sband,
>>       return 0;
>>  }
>>
>> +static inline
>> +bool rate_usable_index_exists(struct ieee80211_supported_band *sband,
>> +                           struct ieee80211_sta *sta)
>> +{
>> +     unsigned int i;
>> +
>> +     for (i = 0; i < sband->n_bitrates; i++)
>> +             if (rate_supported(sta, sband->band, i))
>> +                     return true;
>> +     return false;
>> +}
>>
>>  int ieee80211_rate_control_register(struct rate_control_ops *ops);
>>  void ieee80211_rate_control_unregister(struct rate_control_ops *ops);
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 1436f74..03f9a4e 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -511,6 +511,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>>       int i, len;
>>       bool inval = false, rts = false, short_preamble = false;
>>       struct ieee80211_tx_rate_control txrc;
>> +     u32 sta_flags;
>>
>>       memset(&txrc, 0, sizeof(txrc));
>>
>> @@ -543,7 +544,44 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>>            (tx->sta && test_sta_flags(tx->sta, WLAN_STA_SHORT_PREAMBLE))))
>>               txrc.short_preamble = short_preamble = true;
>>
>> +     sta_flags = tx->sta ? get_sta_flags(tx->sta) : 0;
>> +
>> +     /*
>> +      * Lets not bother rate control if we're associated and cannot
>> +      * talk to the sta. It makes little since for this to happen,
>> +      * it should mean we're scanning on another band somehow some frames
>> +      * got through the TX queue -- these should have been not been added
>> +      * to our TX queue. There is one excemption to this, but we handle
>> +      * these below.
>> +      */
>> +
>> +     if (unlikely((tx->local->sw_scanning) &&
>> +         (sta_flags & WLAN_STA_ASSOC) &&
>> +         !rate_usable_index_exists(sband, &tx->sta->sta))) {
>> +             /*
>> +              *  The only accounted for frames of this type in
>> +              *  mac80211 are probe requests and null func frames,
>> +              *  so just warn for other drop of frames. Drop the
>> +              *  frames anyway as we have no usable bit rate.
>> +              */
>
> Can we not avoid this situation to start with by flushing the master
> device queues before we go scan?

How do you do that?

  Luis

[PATCH 2/3] mac80211: add debugfs counter for not valid birate frames

[Luis R. Rodriguez]

Lets keep track of how many frames we drop due to not having
a valid bitrate for the sta.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/debugfs.c     |    4 ++++
 net/mac80211/ieee80211_i.h |    2 ++
 net/mac80211/tx.c          |    1 +
 3 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 11c7231..6a4b15c 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -248,6 +248,8 @@ DEBUGFS_STATS_FILE(tx_handlers_drop_wep, 20, "%u",
 		   local->tx_handlers_drop_wep);
 DEBUGFS_STATS_FILE(tx_handlers_drop_not_assoc, 20, "%u",
 		   local->tx_handlers_drop_not_assoc);
+DEBUGFS_STATS_FILE(tx_handlers_drop_no_bitrate, 20, "%u",
+		   local->tx_handlers_drop_no_bitrate);
 DEBUGFS_STATS_FILE(tx_handlers_drop_unauth_port, 20, "%u",
 		   local->tx_handlers_drop_unauth_port);
 DEBUGFS_STATS_FILE(rx_handlers_drop, 20, "%u",
@@ -324,6 +326,7 @@ void debugfs_hw_add(struct ieee80211_local *local)
 	DEBUGFS_STATS_ADD(tx_handlers_drop_fragment);
 	DEBUGFS_STATS_ADD(tx_handlers_drop_wep);
 	DEBUGFS_STATS_ADD(tx_handlers_drop_not_assoc);
+	DEBUGFS_STATS_ADD(tx_handlers_drop_no_bitrate);
 	DEBUGFS_STATS_ADD(tx_handlers_drop_unauth_port);
 	DEBUGFS_STATS_ADD(rx_handlers_drop);
 	DEBUGFS_STATS_ADD(rx_handlers_queued);
@@ -370,6 +373,7 @@ void debugfs_hw_del(struct ieee80211_local *local)
 	DEBUGFS_STATS_DEL(tx_handlers_drop_fragment);
 	DEBUGFS_STATS_DEL(tx_handlers_drop_wep);
 	DEBUGFS_STATS_DEL(tx_handlers_drop_not_assoc);
+	DEBUGFS_STATS_DEL(tx_handlers_drop_no_bitrate);
 	DEBUGFS_STATS_DEL(tx_handlers_drop_unauth_port);
 	DEBUGFS_STATS_DEL(rx_handlers_drop);
 	DEBUGFS_STATS_DEL(rx_handlers_queued);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c088c46..b46f46f 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -733,6 +733,7 @@ struct ieee80211_local {
 	unsigned int tx_handlers_drop_fragment;
 	unsigned int tx_handlers_drop_wep;
 	unsigned int tx_handlers_drop_not_assoc;
+	unsigned int tx_handlers_drop_no_bitrate;
 	unsigned int tx_handlers_drop_unauth_port;
 	unsigned int rx_handlers_drop;
 	unsigned int rx_handlers_queued;
@@ -804,6 +805,7 @@ struct ieee80211_local {
 			struct dentry *tx_handlers_drop_fragment;
 			struct dentry *tx_handlers_drop_wep;
 			struct dentry *tx_handlers_drop_not_assoc;
+			struct dentry *tx_handlers_drop_no_bitrate;
 			struct dentry *tx_handlers_drop_unauth_port;
 			struct dentry *rx_handlers_drop;
 			struct dentry *rx_handlers_queued;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 03f9a4e..9f0a23b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -575,6 +575,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 			WARN_ON(1);
 #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
 		}
+		I802_DEBUG_INC(tx->local->tx_handlers_drop_no_bitrate);
 		return TX_DROP;
 	}
 
-- 
1.6.0.6

[PATCH 3/3] ath9k: downgrade assert in rc.c for invalid rate

[Luis R. Rodriguez]

The case where no vaid rate is found should not happen now
but to help debugging and downgrade this to a warn.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath/ath9k/rc.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rc.c b/drivers/net/wireless/ath/ath9k/rc.c
index ba06e78..d7f4030 100644
--- a/drivers/net/wireless/ath/ath9k/rc.c
+++ b/drivers/net/wireless/ath/ath9k/rc.c
@@ -741,10 +741,18 @@ 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 */
+	WARN_ON(1);
+
+	rate = ath_rc_priv->valid_rate_index[0];
 
 	return rate;
 }
-- 
1.6.0.6