Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
On Wed, Mar 3, 2021 at 1:38 AM Kalle Valo wrote:
>
> "Coelho, Luciano" writes:
>
> > On Tue, 2021-03-02 at 11:34 +0100, Jiri Kosina wrote:
> >> From: Jiri Kosina
> >>
> >> 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-2-gbada49429032 #5 Not tainted
> >>
> >> irq/136-iwlwifi/565 just changed the state of lock:
> >> 89f28433b0b0 (>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:
> >>
> >> CPU0CPU1
> >>
> >>lock(napi_hash_lock);
> >> local_irq_disable();
> >> lock(>lock);
> >> lock(napi_hash_lock);
> >>
> >> lock(>lock);
> >>
> >> *** DEADLOCK ***
> >>
> >> 1 lock held by irq/136-iwlwifi/565:
> >> #0: 89f2b1440170 (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
> >>}
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
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.12...@cbobk.fhfr.pm/ Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
Jiri Kosina 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? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
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. -- Jiri Kosina SUSE Labs
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
Jiri Kosina wrote:
> From: Jiri Kosina
>
> 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-2-gbada49429032 #5 Not tainted
>
> irq/136-iwlwifi/565 just changed the state of lock:
> 89f28433b0b0 (>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:
>
> CPU0CPU1
>
>lock(napi_hash_lock);
> local_irq_disable();
> lock(>lock);
> lock(napi_hash_lock);
>
> lock(>lock);
>
> *** DEADLOCK ***
>
> 1 lock held by irq/136-iwlwifi/565:
> #0: 89f2b1440170 (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: [] 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]
>
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
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 > > 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.12...@cbobk.fhfr.pm/ Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
"Coelho, Luciano" writes:
> On Tue, 2021-03-02 at 11:34 +0100, Jiri Kosina wrote:
>> From: Jiri Kosina
>>
>> 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-2-gbada49429032 #5 Not tainted
>>
>> irq/136-iwlwifi/565 just changed the state of lock:
>> 89f28433b0b0 (>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:
>>
>> CPU0CPU1
>>
>> lock(napi_hash_lock);
>> local_irq_disable();
>> lock(>lock);
>> lock(napi_hash_lock);
>>
>> lock(>lock);
>>
>> *** DEADLOCK ***
>>
>> 1 lock held by irq/136-iwlwifi/565:
>> #0: 89f2b1440170 (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: [] 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]
>>
Re: [PATCH v2] iwlwifi: don't call netif_napi_add() with rxq->lock held (was Re: Lockdep warning in iwl_pcie_rx_handle())
On Tue, 2021-03-02 at 11:34 +0100, Jiri Kosina wrote:
> From: Jiri Kosina
>
> 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-2-gbada49429032 #5 Not tainted
>
> irq/136-iwlwifi/565 just changed the state of lock:
> 89f28433b0b0 (>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:
>
> CPU0CPU1
>
> lock(napi_hash_lock);
> local_irq_disable();
> lock(>lock);
> lock(napi_hash_lock);
>
> lock(>lock);
>
> *** DEADLOCK ***
>
> 1 lock held by irq/136-iwlwifi/565:
> #0: 89f2b1440170 (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: [] 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
