Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())

Kalle Valo
2021-03-02T16:51:35Z

"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.

--