From: Kalle Valo <kvalo@codeaurora.org>
To: "Coelho\, Luciano" <luciano.coelho@intel.com>
Cc: "jikos\@kernel.org" <jikos@kernel.org>,
"johannes\@sipsolutions.net" <johannes@sipsolutions.net>,
"linux-wireless\@vger.kernel.org"
<linux-wireless@vger.kernel.org>,
"hkallweit1\@gmail.com" <hkallweit1@gmail.com>,
"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
Date: Tue, 02 Mar 2021 14:17:16 +0200 [thread overview]
Message-ID: <87blc192cz.fsf@codeaurora.org> (raw)
In-Reply-To: <7de7aa19e5ea8a856cce4193ebb3c31b4ef78ca7.camel@intel.com> (Luciano Coelho's message of "Tue, 2 Mar 2021 10:49:51 +0000")
"Coelho, Luciano" <luciano.coelho@intel.com> writes:
> On Tue, 2021-03-02 at 11:34 +0100, Jiri Kosina wrote:
>> From: Jiri Kosina <jkosina@suse.cz>
>>
>> We can't call netif_napi_add() with rxq-lock held, as there is a potential
>> for deadlock as spotted by lockdep (see below). rxq->lock is not
>> protecting anything over the netif_napi_add() codepath anyway, so let's
>> drop it just before calling into NAPI.
>>
>> ========================================================
>> WARNING: possible irq lock inversion dependency detected
>> 5.12.0-rc1-00002-gbada49429032 #5 Not tainted
>> --------------------------------------------------------
>> irq/136-iwlwifi/565 just changed the state of lock:
>> ffff89f28433b0b0 (&rxq->lock){+.-.}-{2:2}, at:
>> iwl_pcie_rx_handle+0x7f/0x960 [iwlwifi]
>> but this lock took another, SOFTIRQ-unsafe lock in the past:
>> (napi_hash_lock){+.+.}-{2:2}
>>
>> 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(napi_hash_lock);
>> local_irq_disable();
>> lock(&rxq->lock);
>> lock(napi_hash_lock);
>> <Interrupt>
>> lock(&rxq->lock);
>>
>> *** DEADLOCK ***
>>
>> 1 lock held by irq/136-iwlwifi/565:
>> #0: ffff89f2b1440170 (sync_cmd_lockdep_map){+.+.}-{0:0}, at:
>> iwl_pcie_irq_handler+0x5/0xb30
>>
>> the shortest dependencies between 2nd lock and 1st lock:
>> -> (napi_hash_lock){+.+.}-{2:2} {
>> HARDIRQ-ON-W at:
>> lock_acquire+0x277/0x3d0
>> _raw_spin_lock+0x2c/0x40
>> netif_napi_add+0x14b/0x270
>> e1000_probe+0x2fe/0xee0 [e1000e]
>> local_pci_probe+0x42/0x90
>> pci_device_probe+0x10b/0x1c0
>> really_probe+0xef/0x4b0
>> driver_probe_device+0xde/0x150
>> device_driver_attach+0x4f/0x60
>> __driver_attach+0x9c/0x140
>> bus_for_each_dev+0x79/0xc0
>> bus_add_driver+0x18d/0x220
>> driver_register+0x5b/0xf0
>> do_one_initcall+0x5b/0x300
>> do_init_module+0x5b/0x21c
>> load_module+0x1dae/0x22c0
>> __do_sys_finit_module+0xad/0x110
>> do_syscall_64+0x33/0x80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>> SOFTIRQ-ON-W at:
>> lock_acquire+0x277/0x3d0
>> _raw_spin_lock+0x2c/0x40
>> netif_napi_add+0x14b/0x270
>> e1000_probe+0x2fe/0xee0 [e1000e]
>> local_pci_probe+0x42/0x90
>> pci_device_probe+0x10b/0x1c0
>> really_probe+0xef/0x4b0
>> driver_probe_device+0xde/0x150
>> device_driver_attach+0x4f/0x60
>> __driver_attach+0x9c/0x140
>> bus_for_each_dev+0x79/0xc0
>> bus_add_driver+0x18d/0x220
>> driver_register+0x5b/0xf0
>> do_one_initcall+0x5b/0x300
>> do_init_module+0x5b/0x21c
>> load_module+0x1dae/0x22c0
>> __do_sys_finit_module+0xad/0x110
>> do_syscall_64+0x33/0x80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>> INITIAL USE at:
>> lock_acquire+0x277/0x3d0
>> _raw_spin_lock+0x2c/0x40
>> netif_napi_add+0x14b/0x270
>> e1000_probe+0x2fe/0xee0 [e1000e]
>> local_pci_probe+0x42/0x90
>> pci_device_probe+0x10b/0x1c0
>> really_probe+0xef/0x4b0
>> driver_probe_device+0xde/0x150
>> device_driver_attach+0x4f/0x60
>> __driver_attach+0x9c/0x140
>> bus_for_each_dev+0x79/0xc0
>> bus_add_driver+0x18d/0x220
>> driver_register+0x5b/0xf0
>> do_one_initcall+0x5b/0x300
>> do_init_module+0x5b/0x21c
>> load_module+0x1dae/0x22c0
>> __do_sys_finit_module+0xad/0x110
>> do_syscall_64+0x33/0x80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>> }
>> ... key at: [<ffffffffae84ef38>] napi_hash_lock+0x18/0x40
>> ... acquired at:
>> _raw_spin_lock+0x2c/0x40
>> netif_napi_add+0x14b/0x270
>> _iwl_pcie_rx_init+0x1f4/0x710 [iwlwifi]
>> iwl_pcie_rx_init+0x1b/0x3b0 [iwlwifi]
>> iwl_trans_pcie_start_fw+0x2ac/0x6a0 [iwlwifi]
>> iwl_mvm_load_ucode_wait_alive+0x116/0x460 [iwlmvm]
>> iwl_run_init_mvm_ucode+0xa4/0x3a0 [iwlmvm]
>> iwl_op_mode_mvm_start+0x9ed/0xbf0 [iwlmvm]
>> _iwl_op_mode_start.isra.4+0x42/0x80 [iwlwifi]
>> iwl_opmode_register+0x71/0xe0 [iwlwifi]
>> iwl_mvm_init+0x34/0x1000 [iwlmvm]
>> do_one_initcall+0x5b/0x300
>> do_init_module+0x5b/0x21c
>> load_module+0x1dae/0x22c0
>> __do_sys_finit_module+0xad/0x110
>> do_syscall_64+0x33/0x80
>> entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> [ ... lockdep output trimmed .... ]
>>
>> Fixes: 25edc8f259c7106 ("iwlwifi: pcie: properly implement NAPI")
>> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>> ---
>>
>> v1->v2: Previous patch was not refreshed against current code-base, sorry.
>>
>> drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>
> Thanks, Jiri! Let's take your patch since you already sent it out.
>
> Kalle, can you please take this directly to wireless-drivers.git?
>
> Acked-by: Luca Coelho <luciano.coelho@intel.com>
Ok but I don't see this either in patchwork or lore, hopefully it shows
up later.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
next prev parent reply other threads:[~2021-03-02 16:51 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-01 20:44 Lockdep warning in iwl_pcie_rx_handle() Jiri Kosina
2021-03-01 21:09 ` Jiri Kosina
2021-03-01 21:12 ` Johannes Berg
2021-03-02 9:27 ` [PATCH] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle()) Jiri Kosina
2021-03-02 10:34 ` [PATCH v2] " Jiri Kosina
2021-03-02 10:49 ` Coelho, Luciano
2021-03-02 12:17 ` Kalle Valo [this message]
2021-03-02 14:55 ` Jiri Kosina
2021-03-03 21:39 ` Sedat Dilek
2021-03-03 15:59 ` Kalle Valo
[not found] ` <20210303155941.25521C43463@smtp.codeaurora.org>
2021-03-03 18:01 ` Jiri Kosina
2021-03-03 18:33 ` Kalle Valo
2021-03-03 19:19 ` Jiri Kosina
2021-03-02 10:48 ` [PATCH] " Coelho, Luciano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87blc192cz.fsf@codeaurora.org \
--to=kvalo@codeaurora.org \
--cc=hkallweit1@gmail.com \
--cc=jikos@kernel.org \
--cc=johannes@sipsolutions.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=luciano.coelho@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).