[PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Luis R. Rodriguez]

It only makes sense to allow BA agreements when we are going to
spend some time on the operating channel. This blocks all new
incoming ADDBA requests while we go offchannel, but keeps
existing BA agreements alive.

Cc: srinivasa.duvvuri@atheros.com
Cc: matt.smith@atheros.com
Cc: bennyam.malavazi@atheros.com
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/agg-rx.c     |    8 ++++++--
 net/mac80211/offchannel.c |   16 ++++++++++++++++
 net/mac80211/sta_info.h   |    3 ++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 720b7a8..a21d120 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -209,8 +209,12 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 
 	if (test_sta_flags(sta, WLAN_STA_BLOCK_BA)) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
-		printk(KERN_DEBUG "Suspend in progress. "
-		       "Denying ADDBA request\n");
+		if (local->quiescing)
+			printk(KERN_DEBUG "Suspend in progress. "
+			      "Denying ADDBA request\n");
+		else
+			printk(KERN_DEBUG "Offchannel operation in progress, "
+			      "Denying ADDBA request\n");
 #endif
 		goto end_no_lock;
 	}
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 4b56409..f3a7d2a 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -34,6 +34,14 @@ static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
 
 	cancel_work_sync(&local->dynamic_ps_enable_work);
 
+	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		struct sta_info *sta;
+		mutex_lock(&local->sta_mtx);
+		list_for_each_entry(sta, &local->sta_list, list)
+			set_sta_flags(sta, WLAN_STA_BLOCK_BA);
+		mutex_unlock(&local->sta_mtx);
+	}
+
 	if (local->hw.conf.flags & IEEE80211_CONF_PS) {
 		local->offchannel_ps_enabled = true;
 		local->hw.conf.flags &= ~IEEE80211_CONF_PS;
@@ -92,6 +100,14 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_sta_reset_beacon_monitor(sdata);
 	ieee80211_sta_reset_conn_monitor(sdata);
+
+	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		struct sta_info *sta;
+		mutex_lock(&local->sta_mtx);
+		list_for_each_entry(sta, &local->sta_list, list)
+			clear_sta_flags(sta, WLAN_STA_BLOCK_BA);
+		mutex_unlock(&local->sta_mtx);
+	}
 }
 
 void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index cf21a2e..2439156 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -36,7 +36,8 @@
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
  * @WLAN_STA_BLOCK_BA: Used to deny ADDBA requests (both TX and RX)
- *	during suspend/resume and station removal.
+ *	during suspend/resume, station removal, and when we go offchannel
+ *	when associated.
  * @WLAN_STA_PS_DRIVER: driver requires keeping this station in
  *	power-save mode logically to flush frames that might still
  *	be in the queues
-- 
1.7.0.4

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Johannes Berg]

On Wed, 2010-10-06 at 11:50 -0700, Luis R. Rodriguez wrote:
> It only makes sense to allow BA agreements when we are going to
> spend some time on the operating channel. This blocks all new
> incoming ADDBA requests while we go offchannel, but keeps
> existing BA agreements alive.

I just hope the different uses of this bit won't ever collide ... can we
suspend while being offchannel? ;-)

johannes

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Luis R. Rodriguez]

On Wed, Oct 06, 2010 at 11:55:01AM -0700, Johannes Berg wrote:
> On Wed, 2010-10-06 at 11:50 -0700, Luis R. Rodriguez wrote:
> > It only makes sense to allow BA agreements when we are going to
> > spend some time on the operating channel. This blocks all new
> > incoming ADDBA requests while we go offchannel, but keeps
> > existing BA agreements alive.
> 
> I just hope the different uses of this bit won't ever collide ... can we
> suspend while being offchannel? ;-)

Good question, will ieee80211_offchannel_ps_disable() possibly be
called during suspend? I can imagine its possible. So how about we
check for local->quiescing prior to lifting the flag only:

>From 374cbccafadc22ba261dd3bc5f68758106872448 Mon Sep 17 00:00:00 2001
From: Luis R. Rodriguez <lrodriguez@atheros.com>
Date: Tue, 21 Sep 2010 15:27:02 -0400
Subject: [PATCH v3] mac80211: Deny new BA agreements from being started during offchannel operation

It only makes sense to allow BA agreements when we are going to
spend some time on the operating channel. This blocks all new
incoming ADDBA requests while we go offchannel, but keeps
existing BA agreements alive.

