Re: [PATCH v2 01/11] of: Load KUnit DTB from of_core_init()

From: Stephen Boyd <sboyd@kernel.org>
To: Rob Herring <robh@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
	patches@lists.linux.dev,
	Brendan Higgins <brendan.higgins@linux.dev>,
	David Gow <davidgow@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rafael J . Wysocki <rafael@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Christian Marangi <ansuelsmth@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	devicetree@vger.kernel.org, linux-kselftest@vger.kernel.org,
	kunit-dev@googlegroups.com, Maxime Ripard <maxime@cerno.tech>
Subject: Re: [PATCH v2 01/11] of: Load KUnit DTB from of_core_init()
Date: Tue, 21 Mar 2023 10:56:02 -0700	[thread overview]
Message-ID: <d339c3b0e92e11fb9c98671c1e36cd14.sboyd@kernel.org> (raw)
In-Reply-To: <20230321173303.GA950598-robh@kernel.org>

Quoting Rob Herring (2023-03-21 10:33:03)
> On Wed, Mar 15, 2023 at 11:37:18AM -0700, Stephen Boyd wrote:
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index ac6fde53342f..090c5d7925e4 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -16,13 +16,16 @@
> >  
> >  #define pr_fmt(fmt)  "OF: " fmt
> >  
> > +#include <linux/align.h>
> >  #include <linux/console.h>
> >  #include <linux/ctype.h>
> >  #include <linux/cpu.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_fdt.h>
> 
> base.c deals with unflattened trees. There shouldn't be anything FDT 
> related in it.

Ok.

> 
> >  #include <linux/of_graph.h>
> > +#include <linux/printk.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/slab.h>
> >  #include <linux/string.h>
> > @@ -163,10 +166,90 @@ void __of_phandle_cache_inv_entry(phandle handle)
> >               phandle_cache[handle_hash] = NULL;
> >  }
> >  
> > +#ifdef CONFIG_OF_KUNIT
> 
> base.c is already quite big. This should probably be its own file. 
> Perhaps in kunit code because that's what we do for everything else 
> (e.g. DT clock code goes in drivers/clk/). (My goal is to eliminate 
> drivers/of/. That's easier than finding maintainers. ;) )

Heh, sure.

> 
> > +static int __init of_kunit_add_data(void)
> > +{
> > +     void *kunit_fdt;
> > +     void *kunit_fdt_align;
> > +     struct device_node *kunit_node = NULL, *np;
> > +     /*
> > +      * __dtbo_kunit_begin[] and __dtbo_kunit_end[] are magically
> > +      * created by cmd_dt_S_dtbo in scripts/Makefile.lib
> > +      */
> > +     extern uint8_t __dtbo_kunit_begin[];
> > +     extern uint8_t __dtbo_kunit_end[];
> > +     const int size = __dtbo_kunit_end - __dtbo_kunit_begin;
> > +     int rc;
> > +     void *ret;
> > +
> > +     if (!size) {
> > +             pr_warn("kunit.dtbo is empty\n");
> > +             return -ENODATA;
> > +     }
> > +
> > +     kunit_fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> > +     if (!kunit_fdt)
> > +             return -ENOMEM;
> > +
> > +     kunit_fdt_align = PTR_ALIGN(kunit_fdt, FDT_ALIGN_SIZE);
> > +     memcpy(kunit_fdt_align, __dtbo_kunit_begin, size);
> > +
> > +     ret = of_fdt_unflatten_tree(kunit_fdt_align, NULL, &kunit_node);
> 
> I don't understand why this doesn't use of_overlay_fdt_apply(). Your 
> test(s) shouldn't be any different than any other overlay user (granted, 
> there aren't many). You apply the overlay, run your test, then remove 
> the overlay.

This isn't a test. This is loading the kunit.dtso blob, and potentially
the root DTB. Maybe of_overlay_fdt_apply() will work if there is a root
node already?

> 
> > +     if (!ret) {
> > +             pr_warn("unflatten KUnit tree failed\n");
> > +             kfree(kunit_fdt);
> > +             return -ENODATA;
> > +     }
> > +     if (!kunit_node) {
> > +             pr_warn("KUnit tree is empty\n");
> > +             kfree(kunit_fdt);
> > +             return -ENODATA;
> > +     }
> > +
> > +     of_overlay_mutex_lock();
> > +     rc = of_resolve_phandles(kunit_node);
> > +     if (rc) {
> > +             pr_err("Failed to resolve KUnit phandles (rc=%i)\n", rc);
> > +             of_overlay_mutex_unlock();
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!of_root) {
> 
> There's patches from Frank and others under review which will always 
> create and empty DT if the bootloader/arch didn't provide one. This 
> series should rely on that. (Or just assume that when that happens, your 
> tests will run in more environments)

Ok. I'll base v3 on the series here[1] unless told otherwise.

> > @@ -1879,6 +1962,105 @@ int of_update_property(struct device_node *np, struct property *newprop)
> >       return rc;
> >  }
> >  
> > +#if defined(CONFIG_OF_UNITTEST) || defined (CONFIG_KUNIT)
> > +/**
> > + * update_node_properties - adds the properties of np into dup node (present in
> > + * live tree) and updates parent of children of np to dup.
> > + *
> > + * @np: node whose properties are being added to the live tree
> > + * @dup: node present in live tree to be updated
> > + */
> > +static void __init update_node_properties(struct device_node *np,
> > +                                     struct device_node *dup)
> 
> Please split any moving of code to separate patches.

Sure.

> 
> I'm not remembering why we need these test functions vs. just applying 
> overlays. Frank? Perhaps because it's trying to test the overlay code 
> itself. But you should just be a user of the overlay API and not need to 
> do anything special.

Got it.

> > diff --git a/drivers/of/kunit.dtso b/drivers/of/kunit.dtso
> > new file mode 100644
> > index 000000000000..d512057df98d
> > --- /dev/null
> > +++ b/drivers/of/kunit.dtso
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/dts-v1/;
> > +/plugin/;
> > +
> > +/ {
> > +     /* Container node where KUnit tests can load overlays */
> > +     kunit_bus: kunit-bus {
> > +             compatible = "simple-bus";
> > +     };
> > +};
> 
> Why do we need an overlay to apply overlays to? 

I'm applying overlays based on the phandle kunit_bus. Should we allow
overlays to modify the root node? I haven't tried that, but I can give
it a shot and see if it works.

> 
> 
> > diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
> > new file mode 100644
> > index 000000000000..a4d70ac344ad
> > --- /dev/null
> > +++ b/drivers/of/of_test.c
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit tests for OF APIs
> > + */
> > +#include <linux/kconfig.h>
> > +#include <linux/of.h>
> > +
> > +#include <kunit/test.h>
> > +
> > +/*
> > + * Test that the root node / exists.
> > + */
> > +static void dtb_root_node_exists(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, of_find_node_by_path("/"));
> > +}
> > +
> > +/*
> > + * Test that the /__symbols__ node exists.
> > + */
> > +static void dtb_symbols_node_exists(struct kunit *test)
> > +{
> > +     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, of_find_node_by_path("/__symbols__"));
> > +}
> 
> Many base DTs will not have this. And the kunit tests themselves 
> shouldn't need it because they should be independent of the base tree.
> 

Alright. Let me see how modifying the root node works, in which case
maybe none of this patch is necessary besides confirming there is a root
node.

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