irqchip/sifive-plic: Fixup EOI failed when masked
Subsystems (6)
PATCH v7 1/1 irqchip/sifive-plic: Fixup EOI failed when masked
From: Guo Ren <[email protected]>
When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].
Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the
^^ ^^^^^^^^^ ^^^^^^^
completion is silently ignored.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
---
Changes since V7:
- Add Fixes tag
- Add Tested-by
- Add Cc stable
Changes since V6:
- Propagate to plic_irq_eoi for all riscv,plic by Nikita Shubin
- Remove thead related codes
Changes since V5:
- Move back to mask/unmask
- Fixup the problem in eoi callback
- Remove allwinner,sun20i-d1 IRQCHIP_DECLARE
- Rewrite comment log
Changes since V4:
- Update comment by Anup
Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference
Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.
--- drivers/irqchip/irq-sifive-plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/irqchip/irq-sifive-plic.c +7 -1
@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) {struct plic_handler *handler =this_cpu_ptr (&plic_handlers); -writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +if (irqd_irq_masked (d)) { +plic_irq_unmask (d); +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +plic_irq_mask (d); + }else { +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); + } }static struct irq_chip plic_chip = {
Signature · 1 line
2.25.1
Quoted 20 lines · On 2021-11-05 17:47, [email protected] wrote:
From: Guo Ren <[email protected]>
When using "
devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the
^^ ^^^^^^^^^ ^^^^^^^completion is silently ignored.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
Quoted 12 lines
Fixes bb0fed1c60cc ("irqchip/sifive-plic: Switch to fasteoi flow")Reported-by Vincent Pelletier <[email protected]>Tested-by Nikita Shubin <[email protected]>Signed-off-by Guo Ren <[email protected]>Cc Anup Patel <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Marc Zyngier <[email protected]>Cc Palmer Dabbelt <[email protected]>Cc Atish Patra <[email protected]>Cc Nikita Shubin <[email protected]>Cc incent Pelletier <[email protected]>
Thanks for this patch. From what I understand, it fixes among other things the possibility to read the DA9063 RTC more than once.
Does it means that we could now enable it in the device tree? I mean something like the following patch that unfortunately I can't test now:
arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts +12 -0
@@ -70,6 +70,10 @@ pmic@58 { interrupts = <1 IRQ_TYPE_LEVEL_LOW>; interrupt-controller; + onkey { + compatible = "dlg,da9063-onkey"; + }; + regulators { vdd_bcore1: bcore1 { regulator-min-microvolt = <900000>;
@@ -205,6 +209,14 @@ vdd_ldo11: ldo11 { regulator-always-on; }; }; + + rtc { + compatible = "dlg,da9063-rtc"; + }; + + wdt { + compatible = "dlg,da9063-watchdog"; + }; }; };
Signature · 2 lines
Aurelien Jarno GPG: 4096R/1DDD8C9B [email protected] http://www.aurel32.net
Hello, Aurelien!
Quoted 37 lines · On Sat, 6 Nov 2021 13:11:51 +0100 Aurelien Jarno <[email protected]> wrote:
Quoted 23 lines · On 2021-11-05 17:47, [email protected] wrote:
From: Guo Ren <[email protected]>
When using "
devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the ^^ ^^^^^^^^^ ^^^^^^^ completion is silently ignored.
[1]
http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2]
https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adocQuoted 12 lines
Fixes bb0fed1c60cc ("irqchip/sifive-plic: Switch to fasteoi flow")Reported-by Vincent Pelletier <[email protected]>Tested-by Nikita Shubin <[email protected]>Signed-off-by Guo Ren <[email protected]>Cc Anup Patel <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Marc Zyngier <[email protected]>Cc Palmer Dabbelt <[email protected]>Cc Atish Patra <[email protected]>Cc Nikita Shubin <[email protected]>Cc incent Pelletier <[email protected]>
Quoted 2 lines
Thanks for this patch. From what I understand, it fixes among other things the possibility to read the DA9063 RTC more than once.
Well RTC works definitely, through you need some patch to use it as a wakeup source:
https://lkml.org/lkml/2021/11/1/800
and, AFAIK currently SiFive Unmatched lack's PM.
I still haven't tested the Watchdog and Onkey.
Quoted 4 lines
Does it means that we could now enable it in the device tree? I mean something like the following patch that unfortunately I can't test now:
Adding RTC is really useful and adding Watchdog along with Onkey shouldn't make any harm.
Quoted 31 lines
(quoted diff) +0
-0
b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dtsindex2e4ea84f27e7..c357b48582f7100644 ---a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts+++b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts@@ -70,6 +70,10 @@ pmic@58 { interrupts = <1IRQ_TYPE_LEVEL_LOW>;arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts +12
-0interrupt-controller; + onkey { + compatible = "dlg,da9063-onkey"; + }; + regulators { vdd_bcore1: bcore1 { regulator-min-microvolt = <900000>;@@ -205,6 +209,14 @@ vdd_ldo11: ldo11 { regulator-always-on; }; }; + + rtc { + compatible = "dlg,da9063-rtc"; + }; + + wdt { + compatible = "dlg,da9063-watchdog"; + }; }; };
Quoted 18 lines · On Fri, Nov 5, 2021 at 3:18 PM <[email protected]> wrote:
From: Guo Ren <[email protected]>
When using "
devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the
^^ ^^^^^^^^^ ^^^^^^^completion is silently ignored.
Quoted 7 lines
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adocFixes bb0fed1c60cc ("irqchip/sifive-plic: Switch to fasteoi flow")Reported-by Vincent Pelletier <[email protected]>Tested-by Nikita Shubin <[email protected]>Signed-off-by Guo Ren <[email protected]>
Looks good to me.
Regards,
Anup
Quoted 66 lines
Cc Anup Patel <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Marc Zyngier <[email protected]>Cc Palmer Dabbelt <[email protected]>Cc Atish Patra <[email protected]>Cc Nikita Shubin <[email protected]>Cc incent Pelletier <[email protected]>---
Changes since V7:
- Add Fixes tag
- Add Tested-by
- Add Cc stableChanges since V6:
- Propagate toplic_irq_eoifor all riscv,plic by Nikita Shubin
- Remove thead related codesChanges since V5:
- Move back to mask/unmask
- Fixup the problem in eoi callback
- Remove allwinner,sun20i-d1IRQCHIP_DECLARE
- Rewrite comment logChanges since V4:
- Update comment by AnupChanges since V3:
- Rename "c9xx" to "c900"
- Addsifive_plic_chipandthead_plic_chipfor differenceChanges since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block inplic_init()about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.--- drivers/irqchip/irq-sifive-plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/irqchip/irq-sifive-plic.c +7
-2@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) {struct plic_handler *handler =this_cpu_ptr (&plic_handlers); -writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +if (irqd_irq_masked (d)) { +plic_irq_unmask (d); +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +plic_irq_mask (d); + }else { +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); + } }static struct irq_chip plic_chip = { --2.25.1
Quoted 23 lines · Anup Patel
Quoted 21 lines · On Fri, Nov 5, 2021 at 3:18 PM <[email protected]> wrote:
From: Guo Ren <[email protected]>
When using "
devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the
^^ ^^^^^^^^^ ^^^^^^^completion is silently ignored.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
Quoted 8 lines
Quoted 4 lines
Fixes bb0fed1c60cc ("irqchip/sifive-plic: Switch to fasteoi flow")Reported-by Vincent Pelletier <[email protected]>Tested-by Nikita Shubin <[email protected]>Signed-off-by Guo Ren <[email protected]>Looks good to me.
Reviewed-by Anup Patel <[email protected]>
Thx
@Marc Zyngier
Could you help to take the patch into your tree include "Anup's Reviewed-by"? Or Let me update a new version of the patch.
Quoted 70 lines
Regards,
AnupQuoted 66 lines
Cc Anup Patel <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Marc Zyngier <[email protected]>Cc Palmer Dabbelt <[email protected]>Cc Atish Patra <[email protected]>Cc Nikita Shubin <[email protected]>Cc incent Pelletier <[email protected]>---
Changes since V7:
- Add Fixes tag
- Add Tested-by
- Add Cc stableChanges since V6:
- Propagate toplic_irq_eoifor all riscv,plic by Nikita Shubin
- Remove thead related codesChanges since V5:
- Move back to mask/unmask
- Fixup the problem in eoi callback
- Remove allwinner,sun20i-d1IRQCHIP_DECLARE
- Rewrite comment logChanges since V4:
- Update comment by AnupChanges since V3:
- Rename "c9xx" to "c900"
- Addsifive_plic_chipandthead_plic_chipfor differenceChanges since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block inplic_init()about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.--- drivers/irqchip/irq-sifive-plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/irqchip/irq-sifive-plic.c +7
-2@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) {struct plic_handler *handler =this_cpu_ptr (&plic_handlers); -writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +if (irqd_irq_masked (d)) { +plic_irq_unmask (d); +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +plic_irq_mask (d); + }else { +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); + } }static struct irq_chip plic_chip = { --2.25.1
Signature · 4 lines
Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
Quoted 29 lines · Guo Ren
Quoted 28 lines · Anup Patel
Quoted 25 lines · On Fri, Nov 5, 2021 at 3:18 PM <[email protected]> wrote:
From: Guo Ren <[email protected]>
When using "
devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in the driver, only the first interrupt could be handled, and continue irq is blocked by hw. Because the riscv plic couldn't complete masked irq source which has been disabled in enable register. The bug was firstly reported in [1].Here is the description of Interrupt Completion in PLIC spec [2]:
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the
^^ ^^^^^^^^^ ^^^^^^^completion is silently ignored.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adocFixes bb0fed1c60cc ("irqchip/sifive-plic: Switch to fasteoi flow")Reported-by Vincent Pelletier <[email protected]>Tested-by Nikita Shubin <[email protected]>Signed-off-by Guo Ren <[email protected]>
Quoted 8 lines
Quoted 3 lines
Looks good to me.
Reviewed-by Anup Patel <[email protected]>Thx
@Marc Zyngier
Could you help to take the patch into your tree include "Anup's Reviewed-by"? Or Let me update a new version of the patch.
You should have received [1], which shows you what I have done.
M.
[1] https://lore.kernel.org/all/163620881803.626.5045336370262044443.tip-bot2@tip-bot2/
Signature · 1 line
Jazz is not dead. It just smells funny...
The 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: 4d7a0f5ebd8df659d78122c350283a84a36c2e05 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/4d7a0f5ebd8df659d78122c350283a84a36c2e05 Author: Guo Ren <[email protected]> AuthorDate: Fri, 05 Nov 2021 17:47:48 +08:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Sat, 06 Nov 2021 14:24:49
irqchip/sifive-plic: Fixup EOI failed when masked
When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in a driver, only the first interrupt is handled, and following interrupts are never delivered (initially reported in [1]).
That's because the RISC-V PLIC cannot EOI masked interrupts, as explained in the description of Interrupt Completion in the PLIC spec [2]:
<quote>
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that *is currently enabled* for the target, the completion is silently ignored.
</quote>
Re-enable the interrupt before completion if it has been masked during the handling, and remask it afterwards.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
--- drivers/irqchip/irq-sifive-plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/irqchip/irq-sifive-plic.c +7 -1
@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) {struct plic_handler *handler =this_cpu_ptr (&plic_handlers); -writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +if (irqd_irq_masked (d)) { +plic_irq_unmask (d); +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +plic_irq_mask (d); + }else { +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); + } }static struct irq_chip plic_chip = {
The 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: 69ea463021be0d159ab30f96195fb0dd18ee2272 Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/69ea463021be0d159ab30f96195fb0dd18ee2272 Author: Guo Ren <[email protected]> AuthorDate: Fri, 05 Nov 2021 17:47:48 +08:00 Committer: Marc Zyngier <[email protected]> CommitterDate: Fri, 12 Nov 2021 16:09:51
irqchip/sifive-plic: Fixup EOI failed when masked
When using "devm_request_threaded_irq(,,,,IRQF_ONESHOT,,)" in a driver, only the first interrupt is handled, and following interrupts are never delivered (initially reported in [1]).
That's because the RISC-V PLIC cannot EOI masked interrupts, as explained in the description of Interrupt Completion in the PLIC spec [2]:
<quote>
The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that *is currently enabled* for the target, the completion is silently ignored.
</quote>
Re-enable the interrupt before completion if it has been masked during the handling, and remask it afterwards.
[1] http://lists.infradead.org/pipermail/linux-riscv/2021-July/007441.html
[2] https://github.com/riscv/riscv-plic-spec/blob/8bc15a35d07c9edf7b5d23fec9728302595ffc4d/riscv-plic.adoc
--- drivers/irqchip/irq-sifive-plic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/irqchip/irq-sifive-plic.c +7 -1
@@ -163,7 +163,13 @@ static void plic_irq_eoi(struct irq_data *d) {struct plic_handler *handler =this_cpu_ptr (&plic_handlers); -writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +if (irqd_irq_masked (d)) { +plic_irq_unmask (d); +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); +plic_irq_mask (d); + }else { +writel (d->hwirq, handler->hart_base +CONTEXT_CLAIM ); + } }static struct irq_chip plic_chip = {