Cc: srinivasa.duvvuri@atheros.com
Cc: matt.smith@atheros.com
Cc: bennyam.malavazi@atheros.com
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/mac80211/agg-rx.c     |    8 ++++++--
 net/mac80211/offchannel.c |   17 +++++++++++++++++
 net/mac80211/sta_info.h   |    3 ++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 720b7a8..a21d120 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -209,8 +209,12 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
 
 	if (test_sta_flags(sta, WLAN_STA_BLOCK_BA)) {
 #ifdef CONFIG_MAC80211_HT_DEBUG
-		printk(KERN_DEBUG "Suspend in progress. "
-		       "Denying ADDBA request\n");
+		if (local->quiescing)
+			printk(KERN_DEBUG "Suspend in progress. "
+			      "Denying ADDBA request\n");
+		else
+			printk(KERN_DEBUG "Offchannel operation in progress, "
+			      "Denying ADDBA request\n");
 #endif
 		goto end_no_lock;
 	}
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 4b56409..86e9b8d 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -34,6 +34,14 @@ static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
 
 	cancel_work_sync(&local->dynamic_ps_enable_work);
 
+	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		struct sta_info *sta;
+		mutex_lock(&local->sta_mtx);
+		list_for_each_entry(sta, &local->sta_list, list)
+			set_sta_flags(sta, WLAN_STA_BLOCK_BA);
+		mutex_unlock(&local->sta_mtx);
+	}
+
 	if (local->hw.conf.flags & IEEE80211_CONF_PS) {
 		local->offchannel_ps_enabled = true;
 		local->hw.conf.flags &= ~IEEE80211_CONF_PS;
@@ -92,6 +100,15 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_sta_reset_beacon_monitor(sdata);
 	ieee80211_sta_reset_conn_monitor(sdata);
+
+	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION &&
+	    !local->quiescing) {
+		struct sta_info *sta;
+		mutex_lock(&local->sta_mtx);
+		list_for_each_entry(sta, &local->sta_list, list)
+			clear_sta_flags(sta, WLAN_STA_BLOCK_BA);
+		mutex_unlock(&local->sta_mtx);
+	}
 }
 
 void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index cf21a2e..2439156 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -36,7 +36,8 @@
  *	frame to this station is transmitted.
  * @WLAN_STA_MFP: Management frame protection is used with this STA.
  * @WLAN_STA_BLOCK_BA: Used to deny ADDBA requests (both TX and RX)
- *	during suspend/resume and station removal.
+ *	during suspend/resume, station removal, and when we go offchannel
+ *	when associated.
  * @WLAN_STA_PS_DRIVER: driver requires keeping this station in
  *	power-save mode logically to flush frames that might still
  *	be in the queues
-- 
1.7.0.4

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Johannes Berg]

On Wed, 2010-10-06 at 12:12 -0700, Luis R. Rodriguez wrote:

> Good question, will ieee80211_offchannel_ps_disable() possibly be
> called during suspend? I can imagine its possible. So how about we
> check for local->quiescing prior to lifting the flag only:

Or suspended? I'm not really sure .. this will probably interact badly
anyway. Hmm.

johannes

