hung task in mac80211

Commit Message

Stefano Brivio Sept. 6, 2017, 2:27 p.m. UTC

On Wed, 06 Sep 2017 14:58:48 +0200
Johannes Berg <[e-mail]> wrote:

> +void __ieee80211_start_rx_ba_session(struct sta_info *sta,
> +				     u8 dialog_token, u16 timeout,
> +				     u16 start_seq_num, u16 ba_policy, u16 tid,
> +				     u16 buf_size, bool tx, bool auto_seq)
> +{
> +	mutex_lock(&sta->ampdu_mlme.mtx);
> +	___ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
> +					 start_seq_num, ba_policy, tid,
> +					 buf_size, tx, auto_seq);
> +	mutex_unlock(&sta->ampdu_mlme.mtx);
> +}
> +

Sorry for the extended bothering :) but here, you're extending quite a
bit the scope of the lock also when__ieee80211_start_rx_ba_session() is
called by ieee80211_process_addba_request(). No idea what the hit can
be, but we can't safely assume it's nothing either. What about simply
introducing a 'ampdu_mlme_lock_held' argument instead? Something like:

Comments

Johannes Berg Sept. 6, 2017, 2:30 p.m. UTC | #1

On Wed, 2017-09-06 at 16:27 +0200, Stefano Brivio wrote:
> 
> Sorry for the extended bothering :) but here, you're extending quite
> a bit the scope of the lock also
> when__ieee80211_start_rx_ba_session() is called by
> ieee80211_process_addba_request().

I know, but it doesn't matter.

> No idea what the hit can be, but we can't safely assume it's

> nothing either.   

We don't really have to assume anything, we can read the code :-)

Trust me, I probably wrote most of it. It's fine, just sanity checks.

> What about simply introducing a 'ampdu_mlme_lock_held' argument
> instead?

Eww, no.

johannes

Patch

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 2b36eff5d97e..35a9eff1ec66 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -248,7 +248,8 @@  static void ieee80211_send_addba_resp(struct ieee80211_sub_if_data *sdata, u8 *d
 void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 				     u8 dialog_token, u16 timeout,
 				     u16 start_seq_num, u16 ba_policy, u16 tid,
-				     u16 buf_size, bool tx, bool auto_seq)
+				     u16 buf_size, bool tx, bool auto_seq,
+				     bool ampdu_mlme_lock_held)
 {
 	struct ieee80211_local *local = sta->sdata->local;
 	struct tid_ampdu_rx *tid_agg_rx;
@@ -311,7 +312,8 @@  void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 	       buf_size, sta->sta.addr);
 
 	/* examine state machine */
-	mutex_lock(&sta->ampdu_mlme.mtx);
+	if (!ampdu_mlme_lock_held)
+		mutex_lock(&sta->ampdu_mlme.mtx);
 
 	if (test_bit(tid, sta->ampdu_mlme.agg_session_valid)) {
 		if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
@@ -415,7 +417,8 @@  void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 		__clear_bit(tid, sta->ampdu_mlme.unexpected_agg);
 		sta->ampdu_mlme.tid_rx_token[tid] = dialog_token;
 	}
-	mutex_unlock(&sta->ampdu_mlme.mtx);
+	if (!ampdu_mlme_lock_held)
+		mutex_unlock(&sta->ampdu_mlme.mtx);
 
 end_no_lock:
 	if (tx)
@@ -445,7 +448,7 @@  void ieee80211_process_addba_request(struct ieee80211_local *local,
 
 	__ieee80211_start_rx_ba_session(sta, dialog_token, timeout,
 					start_seq_num, ba_policy, tid,
-					buf_size, true, false);
+					buf_size, true, false, false);
 }
 
 void ieee80211_manage_rx_ba_offl(struct ieee80211_vif *vif,
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c
index c92df492e898..59ba67e8942f 100644
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -335,7 +335,7 @@  void ieee80211_ba_session_work(struct work_struct *work)
 				       sta->ampdu_mlme.tid_rx_manage_offl))
 			__ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid,
 							IEEE80211_MAX_AMPDU_BUF,
-							false, true);
+							false, true, true);
 
 		if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
 				       sta->ampdu_mlme.tid_rx_manage_offl))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2197c62a0a6e..5d494ac65853 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1759,7 +1759,8 @@  void __ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid,
 void __ieee80211_start_rx_ba_session(struct sta_info *sta,
 				     u8 dialog_token, u16 timeout,
 				     u16 start_seq_num, u16 ba_policy, u16 tid,
-				     u16 buf_size, bool tx, bool auto_seq);
+				     u16 buf_size, bool tx, bool auto_seq,
+				     bool ampdu_mlme_lock_held);
 void ieee80211_sta_tear_down_BA_sessions(struct sta_info *sta,
 					 enum ieee80211_agg_stop_reason reason);
 void ieee80211_process_delba(struct ieee80211_sub_if_data *sdata,