[PATCH] mac80211: delete addba_resp_timer when stop Tx BA session

[PATCH] mac80211: delete addba_resp_timer when stop Tx BA session

[Zhu Yi]

We start an one shot addba_resp_timer in ieee80211_start_tx_ba_session
but don't delete it in __ieee80211_stop_tx_ba_session. This will cause
problem in suspend and resume. When suspend happens without the timer
deleted, the timer handler will be called immediately after resume and
messes up driver status.

See http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2134
comment #22 for more details.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 net/mac80211/agg-tx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index ceda366..d122082 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -509,6 +509,8 @@ int __ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid,
 	u8 *state;
 	int ret;
 
+	del_timer_sync(&sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer);
+
 	/* check if the TID is in aggregation */
 	state = &sta->ampdu_mlme.tid_state_tx[tid];
 	spin_lock_bh(&sta->lock);
-- 
1.6.3.3

Re: [PATCH] mac80211: delete addba_resp_timer when stop Tx BA session

[Johannes Berg]

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

On Mon, 2009-12-28 at 11:18 +0800, Zhu Yi wrote:
> We start an one shot addba_resp_timer in ieee80211_start_tx_ba_session
> but don't delete it in __ieee80211_stop_tx_ba_session. This will cause
> problem in suspend and resume. When suspend happens without the timer
> deleted, the timer handler will be called immediately after resume and
> messes up driver status.
> 
> See http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2134
> comment #22 for more details.

Hmm. The timer checks

        if ((*state & (HT_ADDBA_REQUESTED_MSK | HT_ADDBA_RECEIVED_MSK)) !=
                                                HT_ADDBA_REQUESTED_MSK) {

which /should/ make it a no-op in the case you mention because we tear
down sessions, or does that not reset their state properly?

Ok, so ___ieee80211_stop_tx_ba_session() doesn't, in fact, clear the
mask right away, so I think we should add HT_AGG_STATE_REQ_STOP_BA_MSK
to the code I quoted above?

In any case the del_timer_sync isn't right there and we also shouldn't
really use it (per Linus)

johannes

> Cc: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Zhu Yi <yi.zhu@intel.com>
> ---
>  net/mac80211/agg-tx.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
> index ceda366..d122082 100644
> --- a/net/mac80211/agg-tx.c
> +++ b/net/mac80211/agg-tx.c
> @@ -509,6 +509,8 @@ int __ieee80211_stop_tx_ba_session(struct sta_info
> *sta, u16 tid,
>  	u8 *state;
>  	int ret;
>  
> +	del_timer_sync(&sta->ampdu_mlme.tid_tx[tid]->addba_resp_timer);
> +
>  	/* check if the TID is in aggregation */
>  	state = &sta->ampdu_mlme.tid_state_tx[tid];
>  	spin_lock_bh(&sta->lock);


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

Re: [PATCH] mac80211: delete addba_resp_timer when stop Tx BA session

[Luis R. Rodriguez]

On Mon, Dec 28, 2009 at 12:18 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-12-28 at 11:18 +0800, Zhu Yi wrote:
>> We start an one shot addba_resp_timer in ieee80211_start_tx_ba_session
>> but don't delete it in __ieee80211_stop_tx_ba_session. This will cause
>> problem in suspend and resume. When suspend happens without the timer
>> deleted, the timer handler will be called immediately after resume and
>> messes up driver status.
>>
>> See http://bugzilla.intellinuxwireless.org/show_bug.cgi?id=2134
>> comment #22 for more details.
>
> Hmm. The timer checks
>
>        if ((*state & (HT_ADDBA_REQUESTED_MSK | HT_ADDBA_RECEIVED_MSK)) !=
>                                                HT_ADDBA_REQUESTED_MSK) {
>
> which /should/ make it a no-op in the case you mention because we tear
> down sessions, or does that not reset their state properly?
>
> Ok, so ___ieee80211_stop_tx_ba_session() doesn't, in fact, clear the
> mask right away, so I think we should add HT_AGG_STATE_REQ_STOP_BA_MSK
> to the code I quoted above?

When the patch addresses the above can you cc stable if applicable.

> In any case the del_timer_sync isn't right there and we also shouldn't
> really use it (per Linus)

Why is that?

  Luis