[PATCH 0/6] of: populate of_root node if bootloader doesn't

[PATCH 0/6] of: populate of_root node if bootloader doesn't

[Stephen Boyd]

Arch maintainers, please ack/review patches.

This is a resend of a series from Frank last year[1]. I worked in Rob's
review comments to unconditionally call unflatten_device_tree() and
fixup/audit calls to of_have_populated_dt() so that behavior doesn't
change.

I need this series so I can add DT based tests in the clk framework.
Either I can merge it through the clk tree once everyone is happy, or
Rob can merge it through the DT tree and provide some branch so I can
base clk patches on it.

Changes from Frank's series[1]:
 * Add a DTB loaded kunit test
 * Make of_have_populated_dt() return false if the DTB isn't from the
   bootloader
 * Architecture calls made unconditional so that a root node is always
   made

Frank Rowand (2):
  of: Create of_root if no dtb provided by firmware
  of: unittest: treat missing of_root as error instead of fixing up

Stephen Boyd (4):
  arm64: Unconditionally call unflatten_device_tree()
  um: Unconditionally call unflatten_device_tree()
  of: Always unflatten in unflatten_and_copy_device_tree()
  of: Add KUnit test to confirm DTB is loaded

 arch/arm64/kernel/setup.c |  3 +-
 arch/um/kernel/dtb.c      | 14 +++---
 drivers/of/.kunitconfig   |  3 ++
 drivers/of/Kconfig        | 16 ++++++-
 drivers/of/Makefile       |  4 +-
 drivers/of/empty_root.dts |  6 +++
 drivers/of/fdt.c          | 57 +++++++++++++++++------
 drivers/of/of_test.c      | 98 +++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c     |  3 --
 drivers/of/unittest.c     | 16 ++-----
 include/linux/of.h        | 17 +++++--
 11 files changed, 191 insertions(+), 46 deletions(-)
 create mode 100644 drivers/of/.kunitconfig
 create mode 100644 drivers/of/empty_root.dts
 create mode 100644 drivers/of/of_test.c

Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Gow <davidgow@google.com>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Weinberger <richard@nod.at>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Will Deacon <will@kernel.org>

[1] https://lore.kernel.org/r/20230317053415.2254616-1-frowand.list@gmail.com

base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

[PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Stephen Boyd]

Call this function unconditionally so that we can populate an empty DTB
on platforms that don't boot with a firmware provided or builtin DTB.
There's no harm in calling unflatten_device_tree() unconditionally. If
there isn't a valid initial_boot_params dtb then unflatten_device_tree()
returns early.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 arch/arm64/kernel/setup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 417a8a86b2db..ede3d59dabf0 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -351,8 +351,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
 	/* Parse the ACPI tables for possible boot-time configuration */
 	acpi_boot_table_init();
 
-	if (acpi_disabled)
-		unflatten_device_tree();
+	unflatten_device_tree();
 
 	bootmem_init();
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Rob Herring]

On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> Call this function unconditionally so that we can populate an empty DTB
> on platforms that don't boot with a firmware provided or builtin DTB.
> There's no harm in calling unflatten_device_tree() unconditionally. If
> there isn't a valid initial_boot_params dtb then unflatten_device_tree()
> returns early.

There's always a valid DTB because that's the boot params even for ACPI 
systems. This does also create a userspace visible change that 
/proc/device-tree will be populated. I don't see an issue with that.

There was worry when ACPI was added that systems would pass both DT and 
ACPI tables and that the kernel must only use ACPI. That was more to 
force ACPI adoption, but I'm not sure if that actually exists in any 
early system. I think we're past forcing adoption now.

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  arch/arm64/kernel/setup.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..ede3d59dabf0 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -351,8 +351,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>  	/* Parse the ACPI tables for possible boot-time configuration */
>  	acpi_boot_table_init();
>  
> -	if (acpi_disabled)
> -		unflatten_device_tree();
> +	unflatten_device_tree();
>  
>  	bootmem_init();
>  
> -- 
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
> 

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Mark Rutland]

Hi Stephen,

On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> Call this function unconditionally so that we can populate an empty DTB
> on platforms that don't boot with a firmware provided or builtin DTB.
> There's no harm in calling unflatten_device_tree() unconditionally.

For better or worse, that's not true: there are systems the provide both a DTB
*and* ACPI tables, and we must not consume both at the same time as those can
clash and cause all sorts of problems. In addition, we don't want people being
"clever" and describing disparate portions of their system in ACPI and DT.

It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
and I don't think we want to reopen this can of worms.

Given that, I'm afraid I must NAK this patch.

Thanks
Mark.

> If there isn't a valid initial_boot_params dtb then unflatten_device_tree()
> returns early.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  arch/arm64/kernel/setup.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..ede3d59dabf0 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -351,8 +351,7 @@ void __init __no_sanitize_address setup_arch(char **cmdline_p)
>  	/* Parse the ACPI tables for possible boot-time configuration */
>  	acpi_boot_table_init();
>  
> -	if (acpi_disabled)
> -		unflatten_device_tree();
> +	unflatten_device_tree();
>  
>  	bootmem_init();
>  
> -- 
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
> 
> 

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Geert Uytterhoeven]

Hi Mark,

On Tue, Jan 16, 2024 at 12:51 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > Call this function unconditionally so that we can populate an empty DTB
> > on platforms that don't boot with a firmware provided or builtin DTB.
> > There's no harm in calling unflatten_device_tree() unconditionally.
>
> For better or worse, that's not true: there are systems the provide both a DTB
> *and* ACPI tables, and we must not consume both at the same time as those can
> clash and cause all sorts of problems. In addition, we don't want people being
> "clever" and describing disparate portions of their system in ACPI and DT.

We'd get to the latter anyway, when plugging in a USB device where the
circuitry on/behind the USB device is described in DT.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Mark Rutland]

On Tue, Jan 16, 2024 at 03:13:42PM +0100, Geert Uytterhoeven wrote:
> Hi Mark,
> 
> On Tue, Jan 16, 2024 at 12:51 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > Call this function unconditionally so that we can populate an empty DTB
> > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > There's no harm in calling unflatten_device_tree() unconditionally.
> >
> > For better or worse, that's not true: there are systems the provide both a DTB
> > *and* ACPI tables, and we must not consume both at the same time as those can
> > clash and cause all sorts of problems. In addition, we don't want people being
> > "clever" and describing disparate portions of their system in ACPI and DT.
> 
> We'd get to the latter anyway, when plugging in a USB device where the
> circuitry on/behind the USB device is described in DT.

I don't understand what you mean there; where is the DT description of the USB
device coming from if the DTB hasn't been unflattened?

Mark.

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Geert Uytterhoeven]

Hi Mark,

On Thu, Jan 18, 2024 at 4:23 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 16, 2024 at 03:13:42PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Jan 16, 2024 at 12:51 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > > Call this function unconditionally so that we can populate an empty DTB
> > > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > > There's no harm in calling unflatten_device_tree() unconditionally.
> > >
> > > For better or worse, that's not true: there are systems the provide both a DTB
> > > *and* ACPI tables, and we must not consume both at the same time as those can
> > > clash and cause all sorts of problems. In addition, we don't want people being
> > > "clever" and describing disparate portions of their system in ACPI and DT.
> >
> > We'd get to the latter anyway, when plugging in a USB device where the
> > circuitry on/behind the USB device is described in DT.
>
> I don't understand what you mean there; where is the DT description of the USB
> device coming from if the DTB hasn't been unflattened?

Either stored in (FLASH) ROM on the USB device, or loaded from
/lib/firmware/.  In both cases that would be handled by the USB driver
for the device.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Stephen Boyd]

