Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <20230926102801.1591126-1-dunaev@tecon.ru> <CAK9=C2UiBNQtv0Q2yJMKqc5pMX_jam+ZmfRz3Rme0ZYuqN68HA@mail.gmail.com>
 <86il7mofmm.wl-maz@kernel.org> <CAK9=C2UUQ+ChueJVs+AnY0oCTb1zSv7qOCTijSchwxiMspX74w@mail.gmail.com>
 <86cyxuo128.wl-maz@kernel.org>
In-Reply-To: <86cyxuo128.wl-maz@kernel.org>
From:   Anup Patel <apatel@ventanamicro.com>
Date:   Wed, 4 Oct 2023 21:38:06 +0530
Message-ID: <CAK9=C2WcOy0_iEF7EF0LwjSRqVf2bbFaVNbxPXa=0OJtUkoVMw@mail.gmail.com>
Subject: Re: [PATCH] irqchip/riscv-intc: Mark INTC nodes for secondary CPUs as initialized.
To:     Marc Zyngier <maz@kernel.org>
Cc:     Dmitry Dunaev <dunaev@tecon.ru>, dunaich@mail.ru,
        Thomas Gleixner <tglx@linutronix.de>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Oct 4, 2023 at 9:02=E2=80=AFPM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 04 Oct 2023 15:59:33 +0100,
> Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Wed, Oct 4, 2023 at 3:48=E2=80=AFPM Marc Zyngier <maz@kernel.org> wr=
ote:
> > >
> > > On Tue, 26 Sep 2023 11:36:31 +0100,
> > > Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Tue, Sep 26, 2023 at 3:59=E2=80=AFPM Dmitry Dunaev <dunaev@tecon=
.ru> wrote:
> > > > >
> > > > > The current Linux driver irq-riscv-intc initialize IRQ domain onl=
y once,
> > > > > when init function called on primary hart. In other cases no IRQ =
domain is
> > > > > created and no operation on interrupt-controller node is performe=
d.
> > > > > This is cause of that no common Linux driver can use per-cpu inte=
rrupts
> > > > > mapped to several CPUs because fwnode of secondary cores INTC is =
not
> > > > > marked as initialized. This device is always will be marked as de=
ferred.
> > > > > For example the system with devicetree
> > > > >
> > > > >     cpu0: cpu@0 {
> > > > >         cpu0_intc: interrupt-controller {
> > > > >             interrupt-controller;
> > > > >             compatible =3D riscv,cpu-intc;
> > > > >         };
> > > > >     };
> > > > >
> > > > >     cpu1: cpu@1 {
> > > > >         cpu1_intc: interrupt-controller {
> > > > >             interrupt-controller;
> > > > >             compatible =3D riscv,cpu-intc;
> > > > >         };
> > > > >     };
> > > > >
> > > > >     buserr {
> > > > >         compatible =3D riscv,buserr;
> > > > >         interrupts-extended =3D <&cpu0_intc 16 &cpu1_intc 16>;
> > > > >     };
> > > > >
> > > > > will always report 'buserr' node as deferred without calling any
> > > > > bus probe function.
> > > > >
> > > > > This patch will mark all secondary nodes passed to irq-riscv-intc
> > > > > driver init function as initialized to be able to act as correct
> > > > > IRQ phandle node.
> > > > >
> > > > > Signed-off-by: Dmitry Dunaev <dunaev@tecon.ru>
> > > > > ---
> > > > >  drivers/irqchip/irq-riscv-intc.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/i=
rq-riscv-intc.c
> > > > > index 4adeee1bc391..c01a4e8d4983 100644
> > > > > --- a/drivers/irqchip/irq-riscv-intc.c
> > > > > +++ b/drivers/irqchip/irq-riscv-intc.c
> > > > > @@ -155,8 +155,10 @@ static int __init riscv_intc_init(struct dev=
ice_node *node,
> > > > >          * for each INTC DT node. We only need to do INTC initial=
ization
> > > > >          * for the INTC DT node belonging to boot CPU (or boot HA=
RT).
> > > > >          */
> > > > > -       if (riscv_hartid_to_cpuid(hartid) !=3D smp_processor_id()=
)
> > > > > +       if (riscv_hartid_to_cpuid(hartid) !=3D smp_processor_id()=
) {
> > > > > +               fwnode_dev_initialized(of_node_to_fwnode(node), t=
rue);
> > > >
> > > > There is already a patch on LKML to address this.
> > > > https://www.spinics.net/lists/kernel/msg4929886.html
> > >
> > > If this is a fix, why is it buried in a huge series and not brought
> > > forward as an independent fix that needs to be picked early?
> >
> > Dmitry saw this issue in a totally different context which is not
> > reproducible with existing DTS files in kernel sources.
>
> I hope you're not suggesting that only the DTs that are present in the
> kernel tree are valid. Because as far as I'm concern, the DTs in the
> kernel tree are only some *examples*, and not a reference.

I am only saying why this issue was not observed before.

>
> I fully expect the vast majority of DTs to live *outside* of the
> kernel tree, provided by the firmware, and never upstreamed. Would you
> expect every PC vendor to upstream their ACPI tables?

I agree. We can't expect all vendors to submit DT to kernel sources.

>
> > This issue only manifests when some platform driver DT node
> > points to the per-HART INTC nodes. For example, RISC-V
> > irqchip device DT nodes point to per-HART INTC nodes.
>
> Is this configuration legal or not as per the DT binding? I don't see
> anything that suggests it isn't legal, and having per-CPU interrupts
> isn't exactly a new thing.

This is a perfect legal configuration in the RISC-V world. We have
similar DT binding for AIA drivers as well.

>
> > Currently, all RISC-V irqchip drivers (INTC and PLIC) are probed
> > early (not as platform drivers) so we don't see this issue with
> > existing irqchip drivers.
>
> You don't, but Dimitry does. Who wins?

I am totally fine taking PATCH3 of the Linux AIA v10 series as a
fix PATCH for 6.6-rcX. The PATCH3 is pretty self contained and
does not depend on any other PATCH of Linux AIA v10 series.

Do you want me to re-send it as an individual PATCH ?

Regards,
Anup

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