linux-kernel - Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Jun 2023 13:33:48 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Song Shuai <suagrfillet@...il.com>
CC:     <paul.walmsley@...ive.com>, <palmer@...belt.com>,
        <aou@...s.berkeley.edu>, <ajones@...tanamicro.com>,
        <sunilvl@...tanamicro.com>, <heiko.stuebner@...ll.eu>,
        <apatel@...tanamicro.com>, <evan@...osinc.com>,
        <greentime.hu@...ive.com>, <leyfoon.tan@...rfivetech.com>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup_smp

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@...il.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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)