Quoting Mark Rutland (2024-01-16 03:51:14)
> Hi Stephen,
> 
> On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > Call this function unconditionally so that we can populate an empty DTB
> > on platforms that don't boot with a firmware provided or builtin DTB.
> > There's no harm in calling unflatten_device_tree() unconditionally.
> 
> For better or worse, that's not true: there are systems the provide both a DTB
> *and* ACPI tables, and we must not consume both at the same time as those can
> clash and cause all sorts of problems. In addition, we don't want people being
> "clever" and describing disparate portions of their system in ACPI and DT.
> 
> It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> and I don't think we want to reopen this can of worms.

Hmm ok. I missed this part. Can we knock out the initial_boot_params in
this case so that we don't unflatten a DTB when ACPI is in use?

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Rob Herring]

On Tue, Jan 16, 2024 at 05:27:18PM -0800, Stephen Boyd wrote:
> Quoting Mark Rutland (2024-01-16 03:51:14)
> > Hi Stephen,
> > 
> > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > Call this function unconditionally so that we can populate an empty DTB
> > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > There's no harm in calling unflatten_device_tree() unconditionally.
> > 
> > For better or worse, that's not true: there are systems the provide both a DTB
> > *and* ACPI tables, and we must not consume both at the same time as those can
> > clash and cause all sorts of problems. In addition, we don't want people being
> > "clever" and describing disparate portions of their system in ACPI and DT.
> > 
> > It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> > and I don't think we want to reopen this can of worms.
> 
> Hmm ok. I missed this part. Can we knock out the initial_boot_params in
> this case so that we don't unflatten a DTB when ACPI is in use?

You mean so we don't unflatten the boot DTB, but instead unflatten the 
empty one, right? That sounds fine.

Another thing to check is kexec because it will still need the original 
DTB I think. Though if you are doing ACPI boot and kexec'ing, kexec may 
write out everything needed by the next kernel and the empty DTB would 
work just fine. Of course those users booting with ACPI and then 
kexec'ing to DT boot will be broken. Perhaps that's a feature...

Rob

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Stephen Boyd]

Quoting Rob Herring (2024-01-17 09:54:48)
> On Tue, Jan 16, 2024 at 05:27:18PM -0800, Stephen Boyd wrote:
> > Quoting Mark Rutland (2024-01-16 03:51:14)
> > > Hi Stephen,
> > > 
> > > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > > Call this function unconditionally so that we can populate an empty DTB
> > > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > > There's no harm in calling unflatten_device_tree() unconditionally.
> > > 
> > > For better or worse, that's not true: there are systems the provide both a DTB
> > > *and* ACPI tables, and we must not consume both at the same time as those can
> > > clash and cause all sorts of problems. In addition, we don't want people being
> > > "clever" and describing disparate portions of their system in ACPI and DT.
> > > 
> > > It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> > > and I don't think we want to reopen this can of worms.
> > 
> > Hmm ok. I missed this part. Can we knock out the initial_boot_params in
> > this case so that we don't unflatten a DTB when ACPI is in use?
> 
> You mean so we don't unflatten the boot DTB, but instead unflatten the 
> empty one, right? That sounds fine.

Yes. Note, I don't have any ACPI arm64 system on hand to test anything
with :-(

> 
> Another thing to check is kexec because it will still need the original 
> DTB I think. Though if you are doing ACPI boot and kexec'ing, kexec may 
> write out everything needed by the next kernel and the empty DTB would 
> work just fine.

Yeah, it looks like dt_is_stub() will keep doing its thing there. The
empty DTB will have nothing in it and so kexec with ACPI and the empty
DTB will continue to use ACPI, and then the empty DTB will be added in
again.

> Of course those users booting with ACPI and then 
> kexec'ing to DT boot will be broken. Perhaps that's a feature...

I don't know how this part works. If you kexec to DT boot won't you run
through startup again and initial_boot_params will have a non-empty DTB
in it? I'd think this would keep working.

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Mark Rutland]

On Tue, Jan 16, 2024 at 05:27:18PM -0800, Stephen Boyd wrote:
> Quoting Mark Rutland (2024-01-16 03:51:14)
> > Hi Stephen,
> > 
> > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > Call this function unconditionally so that we can populate an empty DTB
> > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > There's no harm in calling unflatten_device_tree() unconditionally.
> > 
> > For better or worse, that's not true: there are systems the provide both a DTB
> > *and* ACPI tables, and we must not consume both at the same time as those can
> > clash and cause all sorts of problems. In addition, we don't want people being
> > "clever" and describing disparate portions of their system in ACPI and DT.
> > 
> > It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> > and I don't think we want to reopen this can of worms.
> 
> Hmm ok. I missed this part. Can we knock out the initial_boot_params in
> this case so that we don't unflatten a DTB when ACPI is in use?

Why is that better than just not calling unflatten_device_tree(), as we do
today?

The cover letter says this is all so that we can run DT tests for the clk
framework; why can't that just depend on the system being booted with DT rather
than ACPI? We have other tests which are architecture and/or configuration
dependent...

Mark.

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Geert Uytterhoeven]

Hi Mark,

On Thu, Jan 18, 2024 at 4:27 PM Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 16, 2024 at 05:27:18PM -0800, Stephen Boyd wrote:
> > Quoting Mark Rutland (2024-01-16 03:51:14)
> > > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > > Call this function unconditionally so that we can populate an empty DTB
> > > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > > There's no harm in calling unflatten_device_tree() unconditionally.
> > >
> > > For better or worse, that's not true: there are systems the provide both a DTB
> > > *and* ACPI tables, and we must not consume both at the same time as those can
> > > clash and cause all sorts of problems. In addition, we don't want people being
> > > "clever" and describing disparate portions of their system in ACPI and DT.
> > >
> > > It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> > > and I don't think we want to reopen this can of worms.
> >
> > Hmm ok. I missed this part. Can we knock out the initial_boot_params in
> > this case so that we don't unflatten a DTB when ACPI is in use?
>
> Why is that better than just not calling unflatten_device_tree(), as we do
> today?
>
> The cover letter says this is all so that we can run DT tests for the clk
> framework; why can't that just depend on the system being booted with DT rather
> than ACPI? We have other tests which are architecture and/or configuration
> dependent...

There is definitely a merit in running (platform-independent) DT tests
on any platform, whether the platform actually uses DT to boot or not.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] arm64: Unconditionally call unflatten_device_tree()

[Rob Herring]

On Thu, Jan 18, 2024 at 03:26:43PM +0000, Mark Rutland wrote:
> On Tue, Jan 16, 2024 at 05:27:18PM -0800, Stephen Boyd wrote:
> > Quoting Mark Rutland (2024-01-16 03:51:14)
> > > Hi Stephen,
> > > 
> > > On Fri, Jan 12, 2024 at 12:07:44PM -0800, Stephen Boyd wrote:
> > > > Call this function unconditionally so that we can populate an empty DTB
> > > > on platforms that don't boot with a firmware provided or builtin DTB.
> > > > There's no harm in calling unflatten_device_tree() unconditionally.
> > > 
> > > For better or worse, that's not true: there are systems the provide both a DTB
> > > *and* ACPI tables, and we must not consume both at the same time as those can
> > > clash and cause all sorts of problems. In addition, we don't want people being
> > > "clever" and describing disparate portions of their system in ACPI and DT.
> > > 
> > > It is a very deliberate choice to not unflatten the DTB when ACPI is in use,
> > > and I don't think we want to reopen this can of worms.
> > 
> > Hmm ok. I missed this part. Can we knock out the initial_boot_params in
> > this case so that we don't unflatten a DTB when ACPI is in use?
> 
> Why is that better than just not calling unflatten_device_tree(), as we do
> today?
> 
> The cover letter says this is all so that we can run DT tests for the clk
> framework; why can't that just depend on the system being booted with DT rather
> than ACPI? 

