diff mbox series

iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_gen2_enqueue_hcmd()

Message ID nycvar.YFH.7.76.2104070918090.12405@cbobk.fhfr.pm
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_gen2_enqueue_hcmd() | expand

Commit Message

Jiri Kosina April 7, 2021, 7:51 a.m. UTC
On Wed, 7 Apr 2021, Heiner Kallweit wrote:

> Same fix as in 2800aadc18a6 ("iwlwifi: Fix softirq/hardirq disabling in
> iwl_pcie_enqueue_hcmd()") is needed for iwl_pcie_gen2_enqueue_hcmd.
> I get the same lockdep warning on AX210.

Makes sense, it's being called from exactly the same contexts.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_gen2_enqueue_hcmd()

Analogically to what we did in 2800aadc18a6 ("iwlwifi: Fix 
softirq/hardirq disabling in iwl_pcie_enqueue_hcmd()"), we must apply the 
same fix to iwl_pcie_gen2_enqueue_hcmd(), as it's being called from 
exactly the same contexts.

Reported-by: Heiner Kallweit <hkallweit1@gmail.com
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

Comments

Johannes Berg April 7, 2021, 7:55 a.m. UTC | #1
On Wed, 2021-04-07 at 09:51 +0200, Jiri Kosina wrote:
> On Wed, 7 Apr 2021, Heiner Kallweit wrote:
> 
> > Same fix as in 2800aadc18a6 ("iwlwifi: Fix softirq/hardirq disabling in
> > iwl_pcie_enqueue_hcmd()") is needed for iwl_pcie_gen2_enqueue_hcmd.
> > I get the same lockdep warning on AX210.
> 
> Makes sense, it's being called from exactly the same contexts.

I'm guessing nobody saw this before because the LEDs stuff is not
supported/used on newer devices :)

Still makes sense though.

Btw, I had a fix for your patch for some devices, see below. Already in
our tree, so Luca will send it on the way, just FYI/review here.

Thanks,
johannes


From: Johannes Berg <johannes.berg@intel.com>
Date: Tue, 6 Apr 2021 16:50:23 +0200
Subject: [PATCH] [BUGFIX] iwlwifi: pcie: don't enable BHs with IRQs disabled

After the fix from Jiri that disabled local IRQs instead of
just BHs (necessary to fix an issue with submitting a command
with IRQs already disabled), there was still a situation in
which we could deep in there enable BHs, if the device config
sets the apmg_wake_up_wa configuration, which is true on all
7000 series devices.

To fix that, but not require reverting commit 1ed08f6fb5ae
("iwlwifi: remove flags argument for nic_access"), split up
nic access into a version with BH manipulation to use most
of the time, and without it for this specific case where the
local IRQs are already disabled.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 .../wireless/intel/iwlwifi/pcie/internal.h    |  5 ++++
 .../net/wireless/intel/iwlwifi/pcie/trans.c   | 24 ++++++++++++++++---
 drivers/net/wireless/intel/iwlwifi/pcie/tx.c  |  4 ++--
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index 2c2389feb5e1..0b99f0c34111 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -453,6 +453,11 @@ struct iwl_trans
 		      const struct iwl_cfg_trans_params *cfg_trans);
 void iwl_trans_pcie_free(struct iwl_trans *trans);
 
+bool __iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans);
+#define _iwl_trans_pcie_grab_nic_access(trans)			\
+	__cond_lock(nic_access_nobh,				\
+		    likely(__iwl_trans_pcie_grab_nic_access(trans)))
+
 /*****************************************************
 * RX
 ******************************************************/
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index f2869fc343e3..bf36fa72f22e 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -2047,12 +2047,16 @@ static void iwl_trans_pcie_removal_wk(struct work_struct *wk)
 	module_put(THIS_MODULE);
 }
 
-static bool iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans)
+/*
+ * This version doesn't disable BHs but rather assumes they're
+ * already disabled.
+ */
+bool __iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans)
 {
 	int ret;
 	struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
 
-	spin_lock_bh(&trans_pcie->reg_lock);
+	spin_lock(&trans_pcie->reg_lock);
 
 	if (trans_pcie->cmd_hold_nic_awake)
 		goto out;
@@ -2137,7 +2141,7 @@ static bool iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans)
 		}
 
 err:
