Re: [PATCH v6 04/14] irqchip/sifive-plic: Use platform driver probing for PLIC

From: Anup Patel <apatel@ventanamicro.com>
To: Sunil V L <sunilvl@ventanamicro.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Marc Zyngier <maz@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Conor Dooley <conor+dt@kernel.org>,
	Atish Patra <atishp@atishpatra.org>,
	Andrew Jones <ajones@ventanamicro.com>,
	Saravana Kannan <saravanak@google.com>,
	Anup Patel <anup@brainfault.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v6 04/14] irqchip/sifive-plic: Use platform driver probing for PLIC
Date: Wed, 2 Aug 2023 17:55:17 +0530	[thread overview]
Message-ID: <CAK9=C2V_Pf2JAKRvufU9J2OmFkaAO3MuonDBvZDKTJGNb=vBOw@mail.gmail.com> (raw)
In-Reply-To: <ZMItuhqTCJCirg4s@sunil-laptop>

Hi Sunil,

On Thu, Jul 27, 2023 at 2:11 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Hi Anup,
>
> On Wed, Jul 19, 2023 at 05:05:32PM +0530, Anup Patel wrote:
> > The PLIC driver does not require very early initialization so let us
> > replace use of IRQCHIP_DECLARE() with IRQCHIP_PLATFORM_DRIVER_xyz()
> > so that PLIC is probed through platform driver probing.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 56b0544b1f27..dc02f0761ced 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -569,8 +569,10 @@ static int __init plic_init(struct device_node *node,
> >       return __plic_init(node, parent, 0);
> >  }
> >
> > -IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
> > -IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
> > +IRQCHIP_PLATFORM_DRIVER_BEGIN(sifive_plic)
> > +IRQCHIP_MATCH("sifive,plic-1.0.0", plic_init)
> > +IRQCHIP_MATCH("riscv,plic0", plic_init) /* for legacy systems */
> > +IRQCHIP_PLATFORM_DRIVER_END(sifive_plic)
> >
> Few observations..
>
> 1) IRQCHIP_PLATFORM_DRIVER_xyz work only for DT. Can we have more generic
> platform driver probe similar to APLIC driver?

Okay, I will convert it into generic platform driver which
will work for ACPI as well.

>
> 2) With platform driver model, there is a crash some times.
>
> [    0.198665] CPU: 4 PID: 32 Comm: cpuhp/4 Not tainted
> 6.5.0-rc3-00038-g782440711cdb #42
> [    0.198944] Hardware name: riscv-virtio,qemu (DT)
> [    0.199125] epc : plic_starting_cpu+0x60/0xa6
> [    0.199545]  ra : plic_starting_cpu+0x58/0xa6
> [    0.199676] epc : ffffffff8047396e ra : ffffffff80473966 sp :
> ff20000000173d80
> [    0.199899]  gp : ffffffff81501740 tp : ff600000019bde00 t0 :
> ffffffff81200800
> [    0.200112]  t1 : 0000000000000000 t2 : 0000000000001000 s0 :
> ff20000000173db0
> [    0.200324]  s1 : ff6000007ef91eb0 a0 : ff600000019690b0 a1 :
> 0000000200000020
> [    0.200502]  a2 : 0000000000000000 a3 : 0000000000000000 a4 :
> 0000000000000000
> [    0.200678]  a5 : 0000000000000000 a6 : ff6000000184a108 a7 :
> ff6000000184a100
> [    0.200855]  s2 : 000000000000000b s3 : ffffffff8153a550 s4 :
> 0000000000000004
> [    0.201032]  s5 : 0000000000000001 s6 : 0000000000000002 s7 :
> ffffffff8047390e
> [    0.201210]  s8 : ffffffff80033f8a s9 : ffffffff80033f64 s10:
> 0000000000000000
> [    0.201392]  s11: 0000000000000000 t3 : 0000000000000000 t4 :
> ffffffff81486c20
> [    0.201567]  t5 : 0000000000000002 t6 : 0000000000000002
> [    0.201702] status: 0000000200000100 badaddr: 0000000000000000 cause:
> 000000000000000f
> [    0.201962] [<ffffffff8047396e>] plic_starting_cpu+0x60/0xa6
> [    0.202184] [<ffffffff8001069e>] cpuhp_invoke_callback+0xb4/0x164
> [    0.202346] [<ffffffff80010972>] cpuhp_thread_fun+0x8a/0x11c
> [    0.202493] [<ffffffff80033f62>] smpboot_thread_fn+0xe4/0x1be
> [    0.202639] [<ffffffff8003042a>] kthread+0xc4/0xe0
> [    0.202759] [<ffffffff8000355e>] ret_from_fork+0xa/0x1c
> [    0.203038] Code: 89bd 854a 3097 ffbf 80e7 51e0 000f 0140 4781 6498
> (c31c) 2703

