| Message ID | 20230629105839.1160895-1-suagrfillet@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: BUG_ON() for no cpu nodes in setup_smp | expand |
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 <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. 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 <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 >>
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!
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(); }
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 <suagrfillet@gmail.com> --- arch/riscv/kernel/smpboot.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)