> From 374cbccafadc22ba261dd3bc5f68758106872448 Mon Sep 17 00:00:00 2001
> From: Luis R. Rodriguez <lrodriguez@atheros.com>
> Date: Tue, 21 Sep 2010 15:27:02 -0400
> Subject: [PATCH v3] mac80211: Deny new BA agreements from being started during offchannel operation
> 
> It only makes sense to allow BA agreements when we are going to
> spend some time on the operating channel. This blocks all new
> incoming ADDBA requests while we go offchannel, but keeps
> existing BA agreements alive.
> 
> Cc: srinivasa.duvvuri@atheros.com
> Cc: matt.smith@atheros.com
> Cc: bennyam.malavazi@atheros.com
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  net/mac80211/agg-rx.c     |    8 ++++++--
>  net/mac80211/offchannel.c |   17 +++++++++++++++++
>  net/mac80211/sta_info.h   |    3 ++-
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index 720b7a8..a21d120 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -209,8 +209,12 @@ void ieee80211_process_addba_request(struct ieee80211_local *local,
>  
>  	if (test_sta_flags(sta, WLAN_STA_BLOCK_BA)) {
>  #ifdef CONFIG_MAC80211_HT_DEBUG
> -		printk(KERN_DEBUG "Suspend in progress. "
> -		       "Denying ADDBA request\n");
> +		if (local->quiescing)
> +			printk(KERN_DEBUG "Suspend in progress. "
> +			      "Denying ADDBA request\n");
> +		else
> +			printk(KERN_DEBUG "Offchannel operation in progress, "
> +			      "Denying ADDBA request\n");
>  #endif
>  		goto end_no_lock;
>  	}
> diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
> index 4b56409..86e9b8d 100644
> --- a/net/mac80211/offchannel.c
> +++ b/net/mac80211/offchannel.c
> @@ -34,6 +34,14 @@ static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
>  
>  	cancel_work_sync(&local->dynamic_ps_enable_work);
>  
> +	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> +		struct sta_info *sta;
> +		mutex_lock(&local->sta_mtx);
> +		list_for_each_entry(sta, &local->sta_list, list)
> +			set_sta_flags(sta, WLAN_STA_BLOCK_BA);
> +		mutex_unlock(&local->sta_mtx);
> +	}
> +
>  	if (local->hw.conf.flags & IEEE80211_CONF_PS) {
>  		local->offchannel_ps_enabled = true;
>  		local->hw.conf.flags &= ~IEEE80211_CONF_PS;
> @@ -92,6 +100,15 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
>  
>  	ieee80211_sta_reset_beacon_monitor(sdata);
>  	ieee80211_sta_reset_conn_monitor(sdata);
> +
> +	if (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION &&
> +	    !local->quiescing) {
> +		struct sta_info *sta;
> +		mutex_lock(&local->sta_mtx);
> +		list_for_each_entry(sta, &local->sta_list, list)
> +			clear_sta_flags(sta, WLAN_STA_BLOCK_BA);
> +		mutex_unlock(&local->sta_mtx);
> +	}
>  }
>  
>  void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index cf21a2e..2439156 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -36,7 +36,8 @@
>   *	frame to this station is transmitted.
>   * @WLAN_STA_MFP: Management frame protection is used with this STA.
>   * @WLAN_STA_BLOCK_BA: Used to deny ADDBA requests (both TX and RX)
> - *	during suspend/resume and station removal.
> + *	during suspend/resume, station removal, and when we go offchannel
> + *	when associated.
>   * @WLAN_STA_PS_DRIVER: driver requires keeping this station in
>   *	power-save mode logically to flush frames that might still
>   *	be in the queues

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Luis R. Rodriguez]

On Wed, Oct 6, 2010 at 12:19 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2010-10-06 at 12:12 -0700, Luis R. Rodriguez wrote:
>
>> Good question, will ieee80211_offchannel_ps_disable() possibly be
>> called during suspend? I can imagine its possible. So how about we
>> check for local->quiescing prior to lifting the flag only:
>
> Or suspended?

If we're suspended we would be quiesced so I do not see us calling
ieee80211_offchannel_ps_disable() then, but we can also just bail out
if quescing or suspended on  ieee80211_offchannel_ps_disable() and
friends.

> I'm not really sure .. this will probably interact badly
> anyway. Hmm.

Which part?

  Luis

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Johannes Berg]

On Wed, 2010-10-06 at 13:48 -0700, Luis R. Rodriguez wrote:
> On Wed, Oct 6, 2010 at 12:19 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2010-10-06 at 12:12 -0700, Luis R. Rodriguez wrote:
> >
> >> Good question, will ieee80211_offchannel_ps_disable() possibly be
> >> called during suspend? I can imagine its possible. So how about we
> >> check for local->quiescing prior to lifting the flag only:
> >
> > Or suspended?
> 
> If we're suspended we would be quiesced so I do not see us calling
> ieee80211_offchannel_ps_disable() then, but we can also just bail out
> if quescing or suspended on  ieee80211_offchannel_ps_disable() and
> friends.

Oh, wait, yeah, while quiescing we stop the workqueue so we can't be
processing this, so we should be fine. We might still be off-channel
while suspending which is odd, and then we might lift the BA block
earlier than you'd want.

But then again, come to think of it, why are you doing this anyway? I'm
not convinced of the whole idea, it's all racy. I'm starting to think
that this patch won't help anything since we'll still be racy and start
BA agreements just before we block, _and_ if we're a station and going
off-channel we'll be going into powersave which means we won't actually
get the addBA frame until we return.

johannes

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Luis R. Rodriguez]

