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

On Tue, 2026-07-14 at 21:24 +0800, Haofeng Li wrote:
> From: Haofeng Li <[email protected]>
>=20
> 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.
>=20
> 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.

>=20
> Fixes: fbea754123ae ("irqchip: mips-gic: Inline __gic_init()")
>=20
> Signed-off-by: Haofeng Li <[email protected]>
> ---
> =C2=A0drivers/irqchip/irq-mips-gic.c | 24 +++++++++++++-----------
> =C2=A01 file changed, 13 insertions(+), 11 deletions(-)
>=20
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gi=
c.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 *n=
ode,
> =C2=A0	gic_shared_intrs =3D FIELD_GET(GIC_CONFIG_NUMINTERRUPTS, gicconfig=
);
> =C2=A0	gic_shared_intrs =3D (gic_shared_intrs + 1) * 8;
> =C2=A0
> -	if (cpu_has_veic) {
> -		/* Always use vector 1 in EIC mode */
> -		gic_cpu_pin =3D 0;
> -		set_vi_handler(gic_cpu_pin + GIC_PIN_TO_VEC_OFFSET,
> -			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __gic_irq_dispatch);
> -	} else {
> -		gic_cpu_pin =3D cpu_vec - GIC_CPU_PIN_OFFSET;
> -		irq_set_chained_handler(MIPS_CPU_IRQ_BASE + cpu_vec,
> -					gic_irq_dispatch);
> -	}
> -
> =C2=A0	gic_irq_domain =3D irq_domain_create_simple(of_fwnode_handle(node)=
,
> =C2=A0						=C2=A0 GIC_NUM_LOCAL_INTRS +
> =C2=A0						=C2=A0 gic_shared_intrs, 0,
> =C2=A0						=C2=A0 &gic_irq_domain_ops, NULL);
> =C2=A0	if (!gic_irq_domain) {
> =C2=A0		pr_err("Failed to add IRQ domain");
> +		iounmap(mips_gic_base);
> +		mips_gic_base =3D NULL;
> =C2=A0		return -ENXIO;
> =C2=A0	}
> =C2=A0
> @@ -980,6 +971,17 @@ static int __init gic_of_init(struct device_node *no=
de,
> =C2=A0	if (ret)
> =C2=A0		return ret;
> =C2=A0
> +	if (cpu_has_veic) {
> +		/* Always use vector 1 in EIC mode */
> +		gic_cpu_pin =3D 0;
> +		set_vi_handler(gic_cpu_pin + GIC_PIN_TO_VEC_OFFSET,
> +			=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __gic_irq_dispatch);
> +	} else {
> +		gic_cpu_pin =3D cpu_vec - GIC_CPU_PIN_OFFSET;
> +		irq_set_chained_handler(MIPS_CPU_IRQ_BASE + cpu_vec,
> +					gic_irq_dispatch);
> +	}
> +
> =C2=A0	board_bind_eic_interrupt =3D &gic_bind_eic_interrupt;
> =C2=A0
> =C2=A0	/*