Re: [PATCH] irqchip/riscv-intc: Mark INTC nodes for secondary CPUs as initialized.

[Anup Patel]

On Wed, Oct 4, 2023 at 9:02 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Wed, 04 Oct 2023 15:59:33 +0100,
> Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Oct 4, 2023 at 3:48 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 26 Sep 2023 11:36:31 +0100,
> > > Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Sep 26, 2023 at 3:59 PM Dmitry Dunaev <dunaev@xxxxxxxx> wrote:
> > > > >
> > > > > The current Linux driver irq-riscv-intc initialize IRQ domain only once,
> > > > > when init function called on primary hart. In other cases no IRQ domain is
> > > > > created and no operation on interrupt-controller node is performed.
> > > > > This is cause of that no common Linux driver can use per-cpu interrupts
> > > > > mapped to several CPUs because fwnode of secondary cores INTC is not
> > > > > marked as initialized. This device is always will be marked as deferred.
> > > > > For example the system with devicetree
> > > > >
> > > > >     cpu0: cpu@0 {
> > > > >         cpu0_intc: interrupt-controller {
> > > > >             interrupt-controller;
> > > > >             compatible = riscv,cpu-intc;
> > > > >         };
> > > > >     };
> > > > >
> > > > >     cpu1: cpu@1 {
> > > > >         cpu1_intc: interrupt-controller {
> > > > >             interrupt-controller;
> > > > >             compatible = riscv,cpu-intc;
> > > > >         };
> > > > >     };
> > > > >
> > > > >     buserr {
> > > > >         compatible = riscv,buserr;
> > > > >         interrupts-extended = <&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@xxxxxxxx>
> > > > > ---
> > > > >  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/irq-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 device_node *node,
> > > > >          * for each INTC DT node. We only need to do INTC initialization
> > > > >          * for the INTC DT node belonging to boot CPU (or boot HART).
> > > > >          */
> > > > > -       if (riscv_hartid_to_cpuid(hartid) != smp_processor_id())
> > > > > +       if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) {
> > > > > +               fwnode_dev_initialized(of_node_to_fwnode(node), true);
> > > >
> > > > 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.