Because then the tests can never run on x86 and some people still use 
those systems. It's no different than why do we compile !x86 drivers on 
x86. It is convenient.

> We have other tests which are architecture and/or configuration
> dependent...

There's another usecase of non-discoverable devices behind discoverable 
devices. See my LPC session slides for more details. For this we will 
need some base DT to apply overlays to on DT AND ACPI systems. This is 
what Geert was getting at. Yes, it could be done with some other code 
path, but the DT unittest has done that hack for years and this series 
is getting rid of it.

Rob

[PATCH 2/6] um: Unconditionally call unflatten_device_tree()

[Stephen Boyd]

Call this function unconditionally so that we can populate an empty DTB
on platforms that don't boot with a command line provided DTB.  There's
no harm in calling unflatten_device_tree() unconditionally. If there
isn't a valid initial_boot_params dtb then unflatten_device_tree()
returns early.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Anton Ivanov <anton.ivanov@cambridgegreys.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: <linux-um@lists.infradead.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 arch/um/kernel/dtb.c  | 14 +++++++-------
 drivers/of/unittest.c |  4 ----
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/um/kernel/dtb.c b/arch/um/kernel/dtb.c
index 484141b06938..4954188a6a09 100644
--- a/arch/um/kernel/dtb.c
+++ b/arch/um/kernel/dtb.c
@@ -16,16 +16,16 @@ void uml_dtb_init(void)
 	void *area;
 
 	area = uml_load_file(dtb, &size);
-	if (!area)
-		return;
+	if (area) {
+		if (!early_init_dt_scan(area)) {
+			pr_err("invalid DTB %s\n", dtb);
+			memblock_free(area, size);
+			return;
+		}
 
-	if (!early_init_dt_scan(area)) {
-		pr_err("invalid DTB %s\n", dtb);
-		memblock_free(area, size);
-		return;
+		early_init_fdt_scan_reserved_mem();
 	}
 
-	early_init_fdt_scan_reserved_mem();
 	unflatten_device_tree();
 }
 
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e9e90e96600e..a8b27dd16ecf 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -4075,10 +4075,6 @@ static int __init of_unittest(void)
 	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
 
 	/* adding data for unittest */
-
-	if (IS_ENABLED(CONFIG_UML))
-		unittest_unflatten_overlay_base();
-
 	res = unittest_data_add();
 	if (res)
 		return res;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

[PATCH 3/6] of: Always unflatten in unflatten_and_copy_device_tree()

[Stephen Boyd]

We want to populate an empty DT whenever CONFIG_OF is enabled so that
overlays can be applied and the DT unit tests can be run. Make
unflatten_and_copy_device_tree() stop printing a warning if the
'initial_boot_params' pointer is NULL. Instead, simply copy the dtb if
there is one and then unflatten it. If there isn't a DT to copy, then
the call to unflatten_device_tree() is largely a no-op, so nothing
really changes here.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/fdt.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index bf502ba8da95..b488ad86d456 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1318,6 +1318,21 @@ bool __init early_init_dt_scan(void *params)
 	return true;
 }
 
+static void __init copy_device_tree(void)
+{
+	int size;
+	void *dt;
+
+	size = fdt_totalsize(initial_boot_params);
+	dt = early_init_dt_alloc_memory_arch(size,
+					     roundup_pow_of_two(FDT_V17_SIZE));
+
+	if (dt) {
+		memcpy(dt, initial_boot_params, size);
+		initial_boot_params = dt;
+	}
+}
+
 /**
  * unflatten_device_tree - create tree of device_nodes from flat blob
  *
@@ -1350,22 +1365,9 @@ void __init unflatten_device_tree(void)
  */
 void __init unflatten_and_copy_device_tree(void)
 {
-	int size;
-	void *dt;
+	if (initial_boot_params)
+		copy_device_tree();
 
-	if (!initial_boot_params) {
-		pr_warn("No valid device tree found, continuing without\n");
-		return;
-	}
-
-	size = fdt_totalsize(initial_boot_params);
-	dt = early_init_dt_alloc_memory_arch(size,
-					     roundup_pow_of_two(FDT_V17_SIZE));
-
-	if (dt) {
-		memcpy(dt, initial_boot_params, size);
-		initial_boot_params = dt;
-	}
 	unflatten_device_tree();
 }
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

[PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Stephen Boyd]

From: Frank Rowand <frowand.list@gmail.com>

When enabling CONFIG_OF on a platform where 'of_root' is not populated
by firmware, we end up without a root node. In order to apply overlays
and create subnodes of the root node, we need one. Create this root node
by unflattening an empty builtin dtb.

If firmware provides a flattened device tree (FDT) then the FDT is
unflattened via setup_arch(). Otherwise, the call to
unflatten(_and_copy)?_device_tree() will create an empty root node.

We make of_have_populated_dt() return true only if the DTB was loaded by
firmware so that existing callers don't change behavior after this
patch. The call in the of platform code is removed because it prevents
overlays from creating platform devices when the platform bus isn't
fully initialized.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
Link: https://lore.kernel.org/r/20230317053415.2254616-2-frowand.list@gmail.com
Cc: Rob Herring <robh+dt@kernel.org>
[sboyd@kernel.org: Update of_have_populated_dt() to treat this empty dtb
as not populated. Drop setup_of() initcall]
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/Kconfig        |  7 ++++++-
 drivers/of/Makefile       |  2 +-
 drivers/of/empty_root.dts |  6 ++++++
 drivers/of/fdt.c          | 25 +++++++++++++++++++++++++
 drivers/of/platform.c     |  3 ---
 include/linux/of.h        | 17 ++++++++++++-----
 6 files changed, 50 insertions(+), 10 deletions(-)
 create mode 100644 drivers/of/empty_root.dts

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index da9826accb1b..9628e48baa15 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -54,9 +54,14 @@ config OF_FLATTREE
 	select CRC32
 
 config OF_EARLY_FLATTREE
-	bool
+	bool "Functions for accessing Flat Devicetree (FDT) early in boot"
 	select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
 	select OF_FLATTREE
+	help
+	  Normally selected by platforms that process an FDT that has been
+	  passed to the kernel by the bootloader.  If the bootloader does not
+	  pass an FDT to the kernel and you need an empty devicetree that
+	  contains only a root node to exist, then say Y here.
 
 config OF_PROMTREE
 	bool
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index eff624854575..df305348d1cb 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,7 +2,7 @@
 obj-y = base.o cpu.o device.o module.o platform.o property.o
 obj-$(CONFIG_OF_KOBJ) += kobj.o
 obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
-obj-$(CONFIG_OF_FLATTREE) += fdt.o
+obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
new file mode 100644
index 000000000000..cf9e97a60f48
--- /dev/null
+++ b/drivers/of/empty_root.dts
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/dts-v1/;
+
+/ {
+
+};
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index b488ad86d456..9fc7f8b4f48a 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -32,6 +32,13 @@
 
 #include "of_private.h"
 
