* [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
@ 2019-04-15 11:03 Jiri Kosina
2019-04-15 11:10 ` Johannes Berg
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2019-04-15 11:03 UTC (permalink / raw)
To: Johannes Berg, Emmanuel Grumbach, Luca Coelho
Cc: Intel Linux Wireless, Kalle Valo, David S. Miller, netdev, linux-kernel
From: Jiri Kosina <jkosina@suse.cz>
iwl_mvm_sta->lock can be taken from BH, and therefore BH must be disabled if
taking it from other contexts.
This fixes the lockdep warning below.
IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready
========================================================
WARNING: possible irq lock inversion dependency detected
5.1.0-rc5 #3 Not tainted
--------------------------------------------------------
swapper/1/0 just changed the state of lock:
00000000a18af450 (_xmit_ETHER#2){+.-.}, at: __dev_queue_xmit+0x8da/0xbe0
but this lock took another, SOFTIRQ-unsafe lock in the past:
(&(&mvm_sta->lock)->rlock){+.+.}
and interrupts could create inverse lock ordering between them.
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&(&mvm_sta->lock)->rlock);
local_irq_disable();
lock(_xmit_ETHER#2);
lock(&(&mvm_sta->lock)->rlock);
<Interrupt>
lock(_xmit_ETHER#2);
*** DEADLOCK ***
4 locks held by swapper/1/0:
#0: 000000008e12d112 ((&idev->mc_ifc_timer)){+.-.}, at: call_timer_fn+0x5/0x310
#1: 0000000003eca5ef (rcu_read_lock){....}, at: mld_sendpack+0x5/0x3b0
#2: 000000007eac744b (rcu_read_lock_bh){....}, at: ip6_finish_output2+0x5f/0x960
#3: 000000007eac744b (rcu_read_lock_bh){....}, at: __dev_queue_xmit+0x4e/0xbe0
the shortest dependencies between 2nd lock and 1st lock:
-> (&(&mvm_sta->lock)->rlock){+.+.} {
HARDIRQ-ON-W at:
_raw_spin_lock_bh+0x3d/0x50
iwl_mvm_add_new_dqa_stream_wk+0x102/0x910 [iwlmvm]
process_one_work+0x1f0/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
ret_from_fork+0x3a/0x50
SOFTIRQ-ON-W at:
_raw_spin_lock+0x35/0x50
iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
process_one_work+0x1f0/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
ret_from_fork+0x3a/0x50
INITIAL USE at:
_raw_spin_lock_bh+0x3d/0x50
iwl_mvm_add_new_dqa_stream_wk+0x102/0x910 [iwlmvm]
process_one_work+0x1f0/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
ret_from_fork+0x3a/0x50
}
... key at: [<ffffffffc0e80d80>] __key.87928+0x0/0xfffffffffffd9280 [iwlmvm]
... acquired at:
_raw_spin_lock+0x35/0x50
iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
ieee80211_queue_skb+0x23a/0x470 [mac80211]
ieee80211_tx+0xe1/0x140 [mac80211]
__ieee80211_subif_start_xmit+0x257/0x310 [mac80211]
ieee80211_subif_start_xmit+0x47/0x300 [mac80211]
dev_hard_start_xmit+0xb7/0x340
__dev_queue_xmit+0x88d/0xbe0
packet_sendmsg+0x1025/0x1920 [af_packet]
sock_sendmsg+0x36/0x50
__sys_sendto+0xdc/0x160
__x64_sys_sendto+0x24/0x30
do_syscall_64+0x5d/0x190
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> (_xmit_ETHER#2){+.-.} {
HARDIRQ-ON-W at:
_raw_spin_lock+0x35/0x50
dev_deactivate_many+0xec/0x3b0
dev_deactivate+0x4f/0x80
linkwatch_do_dev+0x34/0x50
__linkwatch_run_queue+0xf4/0x190
linkwatch_event+0x21/0x30
process_one_work+0x1f0/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
ret_from_fork+0x3a/0x50
IN-SOFTIRQ-W at:
_raw_spin_lock+0x35/0x50
__dev_queue_xmit+0x8da/0xbe0
ip6_finish_output2+0x22b/0x960
ip6_output+0x1a6/0x2b0
mld_sendpack+0x369/0x3b0
mld_ifc_timer_expire+0x19f/0x2e0
call_timer_fn+0x92/0x310
run_timer_softirq+0x216/0x560
__do_softirq+0x12c/0x447
irq_exit+0xac/0xc0
smp_apic_timer_interrupt+0xac/0x2a0
apic_timer_interrupt+0xf/0x20
cpuidle_enter_state+0xbf/0x450
do_idle+0x1cc/0x290
cpu_startup_entry+0x19/0x20
start_secondary+0x179/0x1c0
secondary_startup_64+0xa4/0xb0
INITIAL USE at:
_raw_spin_lock+0x35/0x50
dev_deactivate_many+0xec/0x3b0
dev_deactivate+0x4f/0x80
linkwatch_do_dev+0x34/0x50
__linkwatch_run_queue+0xf4/0x190
linkwatch_event+0x21/0x30
process_one_work+0x1f0/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
ret_from_fork+0x3a/0x50
}
... key at: [<ffffffffa1679a70>] netdev_xmit_lock_key+0x10/0x390
... acquired at:
__lock_acquire+0x1ae/0xfe0
lock_acquire+0xbd/0x1e0
_raw_spin_lock+0x35/0x50
__dev_queue_xmit+0x8da/0xbe0
ip6_finish_output2+0x22b/0x960
ip6_output+0x1a6/0x2b0
mld_sendpack+0x369/0x3b0
mld_ifc_timer_expire+0x19f/0x2e0
call_timer_fn+0x92/0x310
run_timer_softirq+0x216/0x560
__do_softirq+0x12c/0x447
irq_exit+0xac/0xc0
smp_apic_timer_interrupt+0xac/0x2a0
apic_timer_interrupt+0xf/0x20
cpuidle_enter_state+0xbf/0x450
do_idle+0x1cc/0x290
cpu_startup_entry+0x19/0x20
start_secondary+0x179/0x1c0
secondary_startup_64+0xa4/0xb0
stack backtrace:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.1.0-rc5 #3
Hardware name: LENOVO 20K5S22R00/20K5S22R00, BIOS R0IET38W (1.16 ) 05/31/2017
Call Trace:
<IRQ>
dump_stack+0x78/0xb3
print_irq_inversion_bug.part.40+0x19f/0x1aa
check_usage_forwards+0x11e/0x120
? secondary_startup_64+0xa4/0xb0
? check_usage_backwards+0x120/0x120
mark_lock+0x17c/0x280
__lock_acquire+0x1ae/0xfe0
? find_held_lock+0x31/0xa0
? ieee80211_select_queue+0x124/0x2d0 [mac80211]
lock_acquire+0xbd/0x1e0
? __dev_queue_xmit+0x8da/0xbe0
_raw_spin_lock+0x35/0x50
? __dev_queue_xmit+0x8da/0xbe0
__dev_queue_xmit+0x8da/0xbe0
? __dev_queue_xmit+0x4e/0xbe0
? neigh_resolve_output+0x15a/0x230
? ip6_finish_output2+0x22b/0x960
ip6_finish_output2+0x22b/0x960
? ip6_finish_output2+0x5f/0x960
? ip6_mtu+0x12d/0x1f0
? ip6_mtu+0x71/0x1f0
ip6_output+0x1a6/0x2b0
? ip6_output+0xb8/0x2b0
? ip6_fragment+0xb70/0xb70
mld_sendpack+0x369/0x3b0
? mld_sendpack+0x5/0x3b0
? mld_dad_timer_expire+0x90/0x90
mld_ifc_timer_expire+0x19f/0x2e0
? mld_dad_timer_expire+0x90/0x90
? mld_dad_timer_expire+0x90/0x90
call_timer_fn+0x92/0x310
? call_timer_fn+0x5/0x310
? mld_dad_timer_expire+0x90/0x90
run_timer_softirq+0x216/0x560
__do_softirq+0x12c/0x447
irq_exit+0xac/0xc0
smp_apic_timer_interrupt+0xac/0x2a0
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:cpuidle_enter_state+0xbf/0x450
Code: 44 8b 63 04 49 89 c6 0f 1f 44 00 00 31 ff e8 08 3a 9b ff 80 7c 24 0b 00 0f 85 de 02 00 00 e8 e8 a4 a9 ff fb 66 0f 1f 44 00 00 <45> 85 ed 0f 88 b0 02 00 00 49 63 f5 4c 2b 34 24 48 ba cf f7 53 e3
RSP: 0018:ffffa60b00d27ea0 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
RAX: ffff8a5686a30340 RBX: ffffc60affc90f50 RCX: 0000000000000000
RDX: ffff8a5686a30340 RSI: 0000000000000006 RDI: ffff8a5686a30340
RBP: ffffffffa02fb360 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000001 R11: 071c71c71c71c71c R12: 0000000000000001
R13: 0000000000000003 R14: 0000000542fed0f3 R15: 0000000000000001
do_idle+0x1cc/0x290
cpu_startup_entry+0x19/0x20
start_secondary+0x179/0x1c0
secondary_startup_64+0xa4/0xb0
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 0c2aabc842f9..8c8ebb5e12bc 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1107,7 +1107,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
*/
info->flags &= ~IEEE80211_TX_STATUS_EOSP;
- spin_lock(&mvmsta->lock);
+ spin_lock_bh(&mvmsta->lock);
/* nullfunc frames should go to the MGMT queue regardless of QOS,
* the condition of !ieee80211_is_qos_nullfunc(fc) keeps the default
@@ -1146,7 +1146,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (WARN_ONCE(txq_id == IWL_MVM_INVALID_QUEUE, "Invalid TXQ id")) {
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
return 0;
}
@@ -1182,7 +1182,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (tid < IWL_MAX_TID_COUNT && !ieee80211_has_morefrags(fc))
mvmsta->tid_data[tid].seq_number = seq_number + 0x10;
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
if (iwl_mvm_tx_pkt_queued(mvm, mvmsta,
tid == IWL_MAX_TID_COUNT ? 0 : tid))
@@ -1192,7 +1192,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
drop_unlock_sta:
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
drop:
IWL_DEBUG_TX(mvm, "TX to [%d|%d] dropped\n", mvmsta->sta_id, tid);
return -1;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 11:03 [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe Jiri Kosina
@ 2019-04-15 11:10 ` Johannes Berg
2019-04-15 11:33 ` Jiri Kosina
0 siblings, 1 reply; 11+ messages in thread
From: Johannes Berg @ 2019-04-15 11:10 UTC (permalink / raw)
To: Jiri Kosina, Emmanuel Grumbach, Luca Coelho
Cc: Intel Linux Wireless, Kalle Valo, David S. Miller, netdev, linux-kernel
On Mon, 2019-04-15 at 13:03 +0200, Jiri Kosina wrote:
(FWIW, would be good to cc linux-wireless, but no need for linux-kernel
I guess :-) )
> iwl_mvm_sta->lock can be taken from BH, and therefore BH must be disabled if
> taking it from other contexts.
>
> This fixes the lockdep warning below.
I don't think this is the right fix.
The only problem is this path:
> SOFTIRQ-ON-W at:
> _raw_spin_lock+0x35/0x50
> iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
> iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
> iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
> iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
> process_one_work+0x1f0/0x5b0
which I believe we just made use local_bh_disable() since there are
other reasons as well for that, IIRC.
Yes, here's the fix:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a
It's on the way upstream.
Thanks for the patch/report though!
johannes
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 11:10 ` Johannes Berg
@ 2019-04-15 11:33 ` Jiri Kosina
2019-04-15 11:37 ` Johannes Berg
0 siblings, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2019-04-15 11:33 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Mon, 15 Apr 2019, Johannes Berg wrote:
> (FWIW, would be good to cc linux-wireless, but no need for linux-kernel
> I guess :-) )
Hm, no idea where I lost that ML from the get_maintainer output, sorry for
that.
> > iwl_mvm_sta->lock can be taken from BH, and therefore BH must be
> > disabled if taking it from other contexts.
> >
> > This fixes the lockdep warning below.
>
> I don't think this is the right fix.
>
> The only problem is this path:
Agreed, that seems to be the only culprit.
> > SOFTIRQ-ON-W at:
> > _raw_spin_lock+0x35/0x50
> > iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
> > iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
> > iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
> > iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
> > process_one_work+0x1f0/0x5b0
>
> which I believe we just made use local_bh_disable() since there are
> other reasons as well for that, IIRC.
>
> Yes, here's the fix:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a
>
> It's on the way upstream.
If there are other reasons why disable BH for the whole function (are
there?), then this bigger hammer works as well of course.
Tested-by: Jiri Kosina <jkosina@suse.cz>
> Thanks for the patch/report though!
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 11:33 ` Jiri Kosina
@ 2019-04-15 11:37 ` Johannes Berg
2019-04-15 12:06 ` Jiri Kosina
2019-09-11 11:42 ` Jiri Kosina
0 siblings, 2 replies; 11+ messages in thread
From: Johannes Berg @ 2019-04-15 11:37 UTC (permalink / raw)
To: Jiri Kosina
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Mon, 2019-04-15 at 13:33 +0200, Jiri Kosina wrote:
> > Yes, here's the fix:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a
> >
> > It's on the way upstream.
>
> If there are other reasons why disable BH for the whole function (are
> there?), then this bigger hammer works as well of course.
I thought there are, but seeing the commit log here I'm not sure.
In any case, even if not, the function itself is part of the TX fast
path, but the caller from the workqueue is very uncommon (basically only
happens for a handful of packets on each new RA/TID), so I'd say that'd
be a good reason to use the slightly bigger hammer (it's not that much
different really, if you look at how much code is covered by the lock)
and avoid doing it all the time when we know it to be not needed.
> Tested-by: Jiri Kosina <jkosina@suse.cz>
Too late now to add this to the git tree, but thanks for checking! :-)
johannes
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 11:37 ` Johannes Berg
@ 2019-04-15 12:06 ` Jiri Kosina
2019-04-15 12:08 ` Johannes Berg
2019-09-11 11:42 ` Jiri Kosina
1 sibling, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2019-04-15 12:06 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Mon, 15 Apr 2019, Johannes Berg wrote:
> > If there are other reasons why disable BH for the whole function (are
> > there?), then this bigger hammer works as well of course.
>
> I thought there are, but seeing the commit log here I'm not sure.
>
> In any case, even if not, the function itself is part of the TX fast
> path, but the caller from the workqueue is very uncommon (basically only
> happens for a handful of packets on each new RA/TID), so I'd say that'd
> be a good reason to use the slightly bigger hammer (it's not that much
> different really, if you look at how much code is covered by the lock)
> and avoid doing it all the time when we know it to be not needed.
Given that we don't have a primitive in lockdep to assert this invariant,
would you consider queuing the below on top of f5ae2f932e2f in
wireless-drivers.git?
Thanks.
From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] iwlwifi: iwl_mvm_tx_mpdu() must be called with BH disabled
As iwl_mvm_tx_mpdu() is not disabling BH while obtaining iwl_mvm_sta->lock
(which is being taken from BH context as well), it has to be always
invoked with BH disabled. Make that clear in a comment.
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 0c2aabc842f9..ad4760307726 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1063,7 +1063,9 @@ static int iwl_mvm_tx_pkt_queued(struct iwl_mvm *mvm,
}
/*
- * Sets the fields in the Tx cmd that are crypto related
+ * Sets the fields in the Tx cmd that are crypto related.
+ *
+ * This function must be called with BHs disabled.
*/
static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
struct ieee80211_tx_info *info,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 12:06 ` Jiri Kosina
@ 2019-04-15 12:08 ` Johannes Berg
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Berg @ 2019-04-15 12:08 UTC (permalink / raw)
To: Jiri Kosina
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
> Given that we don't have a primitive in lockdep to assert this invariant,
> would you consider queuing the below on top of f5ae2f932e2f in
> wireless-drivers.git?
I think this would probably go via -next, but sure, I've applied it
internally and we'll let it percolate through the trees as usual.
johannes
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-04-15 11:37 ` Johannes Berg
2019-04-15 12:06 ` Jiri Kosina
@ 2019-09-11 11:42 ` Jiri Kosina
2019-09-11 12:04 ` Jiri Kosina
2019-10-01 9:46 ` Johannes Berg
1 sibling, 2 replies; 11+ messages in thread
From: Jiri Kosina @ 2019-09-11 11:42 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Mon, 15 Apr 2019, Johannes Berg wrote:
> > If there are other reasons why disable BH for the whole function (are
> > there?), then this bigger hammer works as well of course.
>
> I thought there are, but seeing the commit log here I'm not sure.
>
> In any case, even if not, the function itself is part of the TX fast
> path, but the caller from the workqueue is very uncommon (basically only
> happens for a handful of packets on each new RA/TID), so I'd say that'd
> be a good reason to use the slightly bigger hammer (it's not that much
> different really, if you look at how much code is covered by the lock)
> and avoid doing it all the time when we know it to be not needed.
So this now popped up on me with current Linus' tree from a different
codepath, see below. Therefore I'd like to propose bringing my previous
patch back to life.
Thanks.
From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
iwl_mvm_sta->lock can be taken from BH, and therefore BH must be disabled if
taking it from other contexts.
This fixes the lockdep warning below.
================================
WARNING: inconsistent lock state
5.3.0-rc8 #3 Tainted: G W
--------------------------------
inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
kworker/u8:2/28401 [HC0[0]:SC0[0]:HE1:SE1] takes:
000000009c020e13 (&(&mvm_sta->lock)->rlock){+.?.}, at: iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
{IN-SOFTIRQ-W} state was registered at:
lock_acquire+0xbd/0x1e0
_raw_spin_lock+0x35/0x50
iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
iwl_mvm_tx_skb+0x1f8/0x460 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xcc/0x200 [iwlmvm]
ieee80211_queue_skb+0x290/0x4c0 [mac80211]
ieee80211_xmit_fast+0x814/0xa70 [mac80211]
__ieee80211_subif_start_xmit+0x132/0x380 [mac80211]
ieee80211_subif_start_xmit+0x49/0x370 [mac80211]
dev_hard_start_xmit+0xaf/0x340
__dev_queue_xmit+0x98d/0xd20
ip6_finish_output2+0x323/0x960
ip6_output+0x19d/0x2d0
mld_sendpack+0x361/0x3a0
mld_ifc_timer_expire+0x19f/0x2e0
call_timer_fn+0x97/0x300
run_timer_softirq+0x233/0x590
__do_softirq+0x12c/0x448
irq_exit+0xa5/0xb0
smp_apic_timer_interrupt+0xac/0x2a0
apic_timer_interrupt+0xf/0x20
cpuidle_enter_state+0xbf/0x450
cpuidle_enter+0x29/0x40
do_idle+0x1cc/0x290
cpu_startup_entry+0x19/0x20
start_secondary+0x15f/0x1a0
secondary_startup_64+0xa4/0xb0
[ ... snip ... ]
stack backtrace:
CPU: 1 PID: 28401 Comm: kworker/u8:2 Tainted: G W 5.3.0-rc8 #3
Hardware name: LENOVO 20K5S22R00/20K5S22R00, BIOS R0IET38W (1.16 ) 05/31/2017
Workqueue: phy0 ieee80211_beacon_connection_loss_work [mac80211]
Call Trace:
dump_stack+0x78/0xb3
mark_lock+0x28a/0x2a0
__lock_acquire+0x568/0x1020
? iwl_mvm_set_tx_cmd+0x1c5/0x400 [iwlmvm]
lock_acquire+0xbd/0x1e0
? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
_raw_spin_lock+0x35/0x50
? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
? ieee80211_tx_h_select_key+0xf1/0x4a0 [mac80211]
iwl_mvm_tx_skb+0x1f8/0x460 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xcc/0x200 [iwlmvm]
? iwl_mvm_mac_itxq_xmit+0x55/0x200 [iwlmvm]
_ieee80211_wake_txqs+0x2cf/0x660 [mac80211]
? _ieee80211_wake_txqs+0x5/0x660 [mac80211]
? __ieee80211_wake_queue+0x219/0x340 [mac80211]
ieee80211_wake_queues_by_reason+0x64/0xa0 [mac80211]
ieee80211_set_disassoc+0x3b1/0x520 [mac80211]
__ieee80211_disconnect+0x81/0x110 [mac80211]
process_one_work+0x1f0/0x5b0
? process_one_work+0x16a/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
? process_one_work+0x5b0/0x5b0
? kthread_bind+0x10/0x10
ret_from_fork+0x3a/0x50
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 6ac114a393cc..bcfb290beaaf 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1106,7 +1106,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
*/
info->flags &= ~IEEE80211_TX_STATUS_EOSP;
- spin_lock(&mvmsta->lock);
+ spin_lock_bh(&mvmsta->lock);
/* nullfunc frames should go to the MGMT queue regardless of QOS,
* the condition of !ieee80211_is_qos_nullfunc(fc) keeps the default
@@ -1145,7 +1145,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (WARN_ONCE(txq_id == IWL_MVM_INVALID_QUEUE, "Invalid TXQ id")) {
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
return 0;
}
@@ -1181,7 +1181,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (tid < IWL_MAX_TID_COUNT && !ieee80211_has_morefrags(fc))
mvmsta->tid_data[tid].seq_number = seq_number + 0x10;
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
if (iwl_mvm_tx_pkt_queued(mvm, mvmsta,
tid == IWL_MAX_TID_COUNT ? 0 : tid))
@@ -1191,7 +1191,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
drop_unlock_sta:
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
drop:
IWL_DEBUG_TX(mvm, "TX to [%d|%d] dropped\n", mvmsta->sta_id, tid);
return -1;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-09-11 11:42 ` Jiri Kosina
@ 2019-09-11 12:04 ` Jiri Kosina
2019-09-20 21:34 ` Jiri Kosina
2019-10-01 9:46 ` Johannes Berg
1 sibling, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2019-09-11 12:04 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Wed, 11 Sep 2019, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
Hm, scratch that, that might actually spuriously enable BHs if called from
contexts that already did disabled BHs.
So what solution would you prefer here? Just stick another par of
bh_disable() / bh_enable() somewhere to the wake_txs() ->
iwl_mvm_mac_itxq_xmit() -> iwl_mvm_tx_skb() -> iwl_mvm_tx_mpdu() path?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-09-11 12:04 ` Jiri Kosina
@ 2019-09-20 21:34 ` Jiri Kosina
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Kosina @ 2019-09-20 21:34 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
On Wed, 11 Sep 2019, Jiri Kosina wrote:
> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
>
> Hm, scratch that, that might actually spuriously enable BHs if called from
> contexts that already did disabled BHs.
>
> So what solution would you prefer here? Just stick another par of
> bh_disable() / bh_enable() somewhere to the wake_txs() ->
> iwl_mvm_mac_itxq_xmit() -> iwl_mvm_tx_skb() -> iwl_mvm_tx_mpdu() path?
Ping? This seems to be still the case.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-09-11 11:42 ` Jiri Kosina
2019-09-11 12:04 ` Jiri Kosina
@ 2019-10-01 9:46 ` Johannes Berg
2019-10-01 9:52 ` Johannes Berg
1 sibling, 1 reply; 11+ messages in thread
From: Johannes Berg @ 2019-10-01 9:46 UTC (permalink / raw)
To: Jiri Kosina
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel
Hi Jiri,
Sorry for the long delay.
> CPU: 1 PID: 28401 Comm: kworker/u8:2 Tainted: G W 5.3.0-rc8 #3
> Hardware name: LENOVO 20K5S22R00/20K5S22R00, BIOS R0IET38W (1.16 ) 05/31/2017
> Workqueue: phy0 ieee80211_beacon_connection_loss_work [mac80211]
> Call Trace:
> dump_stack+0x78/0xb3
> mark_lock+0x28a/0x2a0
> __lock_acquire+0x568/0x1020
> ? iwl_mvm_set_tx_cmd+0x1c5/0x400 [iwlmvm]
> lock_acquire+0xbd/0x1e0
> ? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
> _raw_spin_lock+0x35/0x50
> ? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
> iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
> ? ieee80211_tx_h_select_key+0xf1/0x4a0 [mac80211]
> iwl_mvm_tx_skb+0x1f8/0x460 [iwlmvm]
> iwl_mvm_mac_itxq_xmit+0xcc/0x200 [iwlmvm]
> ? iwl_mvm_mac_itxq_xmit+0x55/0x200 [iwlmvm]
> _ieee80211_wake_txqs+0x2cf/0x660 [mac80211]
> ? _ieee80211_wake_txqs+0x5/0x660 [mac80211]
> ? __ieee80211_wake_queue+0x219/0x340 [mac80211]
> ieee80211_wake_queues_by_reason+0x64/0xa0 [mac80211]
>
I'm a bit confused by this.
ieee80211_wake_queues_by_reason() does
spin_lock_irqsave()/spin_unlock_irqrestore() - why is that "{SOFTIRQ-ON-
W} usage"?
Or what did you snip?
johannes
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
2019-10-01 9:46 ` Johannes Berg
@ 2019-10-01 9:52 ` Johannes Berg
0 siblings, 0 replies; 11+ messages in thread
From: Johannes Berg @ 2019-10-01 9:52 UTC (permalink / raw)
To: Jiri Kosina
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
David S. Miller, netdev, linux-kernel,
Toke Høiland-Jørgensen
On Tue, 2019-10-01 at 11:46 +0200, Johannes Berg wrote:
>
> ieee80211_wake_queues_by_reason() does
> spin_lock_irqsave()/spin_unlock_irqrestore() - why is that "{SOFTIRQ-ON-
> W} usage"?
scratch that - _ieee80211_wake_txqs() unlocks that again...
It does hold RCU critical section, but that's not the same as disabling
BHs.
I think we should do this perhaps - I think it'd be better to ensure
that the drivers' wake_tx_queue op is always called with softirqs
disabled, since that happens in almost all cases already ...
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);
}
}
Perhaps we could add some validation into drv_wake_tx_queue(), but I
didn't find the right thing to call right now ...
Toke, what do you think?
johannes
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-01 9:53 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 11:03 [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe Jiri Kosina
2019-04-15 11:10 ` Johannes Berg
2019-04-15 11:33 ` Jiri Kosina
2019-04-15 11:37 ` Johannes Berg
2019-04-15 12:06 ` Jiri Kosina
2019-04-15 12:08 ` Johannes Berg
2019-09-11 11:42 ` Jiri Kosina
2019-09-11 12:04 ` Jiri Kosina
2019-09-20 21:34 ` Jiri Kosina
2019-10-01 9:46 ` Johannes Berg
2019-10-01 9:52 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).