The cpuhp setup needed to change since as platform driver the
PLIC is probed after all CPUs are brought-up. I will fix this in
the next revision.

>
> 3) Looks like an existing issue but I get below warning also.
>
> [    0.207677] list_add double add: new=ffffffff814a0798,
> prev=ffffffff814a0798, next=ffffffff814be638.
> [    0.208243] WARNING: CPU: 4 PID: 1 at lib/list_debug.c:33
> __list_add_valid+0x46/0xac
> [    0.208677] Modules linked in:
> [    0.208780] CPU: 4 PID: 1 Comm: swapper/0 Not tainted
> 6.5.0-rc3-00038-g782440711cdb #28
> [    0.208862] Hardware name: riscv-virtio,qemu (DT)
> [    0.208926] epc : __list_add_valid+0x46/0xac
> [    0.208961]  ra : __list_add_valid+0x46/0xac
> [    0.208983] epc : ffffffff803cb356 ra : ffffffff803cb356 sp :
> ff200000000439f0
> [    0.208994]  gp : ffffffff81501740 tp : ff600000018f0000 t0 :
> 2000000000000000
> [    0.209005]  t1 : 000000000000006c t2 : 206464615f747369 s0 :
> ff20000000043a00
> [    0.209015]  s1 : ffffffff814a0798 a0 : 0000000000000058 a1 :
> ffffffff81486828
> [    0.209025]  a2 : 0000000000000000 a3 : fffffffffffffffe a4 :
> 0000000000000000
> [    0.209034]  a5 : 0000000000000000 a6 : 0000000000000068 a7 :
> 0000000000000038
> [    0.209044]  s2 : ffffffff814be5f8 s3 : ffffffff814a0798 s4 :
> ffffffff814be638
> [    0.209053]  s5 : ff6000007fff75c0 s6 : 0000000000000060 s7 :
> ffffffff80c20eb0
> [    0.209063]  s8 : 0000000000000008 s9 : 0000000000000004 s10:
> ffffffff80c20eb0
> [    0.209072]  s11: ffffffff81038e80 t3 : ffffffff815159d7 t4 :
> ffffffff815159d7
> [    0.209081]  t5 : ffffffff815159d8 t6 : ff200000000437f8
> [    0.209090] status: 0000000200000120 badaddr: 0000000000000000 cause:
> 0000000000000003
> [    0.209167] [<ffffffff803cb356>] __list_add_valid+0x46/0xac
> [    0.209219] [<ffffffff805812d4>] register_syscore_ops+0x3e/0x70
> [    0.209245] [<ffffffff80a1fd0c>] __plic_init.isra.0+0x3e0/0x4e2
> [    0.209267] [<ffffffff80a1fe3a>] plic_init+0x12/0x1a
> [    0.209281] [<ffffffff80470c10>] platform_irqchip_probe+0x82/0xb4
> [    0.209292] [<ffffffff80582aa8>] platform_probe+0x4e/0xa6
> [    0.209301] [<ffffffff805805a4>] really_probe+0x86/0x242
> [    0.209308] [<ffffffff805807bc>] __driver_probe_device+0x5c/0xda
> [    0.209316] [<ffffffff80580866>] driver_probe_device+0x2c/0xb2
> [    0.209324] [<ffffffff805809e6>] __driver_attach+0x6c/0x11a
> [    0.209331] [<ffffffff8057e73a>] bus_for_each_dev+0x60/0xae
> [    0.209343] [<ffffffff8057fff6>] driver_attach+0x1a/0x22
> [    0.209352] [<ffffffff8057f956>] bus_add_driver+0xd0/0x1ba
> [    0.209362] [<ffffffff80581634>] driver_register+0x3e/0xd8
> [    0.209370] [<ffffffff805827c0>] __platform_driver_register+0x1c/0x24
> [    0.209378] [<ffffffff80a1f902>] sifive_plic_driver_init+0x1a/0x22
> [    0.209389] [<ffffffff8000212c>] do_one_initcall+0x58/0x19c
> [    0.209399] [<ffffffff80a01056>] kernel_init_freeable+0x20c/0x276
> [    0.209410] [<ffffffff808d6686>] kernel_init+0x1e/0x10a
> [    0.209421] [<ffffffff8000355e>] ret_from_fork+0xa/0x1c
> [    0.209477] ---[ end trace 0000000000000000 ]---

This is a bug in current PLIC driver where it unconditionally
registers syscore ops which breaks on multi-socket systems
having multiple PLICs. I will add a separate patch in the
next revision to fix this issue.

Regards,
Anup