Re: [PATCH 04/16] irqchip/crossbar: fix allocation and init cleanup

[Radu Rendec]

On Tue, 2026-07-14 at 21:24 +0800, Haofeng Li wrote:
> From: Haofeng Li <lihaofeng@xxxxxxxxxx>
> 
> crossbar_domain_alloc() returns in the middle of a multi-IRQ
> allocation without freeing IRQs and crossbar slots allocated by
> earlier iterations.
> 
> If irq_domain_create_hierarchy() fails after crossbar_of_init(), the
> global crossbar object and its maps and MMIO mapping are also leaked.
> 
> Roll back prior IRQs through crossbar_domain_free(), and release all
> crossbar resources when domain creation fails.
> 
> Fixes: 783d31863fb8 ("irqchip: crossbar: Convert dra7 crossbar to stacked domains")
> 
> Signed-off-by: Haofeng Li <lihaofeng@xxxxxxxxxx>
> ---
>  drivers/irqchip/irq-crossbar.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 033b08782119..0ac5268aa8e4 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -42,6 +42,9 @@ struct crossbar_device {
>  
>  static struct crossbar_device *cb;
>  
> +static void crossbar_domain_free(struct irq_domain *domain, unsigned int virq,
> +				 unsigned int nr_irqs);
> +
>  static void crossbar_writel(int irq_no, int cb_no)
>  {
>  	writel(cb_no, cb->crossbar_base + cb->register_offsets[irq_no]);
> @@ -127,11 +130,14 @@ static int crossbar_domain_alloc(struct irq_domain *d, unsigned int virq,
>  	for (i = 0; i < nr_irqs; i++) {
>  		int err = allocate_gic_irq(d, virq + i, hwirq + i);
>  
> -		if (err)
> +		if (err) {
> +			if (i)

I wouldn't bother checking if i != 0. This is a corner case, and it's
handled well, both inside crossbar_domain_free() itself and in
irq_domain_free_irqs_parent().

> +				crossbar_domain_free(d, virq, i);
>  			return err;
> +		}
>  
>  		irq_domain_set_hwirq_and_chip(d, virq + i, hwirq + i,
> -					      &crossbar_chip, NULL);
> +					     &crossbar_chip, NULL);

Why change the indentation here?

>  	}
>  
>  	return 0;
> @@ -336,6 +342,18 @@ static int __init crossbar_of_init(struct device_node *node)
>  	return ret;
>  }
>  
> +static void __init crossbar_free(void)
> +{
> +	if (!cb)
> +		return;

cb cannot be NULL here. In fact, the only way this code can be reached
is after the call to irq_domain_create_hierarchy() in irqcrossbar_init()
which already dereferences cb.

> +
> +	kfree(cb->register_offsets);
> +	kfree(cb->irq_map);
> +	iounmap(cb->crossbar_base);
> +	kfree(cb);
> +	cb = NULL;
> +}
> +
>  static int __init irqcrossbar_init(struct device_node *node,
>  				   struct device_node *parent)
>  {
> @@ -361,6 +379,7 @@ static int __init irqcrossbar_init(struct device_node *node,
>  					     of_fwnode_handle(node), &crossbar_domain_ops, NULL);
>  	if (!domain) {
>  		pr_err("%pOF: failed to allocated domain\n", node);
> +		crossbar_free();

Why create a separate function and not do the cleanup here? It's not
called anywhere else, and the driver cannot be compiled as a loadable
module (and therefore doesn't need a global _free() function).

>  		return -ENOMEM;
>  	}
>