+/*
+ * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
+ * cmd_dt_S_dtb in scripts/Makefile.lib
+ */
+extern uint8_t __dtb_empty_root_begin[];
+extern uint8_t __dtb_empty_root_end[];
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -1343,8 +1350,26 @@ static void __init copy_device_tree(void)
  */
 void __init unflatten_device_tree(void)
 {
+	bool firmware_loaded = true;
+
+	if (!initial_boot_params) {
+		initial_boot_params = (void *) __dtb_empty_root_begin;
+		/* fdt_totalsize() will be used for copy size */
+		if (fdt_totalsize(initial_boot_params) >
+		    __dtb_empty_root_end - __dtb_empty_root_begin) {
+			pr_err("invalid size in dtb_empty_root\n");
+			return;
+		}
+		of_fdt_crc32 = crc32_be(~0, initial_boot_params,
+					fdt_totalsize(initial_boot_params));
+		copy_device_tree();
+		firmware_loaded = false;
+	}
+
 	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
 				early_init_dt_alloc_memory_arch, false);
+	if (!firmware_loaded)
+		of_node_set_flag(of_root, OF_EMPTY_ROOT);
 
 	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
 	of_alias_scan(early_init_dt_alloc_memory_arch);
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 126d265aa7d8..20087bb8a46b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -549,9 +549,6 @@ static int __init of_platform_default_populate_init(void)
 
 	device_links_supplier_sync_state_pause();
 
-	if (!of_have_populated_dt())
-		return -ENODEV;
-
 	if (IS_ENABLED(CONFIG_PPC)) {
 		struct device_node *boot_display = NULL;
 		struct platform_device *dev;
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..390ad961ef01 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -151,6 +151,7 @@ extern struct device_node *of_stdout;
 #define OF_POPULATED_BUS	4 /* platform bus created for children */
 #define OF_OVERLAY		5 /* allocated for an overlay */
 #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
+#define OF_EMPTY_ROOT		7 /* the builtin empty root node */
 
 #define OF_BAD_ADDR	((u64)-1)
 
@@ -180,11 +181,6 @@ static inline bool is_of_node(const struct fwnode_handle *fwnode)
 			&__of_fwnode_handle_node->fwnode : NULL;	\
 	})
 
-static inline bool of_have_populated_dt(void)
-{
-	return of_root != NULL;
-}
-
 static inline bool of_node_is_root(const struct device_node *node)
 {
 	return node && (node->parent == NULL);
@@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
 	return test_bit(flag, &n->_flags);
 }
 
+/**
+ * of_have_populated_dt() - Has DT been populated by bootloader
+ *
+ * Return: True if a DTB has been populated by the bootloader and it isn't the
+ * empty builtin one. False otherwise.
+ */
+static inline bool of_have_populated_dt(void)
+{
+	return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
+}
+
 static inline int of_node_test_and_set_flag(struct device_node *n,
 					    unsigned long flag)
 {
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Rob Herring]

On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> From: Frank Rowand <frowand.list@gmail.com>
> 
> When enabling CONFIG_OF on a platform where 'of_root' is not populated
> by firmware, we end up without a root node. In order to apply overlays
> and create subnodes of the root node, we need one. Create this root node
> by unflattening an empty builtin dtb.
> 
> If firmware provides a flattened device tree (FDT) then the FDT is
> unflattened via setup_arch(). Otherwise, the call to
> unflatten(_and_copy)?_device_tree() will create an empty root node.
> 
> We make of_have_populated_dt() return true only if the DTB was loaded by
> firmware so that existing callers don't change behavior after this
> patch. The call in the of platform code is removed because it prevents
> overlays from creating platform devices when the platform bus isn't
> fully initialized.
> 
> Signed-off-by: Frank Rowand <frowand.list@gmail.com>
> Link: https://lore.kernel.org/r/20230317053415.2254616-2-frowand.list@gmail.com
> Cc: Rob Herring <robh+dt@kernel.org>
> [sboyd@kernel.org: Update of_have_populated_dt() to treat this empty dtb
> as not populated. Drop setup_of() initcall]
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/of/Kconfig        |  7 ++++++-
>  drivers/of/Makefile       |  2 +-
>  drivers/of/empty_root.dts |  6 ++++++
>  drivers/of/fdt.c          | 25 +++++++++++++++++++++++++
>  drivers/of/platform.c     |  3 ---
>  include/linux/of.h        | 17 ++++++++++++-----
>  6 files changed, 50 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/of/empty_root.dts
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index da9826accb1b..9628e48baa15 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -54,9 +54,14 @@ config OF_FLATTREE
>  	select CRC32
>  
>  config OF_EARLY_FLATTREE
> -	bool
> +	bool "Functions for accessing Flat Devicetree (FDT) early in boot"

I think we could instead just get rid of this kconfig option. Or 
always enable with CONFIG_OF (except on Sparc). The only cost of 
enabling it is init section functions which get freed anyways.

>  	select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
>  	select OF_FLATTREE
> +	help
> +	  Normally selected by platforms that process an FDT that has been
> +	  passed to the kernel by the bootloader.  If the bootloader does not
> +	  pass an FDT to the kernel and you need an empty devicetree that
> +	  contains only a root node to exist, then say Y here.
>  
>  config OF_PROMTREE
>  	bool
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index eff624854575..df305348d1cb 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -2,7 +2,7 @@
>  obj-y = base.o cpu.o device.o module.o platform.o property.o
>  obj-$(CONFIG_OF_KOBJ) += kobj.o
>  obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
> -obj-$(CONFIG_OF_FLATTREE) += fdt.o
> +obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
>  obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
>  obj-$(CONFIG_OF_PROMTREE) += pdt.o
>  obj-$(CONFIG_OF_ADDRESS)  += address.o
> diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
> new file mode 100644
> index 000000000000..cf9e97a60f48
> --- /dev/null
> +++ b/drivers/of/empty_root.dts
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/dts-v1/;
> +
> +/ {
> +
> +};
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index b488ad86d456..9fc7f8b4f48a 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -32,6 +32,13 @@
>  
>  #include "of_private.h"
>  
> +/*
> + * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
> + * cmd_dt_S_dtb in scripts/Makefile.lib
> + */
> +extern uint8_t __dtb_empty_root_begin[];
> +extern uint8_t __dtb_empty_root_end[];
> +
>  /*
>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>   * @limit: maximum entries
> @@ -1343,8 +1350,26 @@ static void __init copy_device_tree(void)
>   */
>  void __init unflatten_device_tree(void)
>  {
> +	bool firmware_loaded = true;
> +
> +	if (!initial_boot_params) {
> +		initial_boot_params = (void *) __dtb_empty_root_begin;
> +		/* fdt_totalsize() will be used for copy size */
> +		if (fdt_totalsize(initial_boot_params) >
> +		    __dtb_empty_root_end - __dtb_empty_root_begin) {
> +			pr_err("invalid size in dtb_empty_root\n");
> +			return;
> +		}
> +		of_fdt_crc32 = crc32_be(~0, initial_boot_params,
> +					fdt_totalsize(initial_boot_params));
> +		copy_device_tree();
> +		firmware_loaded = false;
> +	}
> +
>  	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
>  				early_init_dt_alloc_memory_arch, false);
> +	if (!firmware_loaded)
> +		of_node_set_flag(of_root, OF_EMPTY_ROOT);
>  
>  	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
>  	of_alias_scan(early_init_dt_alloc_memory_arch);
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 126d265aa7d8..20087bb8a46b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -549,9 +549,6 @@ static int __init of_platform_default_populate_init(void)
>  
>  	device_links_supplier_sync_state_pause();
>  
> -	if (!of_have_populated_dt())
> -		return -ENODEV;
> -
>  	if (IS_ENABLED(CONFIG_PPC)) {
>  		struct device_node *boot_display = NULL;
>  		struct platform_device *dev;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 6a9ddf20e79a..390ad961ef01 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -151,6 +151,7 @@ extern struct device_node *of_stdout;
>  #define OF_POPULATED_BUS	4 /* platform bus created for children */
>  #define OF_OVERLAY		5 /* allocated for an overlay */
>  #define OF_OVERLAY_FREE_CSET	6 /* in overlay cset being freed */
> +#define OF_EMPTY_ROOT		7 /* the builtin empty root node */
>  
>  #define OF_BAD_ADDR	((u64)-1)
>  
> @@ -180,11 +181,6 @@ static inline bool is_of_node(const struct fwnode_handle *fwnode)
>  			&__of_fwnode_handle_node->fwnode : NULL;	\
>  	})
>  
> -static inline bool of_have_populated_dt(void)
> -{
> -	return of_root != NULL;
> -}
> -
>  static inline bool of_node_is_root(const struct device_node *node)
>  {
>  	return node && (node->parent == NULL);
> @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
>  	return test_bit(flag, &n->_flags);
>  }
>  
> +/**
> + * of_have_populated_dt() - Has DT been populated by bootloader
> + *
> + * Return: True if a DTB has been populated by the bootloader and it isn't the
> + * empty builtin one. False otherwise.
> + */
> +static inline bool of_have_populated_dt(void)
> +{
> +	return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);

