RE: [PATCH v2 4/8] irqchip/al-fic: switch to shared parent interrupt

[Farber, Eliav]

On Sun, 2026-10-04 at 19:30 +0000, Radu Rendec wrote:
> On Sun, 2026-09-27 at 08:06 +0000, Eliav Farber wrote:
> > -     for_each_set_bit(hwirq, &pending, NR_FIC_IRQS)
> > -             generic_handle_domain_irq(domain, hwirq);
> > +     for_each_set_bit(hwirq, &pending, NR_FIC_IRQS) {
> > +             if (!generic_handle_domain_irq(domain, hwirq))
> > +                     ret = IRQ_HANDLED;
>
> I'm not sure about this. The only way generic_handle_domain_irq() can
> fail is if hwirq is invalid and doesn't map back to a virq assigned to
> the domain. The handler itself has a void return type, so this doesn't
> tell you whether the interrupt was handled downstream, it just tells
> you whether the downstream handler was called or not.
>
> Since you're looking at exactly NR_FIC_IRQS bits, which is also the
> domain size, I expect the conversion (from hwirq to virq) to always be
> successful.
>
> If you search for "generic_handle_domain_irq" in drivers/irqchip/,
> you'll notice that most drivers don't check the return type. The few
> who do, just log a ratelimited "spurious irq" message.
>
> When the interrupt is shared, the purpose of the handler return value
> is to identify which of the devices generated the interrupt, or in
> other words to tell the irq core whether it should keep looking at the
> remaining devices. If a bit in AL_FIC_CAUSE is set, I would expect this
> instance to be the one that generated the parent interrupt.

Agreed, and fixed - the handler now returns IRQ_HANDLED/IRQ_NONE based on
whether `pending` (the masked cause snapshot) was non-zero, not on
generic_handle_domain_irq()'s return. That is the right signal for a
shared interrupt, as you say, and matches the majority convention you
pointed out.

> By the way, how is AL_FIC_CAUSE cleared? Is it read-to-clear? I'm just
> curious; I assume it's implemented correctly because this hasn't
> changed with the conversion from chained interrupts to shared, and it
> was probably working before.

Not read-to-clear. Per the register spec, writing a CAUSE bit to 0 clears
it and writing 1 has no effect (so it is a write-0-to-clear scheme, not a
separate ack register). irq_gc_ack_clr_bit(), wired as this chip's ack
callback, writes ~d->mask to AL_FIC_CAUSE - this hwirq's bit 0, every
other bit 1 - which clears exactly the serviced bit and leaves the rest
alone. handle_level_irq() calls that as part of the normal ack flow, so
each serviced hwirq clears its own bit. clear_on_read exists as a
separate control-register bit but defaults to 0 (disabled) and this driver
never sets it, so the readl_relaxed() snapshot in the handler has no side
effect, and re-reading it on every shared invocation is safe.

> > +     ret = request_irq(fic->parent_irq, al_fic_irq_handler,
> > +                       IRQF_NO_THREAD | IRQF_SHARED, fic->node->full_name,
>
> I have the same comment here about fic->node->full_name as I did on the
> previous patch. There is an accessor, so I would use it.

Done.

> > +err_remove_generic_chips:
> > +     irq_domain_remove_generic_chips(fic->domain);
>
> As you noted in the commit message, IRQ_DOMAIN_FLAG_DESTROY_GC does
> exactly that, so why not use it? It's safe to set the flag even before
> calling irq_alloc_domain_generic_chips() and knowing it has been
> successful; irq_domain_remove_generic_chips() checks first if the GC
> has been allocated and returns early if not. There is an example of
> using the flag in drivers/irqchip/irq-renesas-irqc.c.

Done - set the flag right after irq_domain_create_linear() succeeds, same
place irq-renesas-irqc.c does it, and both error paths now go through a
single irq_domain_remove(). Commit message updated to match.

All three changes are in v3, which I will post shortly.

Thanks,
Eliav