Re: [PATCH v5 5/9] irqchip/al-fic: switch to shared parent interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] irqchip/al-fic: shared parent mapping destroyed on error path via irq_dispose_mapping
--

commit 2575228eb0e3f9536c071e11d0536822135b2cae
Author: Eliav Farber <farbere@amazon.com>

irqchip/al-fic: switch to shared parent interrupt

This commit changes the driver from using a chained handler to a shared
interrupt (IRQF_SHARED) for the parent interrupt line. This change allows
multiple FIC groups to share a single output line toward the parent GIC SPI.
It also seeds the mask cache from the hardware register before requesting
the interrupt.

> diff --git a/drivers/irqchip/irq-al-fic.c b/drivers/irqchip/irq-al-fic.c
> index 32d8098699a41..8a7eb9009515f 100644
> --- a/drivers/irqchip/irq-al-fic.c
> +++ b/drivers/irqchip/irq-al-fic.c
[ ... ]
> @@ -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;
> +	}

[Severity: High]
Will this transition to a shared parent interrupt cause issues with the
error path in al_fic_init_dt()?

Since multiple FIC groups can now share the same parent interrupt, if a
subsequent group fails initialization (e.g. if this request_irq() fails),
al_fic_init_dt() will call irq_dispose_mapping(parent_irq):

drivers/irqchip/irq-al-fic.c:al_fic_init_dt() {
    ...
    fic = al_fic_wire_init(node, base, parent_irq);
    if (IS_ERR(fic)) {
        pr_err("%pOF: fail to initialize irqchip (%pe)\n", node, fic);
        ret = PTR_ERR(fic);
        goto err_irq_dispose;
    }

    return 0;

err_irq_dispose:
    irq_dispose_mapping(parent_irq);
    ...
}

Because Linux IRQ mappings are not natively reference-counted at the dispose
level, unconditionally unmapping the shared parent interrupt would sever
interrupts for any other FIC instances that already successfully requested
and mapped this shared parent line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011050449.41903-1-farbere@amazon.com?part=5