| Message ID | 20220722091008.2937238-1-williamsukatube@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip: mips-gic: check the return value of ioremap() in gic_of_init() | expand |
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.
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.
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);