Date: Mon, 24 Jun 2024 12:43:04 +0000
From: Yan Zheng（严政） <zhengyan@...micro.com>
To: Nam Cao <namcao@...utronix.de>
CC: "tglx@...utronix.de" <tglx@...utronix.de>,
        "maz@...nel.org"
	<maz@...nel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "paul.walmsley@...ive.com"
	<paul.walmsley@...ive.com>,
        "samuel.holland@...ive.com"
	<samuel.holland@...ive.com>,
        "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>,
        Zhou Qiao(周侨)
	<qiaozhou@...micro.com>
Subject: 答复: 答复: [PATCH] irqchip/sifive-plic: ensure interrupt is enable before EOI

> 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.
Yes, I'd already add it in v2 patch, thx.
> >
> > 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..
> 
No problem, I thought you might not have compiled it. 
I wrote it down just to ensure we are on the same page.
> > > +
> > > +	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

Powered by blists - more mailing lists