[PATCH 0/2] fix iwlwifi needed RX chains

[PATCH 0/2] fix iwlwifi needed RX chains

[Johannes Berg]

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

[PATCH 1/2] mac80211: copy peer MCS TX parameters

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

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

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/ht.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- wireless-testing.orig/net/mac80211/ht.c	2011-02-25 11:07:00.000000000 +0100
+++ wireless-testing/net/mac80211/ht.c	2011-02-25 12:20:22.000000000 +0100
@@ -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]

[PATCH 2/2] iwlagn: fix iwlagn_check_needed_chains

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

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 <johannes.berg@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl-agn-rxon.c |   58 +++++++++++++++++++---------
 1 file changed, 41 insertions(+), 17 deletions(-)

--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c	2011-02-25 11:03:37.000000000 +0100
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c	2011-02-25 12:21:18.000000000 +0100
@@ -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:

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

[Guy, Wey-Yi]

On Fri, 2011-02-25 at 07:39 -0800, Johannes Berg wrote:
> On Fri, 2011-02-25 at 07:36 -0800, wwguy wrote:
> 
> > > @@ -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 :-)

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

Wey 
> 

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

[wwguy]

Hi Johannes,

On Fri, 2011-02-25 at 03:24 -0800, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> 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 <johannes.berg@intel.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-agn-rxon.c |   58 +++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 17 deletions(-)
> 
> --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c	2011-02-25 11:03:37.000000000 +0100
> +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-agn-rxon.c	2011-02-25 12:21:18.000000000 +0100
> @@ -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;

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

Wey

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

[Johannes Berg]

On Fri, 2011-02-25 at 07:36 -0800, wwguy wrote:

> > @@ -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

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

[Johannes Berg]

On Fri, 2011-02-25 at 07:31 -0800, Guy, Wey-Yi wrote:
> On Fri, 2011-02-25 at 07:39 -0800, Johannes Berg wrote:
> > On Fri, 2011-02-25 at 07:36 -0800, wwguy wrote:
> > 
> > > > @@ -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 :-)
> 
> 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

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

[Daniel Halperin]

Acked-by: Daniel Halperin <dhalperi@cs.washington.edu>
Tested-by: Daniel Halperin <dhalperi@cs.washington.edu>

(I suppose those are redundant?)

Dan

On Fri, Feb 25, 2011 at 7:49 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> On Fri, 2011-02-25 at 07:31 -0800, Guy, Wey-Yi wrote:
> > On Fri, 2011-02-25 at 07:39 -0800, Johannes Berg wrote:
> > > On Fri, 2011-02-25 at 07:36 -0800, wwguy wrote:
> > >
> > > > > @@ -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 :-)
> >
> > 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
>

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

[Johannes Berg]

On Sat, 2011-02-26 at 09:59 -0800, Daniel Halperin wrote:
> Acked-by: Daniel Halperin <dhalperi@cs.washington.edu>
> Tested-by: Daniel Halperin <dhalperi@cs.washington.edu>

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

johannes