Re: [PATCH 07/16] irqchip/mips-gic: clean up IRQ domain creation failure

[Radu Rendec <radu@rendec.net>]

On Tue, 2026-07-14 at 21:24 +0800, Haofeng Li wrote:
> From: Haofeng Li <lihaofeng@kylinos.cn>
> 
> After mapping the register base, irq_domain_create_simple() failure
> returns without unmapping it.

It does, but this driver is meant to initialize once during the (early)
kernel start-up and stay there for the rest of the kernel lifetime.
Also, this is the Global Interrupt Controller (GIC) driver. If this
fails to initialize, I very much doubt the system is usable anyway.

> The CPU chained or VEIC handler is also installed before either IRQ
> domain is ready. Unmapping the registers while that handler remains
> published would leave a path to unmapped MMIO,

... which is probably one of the reasons why the IO range was not
unmapped originally.

>  and a later IPI domain
> failure would expose a half-initialized controller.
> 
> Unmap and clear the register base when primary domain creation fails,
> and publish the CPU interrupt handler only after both domains have
> been created successfully.

Why? Please see my point above about the role of this driver. This
driver not only has a fundamental role, but also has a different
lifecycle and does not follow the cleanup paradigm of a typical driver
that can be unloaded/reloaded/re-probed.

> 
> Fixes: fbea754123ae ("irqchip: mips-gic: Inline __gic_init()")
> 
> Signed-off-by: Haofeng Li <lihaofeng@kylinos.cn>
> ---
>  drivers/irqchip/irq-mips-gic.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 19a57c5e2b2e..85fe0d8a34c5 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -956,23 +956,14 @@ static int __init gic_of_init(struct device_node *node,
>  	gic_shared_intrs = FIELD_GET(GIC_CONFIG_NUMINTERRUPTS, gicconfig);
>  	gic_shared_intrs = (gic_shared_intrs + 1) * 8;
>  
> -	if (cpu_has_veic) {
> -		/* Always use vector 1 in EIC mode */
> -		gic_cpu_pin = 0;
> -		set_vi_handler(gic_cpu_pin + GIC_PIN_TO_VEC_OFFSET,
> -			       __gic_irq_dispatch);
> -	} else {
> -		gic_cpu_pin = cpu_vec - GIC_CPU_PIN_OFFSET;
> -		irq_set_chained_handler(MIPS_CPU_IRQ_BASE + cpu_vec,
> -					gic_irq_dispatch);
> -	}
> -
>  	gic_irq_domain = irq_domain_create_simple(of_fwnode_handle(node),
>  						  GIC_NUM_LOCAL_INTRS +
>  						  gic_shared_intrs, 0,
>  						  &gic_irq_domain_ops, NULL);
>  	if (!gic_irq_domain) {
>  		pr_err("Failed to add IRQ domain");
> +		iounmap(mips_gic_base);
> +		mips_gic_base = NULL;
>  		return -ENXIO;
>  	}
>  
> @@ -980,6 +971,17 @@ static int __init gic_of_init(struct device_node *node,
>  	if (ret)
>  		return ret;
>  
> +	if (cpu_has_veic) {
> +		/* Always use vector 1 in EIC mode */
> +		gic_cpu_pin = 0;
> +		set_vi_handler(gic_cpu_pin + GIC_PIN_TO_VEC_OFFSET,
> +			       __gic_irq_dispatch);
> +	} else {
> +		gic_cpu_pin = cpu_vec - GIC_CPU_PIN_OFFSET;
> +		irq_set_chained_handler(MIPS_CPU_IRQ_BASE + cpu_vec,
> +					gic_irq_dispatch);
> +	}
> +
>  	board_bind_eic_interrupt = &gic_bind_eic_interrupt;
>  
>  	/*