Messages in this thread Patch in this message | | | Date | Wed, 11 Sep 2019 12:42:38 +0100 (WEST) | From | Jiri Kosina <> | Subject | Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe |
| |
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
| |