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

[Nam Cao]

On Mon, Jun 24, 2024 at 11:14:47AM +0000, Yan Zheng（严政） wrote:
> > 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?
> > 
> 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);
>  }

Again, I don't know anything about interrupt affinity thingy, so I may be
saying something dumb here:

I think this wouldn't work either. In plic_set_affinity(), I see the
interrupt is disabled, then enabled again. With your new proposed solution,
the interrupt would also be marked completed within plic_set_affinity().
So, the interrupt may be asserted again, earlier than it is supposed to (it
is not supposed to be asserted again until plic_irq_eoi() is called). It's
rare, but I think it's a possible race.

I don't have a better idea, at least for now. So probably we should stick
to your current solution.
> 
> 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

This is useful information, you may want to add it in your commit message.
> 
> 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.

Sorry, should have mentioned I didn't build this patch. Just wanted to
throw out ideas..

> > +
> > +	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