fix iwlwifi needed RX chains

PATCH In review Johannes Berg 3 participants 9 messages Feb 25, 2011 – Feb 26, 2011

Subsystems (2)

Networking drivers (wireless) mac80211

lore

Share & tools

lore

Johannes Berg

author maintainer

2011-02-25 11:26 UTC #0 lore raw

cover fix iwlwifi needed RX chains

Finally, I got around to looking into this problem, I apologise for the delay!

The issue turns out to be that mostly we were working with asymmetric devices (2x3) and there this worked ... otherwise not.

The original mac80211 patch is still needed to fix it, but it's not sufficient, iwlagn also needs to be fixed.

Tested against an ath9k AP (3x3) and also against the same ath9k AP with hostapd hacked to pretend it is 1x1.

johannes

Johannes Berg

author maintainer

2011-02-25 11:26 UTC #1 lore raw

PATCH 1/2 mac80211: copy peer MCS TX parameters

From: Johannes Berg <[email protected]>

We need to copy this to allow drivers to look
at the information where needed.

Signed-off-by Johannes Berg <[email protected]>

---
 net/mac80211/ht.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/mac80211/ht.c +4 -1

@@ -66,6 +66,9 @@ void ieee80211_ht_cap_ie_to_sta_ht_cap(s
 	/* own MCS TX capabilities */
 	tx_mcs_set_cap = sband->ht_cap.mcs.tx_params;
 
+	/* Copy peer MCS TX capabilities, the driver might need them. */
+	ht_cap->mcs.tx_params = ht_cap_ie->mcs.tx_params;
+
 	/* can we TX with MCS rates? */
 	if (!(tx_mcs_set_cap & IEEE80211_HT_MCS_TX_DEFINED))
 		return;

@@ -79,7 +82,7 @@ void ieee80211_ht_cap_ie_to_sta_ht_cap(s
 		max_tx_streams = IEEE80211_HT_MCS_TX_MAX_STREAMS;
 
 	/*
-	 * 802.11n D5.0 20.3.5 / 20.6 says:
+	 * 802.11n-2009 20.3.5 / 20.6 says:
 	 * - indices 0 to 7 and 32 are single spatial stream
 	 * - 8 to 31 are multiple spatial streams using equal modulation
 	 *   [8..15 for two streams, 16..23 for three and 24..31 for four]

Johannes Berg

author maintainer

2011-02-25 11:26 UTC #2 lore raw

PATCH 2/2 iwlagn: fix iwlagn_check_needed_chains

In reply to Johannes Berg

From: Johannes Berg <[email protected]>

This function was intended to calculate the number of RX chains needed, but could only work where the AP's streams were asymmetric, i.e. 2 TX and 3 RX or similar. In the case where IEEE80211_HT_MCS_TX_RX_DIFF was not set, this function would calculate the wrong information.

Additionally, mac80211 didn't pass through the required values at all, so it couldn't work anyway.

Rewrite the logic in this function and add appropriate comments to make it readable.

Signed-off-by Johannes Berg <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-agn-rxon.c |   58 +++++++++++++++++++---------
 1 file changed, 41 insertions(+), 17 deletions(-)

drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +41 ~~-17~~

@@ -471,6 +471,7 @@ static void iwlagn_check_needed_chains(s
 	struct iwl_rxon_context *tmp;
 	struct ieee80211_sta *sta;
 	struct iwl_ht_config *ht_conf = &priv->current_ht_config;
+	struct ieee80211_sta_ht_cap *ht_cap;
 	bool need_multiple;
 
 	lockdep_assert_held(&priv->mutex);

@@ -479,23 +480,7 @@ static void iwlagn_check_needed_chains(s
 	case NL80211_IFTYPE_STATION:
 		rcu_read_lock();
 		sta = ieee80211_find_sta(vif, bss_conf->bssid);
-		if (sta) {
-			struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap;
-			int maxstreams;
-
-			maxstreams = (ht_cap->mcs.tx_params &
-				      IEEE80211_HT_MCS_TX_MAX_STREAMS_MASK)
-					>> IEEE80211_HT_MCS_TX_MAX_STREAMS_SHIFT;
-			maxstreams += 1;
-
-			need_multiple = true;
-
-			if ((ht_cap->mcs.rx_mask[1] == 0) &&
-			    (ht_cap->mcs.rx_mask[2] == 0))
-				need_multiple = false;
-			if (maxstreams <= 1)
-				need_multiple = false;
-		} else {
+		if (!sta) {
 			/*
 			 * If at all, this can only happen through a race
 			 * when the AP disconnects us while we're still

@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
 			 * will soon tell us about that.
 			 */
 			need_multiple = false;
+			rcu_read_unlock();
+			break;
+		}
+
+		ht_cap = &sta->ht_cap;
+
+		need_multiple = true;
+
+		/*
+		 * If the peer advertises no support for receiving 2 and 3
+		 * stream MCS rates, it can't be transmitting them either.
+		 */
+		if (ht_cap->mcs.rx_mask[1] == 0 &&
+		    ht_cap->mcs.rx_mask[2] == 0) {
+			need_multiple = false;
+		} else if (!(ht_cap->mcs.tx_params &
+						IEEE80211_HT_MCS_TX_DEFINED)) {
+			/* If it can't TX MCS at all ... */
+			need_multiple = false;
+		} else if (ht_cap->mcs.tx_params &
+						IEEE80211_HT_MCS_TX_RX_DIFF) {
+			int maxstreams;
+
+			/*
+			 * But if it can receive them, it might still not
+			 * be able to transmit them, which is what we need
+			 * to check here -- so check the number of streams
+			 * it advertises for TX (if different from RX).
+			 */
+
+			maxstreams = (ht_cap->mcs.tx_params &
+				 IEEE80211_HT_MCS_TX_MAX_STREAMS_MASK);
+			maxstreams >>=
+				IEEE80211_HT_MCS_TX_MAX_STREAMS_SHIFT;
+			maxstreams += 1;
+
+			if (maxstreams <= 1)
+				need_multiple = false;
 		}
+
 		rcu_read_unlock();
 		break;
 	case NL80211_IFTYPE_ADHOC:

wwguy

2011-02-25 15:38 UTC #3 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Hi Johannes,

Quoted 58 lines · Johannes Berg

From: Johannes Berg <[email protected]>

Additionally, mac80211 didn't pass through the required values at all, so it couldn't work anyway.

Rewrite the logic in this function and add appropriate comments to make it readable.

Signed-off-by Johannes Berg <[email protected]>

---
 drivers/net/wireless/iwlwifi/iwl-agn-rxon.c |   58 +++++++++++++++++++---------
 1 file changed, 41 insertions(+), 17 deletions(-)

drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 ~~-17~~

@@ -471,6 +471,7 @@ static void iwlagn_check_needed_chains(s
 	struct iwl_rxon_context *tmp;
 	struct ieee80211_sta *sta;
 	struct iwl_ht_config *ht_conf = &priv->current_ht_config;
+	struct ieee80211_sta_ht_cap *ht_cap;
 	bool need_multiple;
 
 	lockdep_assert_held(&priv->mutex);

@@ -479,23 +480,7 @@ static void iwlagn_check_needed_chains(s
 	case NL80211_IFTYPE_STATION:
 		rcu_read_lock();
 		sta = ieee80211_find_sta(vif, bss_conf->bssid);
-		if (sta) {
-			struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap;
-			int maxstreams;
-
-			maxstreams = (ht_cap->mcs.tx_params &
-				      IEEE80211_HT_MCS_TX_MAX_STREAMS_MASK)
-					>> IEEE80211_HT_MCS_TX_MAX_STREAMS_SHIFT;
-			maxstreams += 1;
-
-			need_multiple = true;
-
-			if ((ht_cap->mcs.rx_mask[1] == 0) &&
-			    (ht_cap->mcs.rx_mask[2] == 0))
-				need_multiple = false;
-			if (maxstreams <= 1)
-				need_multiple = false;
-		} else {
+		if (!sta) {
 			/*
 			 * If at all, this can only happen through a race
 			 * when the AP disconnects us while we're still

drivers/net/wireless/iwlwifi/iwl-agn-rxon.c static void iwlagn_check_needed_chains(s · line 492

Quoted 5 lines

drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 -0

@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
 			 * will soon tell us about that.
 			 */
 			need_multiple = false;
+			rcu_read_unlock();
+			break;

why unlock and break here instead stay do it later like before?

Wey

Johannes Berg

author maintainer

2011-02-25 15:39 UTC #4 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Quoted 8 lines · wwguy

drivers/net/wireless/iwlwifi/iwl-agn-rxon.c static void iwlagn_check_needed_chains(s · line 492
Quoted 6 lines
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 -0
@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
 			 * will soon tell us about that.
 			 */
 			need_multiple = false;
+			rcu_read_unlock();
+			break;
why unlock and break here instead stay do it later like before?

Only because the indentation there was getting so deep it was annoying to fit into 80 cols :-)

johannes

Guy, Wey-Yi

2011-02-25 15:46 UTC #5 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Quoted 11 lines · Johannes Berg

Quoted 8 lines · wwguy
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c static void iwlagn_check_needed_chains(s · line 492
Quoted 6 lines
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 -0
@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
 			 * will soon tell us about that.
 			 */
 			need_multiple = false;
+			rcu_read_unlock();
+			break;
why unlock and break here instead stay do it later like before?

Quoted 2 lines

Only because the indentation there was getting so deep it was annoying to fit into 80 cols :-)

how you want to submit this patch? through iwlagn, or yo uwill send it along with mac80211 to John?

Wey

Quoted 1 line

Johannes Berg

author maintainer

2011-02-25 15:49 UTC #6 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Quoted 12 lines · Wey-Yi

Quoted 11 lines · Johannes Berg
Quoted 8 lines · wwguy
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c static void iwlagn_check_needed_chains(s · line 492
Quoted 6 lines
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 -0
@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
 			 * will soon tell us about that.
 			 */
 			need_multiple = false;
+			rcu_read_unlock();
+			break;
why unlock and break here instead stay do it later like before?

Quoted 5 lines

Quoted 2 lines
Only because the indentation there was getting so deep it was annoying to fit into 80 cols :-)
how you want to submit this patch? through iwlagn, or yo uwill send it along with mac80211 to John?

Don't really care. I just sent them both here to keep them together and for Daniel and Swati ... First though, I'd like them to test it maybe.

johannes

Daniel Halperin

Acked-by Tested-by

2011-02-26 18:00 UTC #7 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Acked-by Daniel Halperin <[email protected]>

Tested-by Daniel Halperin <[email protected]>

(I suppose those are redundant?)

Dan

Quoted 25 lines · Johannes Berg

Quoted 12 lines · Wey-Yi
Quoted 11 lines · Johannes Berg
Quoted 8 lines · wwguy
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c static void iwlagn_check_needed_chains(s · line 492
Quoted 6 lines
drivers/net/wireless/iwlwifi/iwl-agn-rxon.c +2 -0
@@ -503,7 +488,46 @@ static void iwlagn_check_needed_chains(s
                          * will soon tell us about that.
                          */
                         need_multiple = false;
+                       rcu_read_unlock();
+                       break;
why unlock and break here instead stay do it later like before?
Quoted 5 lines
Quoted 2 lines
Only because the indentation there was getting so deep it was annoying to fit into 80 cols :-)
how you want to submit this patch? through iwlagn, or yo uwill send it along with mac80211 to John?
Don't really care. I just sent them both here to keep them together and for Daniel and Swati ... First though, I'd like them to test it maybe.
johannes

Johannes Berg

author maintainer

2011-02-26 18:50 UTC #8 lore raw

Re: [PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

Quoted 2 lines · Daniel Halperin

Acked-by Daniel Halperin <[email protected]>
Tested-by Daniel Halperin <[email protected]>

Thanks for testing! John has already applied the patch so I guess this won't be in the logs though.

johannes