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:   Tue, 09 Aug 2022 13:53:21 +0100
Message-ID: <87y1vxvari.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Huacai Chen <chenhuacai@kernel.org>
Cc:     Huacai Chen <chenhuacai@loongson.cn>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-arch <linux-arch@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Xuerui Wang <kernel@xen0n.name>,
        Xuefeng Li <lixuefeng@loongson.cn>,
        Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH] LoongArch: Don't disable EIOINTC master core
In-Reply-To: <CAAhV-H4ZCmenrPvzLCaZZn57NoiGpQfVGpXg42DE2JevF4Tf9w@mail.gmail.com>
References: <20220809074522.2444672-1-chenhuacai@loongson.cn>
        <874jylx0ad.wl-maz@kernel.org>
        <CAAhV-H50HERxjrmfDkSaHAVY7Q_ufpZP-e+qdZWVd=qEuQVXBg@mail.gmail.com>
        <871qtpwwem.wl-maz@kernel.org>
        <CAAhV-H4ZCmenrPvzLCaZZn57NoiGpQfVGpXg42DE2JevF4Tf9w@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 Tue, 09 Aug 2022 11:39:15 +0100,
Huacai Chen <chenhuacai@kernel.org> wrote:
> 
> Hi, Marc,
> 
> On Tue, Aug 9, 2022 at 6:20 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 09 Aug 2022 10:19:31 +0100,
> > Huacai Chen <chenhuacai@kernel.org> wrote:
> > >
> > > Hi, Marc,
> > >
> > > On Tue, Aug 9, 2022 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > On Tue, 09 Aug 2022 08:45:22 +0100,
> > > > Huacai Chen <chenhuacai@loongson.cn> wrote:
> > > > >
> > > > > This patch fix a CPU hotplug issue. The EIOINTC master core (the first
> > > > > core of an EIOINTC node) should not be disabled at runtime, since it has
> > > > > the responsibility of dispatching I/O interrupts.
> > > > >
> > > > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > ---
> > > > >  arch/loongarch/kernel/smp.c            | 9 +++++++++
> > > > >  drivers/irqchip/irq-loongson-eiointc.c | 5 +++++
> > > > >  2 files changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
> > > > > index 09743103d9b3..54901716f8de 100644
> > > > > --- a/arch/loongarch/kernel/smp.c
> > > > > +++ b/arch/loongarch/kernel/smp.c
> > > > > @@ -242,9 +242,18 @@ void loongson3_smp_finish(void)
> > > > >
> > > > >  static bool io_master(int cpu)
> > > > >  {
> > > > > +     int i, node, master;
> > > > > +
> > > > >       if (cpu == 0)
> > > > >               return true;
> > > > >
> > > > > +     for (i = 1; i < loongson_sysconf.nr_io_pics; i++) {
> > > > > +             node = eiointc_get_node(i);
> > > > > +             master = cpu_number_map(node * CORES_PER_EIO_NODE);
> > > > > +             if (cpu == master)
> > > > > +                     return true;
> > > > > +     }
> > > > > +
> > > > >       return false;
> > > > >  }
> > > > >
> > > > > diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
> > > > > index 170dbc96c7d3..6c99a2ff95f5 100644
> > > > > --- a/drivers/irqchip/irq-loongson-eiointc.c
> > > > > +++ b/drivers/irqchip/irq-loongson-eiointc.c
> > > > > @@ -56,6 +56,11 @@ static void eiointc_enable(void)
> > > > >       iocsr_write64(misc, LOONGARCH_IOCSR_MISC_FUNC);
> > > > >  }
> > > > >
> > > > > +int eiointc_get_node(int id)
> > > > > +{
> > > > > +     return eiointc_priv[id]->node;
> > > > > +}
> > > > > +
> > > > >  static int cpu_to_eio_node(int cpu)
> > > > >  {
> > > > >       return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
> > > >
> > > >
> > > > I don't understand why it has to be this complex and make any use of
> > > > the node number.
> > > >
> > > > As I understand it, CPU-0 in any EIOINTC block is a master. So all you
> > > > need to find out is whether the CPU number is a multiple of
> > > > CORES_PER_EIO_NODE.
> > > CPU-0 in any EIOINTC block may be a master, but not absolutely be a
> > > master to dispatch I/O interrupts. If there is no bridge under a
> > > EIOINTC, then this EIOINTC doesn't handle I/O interrupts, and it can
> > > be disabled at runtime.
> >
> > But that's not what your code is checking, is it? You're only
> > reporting the node number, irrespective of whether there is anything
> > behind the EIOINTC.
> The return value of eiointc_get_node() means "this eio-node has a
> downstream bridge, so the master core of this eio-node cannot be
> disabled". :)

So what is exactly the meaning of this node? All the EIOINTCs do have
one (it is coming from ACPI, and taken at face value), so the node
really is only a proxy for the CPU numbers that are attached to it,
isn't it? Can you have cores without an EIOINTC?

Now, if this is relevant to the arch code, I'd rather you keep track
of this directly in the arch code, because it looks really odd to peek
at an irqchip data structure for something that the core code should
have the first place.

It also strikes me that this patch has *zero* effect, as nothing ever
sets loongson_sysconf.nr_io_pics. Try this:

diff --git a/arch/loongarch/include/asm/bootinfo.h b/arch/loongarch/include/asm/bootinfo.h
index 9b8d49d9e61b..13e5e5e21ffd 100644
--- a/arch/loongarch/include/asm/bootinfo.h
+++ b/arch/loongarch/include/asm/bootinfo.h
@@ -28,7 +28,7 @@ struct loongson_board_info {
 struct loongson_system_configuration {
 	int nr_cpus;
 	int nr_nodes;
-	int nr_io_pics;
+//	int nr_io_pics;
 	int boot_cpu_id;
 	int cores_per_node;
 	int cores_per_package;

and see that the kernel still compiles.

	M.

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