On Thu, Oct 7, 2010 at 12:18 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2010-10-06 at 13:48 -0700, Luis R. Rodriguez wrote:
>> On Wed, Oct 6, 2010 at 12:19 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Wed, 2010-10-06 at 12:12 -0700, Luis R. Rodriguez wrote:
>> >
>> >> Good question, will ieee80211_offchannel_ps_disable() possibly be
>> >> called during suspend? I can imagine its possible. So how about we
>> >> check for local->quiescing prior to lifting the flag only:
>> >
>> > Or suspended?
>>
>> If we're suspended we would be quiesced so I do not see us calling
>> ieee80211_offchannel_ps_disable() then, but we can also just bail out
>> if quescing or suspended on  ieee80211_offchannel_ps_disable() and
>> friends.
>
> Oh, wait, yeah, while quiescing we stop the workqueue so we can't be
> processing this, so we should be fine.

Right.

> We might still be off-channel
> while suspending which is odd, and then we might lift the BA block
> earlier than you'd want.

Hehe.. unless we deny going off channel when quiescing or suspended.

> But then again, come to think of it, why are you doing this anyway?

The goal is to clean up a lot of stuff we forgot to cleanup for
offchannel operation and in the end to help with multicast/broadcast
data. This patch just addresses one of the cleanups. The timers kicked
off when going off channel will not make sense, so best to just deny
ADDBA when going offchannel.

> I'm not convinced of the whole idea, it's all racy. I'm starting to think
> that this patch won't help anything since we'll still be racy and start
> BA agreements just before we block,

Establishing the BA agreement is fine, we don't want to cancel
existing BA agreements when going offchannel, we just want to prevent
making the timers for new ADDBA requests from going stale
unnecessarily.

> _and_ if we're a station and going off-channel we'll be going into powersave
> which means we won't actually get the addBA frame until we return.

There's a race between trying to go offchannel, sending the nullfunc
and switching channels. When the race hits the ADDBA timer will just
go stale. If our goal is to go offchannel we should avoid the race by
simply denying new ADDBA requests. I just noticed that we can't
possible receive ADDBA requests when we are scanning though, since
ieee80211_iface_work() already has a check and bail out for
local->scanning, but note that local->scanning will not be checked
when doing other offchannel work.

So perhaps there is another way we can deal with this for the
non-scanning offchannel work case that might also take care of not
handling other queued up management frames when offchannel.

 Luis

Re: [PATCH] mac80211: Deny new BA agreements from being started during offchannel operation

[Johannes Berg]

On Thu, 2010-10-07 at 10:21 -0700, Luis R. Rodriguez wrote:

> > But then again, come to think of it, why are you doing this anyway?
> 
> The goal is to clean up a lot of stuff we forgot to cleanup for
> offchannel operation and in the end to help with multicast/broadcast
> data. This patch just addresses one of the cleanups. The timers kicked
> off when going off channel will not make sense, so best to just deny
> ADDBA when going offchannel.

But this won't actually help in that case -- this problem happens if try
to make a BA just before we go offchannel, and then the timer fires
while we're offchannel. It'll just *very* slightly alter the timing
required to hit it.

> > I'm not convinced of the whole idea, it's all racy. I'm starting to think
> > that this patch won't help anything since we'll still be racy and start
> > BA agreements just before we block,
> 
> Establishing the BA agreement is fine, we don't want to cancel
> existing BA agreements when going offchannel, we just want to prevent
> making the timers for new ADDBA requests from going stale
> unnecessarily.

See above though.

> > _and_ if we're a station and going off-channel we'll be going into powersave
> > which means we won't actually get the addBA frame until we return.
> 
> There's a race between trying to go offchannel, sending the nullfunc
> and switching channels. When the race hits the ADDBA timer will just
> go stale. If our goal is to go offchannel we should avoid the race by
> simply denying new ADDBA requests. I just noticed that we can't
> possible receive ADDBA requests when we are scanning though, since
> ieee80211_iface_work() already has a check and bail out for
> local->scanning, but note that local->scanning will not be checked
> when doing other offchannel work.
> 
> So perhaps there is another way we can deal with this for the
> non-scanning offchannel work case that might also take care of not
> handling other queued up management frames when offchannel.

I think we need to treat TX and RX BA differently.

For TX BA, the best idea would probably be to actually make it _use_ the
work infrastructure -- after we change that to not actually stop all the
things it's doing when the channel is the operating channel, or so.

johannes