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

[Radu Rendec <radu@rendec.net>]

On Sun, 2026-09-27 at 08:06 +0000, Eliav Farber wrote:
> Until now the driver requested its parent interrupt using the chained IRQ
> API (irq_set_chained_handler_and_data()), which only works when each
> parent interrupt is wired to a single FIC instance.
> 
> A FIC controller is built from groups, each described by its own DT node,
> and the groups of one controller share that controller's output line
> toward the parent. So a real devicetree has several FIC nodes on one
> parent GIC SPI, and a chained handler can only be installed once per
> parent. Cascading compounds this: an aggregating group collects several
> peripherals' outputs onto the line above it.
> 
> To support that, request the parent interrupt as a shared interrupt
> (IRQF_SHARED) instead of installing a chained handler. The handler now has
> the standard irqreturn_t prototype and reports whether it handled any
> child interrupt, so the shared-IRQ core can dispatch to the correct
> instance. IRQF_NO_THREAD is set because the handler only demultiplexes to
> the child domain and must not be forced-threaded; all instances sharing a
> parent line agree on this flag, as the shared-IRQ core requires.
> 
> generic_handle_domain_irq() is retained for dispatch; its return value is
> used to determine whether a pending child was actually handled so the
> handler can return IRQ_HANDLED/IRQ_NONE correctly.
> 
> request_irq() can fail, unlike irq_set_chained_handler_and_data(), so add
> an error path for it. It calls irq_domain_remove_generic_chips() before
> irq_domain_remove(), because irq_domain_remove() frees the generic chips
> only when the domain carries IRQ_DOMAIN_FLAG_DESTROY_GC.
> 
> Co-developed-by: Talel Shenhar <talel@amazon.com>
> Signed-off-by: Talel Shenhar <talel@amazon.com>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> v2:
>  - Fix the request_irq() error path: v1 called irq_free_generic_chip(gc),
>    which is kfree(gc) on an interior pointer into the single allocation
>    made by irq_domain_alloc_generic_chips() - an invalid free reachable
>    when request_irq() fails at probe. Replace it with
>    irq_domain_remove_generic_chips() before irq_domain_remove(), and add a
>    commit-message paragraph explaining the teardown ordering.
>  - Add Co-developed-by/Signed-off-by: Talel Shenhar.
>  - Reworded to state the hardware reason for the shared parent (the groups
>    of one controller share that controller's output line).
> 
>  drivers/irqchip/irq-al-fic.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index c7cc2631caf8..091a06abc0bb 100644
> --- a/drivers/irqchip/irq-al-fic.c
> +++ b/drivers/irqchip/irq-al-fic.c
> @@ -4,9 +4,9 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip.h>
> -#include <linux/irqchip/chained_irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -95,24 +95,24 @@ static int al_fic_irq_set_type(struct irq_data *data, unsigned int flow_type)
>  	return 0;
>  }
>  
> -static void al_fic_irq_handler(struct irq_desc *desc)
> +static irqreturn_t al_fic_irq_handler(int irq, void *data)
>  {
> -	struct al_fic *fic = irq_desc_get_handler_data(desc);
> +	struct al_fic *fic = data;
>  	struct irq_domain *domain = fic->domain;
> -	struct irq_chip *irqchip = irq_desc_get_chip(desc);
>  	struct irq_chip_generic *gc = irq_get_domain_generic_chip(domain, 0);
> +	irqreturn_t ret = IRQ_NONE;
>  	unsigned long pending;
>  	u32 hwirq;
>  
> -	chained_irq_enter(irqchip, desc);
> -
>  	pending = readl_relaxed(fic->base + AL_FIC_CAUSE);
>  	pending &= ~gc->mask_cache;
>  
> -	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.

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.

> +	}
>  
> -	chained_irq_exit(irqchip, desc);
> +	return ret;
>  }
>  
>  static int al_fic_irq_retrigger(struct irq_data *data)
> @@ -162,11 +162,18 @@ static int al_fic_register(struct device_node *node,
>  	gc->chip_types->chip.flags = IRQCHIP_SKIP_SET_WAKE;
>  	gc->private = fic;
>  
> -	irq_set_chained_handler_and_data(fic->parent_irq,
> -					 al_fic_irq_handler,
> -					 fic);
> +	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.

> +			  fic);
> +	if (ret) {
> +		pr_err("fail to request irq (%d)\n", ret);
> +		goto err_remove_generic_chips;
> +	}
> +
>  	return 0;
>  
> +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.

>  err_domain_remove:
>  	irq_domain_remove(fic->domain);
>