| Message ID | 20211028183355.360731-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Simplify checking for populated DT | expand |
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 >
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
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
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; /*
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(-)