diff mbox series

[v2] mac80211: keep BHs disabled while calling drv_tx_wake_queue()

Message ID 1569928763-I3e8838c5ecad878e59d4a94eb069a90f6641461a@changeid (mailing list archive)
State Accepted
Delegated to: Johannes Berg
Headers show
Series [v2] mac80211: keep BHs disabled while calling drv_tx_wake_queue() | expand

Commit Message

Johannes Berg Oct. 1, 2019, 11:19 a.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

Drivers typically expect this, as it's the case for almost all cases
where this is called (i.e. from the TX path). Also, the code in mac80211
itself (if the driver calls ieee80211_tx_dequeue()) expects this as it
uses this_cpu_ptr() without additional protection.

This should fix various reports of the problem:
https://bugzilla.kernel.org/show_bug.cgi?id=204127
https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/
https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/

Reported-by: Jiri Kosina <jikos@kernel.org>
Reported-by: Aaron Hill <aa1ronham@gmail.com>
Reported-by: Lukas Redlinger <rel+kernel@agilox.net>
Reported-by: Oleksii Shevchuk <alxchk@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
v2:
 * use local_bh_enable/disable to capture the last occurrence
 * split spin_lock_bh() into local_bh_disable()/spin_lock() to
   make it clearer while we unlock them separately
---
 net/mac80211/util.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Toke Høiland-Jørgensen Oct. 1, 2019, 11:44 a.m. UTC | #1 
Johannes Berg <johannes@sipsolutions.net> writes:

> From: Johannes Berg <johannes.berg@intel.com>
>
> Drivers typically expect this, as it's the case for almost all cases
> where this is called (i.e. from the TX path). Also, the code in mac80211
> itself (if the driver calls ieee80211_tx_dequeue()) expects this as it
> uses this_cpu_ptr() without additional protection.
>
> This should fix various reports of the problem:
> https://bugzilla.kernel.org/show_bug.cgi?id=204127
> https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/
> https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/
>
> Reported-by: Jiri Kosina <jikos@kernel.org>
> Reported-by: Aaron Hill <aa1ronham@gmail.com>
> Reported-by: Lukas Redlinger <rel+kernel@agilox.net>
> Reported-by: Oleksii Shevchuk <alxchk@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Jiri Kosina Oct. 1, 2019, 11:58 a.m. UTC | #2 
On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote:

> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > Drivers typically expect this, as it's the case for almost all cases
> > where this is called (i.e. from the TX path). Also, the code in mac80211
> > itself (if the driver calls ieee80211_tx_dequeue()) expects this as it
> > uses this_cpu_ptr() without additional protection.
> >
> > This should fix various reports of the problem:
> > https://bugzilla.kernel.org/show_bug.cgi?id=204127
> > https://lore.kernel.org/linux-wireless/CAN5HydrWb3o_FE6A1XDnP1E+xS66d5kiEuhHfiGKkLNQokx13Q@mail.gmail.com/
> > https://lore.kernel.org/lkml/nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm/
> >
> > Reported-by: Jiri Kosina <jikos@kernel.org>

Reported-and-tested-by: Jiri Kosina <jkosina@suse.cz>

> > Reported-by: Aaron Hill <aa1ronham@gmail.com>
> > Reported-by: Lukas Redlinger <rel+kernel@agilox.net>
> > Reported-by: Oleksii Shevchuk <alxchk@gmail.com>
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Thanks,
diff mbox series

Patch

diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 051a02ddcb85..32a7a53833c0 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -247,7 +247,8 @@  static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 	struct sta_info *sta;
 	int i;
 
-	spin_lock_bh(&fq->lock);
+	local_bh_disable();
+	spin_lock(&fq->lock);
 
 	if (sdata->vif.type == NL80211_IFTYPE_AP)
 		ps = &sdata->bss->ps;
@@ -273,9 +274,9 @@  static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 						&txqi->flags))
 				continue;
 
-			spin_unlock_bh(&fq->lock);
+			spin_unlock(&fq->lock);
 			drv_wake_tx_queue(local, txqi);
-			spin_lock_bh(&fq->lock);
+			spin_lock(&fq->lock);
 		}
 	}
 
@@ -288,12 +289,14 @@  static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 	    (ps && atomic_read(&ps->num_sta_ps)) || ac != vif->txq->ac)
 		goto out;
 
-	spin_unlock_bh(&fq->lock);
+	spin_unlock(&fq->lock);
 
 	drv_wake_tx_queue(local, txqi);
+	local_bh_enable();
 	return;
 out:
-	spin_unlock_bh(&fq->lock);
+	spin_unlock(&fq->lock);
+	local_bh_enable();
 }
 
 static void