Re: [PATCH v5 3/3] irqchip/plic: add support for UltraRISC DP1000 PLIC

[Thomas Gleixner <tglx@linutronix.de>]

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?

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?

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?

Thanks,

        tglx