lkml.org 
[lkml]   [2023]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] riscv: BUG_ON() for no cpu nodes in setup_smp
From

在 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@gmail.com>
>> ---
>> 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

\
 
 \ /
  Last update: 2023-06-30 05:03    [W:0.041 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site