Just a side comment, but I think many/all callers of this function could 
just be removed.

I don't love new flags. Another possible way to handle this would be 
checking for "compatible" being present in the root node. I guess this 
is fine as-is for now at least.

Rob

Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Stephen Boyd]

Quoting Rob Herring (2024-01-15 12:32:30)
> On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index da9826accb1b..9628e48baa15 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -54,9 +54,14 @@ config OF_FLATTREE
> >       select CRC32
> >  
> >  config OF_EARLY_FLATTREE
> > -     bool
> > +     bool "Functions for accessing Flat Devicetree (FDT) early in boot"
> 
> I think we could instead just get rid of this kconfig option. Or 
> always enable with CONFIG_OF (except on Sparc). The only cost of 
> enabling it is init section functions which get freed anyways.

Getting rid of it is a more massive change. It can be the default and
kept hidden instead? If it can't be selected on Sparc then it should be
hidden there anyway.

> 
> >       select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> >       select OF_FLATTREE
> > +     help
> > +       Normally selected by platforms that process an FDT that has been
> > +       passed to the kernel by the bootloader.  If the bootloader does not
> > +       pass an FDT to the kernel and you need an empty devicetree that
> > +       contains only a root node to exist, then say Y here.
> >  
> >  config OF_PROMTREE
> >       bool
[...]
> > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
> >       return test_bit(flag, &n->_flags);
> >  }
> >  
> > +/**
> > + * of_have_populated_dt() - Has DT been populated by bootloader
> > + *
> > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > + * empty builtin one. False otherwise.
> > + */
> > +static inline bool of_have_populated_dt(void)
> > +{
> > +     return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
> 
> Just a side comment, but I think many/all callers of this function could 
> just be removed.
> 
> I don't love new flags. Another possible way to handle this would be 
> checking for "compatible" being present in the root node. I guess this 
> is fine as-is for now at least.

Ok. I can add a check for a compatible property. That's probably better
anyway. Should there be a compatible property there to signal that this
DT isn't compatible with anything? I worry about DT overlays injecting a
compatible string into the root node, but maybe that is already
prevented.

Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Rob Herring]

On Tue, Jan 16, 2024 at 05:18:15PM -0800, Stephen Boyd wrote:
> Quoting Rob Herring (2024-01-15 12:32:30)
> > On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > index da9826accb1b..9628e48baa15 100644
> > > --- a/drivers/of/Kconfig
> > > +++ b/drivers/of/Kconfig
> > > @@ -54,9 +54,14 @@ config OF_FLATTREE
> > >       select CRC32
> > >  
> > >  config OF_EARLY_FLATTREE
> > > -     bool
> > > +     bool "Functions for accessing Flat Devicetree (FDT) early in boot"
> > 
> > I think we could instead just get rid of this kconfig option. Or 
> > always enable with CONFIG_OF (except on Sparc). The only cost of 
> > enabling it is init section functions which get freed anyways.
> 
> Getting rid of it is a more massive change. It can be the default and
> kept hidden instead? If it can't be selected on Sparc then it should be
> hidden there anyway.

The easier option is certainly fine for this series. I just don't want 
it visible.

