Hi Marc,
On Mon, 20 Sept 2021 at 20:24, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > Is this regmap call atomic? When running this, you are holding a
> > > raw_spinlock already. From what I can see, this is unlikely to work
> > > correctly with the current state of regmap.
> >
> > I didn't even think about it. I will check.
>
> You may want to enable lockdep to verify that.
I've just checked with lockdep and while it doesn't complain about
this code it complains about similar code I have somewhere else that's
almost identical (yet another interrupt controller driver I needed to
write..).
So it probably doesn't work properly as you say. I'll fix that up.
> > Technically I think the EOI callback should actually be ACK instead
> > but from my fuzzy memory with the stack of IRQ controllers that
> > resulted in a null pointer dereference.
>
> That's probably because you are using the wrong flow handler. You
> should turn this irq_eoi into an irq_ack, because that's really what
> it is, and use handle_edge_irq() as the flow handler.
If I do that (put the ack clearing callback into the ack slot, change
the handler to handle_edge_irq()) I get this explosion:
# gpiomon -r 0 44
[ 23.449571] 8<--- cut here ---
[ 23.452673] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 23.460804] pgd = (ptrval)
[ 23.463534] [00000000] *pgd=23530835, *pte=00000000, *ppte=00000000
[ 23.469868] Internal error: Oops: 80000007 [#1] SMP ARM
[ 23.475128] Modules linked in:
[ 23.478211] CPU: 0 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2565
[ 23.484866] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 23.490727] PC is at 0x0
[ 23.493283] LR is at handle_edge_irq+0x88/0xfc
[ 23.497764] pc : [<00000000>] lr : [<c0180694>] psr: a0040193
[ 23.504064] sp : c2405d90 ip : 00000000 fp : c35a786c
[ 23.509318] r10: 00000001 r9 : c1b96fc0 r8 : 00000020
[ 23.514572] r7 : 000000c8 r6 : c35a786c r5 : c35a7818 r4 : c35a7800
[ 23.521135] r3 : 00000000 r2 : 000015d6 r1 : 06f80000 r0 : c35a7818
This one is because the GPIO controller irqchip that is on top of this
doesn't have an ack callback.
So if I set irq_chip_ack_parent as the ack callback I get another explosion:
# gpiomon -r 0 44
[ 22.370689] 8<--- cut here ---
[ 22.373802] Unable to handle kernel NULL pointer dereference at
virtual address 00000018
[ 22.381945] pgd = (ptrval)
[ 22.384685] [00000018] *pgd=235cb835, *pte=00000000, *ppte=00000000
[ 22.391038] Internal error: Oops: 17 [#1] SMP ARM
[ 22.395776] Modules linked in:
[ 22.398860] CPU: 1 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2566
[ 22.405515] Hardware name: MStar/Sigmastar Armv7 (Device Tree)
[ 22.411376] PC is at irq_chip_ack_parent+0x8/0x10
[ 22.416120] LR is at __irq_do_set_handler+0x3c/0x11c
[ 22.421119] pc : [<c017f498>] lr : [<c018029c>] psr: a0040093
[ 22.427419] sp : c3505d68 ip : ffffe000 fp : 00000000
[ 22.432673] r10: c0d592d4 r9 : 00000001 r8 : 00000000
[ 22.437927] r7 : c3502618 r6 : 00000000 r5 : c017b9cc r4 : c3502600
[ 22.444489] r3 : 00000000 r2 : c10bb294 r1 : c10bb294 r0 : c26a3440
[ 22.451053] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment user
[ 22.458317] Control: 10c5387d Table: 235b006a DAC: 00000055
---snip---
[ 22.725196] [<c017f498>] (irq_chip_ack_parent) from [<c018029c>]
(__irq_do_set_handler+0x3c/0x11c)
[ 22.734219] [<c018029c>] (__irq_do_set_handler) from [<c01803b4>]
(__irq_set_handler+0x38/0x50)
[ 22.742976] [<c01803b4>] (__irq_set_handler) from [<c0181880>]
(irq_domain_set_info+0x34/0x48)
[ 22.751649] [<c0181880>] (irq_domain_set_info) from [<c046f838>]
(gpiochip_hierarchy_irq_domain_alloc+0x104/0x228)
[ 22.762069] [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc) from
[<c0182c38>] (__irq_domain_alloc_irqs+0xd8/0x318)
[ 22.772748] [<c0182c38>] (__irq_domain_alloc_irqs) from
[<c01832e8>] (irq_create_fwspec_mapping+0x22c/0x298)
[ 22.782641] [<c01832e8>] (irq_create_fwspec_mapping) from
[<c0470124>] (gpiochip_to_irq+0x60/0x84)
[ 22.791664] [<c0470124>] (gpiochip_to_irq) from [<c046ef18>]
(gpiod_to_irq+0x48/0x60)
[ 22.799552] [<c046ef18>] (gpiod_to_irq) from [<c0477a48>]
(gpio_ioctl+0x1b4/0x420)
[ 22.807178] [<c0477a48>] (gpio_ioctl) from [<c0262e4c>] (vfs_ioctl+0x20/0x38)
[ 22.814371] [<c0262e4c>] (vfs_ioctl) from [<c0263708>] (sys_ioctl+0xb0/0x818)
[ 22.821564] [<c0263708>] (sys_ioctl) from [<c0100060>]
(ret_fast_syscall+0x0/0x1c)
[ 22.829190] Exception stack(0xc3505fa8 to 0xc3505ff0)
[ 22.834273] 5fa0: ???????? ???????? ????????
???????? ???????? ????????
[ 22.842488] 5fc0: ???????? ???????? ???????? ???????? ????????
???????? ???????? ????????
[ 22.850701] 5fe0: ???????? ???????? ???????? ????????
[ 22.855790] Code: e593301c e12fff13 e5900018 e5903010 (e5933018)
[ 22.861919] ---[ end trace 10524aa06eced7e3 ]---
If I do something silly like the following diff the explosion stops
and GPIO interrupt fires correctly each time I press the button it's
connected to:
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index a98bcfc4be7b..969df9207358 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1368,6 +1368,8 @@ EXPORT_SYMBOL_GPL(irq_chip_disable_parent);
void irq_chip_ack_parent(struct irq_data *data)
{
data = data->parent_data;
+ if(!data->chip)
+ return;
data->chip->irq_ack(data);
}
EXPORT_SYMBOL_GPL(irq_chip_ack_parent);
I think I got stuck at this, switched it to the eoi handler instead
and then forgot about it.
I'll work out the proper solution for this for v2.
Cheers,
Daniel
