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

[Marc Zyngier]

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 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?

> 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.

> 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?

	M.

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