irqdomain: Protect the linear revmap with RCU
Subsystems (5)
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00
irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
--- include/linux/irqdomain.h | 5 ++-- kernel/irq/irqdomain.c | 49 +++++++++++++++++--------------------- 2 files changed, 26 insertions(+), 28 deletions(-)
include/linux/irqdomain.h +3 -2
@@ -151,6 +151,7 @@ struct irq_domain_chip_generic;* Revmap data, used internally by irq_domain * @revmap_size: Size of the linear map table @revmap[] * @revmap_tree: Radix map tree for hwirqs that don't fit in the linear map +* @revmap_mutex: Lock for the revmap * @revmap: Linear table of irq_data pointers */ struct irq_domain {
@@ -173,8 +174,8 @@ struct irq_domain {irq_hw_number_t hwirq_max;unsigned int revmap_size;struct radix_tree_root revmap_tree; -struct mutex revmap_tree_mutex; -struct irq_data *revmap[]; +struct mutex revmap_mutex; +struct irq_data __rcu *revmap[]; };/* Irq domain flags */
kernel/irq/irqdomain.c +23 -26
86 lines not on this page · raw message
Hi,
Quoted 15 lines · irqchip-bot for Marc Zyngier
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Quoted 7 lines
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
Signed-off-by Marc Zyngier <[email protected]>
This patch results in various RCU warnings when booting mipsel images in qemu. I can not revert the patch due to subsequent changes, so I don't know if a simple revert fixes the problem. Log messages and bisect log see below.
Guenter
---
...
=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------include/trace/events/lock.h:13 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
no locks held by swapper/0/0.
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000000 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
8209fe9c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209fe38 820fcec0
00000000 00000000 80ecfc64 8209fcc0 fffffbff 00000148 00000000 00000000
00000000 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 00000000
80fb0000 80faea08 00000000 80fb1858 00000000 00000000 00000000 81100000
...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801a4164>] lock_acquire+0x418/0x498
[<801c0d80>] __irq_resolve_mapping+0x60/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------include/linux/rcupdate.h:688 rcu_read_lock() used illegally while idle!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
#0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
00000000 00000000 80ecfc64 8209fd40 fffffbff 00000169 00000000 00000000
81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0ee8>] __irq_resolve_mapping+0x1c8/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------kernel/irq/irqdomain.c:920 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
#0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
00000000 00000000 80ecfc64 8209fd40 fffffbff 00000189 00000000 00000000
81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0e68>] __irq_resolve_mapping+0x148/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------include/linux/rcupdate.h:716 rcu_read_unlock() used illegally while idle!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
#0: 80fd43cc (rcu_read_lock){....}-{1:2}, at: __irq_resolve_mapping+0x28/0x1f0
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 00000080 00000000 00000047 801b2c84 80cd0000 00000004 00000000 00000000
8209ff1c 3bdd5c9d 80fb0000 810f9930 80fb0000 00000001 8209feb8 820fcec0
00000000 00000000 80ecfc64 8209fd40 fffffbff 000001a9 00000000 00000000
81349a50 00000064 00000002 80fb0000 80fb0000 00000001 80ecfc64 80fd0000
81730000 80fb0000 00000080 80fb1858 00000000 00000000 00000000 81100000
...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801c0ea8>] __irq_resolve_mapping+0x188/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8=============================
WARNING: suspicious RCU usage
5.13.0-09882-ga180bd1d7e16 #1 Not tainted
-----------------------------include/trace/events/lock.h:58 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
1 lock held by swapper/0/0:
#0: 80fd3c4c ((console_sem).lock){-...}-{2:2}, at: down_trylock+0x18/0x4c
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.13.0-09882-ga180bd1d7e16 #1
Stack : 817924dc 8209fcfc 817924f0 00001fec 80ee11b8 80fb0000 81061e40 8071e768
00000000 3bdd5c9d 80fb0000 810f9930 000001ec 00000001 8209fc98 820fcec0
00000000 00000000 80ecfc64 00000001 0000000a 00000003 00000001 6d6f4320
00000000 817926e9 817944dc 00000001 80fb0000 00000001 80ecfc64 81061e20
80faea08 80e92708 00000000 80fb0000 00000000 00000000 00000000 81100000
...
Call Trace:
[<8010a2ec>] show_stack+0x38/0x118
[<80c490ac>] dump_stack_lvl+0xac/0x104
[<801a834c>] lock_release+0x3d0/0x46c
[<80c6080c>] _raw_spin_unlock_irqrestore+0x24/0xa0
[<80198854>] down_trylock+0x34/0x4c
[<801aef18>] __down_trylock_console_sem+0x44/0x120
[<801af370>] console_trylock+0x1c/0x88
[<801b2ccc>] vprintk_emit+0x124/0x404
[<801b2fcc>] vprintk_default+0x20/0x2c
[<80c3e6ac>] printk+0x34/0x58
[<80c3d884>] lockdep_rcu_suspicious+0x40/0x114
[<801a4164>] lock_acquire+0x418/0x498
[<801c0d80>] __irq_resolve_mapping+0x60/0x1f0
[<807275a8>] plat_irq_dispatch+0x84/0x114
[<801038c0>] handle_int+0x160/0x16c
[<80103720>] __r4k_wait+0x20/0x40
[<80c600a4>] default_idle_call+0x9c/0x2e0
[<80173024>] do_idle+0x10c/0x1cc
[<801734b8>] cpu_startup_entry+0x24/0x3c
[<8106cf54>] start_kernel+0x790/0x7d8
[<80c55f50>] kernel_entry+0x0/0xa8---
bisect log:
# bad: [007b350a58754a93ca9fe50c498cc27780171153] Merge tag 'dlm-5.14' of git://git.kernel.org/pub/scm/linux/kernel/git/teigland/linux-dlm
# good: [62fb9874f5da54fdb243003b386128037319b219] Linux 5.13
git bisect start '007b350a5875' '62fb9874f5da'
# good: [36824f198c621cebeb22966b5e244378fa341295] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
git bisect good 36824f198c621cebeb22966b5e244378fa341295
# good: [122fa8c588316aacafe7e5a393bb3e875eaf5b25] Merge tag 'for-5.14-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
git bisect good 122fa8c588316aacafe7e5a393bb3e875eaf5b25
# bad: [3563f55ce65462063543dfa6a8d8c7fbfb9d7772] Merge tag 'pm-5.14-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect bad 3563f55ce65462063543dfa6a8d8c7fbfb9d7772
# good: [c54b245d011855ea91c5beff07f1db74143ce614] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace
git bisect good c54b245d011855ea91c5beff07f1db74143ce614
# bad: [21edf50948728f55b685ad95f196ba46196eb767] Merge tag 'irq-core-2021-06-29' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect bad 21edf50948728f55b685ad95f196ba46196eb767
# bad: [c64638d5091a5630d0f5f5ab7001bdee1ad194f2] Merge branch irq/generic_handle_domain_irq-core into irq/irqchip-next
git bisect bad c64638d5091a5630d0f5f5ab7001bdee1ad194f2
# bad: [d4a45c68dc81f9117ceaff9f058d5fae674181b9] irqdomain: Protect the linear revmap with RCU
git bisect bad d4a45c68dc81f9117ceaff9f058d5fae674181b9
# good: [13a9a5d17d07cec8181ea0843674ce48c191628e] powerpc: Add missing linux/{of.h,irqdomain.h} include directives
git bisect good 13a9a5d17d07cec8181ea0843674ce48c191628e
# good: [1da027362a7db422243601e895e6f8288389f435] irqdomain: Reimplement irq_linear_revmap() with irq_find_mapping()
git bisect good 1da027362a7db422243601e895e6f8288389f435
# good: [4f86a06e2d6ece5316e4c42fbf946ee22acb30f3] irqdomain: Make normal and nomap irqdomains exclusive
git bisect good 4f86a06e2d6ece5316e4c42fbf946ee22acb30f3
# good: [48b15a7921d60680babe59f64e127816585a585c] irqdomain: Cache irq_data instead of a virq number in the revmap
git bisect good 48b15a7921d60680babe59f64e127816585a585c
# first bad commit: [d4a45c68dc81f9117ceaff9f058d5fae674181b9] irqdomain: Protect the linear revmap with RCU
Hi Guenter,
Quoted 25 lines · Guenter Roeck
Hi,
Quoted 21 lines · irqchip-bot for Marc Zyngier
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
Quoted 6 lines
Quoted 1 line
Signed-off-by Marc Zyngier <[email protected]>This patch results in various RCU warnings when booting mipsel images in qemu. I can not revert the patch due to subsequent changes, so I don't know if a simple revert fixes the problem. Log messages and bisect log see below.
Thanks for the heads up. Do you have a config file I can use to reproduce this? The QEMU invocation runes would certainly help too.
It strikes me that in drivers/irqchip/irq-mips-cpu.c, plat_irq_dispatch() now uses the irqdomain resolution before irq_enter() took place. That's certainly a latent bug. I'll fix that regardless, but I'd like to make sure this is what you are seeing too.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Hi Marc,
Quoted 36 lines · Marc Zyngier
Hi Guenter,
Quoted 25 lines · Guenter Roeck
Hi,
Quoted 21 lines · irqchip-bot for Marc Zyngier
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
Quoted 6 lines
Quoted 1 line
Signed-off-by Marc Zyngier <[email protected]>This patch results in various RCU warnings when booting mipsel images in qemu. I can not revert the patch due to subsequent changes, so I don't know if a simple revert fixes the problem. Log messages and bisect log see below.
Quoted 8 lines
Thanks for the heads up. Do you have a config file I can use to reproduce this? The QEMU invocation runes would certainly help too.
It strikes me that in
drivers/irqchip/irq-mips-cpu.c,plat_irq_dispatch()now uses the irqdomain resolution beforeirq_enter()took place. That's certainly a latent bug. I'll fix that regardless, but I'd like to make sure this is what you are seeing too.
See http://server.roeck-us.net/qemu/mipsel/
config Complete configuration file
defconfig Shortened configuration file
rootfs.cpio root file system (initrd)run.sh qemu run script (tested with qemu 4.2.1 and 6.0.0)
vmlinux Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)
Hope this helps,
Guenter
Quoted 51 lines · Guenter Roeck
Hi Marc,
Quoted 36 lines · Marc Zyngier
Hi Guenter,
Quoted 25 lines · Guenter Roeck
Hi,
Quoted 21 lines · irqchip-bot for Marc Zyngier
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
Quoted 6 lines
Quoted 1 line
Signed-off-by Marc Zyngier <[email protected]>This patch results in various RCU warnings when booting mipsel images in qemu. I can not revert the patch due to subsequent changes, so I don't know if a simple revert fixes the problem. Log messages and bisect log see below.
Quoted 8 lines
Thanks for the heads up. Do you have a config file I can use to reproduce this? The QEMU invocation runes would certainly help too.
It strikes me that in
drivers/irqchip/irq-mips-cpu.c,plat_irq_dispatch()now uses the irqdomain resolution beforeirq_enter()took place. That's certainly a latent bug. I'll fix that regardless, but I'd like to make sure this is what you are seeing too.
Quoted 7 lines
config Complete configuration file
defconfig Shortened configuration file
rootfs.cpio root file system (initrd)run.shqemu run script (tested with qemu 4.2.1 and 6.0.0)
vmlinux Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)Hope this helps,
It definitely helps, and confirms my hunch. With the patch below, I'm not getting the warnings anymore. I'm pretty sure a number of other MIPS systems suffer from similar issues, which I'll address similarly.
Please let me know if that addresses the issue on your end.
Thanks,
M.
arch/mips/include/asm/irq.h +3 -0
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);extern void do_IRQ (unsigned int irq); +struct irq_domain ; +extern void do_domain_IRQ (struct irq_domain *domain,unsigned int irq); +extern void arch_init_irq (void );extern void spurious_interrupt (void );
arch/mips/kernel/irq.c +14 -0
@@ -21,6 +21,7 @@#include <linux/kallsyms.h> #include <linux/kgdb.h> #include <linux/ftrace.h> +#include <linux/irqdomain.h> #include <linux/atomic.h> #include <linux/uaccess.h>
@@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)irq_exit (); } +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) +{ +struct irq_desc *desc; + +irq_enter (); +check_stack_overflow (); + + desc =irq_resolve_mapping (domain, hwirq); +if (likely (desc)) +handle_irq_desc (desc); + +irq_exit (); +}
drivers/irqchip/irq-mips-cpu.c +6 -4
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { asmlinkage void __weak plat_irq_dispatch(void) {unsigned long pending =read_c0_cause () &read_c0_status () &ST0_IM ; -unsigned int virq;int irq;if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) pending >>=CAUSEB_IP ;while (pending) { +struct irq_domain *d; + irq =fls (pending) -1 ;if (IS_ENABLED (CONFIG_GENERIC_IRQ_IPI ) && irq <2 ) - virq =irq_linear_revmap (ipi_domain, irq); + d = ipi_domain;else - virq =irq_linear_revmap (irq_domain, irq); -do_IRQ (virq); + d = irq_domain; + +do_domain_IRQ (d, irq); pending &= ~BIT (irq); } }
Signature · 1 line
Without deviation from the norm, progress is not possible.
Guenter Roeck
Tested-byQuoted 59 lines · Marc Zyngier
Quoted 57 lines · Guenter Roeck
Hi Marc,
Quoted 36 lines · Marc Zyngier
Hi Guenter,
Quoted 25 lines · Guenter Roeck
Hi,
Quoted 21 lines · irqchip-bot for Marc Zyngier
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: d4a45c68dc81f9117ceaff9f058d5fae674181b9 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d4a45c68dc81f9117ceaff9f058d5fae674181b9 Author: Marc Zyngier <[email protected]> AuthorDate: Mon, 05 Apr 2021 12:57:27 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Thu, 10 Jun 2021 13:09:18 +01:00irqdomain: Protect the linear revmap with RCU
It is pretty odd that the radix tree uses RCU while the linear portion doesn't, leading to potential surprises for the users, depending on how the irqdomain has been created.
Fix this by moving the update of the linear revmap under the mutex, and the lookup under the RCU read-side lock.
The mutex name is updated to reflect that it doesn't only cover the radix-tree anymore.
Quoted 6 lines
Quoted 1 line
Signed-off-by Marc Zyngier <[email protected]>This patch results in various RCU warnings when booting mipsel images in qemu. I can not revert the patch due to subsequent changes, so I don't know if a simple revert fixes the problem. Log messages and bisect log see below.
Quoted 8 lines
Thanks for the heads up. Do you have a config file I can use to reproduce this? The QEMU invocation runes would certainly help too.
It strikes me that in
drivers/irqchip/irq-mips-cpu.c,plat_irq_dispatch()now uses the irqdomain resolution beforeirq_enter()took place. That's certainly a latent bug. I'll fix that regardless, but I'd like to make sure this is what you are seeing too.See http://server.roeck-us.net/qemu/mipsel/
config Complete configuration file
defconfig Shortened configuration file
rootfs.cpio root file system (initrd)run.shqemu run script (tested with qemu 4.2.1 and 6.0.0)
vmlinux Kernel image experiencing the problem (v5.13-9883-gaf9efb8b661)
Quoted 8 lines
Quoted 1 line
Hope this helps,
It definitely helps, and confirms my hunch. With the patch below, I'm not getting the warnings anymore. I'm pretty sure a number of other MIPS systems suffer from similar issues, which I'll address similarly.
Please let me know if that addresses the issue on your end.
Yes, it does. Feel free to add
to the real patch.
Now the big question: Why does this only affect 32-bit little endian mips images, but not the matching big endian images, nor 64-bit images ?
Thanks,
Guenter
Quoted 79 lines
Thanks,
M.arch/mips/include/asm/irq.h +3
-0@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);extern void do_IRQ (unsigned int irq); +struct irq_domain ; +extern void do_domain_IRQ (struct irq_domain *domain,unsigned int irq); +extern void arch_init_irq (void );extern void spurious_interrupt (void );arch/mips/kernel/irq.c +14
-0@@ -21,6 +21,7 @@#include <linux/kallsyms.h> #include <linux/kgdb.h> #include <linux/ftrace.h> +#include <linux/irqdomain.h> #include <linux/atomic.h> #include <linux/uaccess.h> @@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)irq_exit (); } +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) +{ +struct irq_desc *desc; + +irq_enter (); +check_stack_overflow (); + + desc =irq_resolve_mapping (domain, hwirq); +if (likely (desc)) +handle_irq_desc (desc); + +irq_exit (); +}drivers/irqchip/irq-mips-cpu.c +6
-4@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { asmlinkage void __weak plat_irq_dispatch(void) {unsigned long pending =read_c0_cause () &read_c0_status () &ST0_IM ; -unsigned int virq;int irq;if (!pending) {@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) pending >>=CAUSEB_IP ;while (pending) { +struct irq_domain *d; + irq =fls (pending) -1 ;if (IS_ENABLED (CONFIG_GENERIC_IRQ_IPI ) && irq <2 ) - virq =irq_linear_revmap (ipi_domain, irq); + d = ipi_domain;else - virq =irq_linear_revmap (irq_domain, irq); -do_IRQ (virq); + d = irq_domain; + +do_domain_IRQ (d, irq); pending &= ~BIT (irq); } }
Quoted 7 lines · Guenter Roeck
Quoted 4 lines · Marc Zyngier
It definitely helps, and confirms my hunch. With the patch below, I'm not getting the warnings anymore. I'm pretty sure a number of other MIPS systems suffer from similar issues, which I'll address similarly.
Quoted 8 lines
Quoted 2 lines
Please let me know if that addresses the issue on your end.
Yes, it does. Feel free to add
Tested-by Guenter Roeck <[email protected]>to the real patch.
Thanks.
Quoted 2 lines
Now the big question: Why does this only affect 32-bit little endian mips images, but not the matching big endian images, nor 64-bit images ?
Are you sure these images are using the exact same HW? A bunch (most?) of MIPS systems do not use irqdomains in their root interrupt handling, so this issue wouldn't be visible (irq_enter() will already have been done for the chained interrupt handling).
FWIW, I can reproduce the problem by switching your mipsel config to BE, and adding this patch fixes it.
Thanks,
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Quoted 12 lines · Marc Zyngier
Quoted 10 lines · Guenter Roeck
Quoted 6 lines · Marc Zyngier
It definitely helps, and confirms my hunch. With the patch below, I'm not getting the warnings anymore. I'm pretty sure a number of other MIPS systems suffer from similar issues, which I'll address similarly.
Please let me know if that addresses the issue on your end.
Quoted 11 lines
Quoted 5 lines
Yes, it does. Feel free to add
Tested-by Guenter Roeck <[email protected]>to the real patch.
Thanks.
Quoted 2 lines
Now the big question: Why does this only affect 32-bit little endian mips images, but not the matching big endian images, nor 64-bit images ?
Actually they do.
Quoted 8 lines
Are you sure these images are using the exact same HW? A bunch (most?) of MIPS systems do not use irqdomains in their root interrupt handling, so this issue wouldn't be visible (
irq_enter()will already have been done for the chained interrupt handling).FWIW, I can reproduce the problem by switching your mipsel config to BE, and adding this patch fixes it.
Yes, turns out I did not have the necessary debugging options enabled in my other mips tests.
Thanks,
Guenter
[irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
In reply to Guenter RoeckThe following commit has been merged into the irq/irqchip-fixes branch of irqchip:
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: f333d6bc4a8b2ef6c999e46c508d50277baddcd6 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/f333d6bc4a8b2ef6c999e46c508d50277baddcd6 Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 06 Jul 2021 11:38:59 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Tue, 06 Jul 2021 13:55:57 +01:00
irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), any irqdomain lookup requires the RCU read lock to be held.
This assumes that the architecture code will be structured such as irq_enter() will be called *before* the interrupt is looked up in the irq domain. However, this isn't the case for MIPS, and a number of drivers are structured to do it the other way around when handling an interrupt in their root irqchip (secondary irqchips are OK by construction).
This results in a RCU splat on a lockdep-enabled kernel when the kernel takes an interrupt from idle, as reported by Guenter Roeck.
Note that this could have fired previously if any driver had used tree-based irqdomain, which always had the RCU requirement.
To solve this, provide a MIPS-specific helper (do_domain_IRQ()) as the pendent of do_IRQ() that will do thing in the right order (and maybe save some cycles in the process).
Ideally, MIPS would be moved over to using handle_domain_irq(), but that's much more ambitious.
--- arch/mips/include/asm/irq.h | 3 +++ arch/mips/kernel/irq.c | 14 ++++++++++++++ drivers/irqchip/irq-mips-cpu.c | 10 ++++++---- drivers/irqchip/irq-mips-gic.c | 8 ++++---- drivers/irqchip/irq-pic32-evic.c | 5 ++--- 5 files changed, 29 insertions(+), 11 deletions(-)
arch/mips/include/asm/irq.h +3 -0
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);extern void do_IRQ (unsigned int irq); +struct irq_domain ; +extern void do_domain_IRQ (struct irq_domain *domain,unsigned int irq); +extern void arch_init_irq (void );extern void spurious_interrupt (void );
arch/mips/kernel/irq.c +14 -0
@@ -21,6 +21,7 @@#include <linux/kallsyms.h> #include <linux/kgdb.h> #include <linux/ftrace.h> +#include <linux/irqdomain.h> #include <linux/atomic.h> #include <linux/uaccess.h>
@@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq)irq_exit (); } +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) +{ +struct irq_desc *desc; + +irq_enter (); +check_stack_overflow (); + + desc =irq_resolve_mapping (domain, hwirq); +if (likely (desc)) +handle_irq_desc (desc); + +irq_exit (); +}
drivers/irqchip/irq-mips-cpu.c +6 -4
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { asmlinkage void __weak plat_irq_dispatch(void) {unsigned long pending =read_c0_cause () &read_c0_status () &ST0_IM ; -unsigned int virq;int irq;if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) pending >>=CAUSEB_IP ;while (pending) { +struct irq_domain *d; + irq =fls (pending) -1 ;if (IS_ENABLED (CONFIG_GENERIC_IRQ_IPI ) && irq <2 ) - virq =irq_linear_revmap (ipi_domain, irq); + d = ipi_domain;else - virq =irq_linear_revmap (irq_domain, irq); -do_IRQ (virq); + d = irq_domain; + +do_domain_IRQ (d, irq); pending &= ~BIT (irq); } }
drivers/irqchip/irq-mips-gic.c +4 -4
@@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained)generic_handle_domain_irq (gic_irq_domain,GIC_SHARED_TO_HWIRQ (intr)); else -do_IRQ (irq_find_mapping (gic_irq_domain , -GIC_SHARED_TO_HWIRQ (intr ))); +do_domain_IRQ (gic_irq_domain , +GIC_SHARED_TO_HWIRQ (intr )); } }
@@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained)generic_handle_domain_irq (gic_irq_domain,GIC_LOCAL_TO_HWIRQ (intr)); else -do_IRQ (irq_find_mapping (gic_irq_domain , -GIC_LOCAL_TO_HWIRQ (intr ))); +do_domain_IRQ (gic_irq_domain , +GIC_LOCAL_TO_HWIRQ (intr )); } }
drivers/irqchip/irq-pic32-evic.c +2 -3
@@ -42,11 +42,10 @@ static void __iomem *evic_base; asmlinkage void __weak plat_irq_dispatch(void) { -unsigned int irq, hwirq; +unsigned int hwirq; hwirq =readl (evic_base +REG_INTSTAT ) &0xFF ; - irq =irq_linear_revmap (evic_irq_domain, hwirq); -do_IRQ (irq); +do_domain_IRQ (evic_irq_domain, hwirq); }static struct evic_chip_data *irqd_to_priv (struct irq_data *data)
[irqchip: irq/irqchip-fixes] irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
In reply to Guenter RoeckThe following commit has been merged into the irq/irqchip-fixes branch of irqchip:
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 1fee9db9b42d821e8007289d4eea74bdf85b1543 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/1fee9db9b42d821e8007289d4eea74bdf85b1543 Author: Marc Zyngier <[email protected]> AuthorDate: Tue, 06 Jul 2021 11:38:59 +01:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Fri, 09 Jul 2021 10:18:58 +01:00
irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry
Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), any irqdomain lookup requires the RCU read lock to be held.
This assumes that the architecture code will be structured such as irq_enter() will be called *before* the interrupt is looked up in the irq domain. However, this isn't the case for MIPS, and a number of drivers are structured to do it the other way around when handling an interrupt in their root irqchip (secondary irqchips are OK by construction).
This results in a RCU splat on a lockdep-enabled kernel when the kernel takes an interrupt from idle, as reported by Guenter Roeck.
Note that this could have fired previously if any driver had used tree-based irqdomain, which always had the RCU requirement.
To solve this, provide a MIPS-specific helper (do_domain_IRQ()) as the pendent of do_IRQ() that will do thing in the right order (and maybe save some cycles in the process).
Ideally, MIPS would be moved over to using handle_domain_irq(), but that's much more ambitious.
--- arch/mips/include/asm/irq.h | 3 +++ arch/mips/kernel/irq.c | 16 ++++++++++++++++ drivers/irqchip/irq-mips-cpu.c | 10 ++++++---- drivers/irqchip/irq-mips-gic.c | 8 ++++---- drivers/irqchip/irq-pic32-evic.c | 5 ++--- 5 files changed, 31 insertions(+), 11 deletions(-)
arch/mips/include/asm/irq.h +3 -0
@@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void);extern void do_IRQ (unsigned int irq); +struct irq_domain ; +extern void do_domain_IRQ (struct irq_domain *domain,unsigned int irq); +extern void arch_init_irq (void );extern void spurious_interrupt (void );
arch/mips/kernel/irq.c +16 -0
@@ -21,6 +21,7 @@#include <linux/kallsyms.h> #include <linux/kgdb.h> #include <linux/ftrace.h> +#include <linux/irqdomain.h> #include <linux/atomic.h> #include <linux/uaccess.h>
@@ -107,3 +108,18 @@ void __irq_entry do_IRQ(unsigned int irq)irq_exit (); } +#ifdef CONFIG_IRQ_DOMAIN +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) +{ +struct irq_desc *desc; + +irq_enter (); +check_stack_overflow (); + + desc =irq_resolve_mapping (domain, hwirq); +if (likely (desc)) +handle_irq_desc (desc); + +irq_exit (); +} +#endif
drivers/irqchip/irq-mips-cpu.c +6 -4
@@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { asmlinkage void __weak plat_irq_dispatch(void) {unsigned long pending =read_c0_cause () &read_c0_status () &ST0_IM ; -unsigned int virq;int irq;if (!pending) {
@@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) pending >>=CAUSEB_IP ;while (pending) { +struct irq_domain *d; + irq =fls (pending) -1 ;if (IS_ENABLED (CONFIG_GENERIC_IRQ_IPI ) && irq <2 ) - virq =irq_linear_revmap (ipi_domain, irq); + d = ipi_domain;else - virq =irq_linear_revmap (irq_domain, irq); -do_IRQ (virq); + d = irq_domain; + +do_domain_IRQ (d, irq); pending &= ~BIT (irq); } }
drivers/irqchip/irq-mips-gic.c +4 -4
@@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained)generic_handle_domain_irq (gic_irq_domain,GIC_SHARED_TO_HWIRQ (intr)); else -do_IRQ (irq_find_mapping (gic_irq_domain , -GIC_SHARED_TO_HWIRQ (intr ))); +do_domain_IRQ (gic_irq_domain , +GIC_SHARED_TO_HWIRQ (intr )); } }
@@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained)generic_handle_domain_irq (gic_irq_domain,GIC_LOCAL_TO_HWIRQ (intr)); else -do_IRQ (irq_find_mapping (gic_irq_domain , -GIC_LOCAL_TO_HWIRQ (intr ))); +do_domain_IRQ (gic_irq_domain , +GIC_LOCAL_TO_HWIRQ (intr )); } }
drivers/irqchip/irq-pic32-evic.c +2 -3
@@ -42,11 +42,10 @@ static void __iomem *evic_base; asmlinkage void __weak plat_irq_dispatch(void) { -unsigned int irq, hwirq; +unsigned int hwirq; hwirq =readl (evic_base +REG_INTSTAT ) &0xFF ; - irq =irq_linear_revmap (evic_irq_domain, hwirq); -do_IRQ (irq); +do_domain_IRQ (evic_irq_domain, hwirq); }static struct evic_chip_data *irqd_to_priv (struct irq_data *data)