> > >       select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> > >       select OF_FLATTREE
> > > +     help
> > > +       Normally selected by platforms that process an FDT that has been
> > > +       passed to the kernel by the bootloader.  If the bootloader does not
> > > +       pass an FDT to the kernel and you need an empty devicetree that
> > > +       contains only a root node to exist, then say Y here.
> > >  
> > >  config OF_PROMTREE
> > >       bool
> [...]
> > > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
> > >       return test_bit(flag, &n->_flags);
> > >  }
> > >  
> > > +/**
> > > + * of_have_populated_dt() - Has DT been populated by bootloader
> > > + *
> > > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > > + * empty builtin one. False otherwise.
> > > + */
> > > +static inline bool of_have_populated_dt(void)
> > > +{
> > > +     return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
> > 
> > Just a side comment, but I think many/all callers of this function could 
> > just be removed.
> > 
> > I don't love new flags. Another possible way to handle this would be 
> > checking for "compatible" being present in the root node. I guess this 
> > is fine as-is for now at least.
> 
> Ok. I can add a check for a compatible property. That's probably better
> anyway. Should there be a compatible property there to signal that this
> DT isn't compatible with anything? I worry about DT overlays injecting a
> compatible string into the root node, but maybe that is already
> prevented.

I worry about DT overlays injecting anything...

I don't think it is explicitly forbidden, but I have asked that any 
general purpose interface to apply overlays be restricted to nodes 
explicitly allowed (e.g. downstream of a connector node). For now, the 
places (i.e. drivers) overlays are applied are limited.

We could probably restrict the root node to new nodes only and no new 
or changed properties.

Rob

Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jan 17, 2024 at 6:41 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Jan 16, 2024 at 05:18:15PM -0800, Stephen Boyd wrote:
> > Quoting Rob Herring (2024-01-15 12:32:30)
> > > On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> > > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > > index da9826accb1b..9628e48baa15 100644
> > > > --- a/drivers/of/Kconfig
> > > > +++ b/drivers/of/Kconfig
> > > > @@ -54,9 +54,14 @@ config OF_FLATTREE
> > > >       select CRC32
> > > >
> > > >  config OF_EARLY_FLATTREE
> > > > -     bool
> > > > +     bool "Functions for accessing Flat Devicetree (FDT) early in boot"
> > >
> > > I think we could instead just get rid of this kconfig option. Or
> > > always enable with CONFIG_OF (except on Sparc). The only cost of
> > > enabling it is init section functions which get freed anyways.
> >
> > Getting rid of it is a more massive change. It can be the default and
> > kept hidden instead? If it can't be selected on Sparc then it should be
> > hidden there anyway.
>
> The easier option is certainly fine for this series. I just don't want
> it visible.
>
> > > >       select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> > > >       select OF_FLATTREE
> > > > +     help
> > > > +       Normally selected by platforms that process an FDT that has been
> > > > +       passed to the kernel by the bootloader.  If the bootloader does not
> > > > +       pass an FDT to the kernel and you need an empty devicetree that
> > > > +       contains only a root node to exist, then say Y here.
> > > >
> > > >  config OF_PROMTREE
> > > >       bool
> > [...]
> > > > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
> > > >       return test_bit(flag, &n->_flags);
> > > >  }
> > > >
> > > > +/**
> > > > + * of_have_populated_dt() - Has DT been populated by bootloader
> > > > + *
> > > > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > > > + * empty builtin one. False otherwise.
> > > > + */
> > > > +static inline bool of_have_populated_dt(void)
> > > > +{
> > > > +     return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
> > >
> > > Just a side comment, but I think many/all callers of this function could
> > > just be removed.
> > >
> > > I don't love new flags. Another possible way to handle this would be
> > > checking for "compatible" being present in the root node. I guess this
> > > is fine as-is for now at least.
> >
> > Ok. I can add a check for a compatible property. That's probably better
> > anyway. Should there be a compatible property there to signal that this
> > DT isn't compatible with anything? I worry about DT overlays injecting a
> > compatible string into the root node, but maybe that is already
> > prevented.
>
> I worry about DT overlays injecting anything...
>
> I don't think it is explicitly forbidden, but I have asked that any
> general purpose interface to apply overlays be restricted to nodes
> explicitly allowed (e.g. downstream of a connector node). For now, the
> places (i.e. drivers) overlays are applied are limited.
>
> We could probably restrict the root node to new nodes only and no new
> or changed properties.

Changing (<wild dream>or appending to</wild dream>) the root
"compatible" and/or "model" properties is useful in case of large
extension boards, though.  This is also the case for DTBs created from
a base DTB and one or more overlays using fdtoverlay.

For the latter, see also the following threads, where you weren't
(but probably should have been) CCed:

[1] "[PATCH v9 2/2] arm64: boot: Support Flat Image Tree"
     https://lore.kernel.org/all/20231202035511.487946-3-sjg@chromium.org
[2] "Proposal: FIT support for extension boards / overlays"
    https://lore.kernel.org/all/CAPnjgZ06s64C2ux1rABNAnMv3q4W++sjhNGCO_uPMH_9sTF7Mw@mail.gmail.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/6] of: Create of_root if no dtb provided by firmware

[Rob Herring]

On Thu, Jan 18, 2024 at 2:46 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, Jan 17, 2024 at 6:41 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jan 16, 2024 at 05:18:15PM -0800, Stephen Boyd wrote:
> > > Quoting Rob Herring (2024-01-15 12:32:30)
> > > > On Fri, Jan 12, 2024 at 12:07:47PM -0800, Stephen Boyd wrote:
> > > > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > > > index da9826accb1b..9628e48baa15 100644
> > > > > --- a/drivers/of/Kconfig
> > > > > +++ b/drivers/of/Kconfig
> > > > > @@ -54,9 +54,14 @@ config OF_FLATTREE
> > > > >       select CRC32
> > > > >
> > > > >  config OF_EARLY_FLATTREE
> > > > > -     bool
> > > > > +     bool "Functions for accessing Flat Devicetree (FDT) early in boot"
> > > >
> > > > I think we could instead just get rid of this kconfig option. Or
> > > > always enable with CONFIG_OF (except on Sparc). The only cost of
> > > > enabling it is init section functions which get freed anyways.
> > >
> > > Getting rid of it is a more massive change. It can be the default and
> > > kept hidden instead? If it can't be selected on Sparc then it should be
> > > hidden there anyway.
> >
> > The easier option is certainly fine for this series. I just don't want
> > it visible.
> >
> > > > >       select DMA_DECLARE_COHERENT if HAS_DMA && HAS_IOMEM
> > > > >       select OF_FLATTREE
> > > > > +     help
> > > > > +       Normally selected by platforms that process an FDT that has been
> > > > > +       passed to the kernel by the bootloader.  If the bootloader does not
> > > > > +       pass an FDT to the kernel and you need an empty devicetree that
> > > > > +       contains only a root node to exist, then say Y here.
> > > > >
> > > > >  config OF_PROMTREE
> > > > >       bool
> > > [...]
> > > > > @@ -195,6 +191,17 @@ static inline int of_node_check_flag(const struct device_node *n, unsigned long
> > > > >       return test_bit(flag, &n->_flags);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * of_have_populated_dt() - Has DT been populated by bootloader
> > > > > + *
> > > > > + * Return: True if a DTB has been populated by the bootloader and it isn't the
> > > > > + * empty builtin one. False otherwise.
> > > > > + */
> > > > > +static inline bool of_have_populated_dt(void)
> > > > > +{
> > > > > +     return of_root != NULL && !of_node_check_flag(of_root, OF_EMPTY_ROOT);
> > > >
> > > > Just a side comment, but I think many/all callers of this function could
> > > > just be removed.
> > > >
> > > > I don't love new flags. Another possible way to handle this would be
> > > > checking for "compatible" being present in the root node. I guess this
> > > > is fine as-is for now at least.
> > >
> > > Ok. I can add a check for a compatible property. That's probably better
> > > anyway. Should there be a compatible property there to signal that this
> > > DT isn't compatible with anything? I worry about DT overlays injecting a
> > > compatible string into the root node, but maybe that is already
> > > prevented.
> >
> > I worry about DT overlays injecting anything...
> >
> > I don't think it is explicitly forbidden, but I have asked that any
> > general purpose interface to apply overlays be restricted to nodes
> > explicitly allowed (e.g. downstream of a connector node). For now, the
> > places (i.e. drivers) overlays are applied are limited.
> >
> > We could probably restrict the root node to new nodes only and no new
> > or changed properties.
>
> Changing (<wild dream>or appending to</wild dream>) the root
> "compatible" and/or "model" properties is useful in case of large
> extension boards, though.  This is also the case for DTBs created from
> a base DTB and one or more overlays using fdtoverlay.

I think appending by adding another compatible value could be okay.
Removing or appending to an existing entry is not. We don't want the
following sequence to be possible:

of_machine_is_compatible("foo") --> true
apply overlay
of_machine_is_compatible("foo") --> false

For Stephen's case, it's going from no root compatible at all to
something. I don't think your case would apply here. To put it another
way, if we've booted with ACPI, compatible in the root node is not
valid.


> For the latter, see also the following threads, where you weren't
> (but probably should have been) CCed:
>
> [1] "[PATCH v9 2/2] arm64: boot: Support Flat Image Tree"
>      https://lore.kernel.org/all/20231202035511.487946-3-sjg@chromium.org
> [2] "Proposal: FIT support for extension boards / overlays"
>     https://lore.kernel.org/all/CAPnjgZ06s64C2ux1rABNAnMv3q4W++sjhNGCO_uPMH_9sTF7Mw@mail.gmail.com

That all seems pretty orthogonal to the issues here.

Rob

[PATCH 5/6] of: unittest: treat missing of_root as error instead of fixing up

[Stephen Boyd]

From: Frank Rowand <frowand.list@gmail.com>

unflatten_device_tree() now ensures that the 'of_root' node is populated
with the root of a default empty devicetree. Remove the unittest code
that created 'of_root' if it was missing. Verify that 'of_root' is valid
before attempting to attach the testcase-data subtree. Remove the
unittest code that unflattens the unittest overlay base if architecture
is UML because that is always done now.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
Link: https://lore.kernel.org/r/20230317053415.2254616-3-frowand.list@gmail.com
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/unittest.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index a8b27dd16ecf..742d919e8ab4 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1732,20 +1732,16 @@ static int __init unittest_data_add(void)
 		return -EINVAL;
 	}
 
+	/* attach the sub-tree to live tree */
 	if (!of_root) {
-		of_root = unittest_data_node;
-		for_each_of_allnodes(np)
-			__of_attach_node_sysfs(np);
-		of_aliases = of_find_node_by_path("/aliases");
-		of_chosen = of_find_node_by_path("/chosen");
-		of_overlay_mutex_unlock();
-		return 0;
+		pr_warn("%s: no live tree to attach sub-tree\n", __func__);
+		kfree(unittest_data);
+		return -ENODEV;
 	}
 
 	EXPECT_BEGIN(KERN_INFO,
 		     "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
 
-	/* attach the sub-tree to live tree */
 	np = unittest_data_node->child;
 	while (np) {
 		struct device_node *next = np->sibling;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

[PATCH 6/6] of: Add KUnit test to confirm DTB is loaded

[Stephen Boyd]

Add a KUnit test that confirms a DTB has been loaded, i.e. there is a
root node, and that the of_have_populated_dt() API works properly.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: David Gow <davidgow@google.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/of/.kunitconfig |  3 ++
 drivers/of/Kconfig      |  9 ++++
 drivers/of/Makefile     |  2 +
 drivers/of/of_test.c    | 98 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 112 insertions(+)
 create mode 100644 drivers/of/.kunitconfig
 create mode 100644 drivers/of/of_test.c

diff --git a/drivers/of/.kunitconfig b/drivers/of/.kunitconfig
new file mode 100644
index 000000000000..5a8fee11978c
--- /dev/null
+++ b/drivers/of/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_OF=y
+CONFIG_OF_KUNIT_TEST=y
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 9628e48baa15..a527ba8451d9 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -37,6 +37,15 @@ config OF_UNITTEST
 
 	  If unsure, say N here. This option is not safe to enable.
 
+config OF_KUNIT_TEST
+	tristate "Devicetree KUnit DTB Test" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This option builds KUnit unit tests for device tree infrastructure.
+
+	  If unsure, say N here, but this option is safe to enable.
+
 config OF_ALL_DTBS
 	bool "Build all Device Tree Blobs"
 	depends on COMPILE_TEST
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index df305348d1cb..251d33532148 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -19,4 +19,6 @@ obj-y	+= kexec.o
 endif
 endif
 
+obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
+
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
new file mode 100644
index 000000000000..7609ad3081b9
--- /dev/null
+++ b/drivers/of/of_test.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for OF APIs
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <kunit/test.h>
+
+/*
+ * Test that the root node "/" exists.
+ */
+static void dtb_root_node_found_by_path(struct kunit *test)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/");
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
+	of_node_put(np);
+}
+
+/*
+ * Test that the of_root global variable is always populated when DT
+ * code is enabled.
+ */
+static void dtb_root_node_populates_of_root(struct kunit *test)
+{
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, of_root);
+}
+
+static struct kunit_case dtb_test_cases[] = {
+	KUNIT_CASE(dtb_root_node_found_by_path),
+	KUNIT_CASE(dtb_root_node_populates_of_root),
+	{}
+};
+
+/*
+ * Test suite to confirm a live DTB is loaded.
+ */
+static struct kunit_suite dtb_suite = {
+	.name = "dtb",
+	.test_cases = dtb_test_cases,
+};
+
+/*
+ * Test that calling of_have_populated_dt() returns false when
+ * the OF_EMPTY_ROOT flag isn't set.
+ */
+static void of_have_populated_dt_false_when_flag_set(struct kunit *test)
+{
+	bool was_set;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
+
+	was_set = of_node_test_and_set_flag(of_root, OF_EMPTY_ROOT);
+	KUNIT_EXPECT_FALSE(test, of_have_populated_dt());
+
+	if (!was_set)
+		of_node_clear_flag(of_root, OF_EMPTY_ROOT);
+}
+
+/*
+ * Test that calling of_have_populated_dt() returns false when
+ * the OF_EMPTY_ROOT flag isn't set.
+ */
+static void of_have_populated_dt_true_when_flag_clear(struct kunit *test)
+{
+	bool was_set;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
+
+	was_set = of_node_check_flag(of_root, OF_EMPTY_ROOT);
+	of_node_set_flag(of_root, OF_EMPTY_ROOT);
+	KUNIT_EXPECT_FALSE(test, of_have_populated_dt());
+
+	if (was_set)
+		of_node_set_flag(of_root, OF_EMPTY_ROOT);
+}
+
+static struct kunit_case of_have_populated_dt_test_cases[] = {
+	KUNIT_CASE(of_have_populated_dt_false_when_flag_set),
+	KUNIT_CASE(of_have_populated_dt_true_when_flag_clear),
+	{}
+};
+
+/*
+ * Test suite to confirm behavior of of_have_populated_dt().
+ */
+static struct kunit_suite of_have_populated_dt_suite = {
+	.name = "of_have_populated_dt",
+	.test_cases = of_have_populated_dt_test_cases,
+};
+
+kunit_test_suites(
+	&dtb_suite,
+	&of_have_populated_dt_suite,
+);
+MODULE_LICENSE("GPL");
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Re: [PATCH 6/6] of: Add KUnit test to confirm DTB is loaded

[David Gow]

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

On Sat, 13 Jan 2024 at 04:07, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Add a KUnit test that confirms a DTB has been loaded, i.e. there is a
> root node, and that the of_have_populated_dt() API works properly.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

I won't pretend to be a devicetree expert, but this looks good to me
from a KUnit point of view, and passes comfortably here.

checkpatch seems to have one complaint about the kconfig help text.
Personally, I think the brief description is fine.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  drivers/of/.kunitconfig |  3 ++
>  drivers/of/Kconfig      |  9 ++++
>  drivers/of/Makefile     |  2 +
>  drivers/of/of_test.c    | 98 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 112 insertions(+)
>  create mode 100644 drivers/of/.kunitconfig
>  create mode 100644 drivers/of/of_test.c
>
> diff --git a/drivers/of/.kunitconfig b/drivers/of/.kunitconfig
> new file mode 100644
> index 000000000000..5a8fee11978c
> --- /dev/null
> +++ b/drivers/of/.kunitconfig
> @@ -0,0 +1,3 @@
> +CONFIG_KUNIT=y
> +CONFIG_OF=y
> +CONFIG_OF_KUNIT_TEST=y
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 9628e48baa15..a527ba8451d9 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -37,6 +37,15 @@ config OF_UNITTEST
>
>           If unsure, say N here. This option is not safe to enable.
>
> +config OF_KUNIT_TEST
> +       tristate "Devicetree KUnit DTB Test" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       default KUNIT_ALL_TESTS
> +       help
> +         This option builds KUnit unit tests for device tree infrastructure.
> +
> +         If unsure, say N here, but this option is safe to enable.
> +

FYI:
WARNING: please write a help paragraph that fully describes the config symbol
#111: FILE: drivers/of/Kconfig:40:

>  config OF_ALL_DTBS
>         bool "Build all Device Tree Blobs"
>         depends on COMPILE_TEST
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index df305348d1cb..251d33532148 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -19,4 +19,6 @@ obj-y += kexec.o
>  endif
>  endif
>
> +obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> +
>  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
> new file mode 100644
> index 000000000000..7609ad3081b9
> --- /dev/null
> +++ b/drivers/of/of_test.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for OF APIs
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include <kunit/test.h>
> +
> +/*
> + * Test that the root node "/" exists.
> + */
> +static void dtb_root_node_found_by_path(struct kunit *test)
> +{
> +       struct device_node *np;
> +
> +       np = of_find_node_by_path("/");
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
> +       of_node_put(np);
> +}
> +
> +/*
> + * Test that the of_root global variable is always populated when DT
> + * code is enabled.
> + */
> +static void dtb_root_node_populates_of_root(struct kunit *test)
> +{
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, of_root);
> +}
> +
> +static struct kunit_case dtb_test_cases[] = {
> +       KUNIT_CASE(dtb_root_node_found_by_path),
> +       KUNIT_CASE(dtb_root_node_populates_of_root),
> +       {}
> +};
> +
> +/*
> + * Test suite to confirm a live DTB is loaded.
> + */
> +static struct kunit_suite dtb_suite = {
> +       .name = "dtb",
> +       .test_cases = dtb_test_cases,
> +};
> +
> +/*
> + * Test that calling of_have_populated_dt() returns false when
> + * the OF_EMPTY_ROOT flag isn't set.
> + */
> +static void of_have_populated_dt_false_when_flag_set(struct kunit *test)
> +{
> +       bool was_set;
> +
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
> +
> +       was_set = of_node_test_and_set_flag(of_root, OF_EMPTY_ROOT);
> +       KUNIT_EXPECT_FALSE(test, of_have_populated_dt());
> +
> +       if (!was_set)
> +               of_node_clear_flag(of_root, OF_EMPTY_ROOT);
> +}
> +
> +/*
> + * Test that calling of_have_populated_dt() returns false when
> + * the OF_EMPTY_ROOT flag isn't set.
> + */
> +static void of_have_populated_dt_true_when_flag_clear(struct kunit *test)
> +{
> +       bool was_set;
> +
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, of_root);
> +
> +       was_set = of_node_check_flag(of_root, OF_EMPTY_ROOT);
> +       of_node_set_flag(of_root, OF_EMPTY_ROOT);
> +       KUNIT_EXPECT_FALSE(test, of_have_populated_dt());
> +
> +       if (was_set)
> +               of_node_set_flag(of_root, OF_EMPTY_ROOT);
> +}
> +
> +static struct kunit_case of_have_populated_dt_test_cases[] = {
> +       KUNIT_CASE(of_have_populated_dt_false_when_flag_set),
> +       KUNIT_CASE(of_have_populated_dt_true_when_flag_clear),
> +       {}
> +};
> +
> +/*
> + * Test suite to confirm behavior of of_have_populated_dt().
> + */
> +static struct kunit_suite of_have_populated_dt_suite = {
> +       .name = "of_have_populated_dt",
> +       .test_cases = of_have_populated_dt_test_cases,
> +};
> +
> +kunit_test_suites(
> +       &dtb_suite,
> +       &of_have_populated_dt_suite,
> +);
> +MODULE_LICENSE("GPL");
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

