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

[Radu Rendec <radu@rendec.net>]

On Thu, 2026-10-08 at 09:00 +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 this instance had
> anything pending, so the shared-IRQ core can tell which instance on the
> line raised the interrupt. 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.
> 
> The handler reads the group's cause register and returns IRQ_HANDLED when
> any unmasked cause bit is set, IRQ_NONE otherwise. That is the signal the
> shared-IRQ core needs - "did this instance's hardware raise the line" -
> rather than the result of dispatching to the child domain.
> 
> For that filter to be right, gc->mask_cache must be valid before the parent
> is requested. IRQ_GC_INIT_MASK_CACHE seeds it from the mask register only
> on the first child mapping, and never at all for a group with no consumer
> in the devicetree; until then the cache reads 0 and every latched cause bit
> passes the filter. Seed it from the value al_fic_wire_init() programmed and
> drop the flag; nothing can change the register in between, so the read
> could only have returned that same value.
> 
> request_irq() can fail, unlike irq_set_chained_handler_and_data(), so add
> an error path for it. Set IRQ_DOMAIN_FLAG_DESTROY_GC on the domain after
> creating it, so irq_domain_remove() tears the generic chips down too and
> the single call suffices for both the chip-allocation and request_irq()
> failure paths.
> 
> Co-developed-by: Talel Shenhar <talel@amazon.com>
> Signed-off-by: Talel Shenhar <talel@amazon.com>
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
> v4:
>  - Seed gc->mask_cache before request_irq() and drop
>    IRQ_GC_INIT_MASK_CACHE. The flag only seeds the cache on the first
>    child mapping, which is too late once the handler is shared and never
>    happens for a group with no consumer in the devicetree. Found by
>    sashiko-bot on the v3 posting.
>  - Drop Radu Rendec's Reviewed-by, since the above is a functional
>    change.
>  - The commit message no longer claims the domain-sized dispatch loop
>    always succeeds; with the cache seeded, only mapped bits reach it.
> 
> v3:
>  - al_fic_irq_handler() no longer derives IRQ_HANDLED/IRQ_NONE from
>    generic_handle_domain_irq(), whose return value only reports whether
>    the hwirq to virq mapping succeeded - and since the loop iterates
>    exactly NR_FIC_IRQS bits, which is the domain's own size, that mapping
>    always succeeds. Return IRQ_HANDLED when the masked CAUSE snapshot is
>    non-zero instead, which is the correct signal for a shared interrupt.
>  - Set IRQ_DOMAIN_FLAG_DESTROY_GC on the domain and let
>    irq_domain_remove() free the generic chips, instead of calling
>    irq_domain_remove_generic_chips() by hand. Both error paths now go
>    through one label. The invalid-free fix from v2 is unaffected; only the
>    teardown mechanism changed.
>  - Use of_node_full_name() in the request_irq() call.
> 
> 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 | 38 +++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index ee06d0123b7a..fda9c05a639f 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,21 @@ 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);
>  	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);
>  
> -	chained_irq_exit(irqchip, desc);
> +	return pending ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
>  static int al_fic_irq_retrigger(struct irq_data *data)
> @@ -140,11 +137,17 @@ static int al_fic_register(struct device_node *node,
>  		return -ENOMEM;
>  	}
>  
> +	/*
> +	 * Let irq_domain_remove() free the generic chips on either error path
> +	 * below, instead of calling irq_domain_remove_generic_chips() by hand.
> +	 */
> +	fic->domain->flags |= IRQ_DOMAIN_FLAG_DESTROY_GC;
> +
>  	ret = irq_alloc_domain_generic_chips(fic->domain,
>  					     NR_FIC_IRQS,
>  					     1, of_node_full_name(fic->node),
>  					     handle_level_irq,
> -					     0, 0, IRQ_GC_INIT_MASK_CACHE);
> +					     0, 0, 0);
>  	if (ret) {
>  		pr_err("fail to allocate generic chip (%d)\n", ret);
>  		goto err_domain_remove;
> @@ -162,9 +165,22 @@ 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);
> +	/*
> +	 * Seed the mask cache with the value al_fic_wire_init() programmed,
> +	 * rather than having the generic chip read the register back on the
> +	 * first child mapping: that is later than the parent is requested, and
> +	 * never happens at all for a group with no consumer in the devicetree.
> +	 */
> +	gc->mask_cache = ~0U;
> +
> +	ret = request_irq(fic->parent_irq, al_fic_irq_handler,
> +			  IRQF_NO_THREAD | IRQF_SHARED,
> +			  of_node_full_name(fic->node), fic);
> +	if (ret) {
> +		pr_err("fail to request irq (%d)\n", ret);
> +		goto err_domain_remove;
> +	}
> +
>  	return 0;
>  
>  err_domain_remove:

Reviewed-by: Radu Rendec <radu@rendec.net>