[PATCH] arm64: Simplify checking for populated DT

[PATCH] arm64: Simplify checking for populated DT

[Rob Herring]

Use of of_scan_flat_dt() function predates libfdt and is discouraged as
libfdt provides a nicer set of APIs. Rework dt_scan_depth1_nodes to use
libfdt calls directly. Rather than searching for any node not /chosen or
/hypervisor, let's just check for something always required which is the
arch timer.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 arch/arm64/kernel/acpi.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 1c9c2f7a1c04..f1bb0fb4a604 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -22,6 +22,7 @@
 #include <linux/irq_work.h>
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
+#include <linux/libfdt.h>
 #include <linux/smp.h>
 #include <linux/serial_core.h>
 #include <linux/pgtable.h>
@@ -62,29 +63,12 @@ static int __init parse_acpi(char *arg)
 }
 early_param("acpi", parse_acpi);
 
-static int __init dt_scan_depth1_nodes(unsigned long node,
-				       const char *uname, int depth,
-				       void *data)
+static bool __init dt_is_populated(void)
 {
-	/*
-	 * Ignore anything not directly under the root node; we'll
-	 * catch its parent instead.
-	 */
-	if (depth != 1)
-		return 0;
-
-	if (strcmp(uname, "chosen") == 0)
-		return 0;
+	const void *fdt = initial_boot_params;
+	int node = fdt_node_offset_by_compatible(fdt, -1, "arm,armv8-timer");
 
-	if (strcmp(uname, "hypervisor") == 0 &&
-	    of_flat_dt_is_compatible(node, "xen,xen"))
-		return 0;
-
-	/*
-	 * This node at depth 1 is neither a chosen node nor a xen node,
-	 * which we do not expect.
-	 */
-	return 1;
+	return node > 0 ? true : false;
 }
 
 /*
@@ -205,8 +189,7 @@ void __init acpi_boot_table_init(void)
 	 *   and ACPI has not been [force] enabled (acpi=on|force)
 	 */
 	if (param_acpi_off ||
-	    (!param_acpi_on && !param_acpi_force &&
-	     of_scan_flat_dt(dt_scan_depth1_nodes, NULL)))
+	    (!param_acpi_on && !param_acpi_force && dt_is_populated()))
 		goto done;
 
 	/*
-- 
2.32.0

Re: [PATCH] arm64: Simplify checking for populated DT

[Mark Rutland]

Hi Rob,

On Thu, Oct 28, 2021 at 01:33:55PM -0500, Rob Herring wrote:
> Use of of_scan_flat_dt() function predates libfdt and is discouraged as
> libfdt provides a nicer set of APIs. Rework dt_scan_depth1_nodes to use
> libfdt calls directly. Rather than searching for any node not /chosen or
> /hypervisor, let's just check for something always required which is the
> arch timer.

The reason for checking for /chosen and /hypervisor specifically was
that we specifically permit a "stub" DT which only contains those nodes
and nothing more. This is also mentioned in the comment block in
acpi_boot_table_init().

I'm not keen on this change because it opens the door for people to
place arbitrary things in the DT as long as they don't add a timer node,
and I'd prefer that we continue to check for /chosen and /hypervisor
specifically.

Can we re-implement the existing logic using libfdt calls? e.g. using
fdt_for_each_subnode() on the root node?

Thanks,
Mark.

> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  arch/arm64/kernel/acpi.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 1c9c2f7a1c04..f1bb0fb4a604 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -22,6 +22,7 @@
>  #include <linux/irq_work.h>
>  #include <linux/memblock.h>
>  #include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
>  #include <linux/smp.h>
>  #include <linux/serial_core.h>
>  #include <linux/pgtable.h>
> @@ -62,29 +63,12 @@ static int __init parse_acpi(char *arg)
>  }
>  early_param("acpi", parse_acpi);
>  
> -static int __init dt_scan_depth1_nodes(unsigned long node,
> -				       const char *uname, int depth,
> -				       void *data)
> +static bool __init dt_is_populated(void)
>  {
> -	/*
> -	 * Ignore anything not directly under the root node; we'll
> -	 * catch its parent instead.
> -	 */
> -	if (depth != 1)
> -		return 0;
> -
> -	if (strcmp(uname, "chosen") == 0)
> -		return 0;
> +	const void *fdt = initial_boot_params;
> +	int node = fdt_node_offset_by_compatible(fdt, -1, "arm,armv8-timer");
>  
> -	if (strcmp(uname, "hypervisor") == 0 &&
> -	    of_flat_dt_is_compatible(node, "xen,xen"))
> -		return 0;
> -
> -	/*
> -	 * This node at depth 1 is neither a chosen node nor a xen node,
> -	 * which we do not expect.
> -	 */
> -	return 1;
> +	return node > 0 ? true : false;
>  }
>  
>  /*
> @@ -205,8 +189,7 @@ void __init acpi_boot_table_init(void)
>  	 *   and ACPI has not been [force] enabled (acpi=on|force)
>  	 */
>  	if (param_acpi_off ||
> -	    (!param_acpi_on && !param_acpi_force &&
> -	     of_scan_flat_dt(dt_scan_depth1_nodes, NULL)))
> +	    (!param_acpi_on && !param_acpi_force && dt_is_populated()))
>  		goto done;
>  
>  	/*
> -- 
> 2.32.0
> 

Re: [PATCH] arm64: Simplify checking for populated DT

[Rob Herring]

On Fri, Oct 29, 2021 at 4:38 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Rob,
>
> On Thu, Oct 28, 2021 at 01:33:55PM -0500, Rob Herring wrote:
> > Use of of_scan_flat_dt() function predates libfdt and is discouraged as
> > libfdt provides a nicer set of APIs. Rework dt_scan_depth1_nodes to use
> > libfdt calls directly. Rather than searching for any node not /chosen or
> > /hypervisor, let's just check for something always required which is the
> > arch timer.
>
> The reason for checking for /chosen and /hypervisor specifically was
> that we specifically permit a "stub" DT which only contains those nodes
> and nothing more. This is also mentioned in the comment block in
> acpi_boot_table_init().

There will be a compatibility problem if another node is ever added to the stub.

> I'm not keen on this change because it opens the door for people to
> place arbitrary things in the DT as long as they don't add a timer node,
> and I'd prefer that we continue to check for /chosen and /hypervisor
> specifically.

How would one use that? The DT is never unflattened. I guess
early_init_dt_scan() could find something.

> Can we re-implement the existing logic using libfdt calls? e.g. using
> fdt_for_each_subnode() on the root node?

Sure.

Rob

Re: [PATCH] arm64: Simplify checking for populated DT

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

Hi Rob,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on arm-perf/for-next/perf arm/for-next soc/for-next kvmarm/next v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rob-Herring/arm64-Simplify-checking-for-populated-DT/20211029-023532
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm64-randconfig-c004-20211028 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cocci warnings: (new ones prefixed by >>)
>> arch/arm64/kernel/acpi.c:71:26-31: WARNING: conversion to bool not needed here

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45242 bytes --]

[PATCH] arm64: fix boolconv.cocci warnings

[kernel test robot]

From: kernel test robot <lkp@intel.com>

arch/arm64/kernel/acpi.c:71:26-31: WARNING: conversion to bool not needed here

 Remove unneeded conversion to bool

Semantic patch information:
 Relational and logical operators evaluate to bool,
 explicit conversion is overly verbose and unneeded.

Generated by: scripts/coccinelle/misc/boolconv.cocci

CC: Rob Herring <robh@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Rob-Herring/arm64-Simplify-checking-for-populated-DT/20211029-023532
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
:::::: branch date: 21 hours ago
:::::: commit date: 21 hours ago

 acpi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -68,7 +68,7 @@ static bool __init dt_is_populated(void)
 	const void *fdt = initial_boot_params;
 	int node = fdt_node_offset_by_compatible(fdt, -1, "arm,armv8-timer");
 
-	return node > 0 ? true : false;
+	return node > 0;
 }
 
 /*