答复: [PATCH] irqchip/sifive-plic: ensure interrupt is enable before EOI

[Yan Zheng（严政）]

> On Mon, Jun 24, 2024 at 08:53:41AM +0000, zhengyan wrote:
> > RISC-V PLIC cannot "end-of-interrupt" (EOI) disabled interrupts, as
> > explained in the description of Interrupt Completion in the PLIC spec:
> > "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."
> >
> > Commit 9c92006b896c ("irqchip/sifive-plic: Enable interrupt if needed
> > before EOI") ensured that EOI is enable when irqd IRQD_IRQ_DISABLED is
> > set, before EOI
> >
> > Commit 69ea463021be ("irqchip/sifive-plic: Fixup EOI failed when
> > masked") ensured that EOI is successful by enabling interrupt first, before
> EOI.
> >
> > Commit a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and
> > mask
> > operations") removed the interrupt enabling code from the previous
> > commit, because it assumes that interrupt should already be enabled at
> > the point of EOI.
> >
> > However, here still miss a corner case that if SMP is enabled. When
> > someone need to set affinity from a cpu to another (Maybe like
> > boardcast-tick) the original cpu when handle the EOI meanwhile the IE
> > is disabled by plic_set_affinity
> >
> > So this patch ensure that won't happened
> >
> > Signed-off-by: zhengyan <zhengyan at asrmicro.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c
> > b/drivers/irqchip/irq-sifive-plic.c
> > index 9e22f7e378f5..e6acd134a691 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -149,8 +149,10 @@ static void plic_irq_mask(struct irq_data *d)
> > static void plic_irq_eoi(struct irq_data *d)  {
> >  	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > +	void __iomem *reg = handler->enable_base + (d->hwirq / 32) *
> sizeof(u32);
> > +	u32 hwirq_mask = 1 << (d->hwirq % 32);
> >
> > -	if (unlikely(irqd_irq_disabled(d))) {
> > +	if (unlikely(irqd_irq_disabled(d)) || (readl(reg) & hwirq_mask) ==
> > +0) {
> 
> If we read interrupt enable state from hardware, then reading the software
> state (irqd_irq_disabled) is redundant.
> 
Yes, you are right. I was afraid of missing some corner cases, so I kept the original conditional checks.
I think it over, it should be safe to only check hardware states
And I’d like to put it into "unlikely" path as 
if (unlikely((readl(reg) & hwirq_mask)) ==
> >  		plic_toggle(handler, d->hwirq, 1);
> >  		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >  		plic_toggle(handler, d->hwirq, 0);
> 
> I have no knowledge about affinity stuff, so I don't really understand this
> patch. But there is another idea regarding this "ignored EOI" problem:
> always "complete" the interrupt while enabling. That would move this extra
> complication out of the hot path, and also looks simpler in my opinion.
> 
> Something like the patch below. Would this solve this "affinity problem"
> too?
> 
> Best regards,
> Nam
> 
No, I'm afraid the following patch can't solve this corner case. I thought it's because the core
Who executes plic_irq_enable is not the core who missing a write claim.
So if we want to do it in enable it might be something like follows :
static void plic_toggle(struct plic_handler *handler, int hwirq, int enable)
 {
        raw_spin_lock(&handler->enable_lock);
-       __plic_toggle(handler->enable_base, hwirq, enable);
+       if (enable) {
+               writel(hwirq, handler->hart_base + CONTEXT_CLAIM);
+               __plic_toggle(handler->enable_base, hwirq, enable);
+       }
        raw_spin_unlock(&handler->enable_lock);
 }

But there is a little difference:
a. check whether it's enabled  when do write claim
b. write claim anyway before enable 

sounds like a. is better?

And I'd like to illustrate more about this case:
For example, broadcast tick is working, cpu0 is about to response, cpu1 is the next
1. cpu0  response the timer irq, read the claim REG, and do timer isr event, 
2.  during the timer isr it will set next event 
tick_broadcast_set_event ->  irq_set_affinity-> xxx-> plic_set_affinity -> plic_irq_enable
3. in plic_set_affinity  disable cpu0's IE and enable cpu1'IE
4. cpu0 do the write claim to finish this irq, while cpu0's IE is disabled , left an active state in plic

Best regards,
zhengyan

> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 0a233e9d9607..63f2111ced4a 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -122,7 +122,15 @@ static inline void plic_irq_toggle(const struct
> cpumask *mask,
> 
>  static void plic_irq_enable(struct irq_data *d)  {
> +	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
missing a definition? If adds like this will cause a problem.
> +
> +	writel(0, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> +
> +	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> +
>  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);
> +
> +	writel(1, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>  }
> 
>  static void plic_irq_disable(struct irq_data *d) @@ -148,13 +156,7 @@ static
> void plic_irq_eoi(struct irq_data *d)  {
>  	struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> 
> -	if (unlikely(irqd_irq_disabled(d))) {
> -		plic_toggle(handler, d->hwirq, 1);
> -		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> -		plic_toggle(handler, d->hwirq, 0);
> -	} else {
> -		writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> -	}
> +	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
>  }
> 
>  #ifdef CONFIG_SMP