* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 22:26 ` [PATCH] irqchip/gic: Convert to handle_strict_flow_irq() Valentin Schneider
@ 2021-08-14 22:31 ` Valentin Schneider
2021-08-14 23:41 ` Guenter Roeck
2021-08-14 23:15 ` Guenter Roeck
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Valentin Schneider @ 2021-08-14 22:31 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, linux-arm-kernel, Marc Zyngier
On 14/08/21 23:26, Valentin Schneider wrote:
>
> Now, the above makes me feel like this is the start of a wild goose chase
> for irqchips in a similar situation.
The ones in arch/arm are easy enough to catch (I see gpc, omap-wakeupgen
and some exynos suspend thing), less so for the ones in drivers/irqchip...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 22:31 ` Valentin Schneider
@ 2021-08-14 23:41 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2021-08-14 23:41 UTC (permalink / raw)
To: Valentin Schneider; +Cc: linux-kernel, linux-arm-kernel, Marc Zyngier
On Sat, Aug 14, 2021 at 11:31:45PM +0100, Valentin Schneider wrote:
> On 14/08/21 23:26, Valentin Schneider wrote:
> >
> > Now, the above makes me feel like this is the start of a wild goose chase
> > for irqchips in a similar situation.
>
> The ones in arch/arm are easy enough to catch (I see gpc, omap-wakeupgen
> and some exynos suspend thing), less so for the ones in drivers/irqchip...
Try:
make coccicheck COCCI=irq_chip.cocci MODE=report M=.
with the script below. I am sure the script could be augmented if
there is some secondary condition that makes irq_ack mandatory.
Guenter
---
irq_chip.cocci:
virtual report
@c@
identifier chip;
position p;
@@
struct irq_chip chip@p = {
...
};
@i@
identifier c.chip;
identifier f;
@@
struct irq_chip chip = {
.irq_ack = f,
};
@script:python depends on c && !i && report@
p << c.p;
@@
print "Found %s:%s" % (p[0].file, p[0].line)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 22:26 ` [PATCH] irqchip/gic: Convert to handle_strict_flow_irq() Valentin Schneider
2021-08-14 22:31 ` Valentin Schneider
@ 2021-08-14 23:15 ` Guenter Roeck
2021-08-14 23:36 ` Guenter Roeck
2021-08-15 6:54 ` Marc Zyngier
3 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2021-08-14 23:15 UTC (permalink / raw)
To: Valentin Schneider; +Cc: linux-kernel, linux-arm-kernel, Marc Zyngier
On 8/14/21 3:26 PM, Valentin Schneider wrote:
> Hi,
>
> On 14/08/21 12:47, Guenter Roeck wrote:
>> On Tue, Jun 29, 2021 at 01:50:09PM +0100, Valentin Schneider wrote:
>>> Now that the proper infrastructure is in place, convert the irq-gic chip to
>>> use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
>>>
>>> For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
>>> chip->irq_ack(). This effectively pushes the EOI write down into
>>> ->handle_irq(), but doesn't change its ordering wrt the irqaction
>>> handling.
>>>
>>> The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
>>> ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
>>> means a threaded ONESHOT IRQ can now be handled entirely without a single
>>> chip->irq_mask() call.
>>>
>>> EOImode=0 handling remains unchanged.
>>>
>>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> Link: https://lore.kernel.org/r/20210629125010.458872-13-valentin.schneider@arm.com
>>> ---
>>
>> This patch results in a variety of crashes in -next.
>>
>
> Thanks for testing & reporting.
>
>> Example:
>>
>> [ 0.025729] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> [ 0.025811] pgd = (ptrval)
>> [ 0.026029] [00000000] *pgd=00000000
>> [ 0.027301] Internal error: Oops: 80000005 [#1] SMP ARM
>> [ 0.027650] Modules linked in:
>> [ 0.027986] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.14.0-rc5-next-20210813 #1
>> [ 0.028162] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
>> [ 0.028311] PC is at 0x0
>> [ 0.028485] LR is at handle_strict_flow_irq+0xc8/0x2b8
>> [ 0.028538] pc : [<00000000>] lr : [<c01a59c4>] psr: 400001d3
>> [ 0.028566] sp : c1701e60 ip : c17097cc fp : 00000050
>> [ 0.028594] r10: c1700000 r9 : 00000057 r8 : c2012400
>> [ 0.028641] r7 : 00000000 r6 : c1711268 r5 : c2083c6c r4 : c2083c00
>> [ 0.028674] r3 : 00000000 r2 : 00000000 r1 : 0a728000 r0 : c2083c18
>> [ 0.028766] Flags: nZcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
>> [ 0.028821] Control: 10c5387d Table: 8000406a DAC: 00000051
>> [ 0.028881] Register r0 information: slab kmalloc-512 start c2083c00 pointer offset 24 size 512
>> [ 0.029482] Register r1 information: non-paged memory
>> [ 0.029649] Register r2 information: NULL pointer
>> [ 0.029672] Register r3 information: NULL pointer
>> [ 0.029692] Register r4 information: slab kmalloc-512 start c2083c00 pointer offset 0 size 512
>> [ 0.029752] Register r5 information: slab kmalloc-512 start c2083c00 pointer offset 108 size 512
>> [ 0.029809] Register r6 information: non-slab/vmalloc memory
>> [ 0.029876] Register r7 information: NULL pointer
>> [ 0.029897] Register r8 information: slab kmalloc-1k start c2012400 pointer offset 0 size 1024
>> [ 0.029958] Register r9 information: non-paged memory
>> [ 0.029980] Register r10 information: non-slab/vmalloc memory
>> [ 0.030002] Register r11 information: non-paged memory
>> [ 0.030023] Register r12 information: non-slab/vmalloc memory
>> [ 0.030064] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
>> [ 0.030167] Stack: (0xc1701e60 to 0xc1702000)
>> [ 0.030562] 1e60: c168fcd8 c1700000 c17097cc c019fa2c c1709a44 d080c00c c168fcfc c1700000
>> [ 0.030754] 1e80: c17097cc c1701eb8 c1700000 c076fe04 c1700000 600000d3 c0824188 80000053
>> [ 0.030924] 1ea0: ffffffff c1701eec 00000000 c1700000 c1700000 c0100b70 c2013400 c20a0000
>> [ 0.031096] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
>> [ 0.031267] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
>> [ 0.031439] 1f00: 00000051 00000000 00000000 c168f31c c17097cc c2013400 c2013708 3123ccba
>> [ 0.031614] 1f20: 00000001 c2013400 00000960 c20a0000 c1422958 c1709380 c16804d8 c1646020
>> [ 0.031790] 1f40: c14d1eec c082626c c1f6f4e0 c2013400 00000000 c1422958 c1709380 c16461d4
>> [ 0.031961] 1f60: 00000000 00000000 00000000 c0f7d3a4 c186fdcc c1700000 c17093cc c16804d8
>> [ 0.032155] 1f80: c18f8e20 c160fe80 c1903000 c1663a80 00000012 c1700000 c1709380 00000000
>> [ 0.032327] 1fa0: c170caf0 c1600e50 ffffffff ffffffff 00000000 c1600588 00000000 c1700000
>> [ 0.032500] 1fc0: 00000000 c1663a80 3126c2ba 00000000 00000000 c1600330 00000051 10c0387d
>> [ 0.032671] 1fe0: ffffffff 8833b000 410fc075 10c5387d 00000000 00000000 00000000 00000000
>> [ 0.032889] [<c01a59c4>] (handle_strict_flow_irq) from [<c019fa2c>] (handle_domain_irq+0x64/0xa4)
>> [ 0.033027] [<c019fa2c>] (handle_domain_irq) from [<c076fe04>] (gic_handle_irq+0x84/0xbc)
>> [ 0.033076] [<c076fe04>] (gic_handle_irq) from [<c0100b70>] (__irq_svc+0x70/0x98)
>> [ 0.033140] Exception stack(0xc1701eb8 to 0xc1701f00)
>> [ 0.033204] 1ea0: c2013400 c20a0000
>> [ 0.033377] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
>> [ 0.033551] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
>> [ 0.033596] [<c0100b70>] (__irq_svc) from [<c0824188>] (do_update_region+0x108/0x19c)
>> [ 0.033652] [<c0824188>] (do_update_region) from [<c082626c>] (csi_J+0xf0/0x290)
>> [ 0.033685] [<c082626c>] (csi_J) from [<c16461d4>] (con_init+0x1b4/0x248)
>> [ 0.033726] [<c16461d4>] (con_init) from [<c160fe80>] (console_init+0x21c/0x330)
>> [ 0.033760] [<c160fe80>] (console_init) from [<c1600e50>] (start_kernel+0x508/0x6d4)
>> [ 0.033797] [<c1600e50>] (start_kernel) from [<00000000>] (0x0)
>> [ 0.034079] Code: bad PC value
>> [ 0.034726] ---[ end trace c1324fc4facef313 ]---
>> [ 0.035004] Kernel panic - not syncing: Fatal exception in interrupt
>>
>> https://kerneltests.org/builders/qemu-arm-v7-next/builds/66 provides
>> a more detailed log.
>>
>> Reverting this patch fixes the problem.
>>
>
> Passing the stacktrace through scripts/decode_stracktrace.sh would help by
> giving line numbers - that said I'm pretty sure this is caused by the
> flow-handler now issuing a chip->irq_ack(), and this has an irqchip that
> doesn't have one.
>
> The hardware name points me at arch/arm/mach-imx/mach-imx6ul.c, but I can't
> figure out an actual devicetree/irqchip layout from this.
>
This one was imx6ul-14x14-evk, and I had seen a similar crash with mcimx7d-sabre
and imx7d-sdb.
> Ah, with some more grepping I did find arch/arm/mach-imx/gpc.c, and bingo,
> no .irq_ack().
>
> Now, the above makes me feel like this is the start of a wild goose chase
> for irqchips in a similar situation. I'm tempted to apply a dumb
>
> .irq_ack = irq_chip_ack_parent
>
> to all irqchips without one specified. Let's see how bad this gets, does
> the below help?
>
Yes, it does, for both mcimx7d-sabre and mcimx6ul-evk bot tests with qemu.
That makes me wonder - would it make sense to (also) change the calling
code to check if irq_ack is NULL, and call irq_chip_ack_parent if it is ?
Thanks,
Guenter
> ---
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index ebc4339b8be4..a4e10daa3ae7 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -158,6 +158,7 @@ static void imx_gpc_irq_mask(struct irq_data *d)
>
> static struct irq_chip imx_gpc_chip = {
> .name = "GPC",
> + .irq_ack = irq_chip_ack_parent,
> .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = imx_gpc_irq_mask,
> .irq_unmask = imx_gpc_irq_unmask,
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 5b5a365dbd5e..e304c80cd403 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -126,6 +126,7 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>
> static struct irq_chip gpcv2_irqchip_data_chip = {
> .name = "GPCv2",
> + .irq_ack = irq_chip_ack_parent,
> .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = imx_gpcv2_irq_mask,
> .irq_unmask = imx_gpcv2_irq_unmask,
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 22:26 ` [PATCH] irqchip/gic: Convert to handle_strict_flow_irq() Valentin Schneider
2021-08-14 22:31 ` Valentin Schneider
2021-08-14 23:15 ` Guenter Roeck
@ 2021-08-14 23:36 ` Guenter Roeck
2021-08-15 7:01 ` Marc Zyngier
2021-08-15 6:54 ` Marc Zyngier
3 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2021-08-14 23:36 UTC (permalink / raw)
To: Valentin Schneider; +Cc: linux-kernel, linux-arm-kernel, Marc Zyngier
On 8/14/21 3:26 PM, Valentin Schneider wrote:
> Hi,
>
> On 14/08/21 12:47, Guenter Roeck wrote:
>> On Tue, Jun 29, 2021 at 01:50:09PM +0100, Valentin Schneider wrote:
>>> Now that the proper infrastructure is in place, convert the irq-gic chip to
>>> use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
>>>
>>> For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
>>> chip->irq_ack(). This effectively pushes the EOI write down into
>>> ->handle_irq(), but doesn't change its ordering wrt the irqaction
>>> handling.
>>>
>>> The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
>>> ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
>>> means a threaded ONESHOT IRQ can now be handled entirely without a single
>>> chip->irq_mask() call.
>>>
>>> EOImode=0 handling remains unchanged.
>>>
>>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> Link: https://lore.kernel.org/r/20210629125010.458872-13-valentin.schneider@arm.com
>>> ---
>>
>> This patch results in a variety of crashes in -next.
>>
>
> Thanks for testing & reporting.
>
>> Example:
>>
>> [ 0.025729] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> [ 0.025811] pgd = (ptrval)
>> [ 0.026029] [00000000] *pgd=00000000
>> [ 0.027301] Internal error: Oops: 80000005 [#1] SMP ARM
>> [ 0.027650] Modules linked in:
>> [ 0.027986] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.14.0-rc5-next-20210813 #1
>> [ 0.028162] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
>> [ 0.028311] PC is at 0x0
>> [ 0.028485] LR is at handle_strict_flow_irq+0xc8/0x2b8
>> [ 0.028538] pc : [<00000000>] lr : [<c01a59c4>] psr: 400001d3
>> [ 0.028566] sp : c1701e60 ip : c17097cc fp : 00000050
>> [ 0.028594] r10: c1700000 r9 : 00000057 r8 : c2012400
>> [ 0.028641] r7 : 00000000 r6 : c1711268 r5 : c2083c6c r4 : c2083c00
>> [ 0.028674] r3 : 00000000 r2 : 00000000 r1 : 0a728000 r0 : c2083c18
>> [ 0.028766] Flags: nZcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
>> [ 0.028821] Control: 10c5387d Table: 8000406a DAC: 00000051
>> [ 0.028881] Register r0 information: slab kmalloc-512 start c2083c00 pointer offset 24 size 512
>> [ 0.029482] Register r1 information: non-paged memory
>> [ 0.029649] Register r2 information: NULL pointer
>> [ 0.029672] Register r3 information: NULL pointer
>> [ 0.029692] Register r4 information: slab kmalloc-512 start c2083c00 pointer offset 0 size 512
>> [ 0.029752] Register r5 information: slab kmalloc-512 start c2083c00 pointer offset 108 size 512
>> [ 0.029809] Register r6 information: non-slab/vmalloc memory
>> [ 0.029876] Register r7 information: NULL pointer
>> [ 0.029897] Register r8 information: slab kmalloc-1k start c2012400 pointer offset 0 size 1024
>> [ 0.029958] Register r9 information: non-paged memory
>> [ 0.029980] Register r10 information: non-slab/vmalloc memory
>> [ 0.030002] Register r11 information: non-paged memory
>> [ 0.030023] Register r12 information: non-slab/vmalloc memory
>> [ 0.030064] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
>> [ 0.030167] Stack: (0xc1701e60 to 0xc1702000)
>> [ 0.030562] 1e60: c168fcd8 c1700000 c17097cc c019fa2c c1709a44 d080c00c c168fcfc c1700000
>> [ 0.030754] 1e80: c17097cc c1701eb8 c1700000 c076fe04 c1700000 600000d3 c0824188 80000053
>> [ 0.030924] 1ea0: ffffffff c1701eec 00000000 c1700000 c1700000 c0100b70 c2013400 c20a0000
>> [ 0.031096] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
>> [ 0.031267] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
>> [ 0.031439] 1f00: 00000051 00000000 00000000 c168f31c c17097cc c2013400 c2013708 3123ccba
>> [ 0.031614] 1f20: 00000001 c2013400 00000960 c20a0000 c1422958 c1709380 c16804d8 c1646020
>> [ 0.031790] 1f40: c14d1eec c082626c c1f6f4e0 c2013400 00000000 c1422958 c1709380 c16461d4
>> [ 0.031961] 1f60: 00000000 00000000 00000000 c0f7d3a4 c186fdcc c1700000 c17093cc c16804d8
>> [ 0.032155] 1f80: c18f8e20 c160fe80 c1903000 c1663a80 00000012 c1700000 c1709380 00000000
>> [ 0.032327] 1fa0: c170caf0 c1600e50 ffffffff ffffffff 00000000 c1600588 00000000 c1700000
>> [ 0.032500] 1fc0: 00000000 c1663a80 3126c2ba 00000000 00000000 c1600330 00000051 10c0387d
>> [ 0.032671] 1fe0: ffffffff 8833b000 410fc075 10c5387d 00000000 00000000 00000000 00000000
>> [ 0.032889] [<c01a59c4>] (handle_strict_flow_irq) from [<c019fa2c>] (handle_domain_irq+0x64/0xa4)
>> [ 0.033027] [<c019fa2c>] (handle_domain_irq) from [<c076fe04>] (gic_handle_irq+0x84/0xbc)
>> [ 0.033076] [<c076fe04>] (gic_handle_irq) from [<c0100b70>] (__irq_svc+0x70/0x98)
>> [ 0.033140] Exception stack(0xc1701eb8 to 0xc1701f00)
>> [ 0.033204] 1ea0: c2013400 c20a0000
>> [ 0.033377] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
>> [ 0.033551] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
>> [ 0.033596] [<c0100b70>] (__irq_svc) from [<c0824188>] (do_update_region+0x108/0x19c)
>> [ 0.033652] [<c0824188>] (do_update_region) from [<c082626c>] (csi_J+0xf0/0x290)
>> [ 0.033685] [<c082626c>] (csi_J) from [<c16461d4>] (con_init+0x1b4/0x248)
>> [ 0.033726] [<c16461d4>] (con_init) from [<c160fe80>] (console_init+0x21c/0x330)
>> [ 0.033760] [<c160fe80>] (console_init) from [<c1600e50>] (start_kernel+0x508/0x6d4)
>> [ 0.033797] [<c1600e50>] (start_kernel) from [<00000000>] (0x0)
>> [ 0.034079] Code: bad PC value
>> [ 0.034726] ---[ end trace c1324fc4facef313 ]---
>> [ 0.035004] Kernel panic - not syncing: Fatal exception in interrupt
>>
>> https://kerneltests.org/builders/qemu-arm-v7-next/builds/66 provides
>> a more detailed log.
>>
>> Reverting this patch fixes the problem.
>>
>
> Passing the stacktrace through scripts/decode_stracktrace.sh would help by
> giving line numbers - that said I'm pretty sure this is caused by the
> flow-handler now issuing a chip->irq_ack(), and this has an irqchip that
> doesn't have one.
>
> The hardware name points me at arch/arm/mach-imx/mach-imx6ul.c, but I can't
> figure out an actual devicetree/irqchip layout from this.
>
> Ah, with some more grepping I did find arch/arm/mach-imx/gpc.c, and bingo,
> no .irq_ack().
>
> Now, the above makes me feel like this is the start of a wild goose chase
> for irqchips in a similar situation. I'm tempted to apply a dumb
>
> .irq_ack = irq_chip_ack_parent
>
> to all irqchips without one specified. Let's see how bad this gets, does
> the below help?
>
As a follow-up, Coccinelle tells me that there are more than 300 declarations of
struct irq_chip variables without irq_ack pointer in the kernel. Does this problem
affect all of them or just a subset ? If it doesn't affect all of them, how does
one know when irq_ack is mandatory and when not ?
Thanks,
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 23:36 ` Guenter Roeck
@ 2021-08-15 7:01 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2021-08-15 7:01 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Valentin Schneider, linux-kernel, linux-arm-kernel
On Sun, 15 Aug 2021 00:36:39 +0100,
Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 8/14/21 3:26 PM, Valentin Schneider wrote:
> > Hi,
> >
> > On 14/08/21 12:47, Guenter Roeck wrote:
> >> On Tue, Jun 29, 2021 at 01:50:09PM +0100, Valentin Schneider wrote:
> >>> Now that the proper infrastructure is in place, convert the irq-gic chip to
> >>> use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
> >>>
> >>> For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
> >>> chip->irq_ack(). This effectively pushes the EOI write down into
> >>> ->handle_irq(), but doesn't change its ordering wrt the irqaction
> >>> handling.
> >>>
> >>> The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
> >>> ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
> >>> means a threaded ONESHOT IRQ can now be handled entirely without a single
> >>> chip->irq_mask() call.
> >>>
> >>> EOImode=0 handling remains unchanged.
> >>>
> >>> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> >>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >>> Link: https://lore.kernel.org/r/20210629125010.458872-13-valentin.schneider@arm.com
> >>> ---
> >>
> >> This patch results in a variety of crashes in -next.
> >>
> >
> > Thanks for testing & reporting.
> >
> >> Example:
> >>
> >> [ 0.025729] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> >> [ 0.025811] pgd = (ptrval)
> >> [ 0.026029] [00000000] *pgd=00000000
> >> [ 0.027301] Internal error: Oops: 80000005 [#1] SMP ARM
> >> [ 0.027650] Modules linked in:
> >> [ 0.027986] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.14.0-rc5-next-20210813 #1
> >> [ 0.028162] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> >> [ 0.028311] PC is at 0x0
> >> [ 0.028485] LR is at handle_strict_flow_irq+0xc8/0x2b8
> >> [ 0.028538] pc : [<00000000>] lr : [<c01a59c4>] psr: 400001d3
> >> [ 0.028566] sp : c1701e60 ip : c17097cc fp : 00000050
> >> [ 0.028594] r10: c1700000 r9 : 00000057 r8 : c2012400
> >> [ 0.028641] r7 : 00000000 r6 : c1711268 r5 : c2083c6c r4 : c2083c00
> >> [ 0.028674] r3 : 00000000 r2 : 00000000 r1 : 0a728000 r0 : c2083c18
> >> [ 0.028766] Flags: nZcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
> >> [ 0.028821] Control: 10c5387d Table: 8000406a DAC: 00000051
> >> [ 0.028881] Register r0 information: slab kmalloc-512 start c2083c00 pointer offset 24 size 512
> >> [ 0.029482] Register r1 information: non-paged memory
> >> [ 0.029649] Register r2 information: NULL pointer
> >> [ 0.029672] Register r3 information: NULL pointer
> >> [ 0.029692] Register r4 information: slab kmalloc-512 start c2083c00 pointer offset 0 size 512
> >> [ 0.029752] Register r5 information: slab kmalloc-512 start c2083c00 pointer offset 108 size 512
> >> [ 0.029809] Register r6 information: non-slab/vmalloc memory
> >> [ 0.029876] Register r7 information: NULL pointer
> >> [ 0.029897] Register r8 information: slab kmalloc-1k start c2012400 pointer offset 0 size 1024
> >> [ 0.029958] Register r9 information: non-paged memory
> >> [ 0.029980] Register r10 information: non-slab/vmalloc memory
> >> [ 0.030002] Register r11 information: non-paged memory
> >> [ 0.030023] Register r12 information: non-slab/vmalloc memory
> >> [ 0.030064] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
> >> [ 0.030167] Stack: (0xc1701e60 to 0xc1702000)
> >> [ 0.030562] 1e60: c168fcd8 c1700000 c17097cc c019fa2c c1709a44 d080c00c c168fcfc c1700000
> >> [ 0.030754] 1e80: c17097cc c1701eb8 c1700000 c076fe04 c1700000 600000d3 c0824188 80000053
> >> [ 0.030924] 1ea0: ffffffff c1701eec 00000000 c1700000 c1700000 c0100b70 c2013400 c20a0000
> >> [ 0.031096] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
> >> [ 0.031267] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
> >> [ 0.031439] 1f00: 00000051 00000000 00000000 c168f31c c17097cc c2013400 c2013708 3123ccba
> >> [ 0.031614] 1f20: 00000001 c2013400 00000960 c20a0000 c1422958 c1709380 c16804d8 c1646020
> >> [ 0.031790] 1f40: c14d1eec c082626c c1f6f4e0 c2013400 00000000 c1422958 c1709380 c16461d4
> >> [ 0.031961] 1f60: 00000000 00000000 00000000 c0f7d3a4 c186fdcc c1700000 c17093cc c16804d8
> >> [ 0.032155] 1f80: c18f8e20 c160fe80 c1903000 c1663a80 00000012 c1700000 c1709380 00000000
> >> [ 0.032327] 1fa0: c170caf0 c1600e50 ffffffff ffffffff 00000000 c1600588 00000000 c1700000
> >> [ 0.032500] 1fc0: 00000000 c1663a80 3126c2ba 00000000 00000000 c1600330 00000051 10c0387d
> >> [ 0.032671] 1fe0: ffffffff 8833b000 410fc075 10c5387d 00000000 00000000 00000000 00000000
> >> [ 0.032889] [<c01a59c4>] (handle_strict_flow_irq) from [<c019fa2c>] (handle_domain_irq+0x64/0xa4)
> >> [ 0.033027] [<c019fa2c>] (handle_domain_irq) from [<c076fe04>] (gic_handle_irq+0x84/0xbc)
> >> [ 0.033076] [<c076fe04>] (gic_handle_irq) from [<c0100b70>] (__irq_svc+0x70/0x98)
> >> [ 0.033140] Exception stack(0xc1701eb8 to 0xc1701f00)
> >> [ 0.033204] 1ea0: c2013400 c20a0000
> >> [ 0.033377] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
> >> [ 0.033551] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
> >> [ 0.033596] [<c0100b70>] (__irq_svc) from [<c0824188>] (do_update_region+0x108/0x19c)
> >> [ 0.033652] [<c0824188>] (do_update_region) from [<c082626c>] (csi_J+0xf0/0x290)
> >> [ 0.033685] [<c082626c>] (csi_J) from [<c16461d4>] (con_init+0x1b4/0x248)
> >> [ 0.033726] [<c16461d4>] (con_init) from [<c160fe80>] (console_init+0x21c/0x330)
> >> [ 0.033760] [<c160fe80>] (console_init) from [<c1600e50>] (start_kernel+0x508/0x6d4)
> >> [ 0.033797] [<c1600e50>] (start_kernel) from [<00000000>] (0x0)
> >> [ 0.034079] Code: bad PC value
> >> [ 0.034726] ---[ end trace c1324fc4facef313 ]---
> >> [ 0.035004] Kernel panic - not syncing: Fatal exception in interrupt
> >>
> >> https://kerneltests.org/builders/qemu-arm-v7-next/builds/66 provides
> >> a more detailed log.
> >>
> >> Reverting this patch fixes the problem.
> >>
> >
> > Passing the stacktrace through scripts/decode_stracktrace.sh would help by
> > giving line numbers - that said I'm pretty sure this is caused by the
> > flow-handler now issuing a chip->irq_ack(), and this has an irqchip that
> > doesn't have one.
> >
> > The hardware name points me at arch/arm/mach-imx/mach-imx6ul.c, but I can't
> > figure out an actual devicetree/irqchip layout from this.
> >
> > Ah, with some more grepping I did find arch/arm/mach-imx/gpc.c, and bingo,
> > no .irq_ack().
> >
> > Now, the above makes me feel like this is the start of a wild goose chase
> > for irqchips in a similar situation. I'm tempted to apply a dumb
> >
> > .irq_ack = irq_chip_ack_parent
> >
> > to all irqchips without one specified. Let's see how bad this gets, does
> > the below help?
> >
>
> As a follow-up, Coccinelle tells me that there are more than 300
> declarations of struct irq_chip variables without irq_ack pointer in
> the kernel. Does this problem affect all of them or just a subset ?
> If it doesn't affect all of them, how does one know when irq_ack is
> mandatory and when not ?
One needs to understand what flow is being used. For irqchip stacks
using fast-eoi, there was no need for an irq_ack callback until we
switched to Valentin's "strict" flow.
The problem is that although irqchip *look* like they can be composed
at will, the composition only makes sense if they all agree on the
flow that is being used.
So we can either identify the irqchips that need to be updated (not an
easy task, but not impossible: just look for anything that feeds into
a GIC through a hierarchy and not a mux). Or we can make the flow
iterate over the irqchip layers.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-14 22:26 ` [PATCH] irqchip/gic: Convert to handle_strict_flow_irq() Valentin Schneider
` (2 preceding siblings ...)
2021-08-14 23:36 ` Guenter Roeck
@ 2021-08-15 6:54 ` Marc Zyngier
2021-08-17 0:30 ` Valentin Schneider
2021-08-30 16:54 ` Valentin Schneider
3 siblings, 2 replies; 14+ messages in thread
From: Marc Zyngier @ 2021-08-15 6:54 UTC (permalink / raw)
To: Valentin Schneider; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On Sat, 14 Aug 2021 23:26:07 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
>
> Hi,
>
> On 14/08/21 12:47, Guenter Roeck wrote:
> > On Tue, Jun 29, 2021 at 01:50:09PM +0100, Valentin Schneider wrote:
> >> Now that the proper infrastructure is in place, convert the irq-gic chip to
> >> use handle_strict_flow_irq() along with IRQCHIP_AUTOMASKS_FLOW.
> >>
> >> For EOImode=1, the Priority Drop is moved from gic_handle_irq() into
> >> chip->irq_ack(). This effectively pushes the EOI write down into
> >> ->handle_irq(), but doesn't change its ordering wrt the irqaction
> >> handling.
> >>
> >> The EOImode=1 irqchip also gains IRQCHIP_EOI_THREADED, which allows the
> >> ->irq_eoi() call to be deferred to the tail of ONESHOT IRQ threads. This
> >> means a threaded ONESHOT IRQ can now be handled entirely without a single
> >> chip->irq_mask() call.
> >>
> >> EOImode=0 handling remains unchanged.
> >>
> >> Signed-off-by: Valentin Schneider <valentin.schneider@arm.com>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> Link: https://lore.kernel.org/r/20210629125010.458872-13-valentin.schneider@arm.com
> >> ---
> >
> > This patch results in a variety of crashes in -next.
> >
>
> Thanks for testing & reporting.
>
> > Example:
> >
> > [ 0.025729] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> > [ 0.025811] pgd = (ptrval)
> > [ 0.026029] [00000000] *pgd=00000000
> > [ 0.027301] Internal error: Oops: 80000005 [#1] SMP ARM
> > [ 0.027650] Modules linked in:
> > [ 0.027986] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.14.0-rc5-next-20210813 #1
> > [ 0.028162] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> > [ 0.028311] PC is at 0x0
> > [ 0.028485] LR is at handle_strict_flow_irq+0xc8/0x2b8
> > [ 0.028538] pc : [<00000000>] lr : [<c01a59c4>] psr: 400001d3
> > [ 0.028566] sp : c1701e60 ip : c17097cc fp : 00000050
> > [ 0.028594] r10: c1700000 r9 : 00000057 r8 : c2012400
> > [ 0.028641] r7 : 00000000 r6 : c1711268 r5 : c2083c6c r4 : c2083c00
> > [ 0.028674] r3 : 00000000 r2 : 00000000 r1 : 0a728000 r0 : c2083c18
> > [ 0.028766] Flags: nZcv IRQs off FIQs off Mode SVC_32 ISA ARM Segment none
> > [ 0.028821] Control: 10c5387d Table: 8000406a DAC: 00000051
> > [ 0.028881] Register r0 information: slab kmalloc-512 start c2083c00 pointer offset 24 size 512
> > [ 0.029482] Register r1 information: non-paged memory
> > [ 0.029649] Register r2 information: NULL pointer
> > [ 0.029672] Register r3 information: NULL pointer
> > [ 0.029692] Register r4 information: slab kmalloc-512 start c2083c00 pointer offset 0 size 512
> > [ 0.029752] Register r5 information: slab kmalloc-512 start c2083c00 pointer offset 108 size 512
> > [ 0.029809] Register r6 information: non-slab/vmalloc memory
> > [ 0.029876] Register r7 information: NULL pointer
> > [ 0.029897] Register r8 information: slab kmalloc-1k start c2012400 pointer offset 0 size 1024
> > [ 0.029958] Register r9 information: non-paged memory
> > [ 0.029980] Register r10 information: non-slab/vmalloc memory
> > [ 0.030002] Register r11 information: non-paged memory
> > [ 0.030023] Register r12 information: non-slab/vmalloc memory
> > [ 0.030064] Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
> > [ 0.030167] Stack: (0xc1701e60 to 0xc1702000)
> > [ 0.030562] 1e60: c168fcd8 c1700000 c17097cc c019fa2c c1709a44 d080c00c c168fcfc c1700000
> > [ 0.030754] 1e80: c17097cc c1701eb8 c1700000 c076fe04 c1700000 600000d3 c0824188 80000053
> > [ 0.030924] 1ea0: ffffffff c1701eec 00000000 c1700000 c1700000 c0100b70 c2013400 c20a0000
> > [ 0.031096] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
> > [ 0.031267] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
> > [ 0.031439] 1f00: 00000051 00000000 00000000 c168f31c c17097cc c2013400 c2013708 3123ccba
> > [ 0.031614] 1f20: 00000001 c2013400 00000960 c20a0000 c1422958 c1709380 c16804d8 c1646020
> > [ 0.031790] 1f40: c14d1eec c082626c c1f6f4e0 c2013400 00000000 c1422958 c1709380 c16461d4
> > [ 0.031961] 1f60: 00000000 00000000 00000000 c0f7d3a4 c186fdcc c1700000 c17093cc c16804d8
> > [ 0.032155] 1f80: c18f8e20 c160fe80 c1903000 c1663a80 00000012 c1700000 c1709380 00000000
> > [ 0.032327] 1fa0: c170caf0 c1600e50 ffffffff ffffffff 00000000 c1600588 00000000 c1700000
> > [ 0.032500] 1fc0: 00000000 c1663a80 3126c2ba 00000000 00000000 c1600330 00000051 10c0387d
> > [ 0.032671] 1fe0: ffffffff 8833b000 410fc075 10c5387d 00000000 00000000 00000000 00000000
> > [ 0.032889] [<c01a59c4>] (handle_strict_flow_irq) from [<c019fa2c>] (handle_domain_irq+0x64/0xa4)
> > [ 0.033027] [<c019fa2c>] (handle_domain_irq) from [<c076fe04>] (gic_handle_irq+0x84/0xbc)
> > [ 0.033076] [<c076fe04>] (gic_handle_irq) from [<c0100b70>] (__irq_svc+0x70/0x98)
> > [ 0.033140] Exception stack(0xc1701eb8 to 0xc1701f00)
> > [ 0.033204] 1ea0: c2013400 c20a0000
> > [ 0.033377] 1ec0: 00000050 00000000 c20a00a0 c07c0ad4 00000910 c2013400 00000000 c20a0000
> > [ 0.033551] 1ee0: c1700000 00000050 00000000 c1701f08 c0824188 c0824188 80000053 ffffffff
> > [ 0.033596] [<c0100b70>] (__irq_svc) from [<c0824188>] (do_update_region+0x108/0x19c)
> > [ 0.033652] [<c0824188>] (do_update_region) from [<c082626c>] (csi_J+0xf0/0x290)
> > [ 0.033685] [<c082626c>] (csi_J) from [<c16461d4>] (con_init+0x1b4/0x248)
> > [ 0.033726] [<c16461d4>] (con_init) from [<c160fe80>] (console_init+0x21c/0x330)
> > [ 0.033760] [<c160fe80>] (console_init) from [<c1600e50>] (start_kernel+0x508/0x6d4)
> > [ 0.033797] [<c1600e50>] (start_kernel) from [<00000000>] (0x0)
> > [ 0.034079] Code: bad PC value
> > [ 0.034726] ---[ end trace c1324fc4facef313 ]---
> > [ 0.035004] Kernel panic - not syncing: Fatal exception in interrupt
> >
> > https://kerneltests.org/builders/qemu-arm-v7-next/builds/66 provides
> > a more detailed log.
> >
> > Reverting this patch fixes the problem.
> >
>
> Passing the stacktrace through scripts/decode_stracktrace.sh would help by
> giving line numbers - that said I'm pretty sure this is caused by the
> flow-handler now issuing a chip->irq_ack(), and this has an irqchip that
> doesn't have one.
>
> The hardware name points me at arch/arm/mach-imx/mach-imx6ul.c, but I can't
> figure out an actual devicetree/irqchip layout from this.
>
> Ah, with some more grepping I did find arch/arm/mach-imx/gpc.c, and bingo,
> no .irq_ack().
>
> Now, the above makes me feel like this is the start of a wild goose chase
> for irqchips in a similar situation. I'm tempted to apply a dumb
>
> .irq_ack = irq_chip_ack_parent
>
> to all irqchips without one specified. Let's see how bad this gets, does
> the below help?
You need to be selective. A number of irqchips don't need an irq_ack
callback.
>
> ---
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index ebc4339b8be4..a4e10daa3ae7 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -158,6 +158,7 @@ static void imx_gpc_irq_mask(struct irq_data *d)
>
> static struct irq_chip imx_gpc_chip = {
> .name = "GPC",
> + .irq_ack = irq_chip_ack_parent,
> .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = imx_gpc_irq_mask,
> .irq_unmask = imx_gpc_irq_unmask,
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 5b5a365dbd5e..e304c80cd403 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -126,6 +126,7 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>
> static struct irq_chip gpcv2_irqchip_data_chip = {
> .name = "GPCv2",
> + .irq_ack = irq_chip_ack_parent,
> .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = imx_gpcv2_irq_mask,
> .irq_unmask = imx_gpcv2_irq_unmask,
>
This is going and-up in a wack-a-mole game. There is probably a bunch
of these all over the place. I'd rather squash it at the root,
i.e. with something like this (untested):
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 099bc7e13d1b..601ad3fc47cd 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -410,7 +410,12 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
void ack_irq(struct irq_desc *desc)
{
- desc->irq_data.chip->irq_ack(&desc->irq_data);
+ struct irq_data *data = &desc->irq_data;
+
+ while (!data->chip->irq_ack)
+ data = data->parent_data;
+
+ data->chip->irq_ack(&desc->irq_data);
if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
irq_state_set_flow_masked(desc);
We probably need something similar for irq_eoi().
This however shows a more fundamental problem, I'm afraid. We set
IRQCHIP_AUTOMASKS_FLOW in the GIC drivers (i.e. at the root), but test
for it at the top of the hierarchy. As soon as we have more than a
single layer of irqchip, this will do the wrong thing (or at least
miss the masking optimisation).
This probably advocates for moving the flag into the descriptor. This
really makes sense, as the flow is global to the whole stack, not just
to the localised irqchip.
In order to restore -next into a working state, I'm temporarily
dropping this series. Hopefully, we can sort this out before the merge
window and reinstate it.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-15 6:54 ` Marc Zyngier
@ 2021-08-17 0:30 ` Valentin Schneider
2021-08-18 16:58 ` Marc Zyngier
2021-08-30 16:54 ` Valentin Schneider
1 sibling, 1 reply; 14+ messages in thread
From: Valentin Schneider @ 2021-08-17 0:30 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On 15/08/21 07:54, Marc Zyngier wrote:
> This is going and-up in a wack-a-mole game. There is probably a bunch
> of these all over the place. I'd rather squash it at the root,
> i.e. with something like this (untested):
>
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 099bc7e13d1b..601ad3fc47cd 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -410,7 +410,12 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
>
> void ack_irq(struct irq_desc *desc)
> {
> - desc->irq_data.chip->irq_ack(&desc->irq_data);
> + struct irq_data *data = &desc->irq_data;
> +
> + while (!data->chip->irq_ack)
> + data = data->parent_data;
> +
> + data->chip->irq_ack(&desc->irq_data);
>
> if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> irq_state_set_flow_masked(desc);
>
> We probably need something similar for irq_eoi().
>
> This however shows a more fundamental problem, I'm afraid. We set
> IRQCHIP_AUTOMASKS_FLOW in the GIC drivers (i.e. at the root), but test
> for it at the top of the hierarchy. As soon as we have more than a
> single layer of irqchip, this will do the wrong thing (or at least
> miss the masking optimisation).
>
Yup.
> This probably advocates for moving the flag into the descriptor. This
> really makes sense, as the flow is global to the whole stack, not just
> to the localised irqchip.
>
Are we guaranteed to have
.irq_ack \in {NULL, irq_chip_ack_parent}
for all intermediate (!root) irqchips? I don't see why that wouldn't be the
case, and with that in mind what you described makes sense to me.
> In order to restore -next into a working state, I'm temporarily
> dropping this series. Hopefully, we can sort this out before the merge
> window and reinstate it.
>
I'm away from any keyboard for most of this week, but I'll get to it by the
weekend.
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-17 0:30 ` Valentin Schneider
@ 2021-08-18 16:58 ` Marc Zyngier
2021-08-22 22:16 ` Valentin Schneider
0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2021-08-18 16:58 UTC (permalink / raw)
To: Valentin Schneider; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On Tue, 17 Aug 2021 01:30:43 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
>
> On 15/08/21 07:54, Marc Zyngier wrote:
> > This is going and-up in a wack-a-mole game. There is probably a bunch
> > of these all over the place. I'd rather squash it at the root,
> > i.e. with something like this (untested):
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 099bc7e13d1b..601ad3fc47cd 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -410,7 +410,12 @@ void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
> >
> > void ack_irq(struct irq_desc *desc)
> > {
> > - desc->irq_data.chip->irq_ack(&desc->irq_data);
> > + struct irq_data *data = &desc->irq_data;
> > +
> > + while (!data->chip->irq_ack)
> > + data = data->parent_data;
> > +
> > + data->chip->irq_ack(&desc->irq_data);
> >
> > if (desc->irq_data.chip->flags & IRQCHIP_AUTOMASKS_FLOW)
> > irq_state_set_flow_masked(desc);
> >
> > We probably need something similar for irq_eoi().
> >
> > This however shows a more fundamental problem, I'm afraid. We set
> > IRQCHIP_AUTOMASKS_FLOW in the GIC drivers (i.e. at the root), but test
> > for it at the top of the hierarchy. As soon as we have more than a
> > single layer of irqchip, this will do the wrong thing (or at least
> > miss the masking optimisation).
> >
>
> Yup.
>
> > This probably advocates for moving the flag into the descriptor. This
> > really makes sense, as the flow is global to the whole stack, not just
> > to the localised irqchip.
> >
>
> Are we guaranteed to have
>
> .irq_ack \in {NULL, irq_chip_ack_parent}
>
> for all intermediate (!root) irqchips? I don't see why that wouldn't
> be the case, and with that in mind what you described makes sense to
> me.
An intermediate layer is allowed to implement its own irq_ack that is
not irq_chip_ack_parent, but it then has to call irq_chip_ack_parent
itself.
There is the bizarre case of drivers/gpio/gpio-thunderx.c that changes
the irqchip flow to use either handle_fasteoi_ack_irq or
handle_fasteoi_mask_irq, which won't play very nicely with this.
Someone said Cavium?
>
> > In order to restore -next into a working state, I'm temporarily
> > dropping this series. Hopefully, we can sort this out before the merge
> > window and reinstate it.
> >
>
> I'm away from any keyboard for most of this week, but I'll get to it by the
> weekend.
No worries, enjoy your break!
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-18 16:58 ` Marc Zyngier
@ 2021-08-22 22:16 ` Valentin Schneider
2021-08-23 9:33 ` Marc Zyngier
0 siblings, 1 reply; 14+ messages in thread
From: Valentin Schneider @ 2021-08-22 22:16 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On 18/08/21 17:58, Marc Zyngier wrote:
> On Tue, 17 Aug 2021 01:30:43 +0100,
> Valentin Schneider <valentin.schneider@arm.com> wrote:
>> Are we guaranteed to have
>>
>> .irq_ack \in {NULL, irq_chip_ack_parent}
>>
>> for all intermediate (!root) irqchips? I don't see why that wouldn't
>> be the case, and with that in mind what you described makes sense to
>> me.
>
> An intermediate layer is allowed to implement its own irq_ack that is
> not irq_chip_ack_parent, but it then has to call irq_chip_ack_parent
> itself.
>
Right, makes sense.
> There is the bizarre case of drivers/gpio/gpio-thunderx.c that changes
> the irqchip flow to use either handle_fasteoi_ack_irq or
> handle_fasteoi_mask_irq, which won't play very nicely with this.
> Someone said Cavium?
>
Humph...
I'm not familiar at all with the gpiolib irqchips, but I was under the
impression those would involve chained IRQs (it does appear to be the case
for the pl061 GPIOs on a Juno). For those, the innermost desc would be handled
via chained_irq_{enter, exit}() [!!!], and the outermost one via whatever
flow was installed by the relevant driver.
I can't easily grok what goes on between that gpio-thunderx.c driver and
gpiolib, but since that GPIO chip has
.irq_eoi = irq_chip_eoi_parent,
and
girq->parent_domain =
irq_get_irq_data(txgpio->msix_entries[0].vector)->domain;
(GPIOs hooked to MSI-X? Do I want to know?)
I'm guessing it is *not* chained, which means the irq_set_handler_locked()
affects the entire stack :/
[!!!] Speaking of chained IRQs, I'm now thinking this series breaks them;
chained_irq_enter() + chained_irq_exit() will only issue an ->irq_eoi(),
skipping the ->irq_ack()... One more thing to add to the list!
>>
>> > In order to restore -next into a working state, I'm temporarily
>> > dropping this series. Hopefully, we can sort this out before the merge
>> > window and reinstate it.
>> >
>>
>> I'm away from any keyboard for most of this week, but I'll get to it by the
>> weekend.
>
> No worries, enjoy your break!
>
I sure did, Thanks!
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-22 22:16 ` Valentin Schneider
@ 2021-08-23 9:33 ` Marc Zyngier
2021-08-23 10:38 ` Valentin Schneider
0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2021-08-23 9:33 UTC (permalink / raw)
To: Valentin Schneider; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On Sun, 22 Aug 2021 23:16:10 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
>
> On 18/08/21 17:58, Marc Zyngier wrote:
> > On Tue, 17 Aug 2021 01:30:43 +0100,
> > Valentin Schneider <valentin.schneider@arm.com> wrote:
> >> Are we guaranteed to have
> >>
> >> .irq_ack \in {NULL, irq_chip_ack_parent}
> >>
> >> for all intermediate (!root) irqchips? I don't see why that wouldn't
> >> be the case, and with that in mind what you described makes sense to
> >> me.
> >
> > An intermediate layer is allowed to implement its own irq_ack that is
> > not irq_chip_ack_parent, but it then has to call irq_chip_ack_parent
> > itself.
> >
>
> Right, makes sense.
>
> > There is the bizarre case of drivers/gpio/gpio-thunderx.c that changes
> > the irqchip flow to use either handle_fasteoi_ack_irq or
> > handle_fasteoi_mask_irq, which won't play very nicely with this.
> > Someone said Cavium?
> >
>
> Humph...
>
> I'm not familiar at all with the gpiolib irqchips, but I was under the
> impression those would involve chained IRQs (it does appear to be the case
> for the pl061 GPIOs on a Juno). For those, the innermost desc would be handled
> via chained_irq_{enter, exit}() [!!!], and the outermost one via whatever
> flow was installed by the relevant driver.
Not all of them are built like this. There is actually a bunch of
these build as full hierarchies (QC, nvidia and some others).
> I can't easily grok what goes on between that gpio-thunderx.c driver and
> gpiolib, but since that GPIO chip has
>
> .irq_eoi = irq_chip_eoi_parent,
>
> and
>
> girq->parent_domain =
> irq_get_irq_data(txgpio->msix_entries[0].vector)->domain;
>
> (GPIOs hooked to MSI-X? Do I want to know?)
It's good, isn't it? TX1 has all its HW appearing as PCI, even if it
clearly isn't PCI underneath.
>
> I'm guessing it is *not* chained, which means the irq_set_handler_locked()
> affects the entire stack :/
It does. We can probably fix that, but I won't be able to test (my TX1
was taken away a few months ago...). I'll accept body donations, for
scientific purposes.
>
> [!!!] Speaking of chained IRQs, I'm now thinking this series breaks them;
> chained_irq_enter() + chained_irq_exit() will only issue an ->irq_eoi(),
> skipping the ->irq_ack()... One more thing to add to the list!
Urghh... Yeah, that's awful.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-23 9:33 ` Marc Zyngier
@ 2021-08-23 10:38 ` Valentin Schneider
2021-08-23 12:17 ` Marc Zyngier
0 siblings, 1 reply; 14+ messages in thread
From: Valentin Schneider @ 2021-08-23 10:38 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On 23/08/21 10:33, Marc Zyngier wrote:
> On Sun, 22 Aug 2021 23:16:10 +0100,
> Valentin Schneider <valentin.schneider@arm.com> wrote:
>>
>> On 18/08/21 17:58, Marc Zyngier wrote:
>> > There is the bizarre case of drivers/gpio/gpio-thunderx.c that changes
>> > the irqchip flow to use either handle_fasteoi_ack_irq or
>> > handle_fasteoi_mask_irq, which won't play very nicely with this.
>> > Someone said Cavium?
>> >
>>
>> Humph...
>>
>> I'm not familiar at all with the gpiolib irqchips, but I was under the
>> impression those would involve chained IRQs (it does appear to be the case
>> for the pl061 GPIOs on a Juno). For those, the innermost desc would be handled
>> via chained_irq_{enter, exit}() [!!!], and the outermost one via whatever
>> flow was installed by the relevant driver.
>
> Not all of them are built like this. There is actually a bunch of
> these build as full hierarchies (QC, nvidia and some others).
>
I see, thanks!
>> I can't easily grok what goes on between that gpio-thunderx.c driver and
>> gpiolib, but since that GPIO chip has
>>
>> .irq_eoi = irq_chip_eoi_parent,
>>
>> and
>>
>> girq->parent_domain =
>> irq_get_irq_data(txgpio->msix_entries[0].vector)->domain;
>>
>> (GPIOs hooked to MSI-X? Do I want to know?)
>
> It's good, isn't it? TX1 has all its HW appearing as PCI, even if it
> clearly isn't PCI underneath.
>
>>
>> I'm guessing it is *not* chained, which means the irq_set_handler_locked()
>> affects the entire stack :/
>
> It does. We can probably fix that, but I won't be able to test (my TX1
> was taken away a few months ago...). I'll accept body donations, for
> scientific purposes.
>
Looks like there are still some over on s/packet/equinix/, so I should be
able to poke at one.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-23 10:38 ` Valentin Schneider
@ 2021-08-23 12:17 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2021-08-23 12:17 UTC (permalink / raw)
To: Valentin Schneider; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On Mon, 23 Aug 2021 11:38:58 +0100,
Valentin Schneider <valentin.schneider@arm.com> wrote:
>
> On 23/08/21 10:33, Marc Zyngier wrote:
> > On Sun, 22 Aug 2021 23:16:10 +0100,
> > Valentin Schneider <valentin.schneider@arm.com> wrote:
> >>
> >> On 18/08/21 17:58, Marc Zyngier wrote:
> >> > There is the bizarre case of drivers/gpio/gpio-thunderx.c that changes
> >> > the irqchip flow to use either handle_fasteoi_ack_irq or
> >> > handle_fasteoi_mask_irq, which won't play very nicely with this.
> >> > Someone said Cavium?
> >> >
> >>
> >> Humph...
> >>
> >> I'm not familiar at all with the gpiolib irqchips, but I was under the
> >> impression those would involve chained IRQs (it does appear to be the case
> >> for the pl061 GPIOs on a Juno). For those, the innermost desc would be handled
> >> via chained_irq_{enter, exit}() [!!!], and the outermost one via whatever
> >> flow was installed by the relevant driver.
> >
> > Not all of them are built like this. There is actually a bunch of
> > these build as full hierarchies (QC, nvidia and some others).
> >
>
> I see, thanks!
>
> >> I can't easily grok what goes on between that gpio-thunderx.c driver and
> >> gpiolib, but since that GPIO chip has
> >>
> >> .irq_eoi = irq_chip_eoi_parent,
> >>
> >> and
> >>
> >> girq->parent_domain =
> >> irq_get_irq_data(txgpio->msix_entries[0].vector)->domain;
> >>
> >> (GPIOs hooked to MSI-X? Do I want to know?)
> >
> > It's good, isn't it? TX1 has all its HW appearing as PCI, even if it
> > clearly isn't PCI underneath.
> >
> >>
> >> I'm guessing it is *not* chained, which means the irq_set_handler_locked()
> >> affects the entire stack :/
> >
> > It does. We can probably fix that, but I won't be able to test (my TX1
> > was taken away a few months ago...). I'll accept body donations, for
> > scientific purposes.
> >
>
> Looks like there are still some over on s/packet/equinix/, so I should be
> able to poke at one.
That's where mine used to live, but the WoA people decided that I
really didn't need one, and I'm now only allowed to borrow one of the
old eMAGs. If you can pull strings, let me know! :D
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] irqchip/gic: Convert to handle_strict_flow_irq()
2021-08-15 6:54 ` Marc Zyngier
2021-08-17 0:30 ` Valentin Schneider
@ 2021-08-30 16:54 ` Valentin Schneider
1 sibling, 0 replies; 14+ messages in thread
From: Valentin Schneider @ 2021-08-30 16:54 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Guenter Roeck, linux-kernel, linux-arm-kernel
On 15/08/21 07:54, Marc Zyngier wrote:
> In order to restore -next into a working state, I'm temporarily
> dropping this series. Hopefully, we can sort this out before the merge
> window and reinstate it.
>
So what I thought would be a small weekend job turned to be a bit more
involved...
- The throughput improvement with v4 is half of what it was on v3. I
rebased v3 on the last tip/irq/core and this also took a hit, I'm digging
into it.
- TX1 is being an absolute PITA to work with, I can't seem to boot anything
else than the distro kernel ATM (grub commits seppuku).
- Last but not least, I'll soon be AWOL for 3 weeks, a good chunk of it
involving sitting atop a mountain without electricity. So even if I can
magically fix all of the above say tomorrow, I'd rather be available to
~watch~ handle any unforeseen fireworks.
Hopefully I'll catch the next revision :)
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread