Message ID | 20240202195909.3458162-8-sboyd@kernel.org |
---|---|
State | Superseded |
Headers | show |
Series | [v3,1/7] of: Always unflatten in unflatten_and_copy_device_tree() | expand |
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!
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().
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");
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