Message-ID: <CABe3_aGj68qM1bNZ3LExbexO=9FO4RzJxhUy2T+HKK1qZfBmtw@mail.gmail.com>
Date: Thu, 16 Oct 2025 11:54:41 -0400
From: Charles Mirabile <cmirabil@...hat.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Lucas Zampieri <lzampier@...hat.com>, linux-kernel@...r.kernel.org, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Samuel Holland <samuel.holland@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Alexandre Ghiti <alex@...ti.fr>, Vivian Wang <dramforever@...e.com>, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, Zhang Xincheng <zhangxincheng@...rarisc.com>
Subject: Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC

Hi Thomas—

On Thu, Oct 16, 2025 at 9:17 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>
> On Thu, Oct 16 2025 at 09:42, Lucas Zampieri wrote:
>
> After fixing the corrupted patch up I had a closer look and decided not
> to merge it. See comments below.
>
> > +static bool cp100_isolate_pending_irq(int nr_irq_groups, u32 ie[],
> > +                                    void __iomem *pending,
> > +                                    void __iomem *enable)
> > +{
> > +     u32 pending_irqs = 0;
> > +     int i, j;
> > +
> > +     /* Look for first pending interrupt */
> > +     for (i = 0; i < nr_irq_groups; i++) {
> > +             pending_irqs = ie[i] & readl_relaxed(pending + i * sizeof(u32));
> > +             if (pending_irqs)
> > +                     break;
> > +     }
> > +
> > +     if (!pending_irqs)
> > +             return false;
> > +
> > +     /* Disable all interrupts but the first pending one */
> > +     for (j = 0; j < nr_irq_groups; j++) {
> > +             u32 new_mask = 0;
> > +
> > +             if (j == i) {
> > +                     /* Extract mask with lowest set bit */
> > +                     new_mask = (pending_irqs & -pending_irqs);
> > +             }
> > +
> > +             writel_relaxed(new_mask, enable + j * sizeof(u32));
> > +     }
> > +
> > +     return true;
> > +}
> > +
> > +static irq_hw_number_t cp100_get_hwirq(struct plic_handler *handler,
> > +                                     void __iomem *claim)
> > +{
> > +     int nr_irq_groups = DIV_ROUND_UP(handler->priv->nr_irqs, 32);
> > +     void __iomem *pending = handler->priv->regs + PENDING_BASE;
> > +     void __iomem *enable = handler->enable_base;
> > +     irq_hw_number_t hwirq = 0;
> > +     int i;
> > +
> > +     guard(raw_spinlock)(&handler->enable_lock);
> > +
> > +     /* Save current interrupt enable state */
> > +     for (i = 0; i < nr_irq_groups; i++)
> > +             handler->enable_save[i] = readl_relaxed(enable + i * sizeof(u32));
>
> This is truly the most inefficient way to solve that problem. The enable
> registers are modified with enabled_lock held, so you can just cache the
> value in plic_handler::enabled_save and avoid this read loop completely.
> After claiming the interrupt you restore from that cache, no?

You mean touch the other functions where the enable bits are modified
to keep the cache in sync so that we don't need to do this read loop
and can have a proper set of values cached?

My concern is that this obviously has an impact on other platforms
which do not have this quirk since keeping the cache in sync would get
pushed all throughout the driver.

I do agree that it would save this loop, but the way this was written
was intentionally designed to minimize the impact on other platforms
at the expense of this one because it is the platform with the bug.

>
> Now for the search and disable mechanism. Of course you need to search
> for th pending interrupt first, but then you can make that masking loop
> very simple by having a plic_handler::enabled_clear[] array which is
> zeroed on initialization:
>
>         unsigned long pending = 0;
>
>         for (group = 0; !pending && group < nr_irq_groups; group++) {
>                 pending = handler->enabled_save[i];
>                 pending =& readl_relaxed(pending + group * sizeof(u32));
>         }
>         if (!pending)
>                 return false;
>
>         bit = ffs(pending) - 1;
>         handler->enabled_clear[group] |= BIT(bit);
>         for (int i = 0; i < nr_irq_groups; i++)
>                 writel_relaxed(handler->enabled_clear[i], enable + i * sizeof(u32));
>         handler->enabled_clear[group] = 0;
>
> No?

Sure that would also work, but why are we using ffs (slow) only to
shift the result back to make a new mask when (x & -x) is faster and
skips the intermediate step delivering immediately the mask of the
lowest bit.

As for making another caching array, I guess, but again that is just a
time vs space trade off with its own invariants to maintain that would
also impact other platforms.

I could definitely adjust the clear loop to move the mask preparation out like:

/* extract lowest set bit */
pending &= -pending

for(j = 0; j < nr_groups; ++j)
    writel_relaxed(i == j ? pending : 0, enable + i * sizeof(u32));

but I am quite sure that the compiler is well able to do this exact
transformation as an optimization pass and I am not sure that it is
more readable than what I originally proposed.

>
> But looking at this makes me wonder about the functional correctness of all
> this. What happens in this case:
>
> Device A raises an interrupt
>
>     handler()
>         ....
>         disable_groups();
>
> Device B raises a now disabled interrupt
>
>         restore_groups();
>
> Is the device B interrupt preserved in the interrupt chip and actually
> raised when the interrupt enable bit is restored or is it lost?

I am not sure how to verify this other than to tell you that without
this quirk (i.e. trying to use normal plic behavior) the device does
not work, but with this quirk I can boot to a desktop with a pcie
graphics card and storage, use networking etc that all obviously
depend on the correct functioning of the interrupt controller.

My reading of the spec for PLIC also suggests (but does not explicitly
confirm) that the pending bits function irrespective of the state of
the corresponding enable bit: "A pending bit in the PLIC core can be
cleared by setting the associated enable bit then performing a claim."
(page 14 plic spec 1.0.0 [1]).

This sentence implies to me that it is possible for a pending bit to
be set even though the corresponding enable bit is not, which lends
credence to the idea that the pending bits operate independently.

>
> Thanks,
>
>         tglx
>

I am very sorry about the corrupt patch, I don't know how it happened.
I hope you will reconsider taking this code. We can send a version
that is not corrupt (or with the slight modification to bring the mask
preparation out of the loop) if you would prefer.

Thanks for the review.

Best—Charlie

[1] https://github.com/riscv/riscv-plic-spec/releases/tag/1.0.0

Powered by blists - more mailing lists