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

[Song Shuai <suagrfillet@gmail.com>]

在 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv