irqchip/sifive-plic: Fixup EOI failed when masked

PATCH Superseded [email protected] 4 participants 6 messages Nov 1, 2021 – Nov 5, 2021

Subsystems (3)

RISC-V irqchip drivers SiFive drivers

lore

Share & tools

lore

[email protected]

author

2021-11-01 13:18 UTC #0 lore raw

PATCH v6 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

Reported-by Vincent Pelletier <[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]>

---

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

index cf74cfa82045..259065d271ef 100644
@@ -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

Nikita Shubin

Tested-by on v6 1/1

2021-11-01 13:47 UTC #1 lore raw

On Mon, 1 Nov 2021 21:17:36 +0800
[email protected] wrote:

Hi Guo Ren,

Thank you for your patch.

May be it should be applied to stable ?

Tested-by Nikita Shubin <[email protected]>

Quoted 82 lines

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
Reported-by Vincent Pelletier <[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]>
---
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(-)
(quoted diff) +0 -0
b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef 100644 --- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
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 = {

Guo Ren

author

2021-11-02 01:34 UTC #2 lore raw

Quoted 9 lines · Nikita Shubin

On Mon, 1 Nov 2021 21:17:36 +0800
[email protected] wrote:
Hi Guo Ren,
Thank you for your patch.
May be it should be applied to stable ?

Yes, I would Cc stable next.

Quoted 86 lines

Tested-by Nikita Shubin <[email protected]>
Quoted 82 lines
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
Reported-by Vincent Pelletier <[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]>
---
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(-)
(quoted diff) +0 -0
b/drivers/irqchip/irq-sifive-plic.c index cf74cfa82045..259065d271ef 100644 --- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
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 · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Anup Patel

2021-11-04 14:41 UTC #3 lore raw

In reply to [email protected]

Quoted 21 lines · On Mon, Nov 1, 2021 at 6:47 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 9 lines

Reported-by Vincent Pelletier <[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]>

Please include a Fixes: tag

Also, I see that you have dropped the DT bindings patch. We still need separate compatible string for T-HEAD PLIC because OpenSBI will use it for other work-arounds.

I suggest to include to more patches in this series:
1) Your latest T-HEAD PLIC DT bindings patch
2) Separate patch to use T-HEAD PLIC compatible in PLIC driver

Regards,
Anup

Quoted 51 lines

---
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 -2
index cf74cfa82045..259065d271ef 100644
@@ -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 = {
--

Quoted 2 lines

2.25.1

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Marc Zyngier

2021-11-04 14:57 UTC #4 lore raw

Quoted 35 lines · Anup Patel

Quoted 21 lines · On Mon, Nov 1, 2021 at 6:47 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 9 lines
Reported-by Vincent Pelletier <[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]>
Please include a Fixes: tag

Quoted 7 lines

Also, I see that you have dropped the DT bindings patch. We still need separate compatible string for T-HEAD PLIC because OpenSBI will use it for other work-arounds.
I suggest to include to more patches in this series:
1) Your latest T-HEAD PLIC DT bindings patch
2) Separate patch to use T-HEAD PLIC compatible in PLIC driver

No, please keep things separate. The PLIC is broken *today*, and I want to take a patch for -rc1. The rest (compatible and such) is a new feature and can wait until 5.17.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Guo Ren

author

2021-11-05 01:07 UTC #5 lore raw

Quoted 36 lines · Marc Zyngier

Quoted 33 lines · Anup Patel
Quoted 30 lines · On Mon, Nov 1, 2021 at 6:47 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
Reported-by Vincent Pelletier <[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 1 line

Quoted 1 line
Please include a Fixes: tag

Okay

Quoted 8 lines

Quoted 8 lines
Also, I see that you have dropped the DT bindings patch. We still need separate compatible string for T-HEAD PLIC because OpenSBI will use it for other work-arounds.
I suggest to include to more patches in this series:
1) Your latest T-HEAD PLIC DT bindings patch
2) Separate patch to use T-HEAD PLIC compatible in PLIC driver

Thx for the suggestion, and I would put above in 5.17 as Mark suggested.

Quoted 4 lines

No, please keep things separate. The PLIC is broken *today*, and I want to take a patch for -rc1. The rest (compatible and such) is a new feature and can wait until 5.17.

Okay

Quoted 5 lines

        M.
--
Without deviation from the norm, progress is not possible.

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/