* [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits
@ 2023-07-17 18:58 Samuel Holland
2023-07-18 9:26 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Samuel Holland @ 2023-07-17 18:58 UTC (permalink / raw)
To: Marc Zyngier
Cc: Samuel Holland, Palmer Dabbelt, Paul Walmsley, Samuel Holland,
Thomas Gleixner, linux-kernel, linux-riscv
Writes to the PLIC completion register are ignored if the enable bit for
that (interrupt, hart) combination is cleared. This leaves the interrupt
in a claimed state, preventing it from being triggered again.
Originally, the enable bit was cleared in the .irq_mask operation, and
commit 69ea463021be ("irqchip/sifive-plic: Fixup EOI failed when masked")
added a workaround for this issue. Later, commit a1706a1c5062
("irqchip/sifive-plic: Separate the enable and mask operations") moved
toggling the enable bit to the .irq_enable/.irq_disable operations and
removed the workaround.
However, there are still places where .irq_disable can be called from
inside the hard IRQ handler, for example in irq_pm_check_wakeup(). As a
result, this issue causes an interrupt to get stuck in a claimed state
after being used to wake the system from s2idle.
There is no real benefit to implementing the .irq_enable/.irq_disable
operations using the enable bits. In fact, the existing mask/unmask
implementation using the threshold register is already more efficient,
as it requires no read/modify/write cycles. So let's leave the enable
bits set for the lifetime of the IRQ, using them only to control its
affinity.
Fixes: a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and mask operations")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---
drivers/irqchip/irq-sifive-plic.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index e1484905b7bd..c2673fdad8e5 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -120,12 +120,14 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
}
}
-static void plic_irq_enable(struct irq_data *d)
+static unsigned int plic_irq_startup(struct irq_data *d)
{
plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
+
+ return 0;
}
-static void plic_irq_disable(struct irq_data *d)
+static void plic_irq_shutdown(struct irq_data *d)
{
plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 0);
}
@@ -169,12 +171,12 @@ static int plic_set_affinity(struct irq_data *d,
if (cpu >= nr_cpu_ids)
return -EINVAL;
- plic_irq_disable(d);
+ plic_irq_shutdown(d);
irq_data_update_effective_affinity(d, cpumask_of(cpu));
- if (!irqd_irq_disabled(d))
- plic_irq_enable(d);
+ if (irqd_is_started(d))
+ plic_irq_startup(d);
return IRQ_SET_MASK_OK_DONE;
}
@@ -182,8 +184,8 @@ static int plic_set_affinity(struct irq_data *d,
static struct irq_chip plic_edge_chip = {
.name = "SiFive PLIC",
- .irq_enable = plic_irq_enable,
- .irq_disable = plic_irq_disable,
+ .irq_startup = plic_irq_startup,
+ .irq_shutdown = plic_irq_shutdown,
.irq_ack = plic_irq_eoi,
.irq_mask = plic_irq_mask,
.irq_unmask = plic_irq_unmask,
@@ -197,8 +199,8 @@ static struct irq_chip plic_edge_chip = {
static struct irq_chip plic_chip = {
.name = "SiFive PLIC",
- .irq_enable = plic_irq_enable,
- .irq_disable = plic_irq_disable,
+ .irq_startup = plic_irq_startup,
+ .irq_shutdown = plic_irq_shutdown,
.irq_mask = plic_irq_mask,
.irq_unmask = plic_irq_unmask,
.irq_eoi = plic_irq_eoi,
--
2.40.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits
2023-07-17 18:58 [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits Samuel Holland
@ 2023-07-18 9:26 ` Marc Zyngier
2023-09-20 10:18 ` Palmer Dabbelt
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2023-07-18 9:26 UTC (permalink / raw)
To: Samuel Holland
Cc: Palmer Dabbelt, Paul Walmsley, Samuel Holland, Thomas Gleixner,
linux-kernel, linux-riscv
On Mon, 17 Jul 2023 19:58:40 +0100,
Samuel Holland <samuel.holland@sifive.com> wrote:
>
> Writes to the PLIC completion register are ignored if the enable bit for
> that (interrupt, hart) combination is cleared. This leaves the interrupt
> in a claimed state, preventing it from being triggered again.
>
> Originally, the enable bit was cleared in the .irq_mask operation, and
> commit 69ea463021be ("irqchip/sifive-plic: Fixup EOI failed when masked")
> added a workaround for this issue. Later, commit a1706a1c5062
> ("irqchip/sifive-plic: Separate the enable and mask operations") moved
> toggling the enable bit to the .irq_enable/.irq_disable operations and
> removed the workaround.
>
> However, there are still places where .irq_disable can be called from
> inside the hard IRQ handler, for example in irq_pm_check_wakeup(). As a
> result, this issue causes an interrupt to get stuck in a claimed state
> after being used to wake the system from s2idle.
>
> There is no real benefit to implementing the .irq_enable/.irq_disable
> operations using the enable bits. In fact, the existing mask/unmask
> implementation using the threshold register is already more efficient,
> as it requires no read/modify/write cycles. So let's leave the enable
> bits set for the lifetime of the IRQ, using them only to control its
> affinity.
Side question, which doesn't affect this patch: what happens with
interrupts that are firing while the interrupt is in a disabled state?
It's fine for levels, but what of edge interrupts?
My reading of the spec is that it is the role of the "gateway" to hold
the signal, and that this is upstream of the PLIC itself, so it
*should* be fine, but I'd like confirmation on that.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits
2023-07-18 9:26 ` Marc Zyngier
@ 2023-09-20 10:18 ` Palmer Dabbelt
0 siblings, 0 replies; 3+ messages in thread
From: Palmer Dabbelt @ 2023-09-20 10:18 UTC (permalink / raw)
To: Marc Zyngier; +Cc: samuel, linux-kernel, Paul Walmsley, tglx, linux-riscv
On Tue, 18 Jul 2023 02:26:37 PDT (-0700), Marc Zyngier wrote:
> On Mon, 17 Jul 2023 19:58:40 +0100,
> Samuel Holland <samuel.holland@sifive.com> wrote:
>>
>> Writes to the PLIC completion register are ignored if the enable bit for
>> that (interrupt, hart) combination is cleared. This leaves the interrupt
>> in a claimed state, preventing it from being triggered again.
>>
>> Originally, the enable bit was cleared in the .irq_mask operation, and
>> commit 69ea463021be ("irqchip/sifive-plic: Fixup EOI failed when masked")
>> added a workaround for this issue. Later, commit a1706a1c5062
>> ("irqchip/sifive-plic: Separate the enable and mask operations") moved
>> toggling the enable bit to the .irq_enable/.irq_disable operations and
>> removed the workaround.
>>
>> However, there are still places where .irq_disable can be called from
>> inside the hard IRQ handler, for example in irq_pm_check_wakeup(). As a
>> result, this issue causes an interrupt to get stuck in a claimed state
>> after being used to wake the system from s2idle.
>>
>> There is no real benefit to implementing the .irq_enable/.irq_disable
>> operations using the enable bits. In fact, the existing mask/unmask
>> implementation using the threshold register is already more efficient,
>> as it requires no read/modify/write cycles. So let's leave the enable
>> bits set for the lifetime of the IRQ, using them only to control its
>> affinity.
>
> Side question, which doesn't affect this patch: what happens with
> interrupts that are firing while the interrupt is in a disabled state?
> It's fine for levels, but what of edge interrupts?
>
> My reading of the spec is that it is the role of the "gateway" to hold
> the signal, and that this is upstream of the PLIC itself, so it
> *should* be fine, but I'd like confirmation on that.
Which spec are you reading? I'm not seeing anything in
<https://github.com/riscv/riscv-plic-spec>, but I've sort of only
skimmed it. I don't remember us ever really figuring out edge triggered
interrupts, it was sort of just a "vendors should make sure they do
something reasonable" type plan.
> Thanks,
>
> M.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-20 10:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-17 18:58 [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits Samuel Holland
2023-07-18 9:26 ` Marc Zyngier
2023-09-20 10:18 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).