[PATCH] mac80211: Complete ampdu work schedule during session tear down

[PATCH] mac80211: Complete ampdu work schedule during session tear down

[Luca Coelho]

From: Ilan Peer <ilan.peer@intel.com>

Commit 7a7c0a6438b8 ("mac80211: fix TX aggregation start/stop callback race")
added a cancellation of the ampdu work after the loop that stopped the
Tx and Rx BA sessions. However, in some cases, e.g., during HW reconfig,
the low level driver might call mac80211 APIs to complete the stopping
of the BA sessions, which would queue the ampdu work to handle the actual
completion. This work needs to be performed as otherwise mac80211 data
structures would not be properly synced.

Fix this by checking if BA session STOP_CB bit is set after the BA session
cancellation and properly clean the session.

Fixes: 7a7c0a6438b8 ("mac80211: fix TX aggregation start/stop callback race")
Signed-off-by: Ilan Peer <ilan.peer@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
 net/mac80211/ht.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..105e2802a48a 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -300,6 +300,21 @@ void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
 
 	/* stopping might queue the work again - so cancel only afterwards */
 	cancel_work_sync(&sta->ampdu_mlme.work);
+
+	/* In case the tear down is part of a reconfigure due to HW restart
+	 * request, it is possible that the low level driver requested to stop
+	 * the BA session, so handle it to properly clean tid_tx data.
+	 */
+	for (i = 0; i < IEEE80211_NUM_TIDS; i++) {
+		struct tid_ampdu_tx *tid_tx =
+			rcu_dereference_protected_tid_tx(sta, i);
+
+		if (!tid_tx)
+			continue;
+
+		if (test_and_clear_bit(HT_AGG_STATE_STOP_CB, &tid_tx->state))
+			ieee80211_stop_tx_ba_cb(sta, i, tid_tx);
+	}
 }
 
 void ieee80211_ba_session_work(struct work_struct *work)
-- 
2.14.1

Re: [PATCH] mac80211: Complete ampdu work schedule during session tear down

[Johannes Berg]

I was going to apply this, but running with lockdep enabled tells me
that this patch is broken.

In the callers of ieee80211_sta_tear_down_BA_sessions(), we only hold
the &local->sta_mtx.

However,

> +			rcu_dereference_protected_tid_tx(sta, i);

requires (and checks, if you have lockdep) that you hold either
	&sta->ampdu_mlme.mtx
or
	&sta->lock.

Additionally, ieee80211_remove_tid_tx(), called via
ieee80211_stop_tx_ba_cb(), requires holding both - and
ieee80211_stop_tx_ba_cb() only requires the spinlock.

johannes