From: Johannes Berg <[email protected]>
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/[email protected]/
Reported-by: Jiri Kosina <[email protected]>
Reported-by: Aaron Hill <[email protected]>
Reported-by: Lukas Redlinger <[email protected]>
Reported-by: Oleksii Shevchuk <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/util.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 051a02ddcb85..ad1e88958da2 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -273,9 +273,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);
}
}
--
2.20.1
Johannes Berg <[email protected]> writes:
> From: Johannes Berg <[email protected]>
>
> 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/[email protected]/
>
> Reported-by: Jiri Kosina <[email protected]>
> Reported-by: Aaron Hill <[email protected]>
> Reported-by: Lukas Redlinger <[email protected]>
> Reported-by: Oleksii Shevchuk <[email protected]>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> net/mac80211/util.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 051a02ddcb85..ad1e88958da2 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -273,9 +273,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);
Okay, so this will mean that the drv_wake_tx_queue() entry point will be
called with bhs disabled. But there are lots of uses of
spin_{,un}lock_bh() in tx.c:
$ git grep lock_bh tx.c
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&local->active_txq_lock[txqi->txq.ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[txqi->txq.ac]);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&sta->lock);
tx.c: spin_unlock_bh(&sta->lock);
tx.c: spin_lock_bh(&sta->lock);
tx.c: spin_unlock_bh(&sta->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_unlock_bh(&fq->lock);
tx.c: spin_lock_bh(&local->active_txq_lock[ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c: spin_lock_bh(&local->active_txq_lock[txq->ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[txq->ac]);
tx.c: spin_lock_bh(&local->active_txq_lock[ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c: spin_lock_bh(&local->active_txq_lock[ac]);
tx.c: spin_unlock_bh(&local->active_txq_lock[ac]);
tx.c: spin_lock_bh(&local->tim_lock);
tx.c: spin_unlock_bh(&local->tim_lock);
so won't that mean that the driver still gets bhs re-enabled after (for
instance) the first call to ieee80211_tx_dequeue()?
I must admit that I'm a bit fuzzy on this whole bh/not-bh thing; I've
mostly been cargo-culting the _bh variant of the locking... :)
-Toke
On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote:
>
> > - spin_unlock_bh(&fq->lock);
> > + spin_unlock(&fq->lock);
> > drv_wake_tx_queue(local, txqi);
> > - spin_lock_bh(&fq->lock);
> > + spin_lock(&fq->lock);
>
> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> called with bhs disabled.
Right.
> But there are lots of uses of
> spin_{,un}lock_bh() in tx.c:
[snip]
> so won't that mean that the driver still gets bhs re-enabled after (for
> instance) the first call to ieee80211_tx_dequeue()?
No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine.
johannes
On Tue, 2019-10-01 at 12:56 +0200, Jiri Kosina wrote:
> On Tue, 1 Oct 2019, Toke Høiland-Jørgensen wrote:
>
> > > - spin_unlock_bh(&fq->lock);
> > > + spin_unlock(&fq->lock);
> > > drv_wake_tx_queue(local, txqi);
> > > - spin_lock_bh(&fq->lock);
> > > + spin_lock(&fq->lock);
> >
> > Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> > called with bhs disabled. But there are lots of uses of
> > spin_{,un}lock_bh() in tx.c:
>
> I am fine with whatever fix for this you guys settle on :) Just for the
> record, I proposed this back then:
>
> http://lore.kernel.org/r/[email protected]
>
> maybe it could be resurected, as I believe it'd fix this one as well?
Yes, it would, but it wouldn't address any other driver that likely has
the same issue :)
johannes
On Tue, 1 Oct 2019, Toke H?iland-J?rgensen wrote:
> > - spin_unlock_bh(&fq->lock);
> > + spin_unlock(&fq->lock);
> > drv_wake_tx_queue(local, txqi);
> > - spin_lock_bh(&fq->lock);
> > + spin_lock(&fq->lock);
>
> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
> called with bhs disabled. But there are lots of uses of
> spin_{,un}lock_bh() in tx.c:
I am fine with whatever fix for this you guys settle on :) Just for the
record, I proposed this back then:
http://lore.kernel.org/r/[email protected]
maybe it could be resurected, as I believe it'd fix this one as well?
But again, I have absolutely no preference either way, only that it'd be
nice to have this fixed :)
Thanks!
--
Jiri Kosina
SUSE Labs
Johannes Berg <[email protected]> writes:
> On Tue, 2019-10-01 at 12:53 +0200, Toke Høiland-Jørgensen wrote:
>>
>> > - spin_unlock_bh(&fq->lock);
>> > + spin_unlock(&fq->lock);
>> > drv_wake_tx_queue(local, txqi);
>> > - spin_lock_bh(&fq->lock);
>> > + spin_lock(&fq->lock);
>>
>> Okay, so this will mean that the drv_wake_tx_queue() entry point will be
>> called with bhs disabled.
>
> Right.
>
>> But there are lots of uses of
>> spin_{,un}lock_bh() in tx.c:
>
> [snip]
>
>> so won't that mean that the driver still gets bhs re-enabled after (for
>> instance) the first call to ieee80211_tx_dequeue()?
>
> No, local_bh_disable()/local_bh_enable() is re-entrant and nests fine.
Ah, right, gotcha. Hmm, in that case, would it be more clear to just add
an outer local_bh_disable()/local_bh_enable() to
___ieee80211_wake_txqs(). With this patch we're relying on the
mismatched use of _bh/non-_bh variants of the locking to ensure the bhs
stay off. Isn't that prone to breaking in the future?
Oh, and also, with just this patch, the additional drv_wake_tx_queue()
call for the vif TXQ at the bottom of __ieee80211_wake_txqs() will still
happen without bhs disabled, won't it?
-Toke