[PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

When the driver has been notified with a STA_REMOVE, it tears down
the internal ADDBA state. On resume, trying to initiate aggregation would
fail because mac80211 has not cleared the operational state for that <TID,STA>.
This can be fixed by tearing down the existing sessions on a suspend.

Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
---
 net/mac80211/pm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
index f845601..ab9c989 100644
--- a/net/mac80211/pm.c
+++ b/net/mac80211/pm.c
@@ -21,6 +21,13 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
 	list_for_each_entry(sdata, &local->interfaces, list)
 		ieee80211_disable_keys(sdata);
 
+	/* Tear down aggregation sessions */
+	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
+		list_for_each_entry(sta, &local->sta_list, list) {
+			ieee80211_sta_tear_down_BA_sessions(sta);
+		}
+	}
+
 	/* remove STAs */
 	if (local->ops->sta_notify) {
 		spin_lock_irqsave(&local->sta_lock, flags);
-- 
1.6.2

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Johannes Berg]

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

On Sun, 2009-03-15 at 12:13 +0530, Sujith wrote:
> When the driver has been notified with a STA_REMOVE, it tears down
> the internal ADDBA state. On resume, trying to initiate aggregation would
> fail because mac80211 has not cleared the operational state for that <TID,STA>.
> This can be fixed by tearing down the existing sessions on a suspend.

Agreed, but Bob said there were some deadlocks with this?

johannes

> Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
> ---
>  net/mac80211/pm.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/pm.c b/net/mac80211/pm.c
> index f845601..ab9c989 100644
> --- a/net/mac80211/pm.c
> +++ b/net/mac80211/pm.c
> @@ -21,6 +21,13 @@ int __ieee80211_suspend(struct ieee80211_hw *hw)
>  	list_for_each_entry(sdata, &local->interfaces, list)
>  		ieee80211_disable_keys(sdata);
>  
> +	/* Tear down aggregation sessions */
> +	if (hw->flags & IEEE80211_HW_AMPDU_AGGREGATION) {
> +		list_for_each_entry(sta, &local->sta_list, list) {
> +			ieee80211_sta_tear_down_BA_sessions(sta);
> +		}
> +	}
> +
>  	/* remove STAs */
>  	if (local->ops->sta_notify) {
>  		spin_lock_irqsave(&local->sta_lock, flags);

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

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Johannes Berg wrote:
> On Sun, 2009-03-15 at 12:13 +0530, Sujith wrote:
> > When the driver has been notified with a STA_REMOVE, it tears down
> > the internal ADDBA state. On resume, trying to initiate aggregation would
> > fail because mac80211 has not cleared the operational state for that <TID,STA>.
> > This can be fixed by tearing down the existing sessions on a suspend.
> 
> Agreed, but Bob said there were some deadlocks with this?
> 

It works with ath9k, I don't see any deadlocks.
And I have been testing with the 'reset' debugfs file.
Too lazy to do a full suspend/resume cycle. :)
It shouldn't matter though, right ?

Sujith

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Bob Copeland]

On Sun, Mar 15, 2009 at 2:02 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Sun, 2009-03-15 at 12:13 +0530, Sujith wrote:
>> When the driver has been notified with a STA_REMOVE, it tears down
>> the internal ADDBA state. On resume, trying to initiate aggregation would
>> fail because mac80211 has not cleared the operational state for that <TID,STA>.
>> This can be fixed by tearing down the existing sessions on a suspend.
>
> Agreed, but Bob said there were some deadlocks with this?

I think the deadlock went away, not sure what fixed it but I guess
the HT rework.  I tested it a couple of weeks ago on ath5k and Luis
did on ath9k so I say let's go ahead and apply the patch... in the
meantime I'll try again on ath5k just to be sure.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Bob Copeland wrote:
> On Sun, Mar 15, 2009 at 2:02 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Sun, 2009-03-15 at 12:13 +0530, Sujith wrote:
> >> When the driver has been notified with a STA_REMOVE, it tears down
> >> the internal ADDBA state. On resume, trying to initiate aggregation would
> >> fail because mac80211 has not cleared the operational state for that <TID,STA>.
> >> This can be fixed by tearing down the existing sessions on a suspend.
> >
> > Agreed, but Bob said there were some deadlocks with this?
> 
> I think the deadlock went away, not sure what fixed it but I guess
> the HT rework.  I tested it a couple of weeks ago on ath5k and Luis
> did on ath9k so I say let's go ahead and apply the patch... in the
> meantime I'll try again on ath5k just to be sure.

There is a window for a race. Something like this:

>From mac80211:

__ieee80211_suspend()
     tear_down_BA_sessions(TX, RX)
          ampdu_action(STOP)
               remove_vifs()

At this point, the driver executes its remove_interface routine.
While we are doing this, a TX completion interrupt could be raised,
(HW hasn't been stopped yet) and nothing stops the driver from calling
ieee80211_start_tx_ba_session().

So the question is: should mac80211 deny ADDBA requests in this case ?

Sujith

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Johannes Berg]

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

On Mon, 2009-03-16 at 10:23 +0530, Sujith wrote:

> > I think the deadlock went away, not sure what fixed it but I guess
> > the HT rework.  I tested it a couple of weeks ago on ath5k and Luis
> > did on ath9k so I say let's go ahead and apply the patch... in the
> > meantime I'll try again on ath5k just to be sure.

Ok.

> There is a window for a race. Something like this:
> 
> From mac80211:
> 
> __ieee80211_suspend()
>      tear_down_BA_sessions(TX, RX)
>           ampdu_action(STOP)
>                remove_vifs()
> 
> At this point, the driver executes its remove_interface routine.
> While we are doing this, a TX completion interrupt could be raised,
> (HW hasn't been stopped yet) and nothing stops the driver from calling
> ieee80211_start_tx_ba_session().
> 
> So the question is: should mac80211 deny ADDBA requests in this case ?

Interesting observation. We probably should indeed reject that, and also
if the peer asks for sending aggregation again right away like some
Broadcom APs will.

johannes

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

Re: [PATCH] mac80211: Tear down aggregation sessions for suspend/resume

[Sujith]

Johannes Berg wrote:
> > There is a window for a race. Something like this:
> > 
> > From mac80211:
> > 
> > __ieee80211_suspend()
> >      tear_down_BA_sessions(TX, RX)
> >           ampdu_action(STOP)
> >                remove_vifs()
> > 
> > At this point, the driver executes its remove_interface routine.
> > While we are doing this, a TX completion interrupt could be raised,
> > (HW hasn't been stopped yet) and nothing stops the driver from calling
> > ieee80211_start_tx_ba_session().
> > 
> > So the question is: should mac80211 deny ADDBA requests in this case ?
> 
> Interesting observation. We probably should indeed reject that, and also
> if the peer asks for sending aggregation again right away like some
> Broadcom APs will.

Yep, both TX and RX aggregation requests have to be denied.
And this patch would be incomplete without handling this case.
Will send a v2.

Sujith