Re: [PATCH] irqchip/sifive-plic: Avoid clearing the per-hart enable bits

[Palmer Dabbelt]

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@xxxxxxxxxx> 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.