When booting with ACPI tables, the tiny devictree created by
EFI Stub doesn't provide cpu nodes.
In setup_smp(), of_parse_and_init_cpus() will bug on !found_boot_cpu
if acpi_disabled. That's unclear, so bug for no cpu nodes before
of_parse_and_init_cpus().
Signed-off-by: Song Shuai <[email protected]>
---
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
+ * 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();
}
--
2.20.1
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.
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..
> 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.
> if acpi_disabled.
Why would of_parse_and_init_cpus() be called in any other case? There's
only this one caller, right?
> 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?
> Signed-off-by: Song Shuai <[email protected]>
> ---
> 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.
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.
Cheers,
Conor.
> }
> --
> 2.20.1
>
在 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 <[email protected]>
>> ---
>> 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
Hey,
On Fri, Jun 30, 2023 at 11:02:18AM +0800, Song Shuai wrote:
> 在 2023/6/29 20:33, Conor Dooley 写道:
> > 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.
Yeah, I figured it was intentionally misconfiguration of your kernel,
given that you've tested other niche conditions and reported bugs, just
was not clear from your commit message whether this was a real bugTM
that should be fixed, or some sort of "you booted a kernel that can't be
used on this system".
> > > 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.
Yup, please put that in the commit message so that people don't have to
figure out your intent ;)
> > 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.
Yup, thanks!