-		spin_unlock_bh(&trans_pcie->reg_lock);
+		spin_unlock(&trans_pcie->reg_lock);
 		return false;
 	}
 
@@ -2150,6 +2154,20 @@ out:
 	return true;
 }
 
+static bool iwl_trans_pcie_grab_nic_access(struct iwl_trans *trans)
+{
+	bool ret;
+
+	local_bh_disable();
+	ret = __iwl_trans_pcie_grab_nic_access(trans);
+	if (ret) {
+		/* keep BHs disabled until iwl_trans_pcie_release_nic_access */
+		return ret;
+	}
+	local_bh_enable();
+	return false;
+}
+
 static void iwl_trans_pcie_release_nic_access(struct iwl_trans *trans)
 {
 	struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
index 0346505351f5..4f6c187eed69 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
@@ -643,7 +643,7 @@ static int iwl_pcie_set_cmd_in_flight(struct iwl_trans *trans,
 	 * returned. This needs to be done only on NICs that have
 	 * apmg_wake_up_wa set (see above.)
 	 */
-	if (!iwl_trans_grab_nic_access(trans))
+	if (!_iwl_trans_pcie_grab_nic_access(trans))
 		return -EIO;
 
 	/*
@@ -652,7 +652,7 @@ static int iwl_pcie_set_cmd_in_flight(struct iwl_trans *trans,
 	 * already true, so it's OK to unconditionally set it to true.
 	 */
 	trans_pcie->cmd_hold_nic_awake = true;
-	spin_unlock_bh(&trans_pcie->reg_lock);
+	spin_unlock(&trans_pcie->reg_lock);
 
 	return 0;
 }
Johannes Berg April 7, 2021, 7:56 a.m. UTC | #2
On Wed, 2021-04-07 at 09:55 +0200, Johannes Berg wrote:
> On Wed, 2021-04-07 at 09:51 +0200, Jiri Kosina wrote:
> > On Wed, 7 Apr 2021, Heiner Kallweit wrote:
> > 
> > > Same fix as in 2800aadc18a6 ("iwlwifi: Fix softirq/hardirq disabling in
> > > iwl_pcie_enqueue_hcmd()") is needed for iwl_pcie_gen2_enqueue_hcmd.
> > > I get the same lockdep warning on AX210.
> > 
> > Makes sense, it's being called from exactly the same contexts.
> 
> I'm guessing nobody saw this before because the LEDs stuff is not
> supported/used on newer devices :)

Eh, wait, Heiner said he sees the same warning? With LEDs? I was pretty
sure that's not supported by the later firmware, but maybe some other
path? Anyway this fix seems right.

johannes
Heiner Kallweit April 7, 2021, 8:30 a.m. UTC | #3
On 07.04.2021 09:56, Johannes Berg wrote:
> On Wed, 2021-04-07 at 09:55 +0200, Johannes Berg wrote:
>> On Wed, 2021-04-07 at 09:51 +0200, Jiri Kosina wrote:
>>> On Wed, 7 Apr 2021, Heiner Kallweit wrote:
>>>
>>>> Same fix as in 2800aadc18a6 ("iwlwifi: Fix softirq/hardirq disabling in
>>>> iwl_pcie_enqueue_hcmd()") is needed for iwl_pcie_gen2_enqueue_hcmd.
>>>> I get the same lockdep warning on AX210.
>>>
>>> Makes sense, it's being called from exactly the same contexts.
>>
>> I'm guessing nobody saw this before because the LEDs stuff is not
>> supported/used on newer devices :)
> 
> Eh, wait, Heiner said he sees the same warning? With LEDs? I was pretty
> sure that's not supported by the later firmware, but maybe some other
> path? Anyway this fix seems right.
> 
> johannes
> 

For your reference here the warning I got.

[   27.645349] ------------[ cut here ]------------
[   27.645453] WARNING: CPU: 1 PID: 2323 at kernel/softirq.c:364 __local_bh_enable_ip+0x85/0xc0
[   27.645516] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio cmac bnep iwlmvm led_class vfat fat mac80211 libarc4 iwlwifi x86_pkg_temp_thermal coretemp i915 btusb btintel bluetooth snd_hda_intel snd_intel_dspcfg aesni_intel r8169 cfg80211 realtek ecdh_generic snd_hda_codec mdio_devres crypto_simd i2c_i801 rfkill intel_gtt ecc mei_me cryptd i2c_smbus snd_hda_core libphy mei i2c_algo_bit snd_pcm drm_kms_helper syscopyarea sysfillrect sysimgblt snd_timer fb_sys_fops snd sch_fq_codel crypto_user drm efivarfs ext4 mbcache jbd2 ums_realtek crc32c_intel ahci libahci libata
[   27.645991] CPU: 1 PID: 2323 Comm: ip Not tainted 5.12.0-rc6-next-20210406+ #4
[   27.646040] Hardware name: NA ZBOX-CI327NANO-GS-01/ZBOX-CI327NANO-GS-01, BIOS 5.12 04/28/2020
[   27.646094] RIP: 0010:__local_bh_enable_ip+0x85/0xc0
[   27.646132] Code: 8b 05 af c6 ba 47 a9 00 ff ff 00 74 32 65 ff 0d a1 c6 ba 47 e8 7c b4 0e 00 fb 5b 41 5c 5d c3 65 8b 05 6b cc ba 47 85 c0 75 ae <0f> 0b eb aa e8 92 b3 0e 00 eb ab 4c 89 e7 e8 18 ff 05 00 eb b4 65
[   27.646275] RSP: 0018:ffffbb574062b1b8 EFLAGS: 00010046
[   27.646311] RAX: 0000000000000000 RBX: 0000000000000201 RCX: 0000000000000000
[   27.646355] RDX: 0000000000000003 RSI: 0000000000000201 RDI: ffffffffc051192b
[   27.646401] RBP: ffffbb574062b1c8 R08: 0000000000000000 R09: 0000000000000001
[   27.646444] R10: ffff9eec03243e00 R11: 0000000000000000 R12: ffffffffc051192b
[   27.646488] R13: ffff9eec07e72b40 R14: 0000000000000001 R15: 0000000000000000
[   27.646533] FS:  00007fce73f11740(0000) GS:ffff9eec7bc80000(0000) knlGS:0000000000000000
[   27.646584] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.646622] CR2: 00005581ebf10cc0 CR3: 0000000102924000 CR4: 00000000003506e0
[   27.646668] Call Trace:
[   27.646687]  _raw_spin_unlock_bh+0x2c/0x40
[   27.646718]  iwl_pcie_gen2_enqueue_hcmd+0x50b/0x890 [iwlwifi]
[   27.646781]  iwl_trans_txq_send_hcmd+0x68/0x3b0 [iwlwifi]
[   27.646836]  iwl_trans_send_cmd+0x7d/0x170 [iwlwifi]
[   27.646888]  iwl_mvm_send_cmd+0x29/0x80 [iwlmvm]
[   27.646942]  iwl_mvm_led_set+0xa5/0xd0 [iwlmvm]
[   27.646995]  iwl_led_brightness_set+0x1a/0x20 [iwlmvm]
[   27.647050]  led_set_brightness_nosleep+0x24/0x50
[   27.647085]  led_set_brightness+0x41/0x50
[   27.647115]  led_trigger_event+0x46/0x70
[   27.647143]  ieee80211_led_radio+0x24/0x30 [mac80211]
[   27.647288]  ieee80211_do_open+0x4c4/0x9a0 [mac80211]
[   27.647379]  ieee80211_open+0x69/0x90 [mac80211]
[   27.647487]  __dev_open+0xd6/0x190
[   27.647517]  __dev_change_flags+0x1b2/0x230
[   27.647548]  dev_change_flags+0x21/0x60
[   27.647577]  do_setlink+0x289/0x1190
[   27.647607]  ? __nla_validate_parse+0x60/0xbd0
[   27.647644]  __rtnl_newlink+0x61c/0xa10
[   27.647677]  ? mark_lock+0x25/0x1110
[   27.647709]  ? rcu_read_lock_sched_held+0x41/0x80
[   27.647744]  ? kmem_cache_alloc_trace+0x21e/0x250
[   27.647781]  rtnl_newlink+0x44/0x70
[   27.647808]  rtnetlink_rcv_msg+0x17f/0x4b0
[   27.647838]  ? netlink_deliver_tap+0x10e/0x3f0
[   27.647873]  ? find_held_lock+0x31/0x90
[   27.647902]  ? netlink_deliver_tap+0x10e/0x3f0
[   27.647934]  ? rtnetlink_put_metrics+0x1d0/0x1d0
[   27.647967]  netlink_rcv_skb+0x50/0xf0
[   27.648000]  rtnetlink_rcv+0x10/0x20
[   27.648027]  netlink_unicast+0x1a3/0x270
[   27.648056]  netlink_sendmsg+0x22c/0x430
[   27.648085]  ? netlink_unicast+0x270/0x270
[   27.648116]  ____sys_sendmsg+0x2a6/0x2e0
[   27.648147]  ? import_iovec+0x1b/0x20
[   27.648179]  ? sendmsg_copy_msghdr+0x79/0xa0
[   27.648211]  ___sys_sendmsg+0x82/0xc0
[   27.648240]  __sys_sendmsg+0x62/0xb0
[   27.648268]  ? syscall_enter_from_user_mode+0x21/0x60
[   27.648307]  __x64_sys_sendmsg+0x1a/0x20
[   27.648337]  do_syscall_64+0x38/0x50
[   27.648366]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   27.648403] RIP: 0033:0x7fce74030737
[   27.648433] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
[   27.648547] RSP: 002b:00007ffdd61d1498 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[   27.648600] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fce74030737
[   27.648645] RDX: 0000000000000000 RSI: 00007ffdd61d1500 RDI: 0000000000000003
[   27.648691] RBP: 00000000606d6d4f R08: 0000000000000001 R09: 00007fce740f1a60
[   27.648736] R10: 0000000000000230 R11: 0000000000000246 R12: 0000000000000001
[   27.648782] R13: 00007ffdd61d15d0 R14: 0000000000000000 R15: 00005581ebec6020
[   27.648830] irq event stamp: 94495
[   27.648855] hardirqs last  enabled at (94493): [<ffffffffb846a864>] __local_bh_enable_ip+0x74/0xc0
[   27.648917] hardirqs last disabled at (94494): [<ffffffffb8bf198f>] _raw_read_lock_irqsave+0x7f/0x90
[   27.648977] softirqs last  enabled at (94492): [<ffffffffc051192b>] iwl_pcie_gen2_enqueue_hcmd+0x50b/0x890 [iwlwifi]
[   27.649064] softirqs last disabled at (94495): [<ffffffffc0511516>] iwl_pcie_gen2_enqueue_hcmd+0xf6/0x890 [iwlwifi]
[   27.649151] ---[ end trace da6e11a6f99dba37 ]---
[   27.649242] ------------[ cut here ]------------
[   27.649326] raw_local_irq_restore() called with IRQs enabled
[   27.649367] WARNING: CPU: 1 PID: 2323 at kernel/locking/irqflag-debug.c:10 warn_bogus_irq_restore+0x22/0x30
[   27.649433] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio cmac bnep iwlmvm led_class vfat fat mac80211 libarc4 iwlwifi x86_pkg_temp_thermal coretemp i915 btusb btintel bluetooth snd_hda_intel snd_intel_dspcfg aesni_intel r8169 cfg80211 realtek ecdh_generic snd_hda_codec mdio_devres crypto_simd i2c_i801 rfkill intel_gtt ecc mei_me cryptd i2c_smbus snd_hda_core libphy mei i2c_algo_bit snd_pcm drm_kms_helper syscopyarea sysfillrect sysimgblt snd_timer fb_sys_fops snd sch_fq_codel crypto_user drm efivarfs ext4 mbcache jbd2 ums_realtek crc32c_intel ahci libahci libata
[   27.649900] CPU: 1 PID: 2323 Comm: ip Tainted: G        W         5.12.0-rc6-next-20210406+ #4
[   27.649956] Hardware name: NA ZBOX-CI327NANO-GS-01/ZBOX-CI327NANO-GS-01, BIOS 5.12 04/28/2020
[   27.650010] RIP: 0010:warn_bogus_irq_restore+0x22/0x30
[   27.650047] Code: cc cc cc cc cc cc cc cc 55 48 89 e5 80 3d ca 51 9f 00 00 74 02 5d c3 48 c7 c7 18 07 21 b9 c6 05 b8 51 9f 00 01 e8 12 f8 fc ff <0f> 0b 5d c3 cc cc cc cc cc cc cc cc cc cc 55 48 89 e5 41 54 53 44
[   27.650177] RSP: 0018:ffffbb574062b3d0 EFLAGS: 00010286
[   27.650216] RAX: 0000000000000000 RBX: 0000000000000200 RCX: ffff9eec7bc976f8
[   27.650303] RDX: 00000000ffffffd8 RSI: 0000000000000027 RDI: ffff9eec7bc976f0
[   27.650423] RBP: ffffbb574062b3d0 R08: 0000000000000001 R09: 0000000000000001
[   27.650542] R10: 00000000ffffe000 R11: 3fffffffffffffff R12: ffff9eec08e9ac78
[   27.650661] R13: 00000000000000ff R14: 0000000000000286 R15: ffff9eec08e9ac78
[   27.650781] FS:  00007fce73f11740(0000) GS:ffff9eec7bc80000(0000) knlGS:0000000000000000
[   27.650919] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.651070] CR2: 00005581ebf10cc0 CR3: 0000000102924000 CR4: 00000000003506e0
[   27.651243] Call Trace:
[   27.651298]  _raw_read_unlock_irqrestore+0x4f/0x60
[   27.651392]  led_trigger_event+0x64/0x70
[   27.651470]  ieee80211_led_radio+0x24/0x30 [mac80211]
[   27.651758]  ieee80211_do_open+0x4c4/0x9a0 [mac80211]
[   27.652006]  ieee80211_open+0x69/0x90 [mac80211]
[   27.652334]  __dev_open+0xd6/0x190
[   27.652407]  __dev_change_flags+0x1b2/0x230
[   27.652490]  dev_change_flags+0x21/0x60
[   27.652566]  do_setlink+0x289/0x1190
[   27.652641]  ? __nla_validate_parse+0x60/0xbd0
[   27.652729]  __rtnl_newlink+0x61c/0xa10
[   27.652809]  ? mark_lock+0x25/0x1110
[   27.652886]  ? rcu_read_lock_sched_held+0x41/0x80
[   27.652977]  ? kmem_cache_alloc_trace+0x21e/0x250
[   27.653068]  rtnl_newlink+0x44/0x70
[   27.653139]  rtnetlink_rcv_msg+0x17f/0x4b0
[   27.653257]  ? netlink_deliver_tap+0x10e/0x3f0
[   27.653343]  ? find_held_lock+0x31/0x90
[   27.653419]  ? netlink_deliver_tap+0x10e/0x3f0
[   27.653504]  ? rtnetlink_put_metrics+0x1d0/0x1d0
[   27.653591]  netlink_rcv_skb+0x50/0xf0
[   27.653667]  rtnetlink_rcv+0x10/0x20
[   27.653738]  netlink_unicast+0x1a3/0x270
[   27.653816]  netlink_sendmsg+0x22c/0x430
[   27.653893]  ? netlink_unicast+0x270/0x270
[   27.653973]  ____sys_sendmsg+0x2a6/0x2e0
[   27.654052]  ? import_iovec+0x1b/0x20
[   27.654127]  ? sendmsg_copy_msghdr+0x79/0xa0
[   27.654250]  ___sys_sendmsg+0x82/0xc0
[   27.654332]  __sys_sendmsg+0x62/0xb0
[   27.654407]  ? syscall_enter_from_user_mode+0x21/0x60
[   27.654502]  __x64_sys_sendmsg+0x1a/0x20
[   27.654581]  do_syscall_64+0x38/0x50
[   27.654654]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   27.654748] RIP: 0033:0x7fce74030737
[   27.654819] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
[   27.655111] RSP: 002b:00007ffdd61d1498 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[   27.655284] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fce74030737
[   27.655405] RDX: 0000000000000000 RSI: 00007ffdd61d1500 RDI: 0000000000000003
[   27.655523] RBP: 00000000606d6d4f R08: 0000000000000001 R09: 00007fce740f1a60
[   27.655641] R10: 0000000000000230 R11: 0000000000000246 R12: 0000000000000001
[   27.655759] R13: 00007ffdd61d15d0 R14: 0000000000000000 R15: 00005581ebec6020
[   27.655879] irq event stamp: 95385
[   27.655944] hardirqs last  enabled at (95393): [<ffffffffb84d2270>] console_unlock+0x4a0/0x5c0
[   27.656093] hardirqs last disabled at (95400): [<ffffffffb84d21e3>] console_unlock+0x413/0x5c0
[   27.656275] softirqs last  enabled at (95022): [<ffffffffb846a9ec>] irq_exit_rcu+0xac/0xe0
[   27.656420] softirqs last disabled at (95017): [<ffffffffb846a9ec>] irq_exit_rcu+0xac/0xe0
[   27.656561] ---[ end trace da6e11a6f99dba38 ]---
Kalle Valo April 17, 2021, 8:50 a.m. UTC | #4
Jiri Kosina <jikos@kernel.org> wrote:

