Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 23 Jul 2022 10:10:19 +0100
Message-ID: <87h738xkhw.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     William Dean <williamsukatube@gmail.com>
Cc:     williamsukatube@163.com, tglx@linutronix.de,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        tsbogend@alpha.franken.de, fancer.lancer@gmail.com,
        Hacash Robot <hacashRobot@santino.com>
Subject: Re: [PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()
In-Reply-To: <CAK6EE7=AWA1fV6j7903V-PTFnUDa70itkbkxv7vBVNrRGAoV+w@mail.gmail.com>
References: <20220722091008.2937238-1-williamsukatube@163.com>
        <87mtd1y02y.wl-maz@kernel.org>
        <CAK6EE7=AWA1fV6j7903V-PTFnUDa70itkbkxv7vBVNrRGAoV+w@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.