[PATCH] arm64: Simplify checking for populated DT

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Oct 29, 2021 at 4:38 AM Mark Rutland [off-list ref] wrote:

Hi Rob,

On Thu, Oct 28, 2021 at 01:33:55PM -0500, Rob Herring wrote:

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: kernel test robot <redacted>

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 <redacted>
Signed-off-by: kernel test robot <redacted>
---

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;
 }
 
 /*

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>


cocci warnings: (new ones prefixed by >>)

quoted

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