Re: [PATCH 6/6] of: Add KUnit test to confirm DTB is loaded

[Stephen Boyd]

Quoting David Gow (2024-01-15 21:03:12)
> On Sat, 13 Jan 2024 at 04:07, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Add a KUnit test that confirms a DTB has been loaded, i.e. there is a
> > root node, and that the of_have_populated_dt() API works properly.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Brendan Higgins <brendan.higgins@linux.dev>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> 
> I won't pretend to be a devicetree expert, but this looks good to me
> from a KUnit point of view, and passes comfortably here.
> 
> checkpatch seems to have one complaint about the kconfig help text.
> Personally, I think the brief description is fine.
> 
> Reviewed-by: David Gow <davidgow@google.com>
> 

Thanks! I noticed that x86 has some devicetree init code. Did you happen
to try on an x86 kvm instance? Or only run on UML?

----8<----
diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index afd09924094e..650752d112a6 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -283,22 +283,24 @@ void __init x86_flattree_get_config(void)
 	u32 size, map_len;
 	void *dt;
 
-	if (!initial_dtb)
-		return;
+	if (initial_dtb) {
+		map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
 
-	map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
+		dt = early_memremap(initial_dtb, map_len);
+		size = fdt_totalsize(dt);
+		if (map_len < size) {
+			early_memunmap(dt, map_len);
+			dt = early_memremap(initial_dtb, size);
+			map_len = size;
+		}
 
-	dt = early_memremap(initial_dtb, map_len);
-	size = fdt_totalsize(dt);
-	if (map_len < size) {
-		early_memunmap(dt, map_len);
-		dt = early_memremap(initial_dtb, size);
-		map_len = size;
+		early_init_dt_verify(dt);
 	}
 
-	early_init_dt_verify(dt);
 	unflatten_and_copy_device_tree();
-	early_memunmap(dt, map_len);
+
+	if (initial_dtb)
+		early_memunmap(dt, map_len);
 }
 #endif

