[PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()

[PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()

[williamsukatube]

From: William Dean <williamsukatube@gmail.com>

The function ioremap() in gic_of_init() can fail, so
its return value should be checked.

Fixes: 4bdc0d676a643 ("remove ioremap_nocache and devm_ioremap_nocache")
Reported-by: Hacash Robot <hacashRobot@santino.com>
Signed-off-by: William Dean <williamsukatube@gmail.com>
---
 drivers/irqchip/irq-mips-gic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index ff89b36267dd..a1f6d955794a 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -734,6 +734,10 @@ static int __init gic_of_init(struct device_node *node,
 	}
 
 	mips_gic_base = ioremap(gic_base, gic_len);
+	if (!mips_gic_base) {
+		pr_err("Failed to ioremap gic_base\n");
+		return -ENOMEM;
+	}
 
 	gicconfig = read_gic_config();
 	gic_shared_intrs = FIELD_GET(GIC_CONFIG_NUMINTERRUPTS, gicconfig);
-- 
2.25.1

Re: [PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()

[Marc Zyngier]

On Fri, 22 Jul 2022 10:10:08 +0100,
williamsukatube@163.com wrote:
> 
> From: William Dean <williamsukatube@gmail.com>
> 
> The function ioremap() in gic_of_init() can fail, so
> its return value should be checked.
> 
> Fixes: 4bdc0d676a643 ("remove ioremap_nocache and devm_ioremap_nocache")

Erm... No. The issue was definitely there before (just look at the
patch you quote here).

> Reported-by: Hacash Robot <hacashRobot@santino.com>
> Signed-off-by: William Dean <williamsukatube@gmail.com>

If sending from a different address, please add a SoB that matches the
address you are using.

> ---
>  drivers/irqchip/irq-mips-gic.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index ff89b36267dd..a1f6d955794a 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -734,6 +734,10 @@ static int __init gic_of_init(struct device_node *node,
>  	}
>  
>  	mips_gic_base = ioremap(gic_base, gic_len);
> +	if (!mips_gic_base) {
> +		pr_err("Failed to ioremap gic_base\n");
> +		return -ENOMEM;
> +	}
>  
>  	gicconfig = read_gic_config();
>  	gic_shared_intrs = FIELD_GET(GIC_CONFIG_NUMINTERRUPTS, gicconfig);

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()

[Marc Zyngier]

On Sat, 23 Jul 2022 03:57:18 +0100,
William Dean <williamsukatube@gmail.com> wrote:
> 
> Thans for your advice.
> 
> > Erm... No. The issue was definitely there before (just look at the
> > patch you quote here).
> 
> Do you mean that I wrote the fixes-tag wrong?  I just checked it, it was
> introduced by the earlier commit fbea754123ae5d9678295398c98e91f1b2159e5b,
> if you mean that, I will send patch v2

Not even that. Here's what this patch has:

-       mips_gic_base = ioremap_nocache(gic_base_addr, gic_addrspace_size);
+       if (mips_cm_present()) {
+               write_gcr_gic_base(gic_base | CM_GCR_GIC_BASE_GICEN);
+               /* Ensure GIC region is enabled before trying to access it */
+               __sync();
+       }
+
+       mips_gic_base = ioremap_nocache(gic_base, gic_len);
 
        gicconfig = read_gic_config();
        gic_shared_intrs = gicconfig & GIC_CONFIG_NUMINTERRUPTS;

Still no check for the ioremap result. You can actually trace it all
the way down to 39b8d5254246a ("[MIPS] Add support for MIPS CMP
platform."), which introduced that code initially. But is it worth
referencing such an old commit? No.

To be honest, given how early this code executes, failing to ioremap
something is utterly unlikely, and adding the check is mostly about
being pedantic.

It is also worth noting that most of the use of this variable are
guarded by mips_gic_present(), which actually checks for mips_gic_base
being NULL and that the driver actually *ignores* such mapping.

Given that, I'm not sure this deserves an actual Fixes: tag. This is
completely harmless. Just resend the patch with a correct SoB, and
I'll queue it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.