Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 13:33:48 +0100
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Song Shuai <suagrfillet@gmail.com>
CC:     <paul.walmsley@sifive.com>, <palmer@dabbelt.com>,
        <aou@eecs.berkeley.edu>, <ajones@ventanamicro.com>,
        <sunilvl@ventanamicro.com>, <heiko.stuebner@vrull.eu>,
        <apatel@ventanamicro.com>, <evan@rivosinc.com>,
        <greentime.hu@sifive.com>, <leyfoon.tan@starfivetech.com>,
        <linux-riscv@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] riscv: BUG_ON() for no cpu nodes in setup_smp
Message-ID: <20230629-maverick-kelp-17327f04482a@wendy>
References: <20230629105839.1160895-1-suagrfillet@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="+Xe4Uyu39cOMQrN6"
Content-Disposition: inline
In-Reply-To: <20230629105839.1160895-1-suagrfillet@gmail.com>
Precedence: bulk

--+Xe4Uyu39cOMQrN6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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(-)
>=20
> 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)
> =20
>  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.

>  }
> --=20
> 2.20.1
>=20

--+Xe4Uyu39cOMQrN6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZJ16LAAKCRB4tDGHoIJi
0kjfAQDJJ0vTpSJCysk3tWFZuO7Zk8aSbHHO0s+tldla0nXdZAD5AYH55vxAQeSf
VUtbo0CsM0uDGuFly0pPfOcvM0iKOgA=
=JAVg
-----END PGP SIGNATURE-----

--+Xe4Uyu39cOMQrN6--