[v3,7/7] of: Add KUnit test to confirm DTB is loaded

  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    | 48 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)
 create mode 100644 drivers/of/.kunitconfig
 create mode 100644 drivers/of/of_test.c
  

On Sat, 3 Feb 2024 at 03:59, 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>
> ---

This looks pretty good to me test-wise, though it still fails on m68k.
(Everything else I tried it on works, though I've definitely not tried
_every_ architecture.)

aarch64: PASSED
i386: PASSED
x86_64: PASSED
x86_64 KASAN: PASSED
powerpc64: PASSED
UML: PASSED
UML LLVM: PASSED
m68k: FAILED
> $ qemu-system-m68k -nodefaults -m 1024 -kernel .kunit-all-m68k/vmlinux -append 'kunit.enable=1 console=hvc0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt
> [11:55:05] ===================== dtb (2 subtests) =====================
> [11:55:05] # dtb_root_node_found_by_path: EXPECTATION FAILED at drivers/of/of_test.c:18
> [11:55:05] Expected np is not null, but is
> [11:55:05] [FAILED] dtb_root_node_found_by_path
> [11:55:05] # dtb_root_node_populates_of_root: EXPECTATION FAILED at drivers/of/of_test.c:28
> [11:55:05] Expected of_root is not null, but is
> [11:55:05] [FAILED] dtb_root_node_populates_of_root
> [11:55:05]     # module: of_test
> [11:55:05] # dtb: pass:0 fail:2 skip:0 total:2
> [11:55:05] # Totals: pass:0 fail:2 skip:0 total:2
> [11:55:05] ======================= [FAILED] dtb =======================


My only other question is about the test names: the mix of 'of' and
'dtb' can be a bit confusing. As is, we have:
kconfig name: OF_KUNIT_TEST
module name: of_test
suite name: dtb
test names: all start with dtb_

Given KUnit only really deals with the suite/test names directly, it's
not trivial to see that 'dtb.dtb_*' is controlled by OF_KUNIT_TEST and
in of_test if built as a module. (This is getting a bit easier now
that we have the 'module' attribute in the output, but still.)

Would 'of_dtb' work as a suite name if it's important to keep both
'of' and 'dtb'?

