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 <[email protected]>
---
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
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
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
Bob Copeland wrote:
> On Sun, Mar 15, 2009 at 2:02 PM, Johannes Berg
> <[email protected]> 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
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
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 <[email protected]>
> ---
> 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);
On Sun, Mar 15, 2009 at 2:02 PM, Johannes Berg
<[email protected]> 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 %% http://www.bobcopeland.com