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

Message ID nycvar.YFH.7.76.2103021134060.12405@cbobk.fhfr.pm
State Accepted
Commit 295d4cd82b0181dd36b145fd535c13d623d7a335
Headers show
Series
  • [v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
Related show

Commit Message

Jiri Kosina March 2, 2021, 10:34 a.m. UTC
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(-)

Comments

Coelho, Luciano March 2, 2021, 10:49 a.m. UTC | #1
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>

--
Cheers,
Luca.
Kalle Valo March 2, 2021, 12:17 p.m. UTC | #2
"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.
Jiri Kosina March 2, 2021, 2:55 p.m. UTC | #3
On Tue, 2 Mar 2021, Kalle Valo wrote:

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

Not sure about patchwork, but vger had hiccup (again) earlier today, 
everything depending on the ML traffic is probably slower.

lore has it now though: 

	https://lore.kernel.org/lkml/nycvar.YFH.7.76.2103021134060.12405@cbobk.fhfr.pm/

Thanks,
Kalle Valo March 3, 2021, 3:59 p.m. UTC | #4
Jiri Kosina <jikos@kernel.org> 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>
> Acked-by: Luca Coelho <luciano.coelho@intel.com>

Patch applied to wireless-drivers.git, thanks.

295d4cd82b01 iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
Jiri Kosina March 3, 2021, 6:01 p.m. UTC | #5
On Wed, 3 Mar 2021, Kalle Valo wrote:

> Patch applied to wireless-drivers.git, thanks.

Thanks, but ...

> 295d4cd82b01 iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())

... i believe you want to drop the "(was ...") part from the patch 
subject.
Kalle Valo March 3, 2021, 6:33 p.m. UTC | #6
Jiri Kosina <jikos@kernel.org> writes:

> On Wed, 3 Mar 2021, Kalle Valo wrote:
>
>> Patch applied to wireless-drivers.git, thanks.
>
> Thanks, but ...
>
>> 295d4cd82b01 iwlwifi: don't call netif_napi_add() with rxq->lock
>> held (was Re: Lockdep warning in iwl_pcie_rx_handle())
>
> ... i believe you want to drop the "(was ...") part from the patch 
> subject.

Too late now, it's already applied and pull request sent. Why was it
there in the first place?
Jiri Kosina March 3, 2021, 7:19 p.m. UTC | #7
On Wed, 3 Mar 2021, Kalle Valo wrote:

> > ... i believe you want to drop the "(was ...") part from the patch 
> > subject.
> 
> Too late now, it's already applied and pull request sent. Why was it
> there in the first place?

Yeah, it was, but I don't think it's a big issue :) So let it be.

BTW, how about the other fix I sent? It's also fixing a real functional 
issue, so it IMHO is a -rc material

	https://lore.kernel.org/linux-wireless/nycvar.YFH.7.76.2103021125430.12405@cbobk.fhfr.pm/

Thanks,
Sedat Dilek March 3, 2021, 9:39 p.m. UTC | #8
On Wed, Mar 3, 2021 at 1:38 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> "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.
>

Is that intended to have a subject like...?

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

- Sedat -

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=295d4cd82b0181dd36b145fd535c13d623d7a335

Patch
diff mbox series

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
index 42426e25cac6..2bec97133119 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c
@@ -1129,6 +1129,8 @@  static int _iwl_pcie_rx_init(struct iwl_trans *trans)
 
 		iwl_pcie_rx_init_rxb_lists(rxq);
 
+		spin_unlock_bh(&rxq->lock);
+
 		if (!rxq->napi.poll) {
 			int (*poll)(struct napi_struct *, int) = iwl_pcie_napi_poll;
 
@@ -1149,7 +1151,6 @@  static int _iwl_pcie_rx_init(struct iwl_trans *trans)
 			napi_enable(&rxq->napi);
 		}
 
-		spin_unlock_bh(&rxq->lock);
 	}
 
 	/* move the pool to the default queue and allocator ownerships */