In general, though, this test looks good to me. Particularly if m68k
can be fixed.

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    | 48 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 62 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 17733285b415..53d1b5dd89e8 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 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..71a767b42b43
> --- /dev/null
> +++ b/drivers/of/of_test.c
> @@ -0,0 +1,48 @@
> +// 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 "/" can be found by path.
> + */
> +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 DTB is loaded.
> + */
> +static struct kunit_suite dtb_suite = {
> +       .name = "dtb",
> +       .test_cases = dtb_test_cases,
> +};
> +
> +kunit_test_suites(
> +       &dtb_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
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240202195909.3458162-8-sboyd%40kernel.org.
  

Quoting David Gow (2024-02-02 20:10:17)
> On Sat, 3 Feb 2024 at 03:59, 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>
> > ---
> 
> This looks pretty good to me test-wise, though it still fails on m68k.
> (Everything else I tried it on works, though I've definitely not tried
> _every_ architecture.)
> 
> aarch64: PASSED
> i386: PASSED
> x86_64: PASSED
> x86_64 KASAN: PASSED
> powerpc64: PASSED
> UML: PASSED
> UML LLVM: PASSED
> m68k: FAILED
> > $ qemu-system-m68k -nodefaults -m 1024 -kernel .kunit-all-m68k/vmlinux -append 'kunit.enable=1 console=hvc0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt
> > [11:55:05] ===================== dtb (2 subtests) =====================
> > [11:55:05] # dtb_root_node_found_by_path: EXPECTATION FAILED at drivers/of/of_test.c:18
> > [11:55:05] Expected np is not null, but is
> > [11:55:05] [FAILED] dtb_root_node_found_by_path
> > [11:55:05] # dtb_root_node_populates_of_root: EXPECTATION FAILED at drivers/of/of_test.c:28
> > [11:55:05] Expected of_root is not null, but is
> > [11:55:05] [FAILED] dtb_root_node_populates_of_root
> > [11:55:05]     # module: of_test
> > [11:55:05] # dtb: pass:0 fail:2 skip:0 total:2
> > [11:55:05] # Totals: pass:0 fail:2 skip:0 total:2
> > [11:55:05] ======================= [FAILED] dtb =======================

Ah yeah I forgot to mention that. m68k fails because it doesn't call the
unflatten_(and_copy)?_device_tree() function, so we don't populate a
root node on that architecture. One solution would be to make CONFIG_OF
unavailable on m68k. Or we have to make sure DT works on any
architecture. Rob, what do you prefer here?

> 
> 
> My only other question is about the test names: the mix of 'of' and
> 'dtb' can be a bit confusing. As is, we have:
> kconfig name: OF_KUNIT_TEST
> module name: of_test
> suite name: dtb
> test names: all start with dtb_
> 
> Given KUnit only really deals with the suite/test names directly, it's
> not trivial to see that 'dtb.dtb_*' is controlled by OF_KUNIT_TEST and
> in of_test if built as a module. (This is getting a bit easier now
> that we have the 'module' attribute in the output, but still.)
> 
> Would 'of_dtb' work as a suite name if it's important to keep both
> 'of' and 'dtb'?

Sure, I can add of_ prefix to the tests.

> 
> In general, though, this test looks good to me. Particularly if m68k
> can be fixed.
> 
> Reviewed-by: David Gow <davidgow@google.com>
> 

Thanks!
  

Hi Stephen,

On Mon, Feb 5, 2024 at 8:19 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting David Gow (2024-02-02 20:10:17)
> > On Sat, 3 Feb 2024 at 03:59, 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>
> > > ---
> >
> > This looks pretty good to me test-wise, though it still fails on m68k.
> > (Everything else I tried it on works, though I've definitely not tried
> > _every_ architecture.)
> >
> > aarch64: PASSED
> > i386: PASSED
> > x86_64: PASSED
> > x86_64 KASAN: PASSED
> > powerpc64: PASSED
> > UML: PASSED
> > UML LLVM: PASSED
> > m68k: FAILED
> > > $ qemu-system-m68k -nodefaults -m 1024 -kernel .kunit-all-m68k/vmlinux -append 'kunit.enable=1 console=hvc0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt
> > > [11:55:05] ===================== dtb (2 subtests) =====================
> > > [11:55:05] # dtb_root_node_found_by_path: EXPECTATION FAILED at drivers/of/of_test.c:18
> > > [11:55:05] Expected np is not null, but is
> > > [11:55:05] [FAILED] dtb_root_node_found_by_path
> > > [11:55:05] # dtb_root_node_populates_of_root: EXPECTATION FAILED at drivers/of/of_test.c:28
> > > [11:55:05] Expected of_root is not null, but is
> > > [11:55:05] [FAILED] dtb_root_node_populates_of_root
> > > [11:55:05]     # module: of_test
> > > [11:55:05] # dtb: pass:0 fail:2 skip:0 total:2
> > > [11:55:05] # Totals: pass:0 fail:2 skip:0 total:2
> > > [11:55:05] ======================= [FAILED] dtb =======================
>
> Ah yeah I forgot to mention that. m68k fails because it doesn't call the
> unflatten_(and_copy)?_device_tree() function, so we don't populate a
> root node on that architecture. One solution would be to make CONFIG_OF
> unavailable on m68k. Or we have to make sure DT works on any
> architecture. Rob, what do you prefer here?

I guess the latter?
Alpha, hexagon, parisc, s390, and sparc are also lacking calls
to unflatten.*device_tree().

Gr{oetje,eeting}s,

                        Geert
  

Quoting Geert Uytterhoeven (2024-02-05 11:55:29)
> On Mon, Feb 5, 2024 at 8:19 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting David Gow (2024-02-02 20:10:17)
> > > On Sat, 3 Feb 2024 at 03:59, 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>
> > > > ---
> > >
> > > This looks pretty good to me test-wise, though it still fails on m68k.
> > > (Everything else I tried it on works, though I've definitely not tried
> > > _every_ architecture.)
> > >
> > > aarch64: PASSED
> > > i386: PASSED
> > > x86_64: PASSED
> > > x86_64 KASAN: PASSED
> > > powerpc64: PASSED
> > > UML: PASSED
> > > UML LLVM: PASSED
> > > m68k: FAILED
> > > > $ qemu-system-m68k -nodefaults -m 1024 -kernel .kunit-all-m68k/vmlinux -append 'kunit.enable=1 console=hvc0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt
> > > > [11:55:05] ===================== dtb (2 subtests) =====================
> > > > [11:55:05] # dtb_root_node_found_by_path: EXPECTATION FAILED at drivers/of/of_test.c:18
> > > > [11:55:05] Expected np is not null, but is
> > > > [11:55:05] [FAILED] dtb_root_node_found_by_path
> > > > [11:55:05] # dtb_root_node_populates_of_root: EXPECTATION FAILED at drivers/of/of_test.c:28
> > > > [11:55:05] Expected of_root is not null, but is
> > > > [11:55:05] [FAILED] dtb_root_node_populates_of_root
> > > > [11:55:05]     # module: of_test
> > > > [11:55:05] # dtb: pass:0 fail:2 skip:0 total:2
> > > > [11:55:05] # Totals: pass:0 fail:2 skip:0 total:2
> > > > [11:55:05] ======================= [FAILED] dtb =======================
> >
> > Ah yeah I forgot to mention that. m68k fails because it doesn't call the
> > unflatten_(and_copy)?_device_tree() function, so we don't populate a
> > root node on that architecture. One solution would be to make CONFIG_OF
> > unavailable on m68k. Or we have to make sure DT works on any
> > architecture. Rob, what do you prefer here?
> 
> I guess the latter?
> Alpha, hexagon, parisc, s390, and sparc are also lacking calls
> to unflatten.*device_tree().
> 

sparc does that on purpose. Perhaps it's simplest to call
unflatten_device_tree() if of_root is still NULL after setup_arch()
returns.

---8<---
diff --git a/init/main.c b/init/main.c
index e24b0780fdff..02f5cf8be6c1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -97,6 +97,8 @@
 #include <linux/jump_label.h>
 #include <linux/kcsan.h>
 #include <linux/init_syscalls.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
 #include <linux/stackdepot.h>
 #include <linux/randomize_kstack.h>
 #include <net/net_namespace.h>
@@ -895,6 +897,8 @@ void start_kernel(void)
 	pr_notice("%s", linux_banner);
 	early_security_init();
 	setup_arch(&command_line);
+	if (!of_root)
+		unflatten_device_tree();
 	setup_boot_config();
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
  

On Fri, Feb 9, 2024 at 8:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Geert Uytterhoeven (2024-02-05 11:55:29)
> > On Mon, Feb 5, 2024 at 8:19 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > Quoting David Gow (2024-02-02 20:10:17)
> > > > On Sat, 3 Feb 2024 at 03:59, 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>
> > > > > ---
> > > >
> > > > This looks pretty good to me test-wise, though it still fails on m68k.
> > > > (Everything else I tried it on works, though I've definitely not tried
> > > > _every_ architecture.)
> > > >
> > > > aarch64: PASSED
> > > > i386: PASSED
> > > > x86_64: PASSED
> > > > x86_64 KASAN: PASSED
> > > > powerpc64: PASSED
> > > > UML: PASSED
> > > > UML LLVM: PASSED
> > > > m68k: FAILED
> > > > > $ qemu-system-m68k -nodefaults -m 1024 -kernel .kunit-all-m68k/vmlinux -append 'kunit.enable=1 console=hvc0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt
> > > > > [11:55:05] ===================== dtb (2 subtests) =====================
> > > > > [11:55:05] # dtb_root_node_found_by_path: EXPECTATION FAILED at drivers/of/of_test.c:18
> > > > > [11:55:05] Expected np is not null, but is
> > > > > [11:55:05] [FAILED] dtb_root_node_found_by_path
> > > > > [11:55:05] # dtb_root_node_populates_of_root: EXPECTATION FAILED at drivers/of/of_test.c:28
> > > > > [11:55:05] Expected of_root is not null, but is
> > > > > [11:55:05] [FAILED] dtb_root_node_populates_of_root
> > > > > [11:55:05]     # module: of_test
> > > > > [11:55:05] # dtb: pass:0 fail:2 skip:0 total:2
> > > > > [11:55:05] # Totals: pass:0 fail:2 skip:0 total:2
> > > > > [11:55:05] ======================= [FAILED] dtb =======================
> > >
> > > Ah yeah I forgot to mention that. m68k fails because it doesn't call the
> > > unflatten_(and_copy)?_device_tree() function, so we don't populate a
> > > root node on that architecture. One solution would be to make CONFIG_OF
> > > unavailable on m68k. Or we have to make sure DT works on any
> > > architecture. Rob, what do you prefer here?
> >
> > I guess the latter?
> > Alpha, hexagon, parisc, s390, and sparc are also lacking calls
> > to unflatten.*device_tree().
> >
>
> sparc does that on purpose. Perhaps it's simplest to call
> unflatten_device_tree() if of_root is still NULL after setup_arch()
> returns.
>
> ---8<---
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..02f5cf8be6c1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -97,6 +97,8 @@
>  #include <linux/jump_label.h>
>  #include <linux/kcsan.h>
>  #include <linux/init_syscalls.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
>  #include <linux/stackdepot.h>
>  #include <linux/randomize_kstack.h>
>  #include <net/net_namespace.h>
> @@ -895,6 +897,8 @@ void start_kernel(void)
>         pr_notice("%s", linux_banner);
>         early_security_init();
>         setup_arch(&command_line);
> +       if (!of_root)

of_root is another thing I'd like to remove direct access to. That
check could be inside unflatten_device_tree().

> +               unflatten_device_tree();

That's back to what Frank had essentially and I wanted to avoid.

I think I'd just disable the tests on the above arches and let them
opt-in. I could be convinced otherwise though.

Rob
  

Quoting Rob Herring (2024-02-13 09:52:00)
> On Fri, Feb 9, 2024 at 8:59 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > ---8<---
> > diff --git a/init/main.c b/init/main.c
> > index e24b0780fdff..02f5cf8be6c1 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -97,6 +97,8 @@
> >  #include <linux/jump_label.h>
> >  #include <linux/kcsan.h>
> >  #include <linux/init_syscalls.h>
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> >  #include <linux/stackdepot.h>
> >  #include <linux/randomize_kstack.h>
> >  #include <net/net_namespace.h>
> > @@ -895,6 +897,8 @@ void start_kernel(void)
> >         pr_notice("%s", linux_banner);
> >         early_security_init();
> >         setup_arch(&command_line);
> > +       if (!of_root)
> 
> of_root is another thing I'd like to remove direct access to. That
> check could be inside unflatten_device_tree().

Ok.

> 
> > +               unflatten_device_tree();
> 
> That's back to what Frank had essentially and I wanted to avoid.

Alright, fair enough.

> 
> I think I'd just disable the tests on the above arches and let them
> opt-in. I could be convinced otherwise though.
> 

Kunit folks would prefer to skip tests when dependencies aren't
satisfied. The OF_UNITTEST config already depends on !SPARC so perhaps
it's simplest to have tests skip if OF_EARLY_FLATREE=n. Then
OF_EARLY_FLATREE can be def_bool OF && !(SPARC || M68K || other arches).
The OF_UNITTEST config can depend on OF_EARLY_FLATREE instead of select
it then. This way new supporting architectures can remove themselves
from the def_bool line when they start calling unflatten_device_tree().