Re: [PATCH 6/6] of: Add KUnit test to confirm DTB is loaded

[David Gow]

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

On Tue, 23 Jan 2024 at 06:48, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting David Gow (2024-01-15 21:03:12)
> > On Sat, 13 Jan 2024 at 04:07, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Add a KUnit test that confirms a DTB has been loaded, i.e. there is a
> > > root node, and that the of_have_populated_dt() API works properly.
> > >
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: David Gow <davidgow@google.com>
> > > Cc: Brendan Higgins <brendan.higgins@linux.dev>
> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > > ---
> >
> > I won't pretend to be a devicetree expert, but this looks good to me
> > from a KUnit point of view, and passes comfortably here.
> >
> > checkpatch seems to have one complaint about the kconfig help text.
> > Personally, I think the brief description is fine.
> >
> > Reviewed-by: David Gow <davidgow@google.com>
> >
>
> Thanks! I noticed that x86 has some devicetree init code. Did you happen
> to try on an x86 kvm instance? Or only run on UML?
>

I admit, I only tested on UML, plus a quick check that the default
kunitconfig wasn't broken on anything else.

A further look is showing:
aarch64: PASSED
UML: PASSED
UML LLVM: PASSED
powerpc64: PASSED
m68k: FAILED
i386: FAILED
x86_64: FAILED
x86_64 KASAN: FAILED

The failures are all of the form:
> [15:18:34] ============ of_have_populated_dt (2 subtests) =============
> [15:18:34] # of_have_populated_dt_false_when_flag_set: ASSERTION FAILED at drivers/of/of_test.c:53
> [15:18:34] Expected of_root is not null, but is
> [15:18:34] [FAILED] of_have_populated_dt_false_when_flag_set
> [15:18:34] # of_have_populated_dt_true_when_flag_clear: ASSERTION FAILED at drivers/of/of_test.c:70
> [15:18:34] Expected of_root is not null, but is
> [15:18:34] [FAILED] of_have_populated_dt_true_when_flag_clear
> [15:18:34]     # module: of_test
> [15:18:34] # of_have_populated_dt: pass:0 fail:2 skip:0 total:2
> [15:18:34] # Totals: pass:0 fail:2 skip:0 total:2
> [15:18:34] ============== [FAILED] of_have_populated_dt ===============

-- David

> ----8<----
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index afd09924094e..650752d112a6 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -283,22 +283,24 @@ void __init x86_flattree_get_config(void)
>         u32 size, map_len;
>         void *dt;
>
> -       if (!initial_dtb)
> -               return;
> +       if (initial_dtb) {
> +               map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
>
> -       map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
> +               dt = early_memremap(initial_dtb, map_len);
> +               size = fdt_totalsize(dt);
> +               if (map_len < size) {
> +                       early_memunmap(dt, map_len);
> +                       dt = early_memremap(initial_dtb, size);
> +                       map_len = size;
> +               }
>
> -       dt = early_memremap(initial_dtb, map_len);
> -       size = fdt_totalsize(dt);
> -       if (map_len < size) {
> -               early_memunmap(dt, map_len);
> -               dt = early_memremap(initial_dtb, size);
> -               map_len = size;
> +               early_init_dt_verify(dt);
>         }
>
> -       early_init_dt_verify(dt);
>         unflatten_and_copy_device_tree();
> -       early_memunmap(dt, map_len);
> +
> +       if (initial_dtb)
> +               early_memunmap(dt, map_len);
>  }
>  #endif

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]