[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6f4787fe98374079b33427471ad715cb@exch03.asrmicro.com>
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