> On Wed, 7 Apr 2021, Heiner Kallweit wrote:
> 
> > Same fix as in 2800aadc18a6 ("iwlwifi: Fix softirq/hardirq disabling in
> > iwl_pcie_enqueue_hcmd()") is needed for iwl_pcie_gen2_enqueue_hcmd.
> > I get the same lockdep warning on AX210.
> 
> Makes sense, it's being called from exactly the same contexts.
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] iwlwifi: Fix softirq/hardirq disabling in iwl_pcie_gen2_enqueue_hcmd()
> 
> Analogically to what we did in 2800aadc18a6 ("iwlwifi: Fix 
> softirq/hardirq disabling in iwl_pcie_enqueue_hcmd()"), we must apply the 
> same fix to iwl_pcie_gen2_enqueue_hcmd(), as it's being called from 
> exactly the same contexts.
> 
> Reported-by: Heiner Kallweit <hkallweit1@gmail.com
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> index 4456abb9a074..34bde8c87324 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> @@ -40,6 +40,7 @@ int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
>  	const u8 *cmddata[IWL_MAX_CMD_TBS_PER_TFD];
>  	u16 cmdlen[IWL_MAX_CMD_TBS_PER_TFD];
>  	struct iwl_tfh_tfd *tfd;
> +	unsigned long flags;
>  
>  	copy_size = sizeof(struct iwl_cmd_header_wide);
>  	cmd_size = sizeof(struct iwl_cmd_header_wide);
> @@ -108,14 +109,14 @@ int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
>  		goto free_dup_buf;
>  	}
>  
> -	spin_lock_bh(&txq->lock);
> +	spin_lock_irqsave(&txq->lock, flags);
>  
>  	idx = iwl_txq_get_cmd_index(txq, txq->write_ptr);
>  	tfd = iwl_txq_get_tfd(trans, txq, txq->write_ptr);
>  	memset(tfd, 0, sizeof(*tfd));
>  
>  	if (iwl_txq_space(trans, txq) < ((cmd->flags & CMD_ASYNC) ? 2 : 1)) {
> -		spin_unlock_bh(&txq->lock);
> +		spin_unlock_irqrestore(&txq->lock, flags);
>  
>  		IWL_ERR(trans, "No space in command queue\n");
>  		iwl_op_mode_cmd_queue_full(trans->op_mode);
> @@ -250,7 +251,7 @@ int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
>  	spin_unlock(&trans_pcie->reg_lock);
>  
>  out:
> -	spin_unlock_bh(&txq->lock);
> +	spin_unlock_irqrestore(&txq->lock, flags);
>  free_dup_buf:
>  	if (idx < 0)
>  		kfree(dup_buf);

This is malformed in patchwork, check the link below. Please resend, and
I strongly recommend to use git send-email to avoid any format issues.

Patch set to Changes Requested.
Jiri Kosina April 17, 2021, 9:12 a.m. UTC | #5
On Sat, 17 Apr 2021, Kalle Valo wrote:

> This is malformed in patchwork, check the link below. Please resend, and
> I strongly recommend to use git send-email to avoid any format issues.

Honestly I have no idea what you are talking about, there is no whitespace 
damage nor anything else that I'd see to be broken. I just took the patch 
from the mail I sent, applied with git-am, and it worked flawlessly.

Anyway, I'll send a patch as a followup to this mail so that it could 
hopefully be picked up by your tooling.

Thanks,
Jiri Kosina April 17, 2021, 9:24 a.m. UTC | #6
On Sat, 17 Apr 2021, Jiri Kosina wrote:

> > This is malformed in patchwork, check the link below. Please resend, and
> > I strongly recommend to use git send-email to avoid any format issues.
> 
> Honestly I have no idea what you are talking about, there is no whitespace 
> damage nor anything else that I'd see to be broken. I just took the patch 
> from the mail I sent, applied with git-am, and it worked flawlessly.
> 
> Anyway, I'll send a patch as a followup to this mail so that it could 
> hopefully be picked up by your tooling.

And it seems to have appeared here:

https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm/
Sedat Dilek April 17, 2021, 12:06 p.m. UTC | #7
On Sat, Apr 17, 2021 at 11:25 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Sat, 17 Apr 2021, Jiri Kosina wrote:
>
> > > This is malformed in patchwork, check the link below. Please resend, and
> > > I strongly recommend to use git send-email to avoid any format issues.
> >
> > Honestly I have no idea what you are talking about, there is no whitespace
> > damage nor anything else that I'd see to be broken. I just took the patch
> > from the mail I sent, applied with git-am, and it worked flawlessly.
> >
> > Anyway, I'll send a patch as a followup to this mail so that it could
> > hopefully be picked up by your tooling.
>
> And it seems to have appeared here:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm/
>
> --
> Jiri Kosina
> SUSE Labs
>

Thanks for the patch Jiri.

This is on top of Linux v5.12-rc7+...

link="https://lore.kernel.org/linux-wireless/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm"

$ b4 -d am $link

$ LC_ALL=C git apply --check --verbose
../v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
Checking patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c...

$ LC_ALL=C git apply --verbose
../v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
Checking patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c...
Applied patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c cleanly.

- Sedat -
Kalle Valo April 18, 2021, 6:46 a.m. UTC | #8
Jiri Kosina <jikos@kernel.org> writes:

> On Sat, 17 Apr 2021, Kalle Valo wrote:
>
>> This is malformed in patchwork, check the link below. Please resend, and
>> I strongly recommend to use git send-email to avoid any format issues.
>
> Honestly I have no idea what you are talking about, there is no whitespace 
> damage nor anything else that I'd see to be broken. I just took the patch 
> from the mail I sent, applied with git-am, and it worked flawlessly.

Compare these two links:

https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104070918090.12405@cbobk.fhfr.pm/

https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm/

In v1 there's email discussion in the commit log which shouldn't be
there.
Sedat Dilek April 18, 2021, 6:47 a.m. UTC | #9
On Sat, Apr 17, 2021 at 2:06 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Sat, Apr 17, 2021 at 11:25 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Sat, 17 Apr 2021, Jiri Kosina wrote:
> >
> > > > This is malformed in patchwork, check the link below. Please resend, and
> > > > I strongly recommend to use git send-email to avoid any format issues.
> > >
> > > Honestly I have no idea what you are talking about, there is no whitespace
> > > damage nor anything else that I'd see to be broken. I just took the patch
> > > from the mail I sent, applied with git-am, and it worked flawlessly.
> > >
> > > Anyway, I'll send a patch as a followup to this mail so that it could
> > > hopefully be picked up by your tooling.
> >
> > And it seems to have appeared here:
> >
> > https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm/
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
>
> Thanks for the patch Jiri.
>
> This is on top of Linux v5.12-rc7+...
>
> link="https://lore.kernel.org/linux-wireless/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm"
>
> $ b4 -d am $link
>
> $ LC_ALL=C git apply --check --verbose
> ../v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
> Checking patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c...
>
> $ LC_ALL=C git apply --verbose
> ../v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
> Checking patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c...
> Applied patch drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c cleanly.
>


Small nit:

Reported-by: Heiner Kallweit <hkallweit1@gmail.com

Here misses closing ">" in the email-address of Heiner.

Feel free to add my:

Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # LLVM/Clang v12.0.0 (x86-64)

- Sedat -
Sedat Dilek April 18, 2021, 7:07 a.m. UTC | #10
On Sun, Apr 18, 2021 at 8:47 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Jiri Kosina <jikos@kernel.org> writes:
>
> > On Sat, 17 Apr 2021, Kalle Valo wrote:
> >
> >> This is malformed in patchwork, check the link below. Please resend, and
> >> I strongly recommend to use git send-email to avoid any format issues.
> >
> > Honestly I have no idea what you are talking about, there is no whitespace
> > damage nor anything else that I'd see to be broken. I just took the patch
> > from the mail I sent, applied with git-am, and it worked flawlessly.
>
> Compare these two links:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104070918090.12405@cbobk.fhfr.pm/
>
> https://patchwork.kernel.org/project/linux-wireless/patch/nycvar.YFH.7.76.2104171112390.18270@cbobk.fhfr.pm/
>

v2 should have this diff:

$ git diff v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
v2_20210417_jikos_iwlwifi_fix_softi
rq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd-dileks.mbx
diff --git a/v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
b/v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl
_pcie_gen2_enqueue_hcmd-dileks.mbx
index 6d250b75305e..63695ce63065 100644
--- a/v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd.mbx
+++ b/v2_20210417_jikos_iwlwifi_fix_softirq_hardirq_disabling_in_iwl_pcie_gen2_enqueue_hcmd-dileks.mbx
@@ -20,9 +20,7 @@ disabling in iwl_pcie_enqueue_hcmd()"), we must
apply the same fix to
iwl_pcie_gen2_enqueue_hcmd(), as it's being called from exactly the same
contexts.

----
-
-Reported-by: Heiner Kallweit <hkallweit1@gmail.com
+Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c

Otherwise Reported-by and S-o-b is dropped when applying to my local
Git because of "---" in v2.
Closing ">" misses in Heiners Reported-by.

Jiri, can you resend a v3?

- Sedat -

> In v1 there's email discussion in the commit log which shouldn't be
> there.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
index 4456abb9a074..34bde8c87324 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
@@ -40,6 +40,7 @@  int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
 	const u8 *cmddata[IWL_MAX_CMD_TBS_PER_TFD];
 	u16 cmdlen[IWL_MAX_CMD_TBS_PER_TFD];
 	struct iwl_tfh_tfd *tfd;
+	unsigned long flags;
 
 	copy_size = sizeof(struct iwl_cmd_header_wide);
 	cmd_size = sizeof(struct iwl_cmd_header_wide);
@@ -108,14 +109,14 @@  int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
 		goto free_dup_buf;
 	}
 
-	spin_lock_bh(&txq->lock);
+	spin_lock_irqsave(&txq->lock, flags);
 
 	idx = iwl_txq_get_cmd_index(txq, txq->write_ptr);
 	tfd = iwl_txq_get_tfd(trans, txq, txq->write_ptr);
 	memset(tfd, 0, sizeof(*tfd));
 
 	if (iwl_txq_space(trans, txq) < ((cmd->flags & CMD_ASYNC) ? 2 : 1)) {
-		spin_unlock_bh(&txq->lock);
+		spin_unlock_irqrestore(&txq->lock, flags);
 
 		IWL_ERR(trans, "No space in command queue\n");
 		iwl_op_mode_cmd_queue_full(trans->op_mode);
@@ -250,7 +251,7 @@  int iwl_pcie_gen2_enqueue_hcmd(struct iwl_trans *trans,
 	spin_unlock(&trans_pcie->reg_lock);
 
 out:
-	spin_unlock_bh(&txq->lock);
+	spin_unlock_irqrestore(&txq->lock, flags);
 free_dup_buf:
 	if (idx < 0)
 		kfree(dup_buf);