| Message ID | 20170906151922.4a320b1d@elisabeth (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote: > On Wed, 06 Sep 2017 14:48:35 +0200 > Johannes Berg <johannes@sipsolutions.net> wrote: > > > I'll look in a bit - but > > > > > + mutex_unlock(&sta->ampdu_mlme.mtx); > > > ___ieee80211_stop_rx_ba_session( > > > sta, tid, WLAN_BACK_RECIPIENT, > > > WLAN_REASON_QSTA_TIMEOUT, > > > true); > > > > This already has three underscores so shouldn't drop. > > Right, of course. > > > [...] > > > + mutex_unlock(&sta->ampdu_mlme.mtx); > > > __ieee80211_start_rx_ba_session(sta, 0, > > > 0, > > > 0, 1, tid, > > > > maybe this one needs a ___ version then? > > Either that, or as it's a single call, perhaps just the following? > Matter of taste I guess... I don't think it's a matter of taste - for me, in principle, dropping locks for small sections of code where the larger section holds it is a bug waiting to happen. It may (may, I don't even know) be OK here, but in general it's something to avoid. johannes
On Wed, 06 Sep 2017 15:21:00 +0200 Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2017-09-06 at 15:19 +0200, Stefano Brivio wrote: > > On Wed, 06 Sep 2017 14:48:35 +0200 > > Johannes Berg <johannes@sipsolutions.net> wrote: > > > > > I'll look in a bit - but > > > > > > > + mutex_unlock(&sta->ampdu_mlme.mtx); > > > > ___ieee80211_stop_rx_ba_session( > > > > sta, tid, WLAN_BACK_RECIPIENT, > > > > WLAN_REASON_QSTA_TIMEOUT, > > > > true); > > > > > > This already has three underscores so shouldn't drop. > > > > Right, of course. > > > > > [...] > > > > + mutex_unlock(&sta->ampdu_mlme.mtx); > > > > __ieee80211_start_rx_ba_session(sta, 0, > > > > 0, > > > > 0, 1, tid, > > > > > > maybe this one needs a ___ version then? > > > > Either that, or as it's a single call, perhaps just the following? > > Matter of taste I guess... > > I don't think it's a matter of taste - for me, in principle, dropping > locks for small sections of code where the larger section holds it is a > bug waiting to happen. It may (may, I don't even know) be OK here, but > in general it's something to avoid. Yes, that was based on the assumption that the initial part of __ieee80211_start_rx_ba_session() can't really affect the AMPDU state-machine in any way. But sure, one small change there in the future and the assumption doesn't hold anymore. -- Stefano
On Wed, 2017-09-06 at 15:27 +0200, Stefano Brivio wrote: > > Yes, that was based on the assumption that the initial part of > __ieee80211_start_rx_ba_session() can't really affect the AMPDU > state-machine in any way. That's not really the point, if that changes that function would have to move the locking around, and nothing else. The point is more that code in ieee80211_ba_session_work() could assume the lock is held across the entire loop, since that's the way it's written and looks like even with your patch. So for example replacing the loop of tid = 0..NUM_TIDS-1 with a list_for_each_entry() would already be unsafe with the dropping if the list were to require the mutex for locking. johannes
On Wed, 06 Sep 2017 15:30:10 +0200 Johannes Berg <johannes@sipsolutions.net> wrote: > So for example replacing the loop of tid = 0..NUM_TIDS-1 with a > list_for_each_entry() would already be unsafe with the dropping if the > list were to require the mutex for locking. Sure. Still, it would need another code change to break, but in general I do agree indeed. :) -- Stefano
diff --git a/net/mac80211/ht.c b/net/mac80211/ht.c index c92df492e898..377dd3c233d3 100644 --- a/net/mac80211/ht.c +++ b/net/mac80211/ht.c @@ -332,10 +332,13 @@ void ieee80211_ba_session_work(struct work_struct *work) WLAN_REASON_UNSPECIFIED, true); if (test_and_clear_bit(tid, - sta->ampdu_mlme.tid_rx_manage_offl)) + sta->ampdu_mlme.tid_rx_manage_offl)) { + mutex_unlock(&sta->ampdu_mlme.mtx); __ieee80211_start_rx_ba_session(sta, 0, 0, 0, 1, tid, IEEE80211_MAX_AMPDU_BUF, false, true); + mutex_lock(&sta->ampdu_mlme.mtx); + } if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS, sta->ampdu_mlme.tid_rx_manage_offl))