Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup_smp

[Song Shuai]

在 2023/6/29 20:33, Conor Dooley 写道:
> Hey,
>
> On Thu, Jun 29, 2023 at 06:58:39PM +0800, Song Shuai wrote:
> > When booting with ACPI tables, the tiny devictree created by
> > EFI Stub doesn't provide cpu nodes.

"When only the ACPI tables are passed to kernel, the tiny..."
That seems more accurate. We can use "acpi=" kernel
parameter to manually enable/disable ACPI.

> What are the conditions that are required to reproduce this issue?
> When booting with ACPI, why is acpi_disabled true?
> In my naivety, that seems like a bigger problem to address. >

Actually, I appended the "acpi=off" to kernel cmdline for testing the 
"off" option. That would set acpi_disabled as true.

> > In setup_smp(), of_parse_and_init_cpus() will bug on !found_boot_cpu
>
> Please, s/on !found_boot_cpu/if the boot cpu is not found in the
> devicetree/, or similar.
ok
> > if acpi_disabled.
>
> Why would of_parse_and_init_cpus() be called in any other case? There's
> only this one caller, right?
yes
> > That's unclear, so bug for no cpu nodes before
> > of_parse_and_init_cpus().
>
> What is unclear? That the reason for the BUG() was that there were no
> cpu nodes, since it could also be that there were CPU nodes but they
> were disabled etc?

The BUG() in of_parse_and_init_cpus() indicates there was no boot cpu 
found in the devicetree , not there were no cpu nodes in the devices.
That's the "unclear" I mean.

> > Signed-off-by: Song Shuai <suagrfillet@xxxxxxxxx>
> > ---
> >   arch/riscv/kernel/smpboot.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> > index 6ca2b5309aab..243a7b533ad7 100644
> > --- a/arch/riscv/kernel/smpboot.c
> > +++ b/arch/riscv/kernel/smpboot.c
> > @@ -187,8 +187,13 @@ static void __init of_parse_and_init_cpus(void)
> >   
> >   void __init setup_smp(void)
> >   {
> > -	if (acpi_disabled)
> > +	if (acpi_disabled) {
> > +		/* When booting with ACPI tables, the devictree created by EFI Stub
>
> This is not netdev, please use the correct comment style :/
>
> > +		 * doesn't provide cpu nodes. So BUG here for any acpi_disabled.
> > +		 */
> > +		BUG_ON(!of_get_next_cpu_node(NULL));
> >   		of_parse_and_init_cpus();
> > +	}
> >   	else
> >   		acpi_parse_and_init_cpus();
>
> checkpatch should have told you that you now need to add braces on all
> arms of this statement.
ok,
> Or, better yet, move the whole thing into of_parse_and_init_cpus() in
> the first place? You could drop most of the comment in the process,
> since I think the details of how you hit this problem would likely not
> be helpful to anyone that hit it under different conditions.
ok, I'll apply these comments if you're ok with my replies.

> Cheers,
> Conor.
>
> >   }
> > --
> > 2.20.1

--
Thanks
Song Shuai