答复: [PATCH] irqchip/sifive-plic: ensure interrupt is enable before EOI
Nam Cao
namcao at linutronix.de
Mon Jun 24 04:56:29 PDT 2024
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
More information about the linux-riscv
mailing list