[v4,01/10] of: Add test managed wrappers for of_overlay_apply()/of_node_put()

Add test managed wrappers for of_overlay_apply() that automatically
removes the overlay when the test is finished. This API is intended for
use by KUnit tests that test code which relies on 'struct device_node's
and of_*() APIs.

KUnit tests will call of_overlay_apply_kunit() to load an overlay that's
been built into the kernel image. When the test is complete, the overlay
will be removed.

This has a few benefits:

 1) It keeps the tests hermetic because the overlay is removed when the
    test is complete. Tests won't even be aware that an overlay was
    loaded in another test.

 2) The overlay code can live right next to the unit test that loads it.
    The overlay and the unit test can be compiled into one kernel module
    if desired.

 3) We can test different device tree configurations by loading
    different overlays. The overlays can be written for a specific test,
    and there can be many of them loaded per-test without needing to jam
    all possible combinations into one DTB.

 4) It also allows KUnit to test device tree dependent code on any
    architecture, not just UML. This allows KUnit tests to test
    architecture specific device tree code.

There are some potential pitfalls though. Test authors need to be
careful to not overwrite properties in the live tree. The easiest way to
do this is to add and remove nodes with a 'kunit-' prefix, almost
guaranteeing that the same node won't be present in the tree loaded at
boot.

Suggested-by: Rob Herring <robh@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 Documentation/dev-tools/kunit/api/index.rst | 11 +++
 Documentation/dev-tools/kunit/api/of.rst    | 13 +++
 drivers/of/Makefile                         |  1 +
 drivers/of/of_kunit.c                       | 99 +++++++++++++++++++++
 include/kunit/of.h                          | 94 +++++++++++++++++++
 5 files changed, 218 insertions(+)
 create mode 100644 Documentation/dev-tools/kunit/api/of.rst
 create mode 100644 drivers/of/of_kunit.c
 create mode 100644 include/kunit/of.h

On Mon, 22 Apr 2024 16:23:54 -0700, Stephen Boyd wrote:
> Add test managed wrappers for of_overlay_apply() that automatically
> removes the overlay when the test is finished. This API is intended for
> use by KUnit tests that test code which relies on 'struct device_node's
> and of_*() APIs.
> 
> KUnit tests will call of_overlay_apply_kunit() to load an overlay that's
> been built into the kernel image. When the test is complete, the overlay
> will be removed.
> 
> This has a few benefits:
> 
>  1) It keeps the tests hermetic because the overlay is removed when the
>     test is complete. Tests won't even be aware that an overlay was
>     loaded in another test.
> 
>  2) The overlay code can live right next to the unit test that loads it.
>     The overlay and the unit test can be compiled into one kernel module
>     if desired.
> 
>  3) We can test different device tree configurations by loading
>     different overlays. The overlays can be written for a specific test,
>     and there can be many of them loaded per-test without needing to jam
>     all possible combinations into one DTB.
> 
>  4) It also allows KUnit to test device tree dependent code on any
>     architecture, not just UML. This allows KUnit tests to test
>     architecture specific device tree code.
> 
> There are some potential pitfalls though. Test authors need to be
> careful to not overwrite properties in the live tree. The easiest way to
> do this is to add and remove nodes with a 'kunit-' prefix, almost
> guaranteeing that the same node won't be present in the tree loaded at
> boot.
> 
> Suggested-by: Rob Herring <robh@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  Documentation/dev-tools/kunit/api/index.rst | 11 +++
>  Documentation/dev-tools/kunit/api/of.rst    | 13 +++
>  drivers/of/Makefile                         |  1 +
>  drivers/of/of_kunit.c                       | 99 +++++++++++++++++++++
>  include/kunit/of.h                          | 94 +++++++++++++++++++
>  5 files changed, 218 insertions(+)
>  create mode 100644 Documentation/dev-tools/kunit/api/of.rst
>  create mode 100644 drivers/of/of_kunit.c
>  create mode 100644 include/kunit/of.h
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Add test managed wrappers for of_overlay_apply() that automatically
> removes the overlay when the test is finished. This API is intended for
> use by KUnit tests that test code which relies on 'struct device_node's
> and of_*() APIs.
>
> KUnit tests will call of_overlay_apply_kunit() to load an overlay that's
> been built into the kernel image. When the test is complete, the overlay
> will be removed.
>
> This has a few benefits:
>
>  1) It keeps the tests hermetic because the overlay is removed when the
>     test is complete. Tests won't even be aware that an overlay was
>     loaded in another test.
>
>  2) The overlay code can live right next to the unit test that loads it.
>     The overlay and the unit test can be compiled into one kernel module
>     if desired.
>
>  3) We can test different device tree configurations by loading
>     different overlays. The overlays can be written for a specific test,
>     and there can be many of them loaded per-test without needing to jam
>     all possible combinations into one DTB.
>
>  4) It also allows KUnit to test device tree dependent code on any
>     architecture, not just UML. This allows KUnit tests to test
>     architecture specific device tree code.
>
> There are some potential pitfalls though. Test authors need to be
> careful to not overwrite properties in the live tree. The easiest way to
> do this is to add and remove nodes with a 'kunit-' prefix, almost
> guaranteeing that the same node won't be present in the tree loaded at
> boot.
>
> Suggested-by: Rob Herring <robh@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

This looks good to me. I'm not an expert on Device Tree Overlays, so
can't guarantee it's perfect and/or the most ergonomic solution for
any given use-case, but I definitely like the look of it from a KUnit
point of view.

A few minor naming and config-related thoughts below, but otherwise:

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

Cheers,
-- David

>  Documentation/dev-tools/kunit/api/index.rst | 11 +++
>  Documentation/dev-tools/kunit/api/of.rst    | 13 +++
>  drivers/of/Makefile                         |  1 +
>  drivers/of/of_kunit.c                       | 99 +++++++++++++++++++++
>  include/kunit/of.h                          | 94 +++++++++++++++++++
>  5 files changed, 218 insertions(+)
>  create mode 100644 Documentation/dev-tools/kunit/api/of.rst
>  create mode 100644 drivers/of/of_kunit.c
>  create mode 100644 include/kunit/of.h
>
> diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> index 2d8f756aab56..282befa17edf 100644
> --- a/Documentation/dev-tools/kunit/api/index.rst
> +++ b/Documentation/dev-tools/kunit/api/index.rst
> @@ -9,11 +9,15 @@ API Reference
>         test
>         resource
>         functionredirection
> +       of
>
>
>  This page documents the KUnit kernel testing API. It is divided into the
>  following sections:
>
> +Core KUnit API
> +==============
> +
>  Documentation/dev-tools/kunit/api/test.rst
>
>   - Documents all of the standard testing API
> @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
>  Documentation/dev-tools/kunit/api/functionredirection.rst
>
>   - Documents the KUnit Function Redirection API
> +
> +Driver KUnit API
> +================

If we're adding a separate 'Driver' section here, it's probably
sensible to move the existing device/driver helper documentation here,
rather than leaving it in resource.rst as-is. I'm happy to do that in
a follow-up patch, though.

> +
> +Documentation/dev-tools/kunit/api/of.rst
> +
> + - Documents the KUnit device tree (OF) API
> diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> new file mode 100644
> index 000000000000..8587591c3e78
> --- /dev/null
> +++ b/Documentation/dev-tools/kunit/api/of.rst
> @@ -0,0 +1,13 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +====================
> +Device Tree (OF) API
> +====================
> +
> +The KUnit device tree API is used to test device tree (of_*) dependent code.
> +
> +.. kernel-doc:: include/kunit/of.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/of/of_kunit.c
> +   :export:
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 251d33532148..0dfd05079313 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -19,6 +19,7 @@ obj-y += kexec.o
>  endif
>  endif
>
> +obj-$(CONFIG_KUNIT) += of_kunit.o

I'm tempted to have this either live in lib/kunit, or be behind a
separate Kconfig option, particularly since this will end up as a
separate module, as-is.

>  obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
>
>  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> new file mode 100644
> index 000000000000..f63527268a51
> --- /dev/null
> +++ b/drivers/of/of_kunit.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test managed device tree APIs
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +
> +#include <kunit/of.h>
> +#include <kunit/test.h>
> +#include <kunit/resource.h>
> +
> +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> +{
> +       of_overlay_remove(ovcs_id);
> +}
> +
> +/**
> + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> + * @test: test context
> + * @overlay_fdt: device tree overlay to apply
> + * @overlay_fdt_size: size in bytes of @overlay_fdt
> + * @ovcs_id: identifier of overlay, used to remove the overlay
> + *
> + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> + * case and is automatically removed with of_overlay_remove() after the test
> + * case concludes.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> +                              u32 overlay_fdt_size, int *ovcs_id)

We're using kunit_ as a prefix for the device helpers (e.g.
kunit_device_register()), so it may make sense to do that here, too.
It's not as important as with the platform_device helpers, which are
very similar to the existing device ones, but if we want to treat
these as "part of KUnit which deals with of_overlays", rather than
"part of "of_overlay which deals with KUnit", this may fit better.

Thoughts?

> +{
> +       int ret;
> +       int *copy_id;
> +
> +       if (!IS_ENABLED(CONFIG_OF_OVERLAY))
> +               kunit_skip(test, "requires CONFIG_OF_OVERLAY");
> +       if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
> +               kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE for root node");
> +
> +       copy_id = kunit_kmalloc(test, sizeof(*copy_id), GFP_KERNEL);
> +       if (!copy_id)
> +               return -ENOMEM;
> +
> +       ret = of_overlay_fdt_apply(overlay_fdt, overlay_fdt_size,
> +                                  ovcs_id, NULL);
> +       if (ret)
> +               return ret;
> +
> +       *copy_id = *ovcs_id;
> +
> +       return kunit_add_action_or_reset(test, of_overlay_fdt_apply_kunit_exit,
> +                                        copy_id);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_fdt_apply_kunit);
> +
> +/**
> + * __of_overlay_apply_kunit() - Test managed of_overlay_fdt_apply() variant
> + * @test: test context
> + * @overlay_begin: start address of overlay to apply
> + * @overlay_end: end address of overlay to apply
> + *
> + * This is mostly internal API. See of_overlay_apply_kunit() for the wrapper
> + * that makes this easier to use.
> + *
> + * Similar to of_overlay_fdt_apply(), except the overlay is managed by the test
> + * case and is automatically removed with of_overlay_remove() after the test
> + * case concludes.
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +int __of_overlay_apply_kunit(struct kunit *test, u8 *overlay_begin,
> +                            const u8 *overlay_end)
> +{
> +       int unused;
> +
> +       return of_overlay_fdt_apply_kunit(test, overlay_begin,
> +                                         overlay_end - overlay_begin,
> +                                         &unused);
> +}
> +EXPORT_SYMBOL_GPL(__of_overlay_apply_kunit);
> +
> +/**
> + * of_node_put_kunit() - Test managed of_node_put()
> + * @test: test context
> + * @node: node to pass to `of_node_put()`
> + *
> + * Just like of_node_put(), except the node is managed by the test case and is
> + * automatically put with of_node_put() after the test case concludes.
> + */
> +void of_node_put_kunit(struct kunit *test, struct device_node *node)
> +{
> +       if (kunit_add_action(test, (kunit_action_t *)&of_node_put, node)) {
> +               KUNIT_FAIL(test,
> +                          "Can't allocate a kunit resource to put of_node\n");
> +       }
> +}
> +EXPORT_SYMBOL_GPL(of_node_put_kunit);
> diff --git a/include/kunit/of.h b/include/kunit/of.h
> new file mode 100644
> index 000000000000..9981442ba578
> --- /dev/null
> +++ b/include/kunit/of.h
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _KUNIT_OF_H
> +#define _KUNIT_OF_H
> +
> +#include <kunit/test.h>
> +
> +struct device_node;
> +
> +#ifdef CONFIG_OF

Do we also need to check for CONFIG_OF_OVERLAY here?

Also, how useful is it to compile but skip tests without
CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
which may make it more obvious that these are disabled if it's
unexpected.

Thoughts?

> +
> +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> +                              u32 overlay_fdt_size, int *ovcs_id);
> +int __of_overlay_apply_kunit(struct kunit *test, u8 *overlay_begin,
> +                            const u8 *overlay_end);
> +
> +void of_node_put_kunit(struct kunit *test, struct device_node *node);
> +
> +#else
> +
> +static inline int
> +of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> +                          u32 overlay_fdt_size, int *ovcs_id)
> +{
> +       kunit_skip(test, "requires CONFIG_OF");
> +       return -EINVAL;
> +}
> +
> +static inline int
> +__of_overlay_apply_kunit(struct kunit *test, u8 *overlay_begin,
> +                        const u8 *overlay_end)
> +{
> +       kunit_skip(test, "requires CONFIG_OF");
> +       return -EINVAL;
> +}
> +
> +static inline
> +void of_node_put_kunit(struct kunit *test, struct device_node *node)
> +{
> +       kunit_skip(test, "requires CONFIG_OF");
> +}
> +
> +#endif /* !CONFIG_OF */
> +
> +/**
> + * of_overlay_apply_kunit() - Test managed of_overlay_fdt_apply() for built-in overlays
> + * @test: test context
> + * @overlay_name: name of overlay to apply
> + *
> + * This macro is used to apply a device tree overlay built with the
> + * cmd_dt_S_dtbo rule in scripts/Makefile.lib that has been compiled into the
> + * kernel image or KUnit test module. The overlay is automatically removed when
> + * the test is finished.
> + *
> + * Unit tests that need device tree nodes should compile an overlay file with
> + * @overlay_name\.dtbo.o in their Makefile along with their unit test and then
> + * load the overlay during their test. The @overlay_name matches the filename
> + * of the overlay without the dtbo filename extension. If CONFIG_OF_OVERLAY is
> + * not enabled, the @test will be skipped.
> + *
> + * In the Makefile
> + *
> + * .. code-block:: none
> + *
> + *     obj-$(CONFIG_OF_OVERLAY_KUNIT_TEST) += overlay_test.o kunit_overlay_test.dtbo.o
> + *
> + * In the test
> + *
> + * .. code-block:: c
> + *
> + *     static void of_overlay_kunit_of_overlay_apply(struct kunit *test)
> + *     {
> + *             struct device_node *np;
> + *
> + *             KUNIT_ASSERT_EQ(test, 0,
> + *                             of_overlay_apply_kunit(test, kunit_overlay_test));
> + *
> + *             np = of_find_node_by_name(NULL, "test-kunit");
> + *             KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
> + *             of_node_put(np);
> + *     }
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +#define of_overlay_apply_kunit(test, overlay_name)             \
> +({                                                             \
> +       extern uint8_t __dtbo_##overlay_name##_begin[];         \
> +       extern uint8_t __dtbo_##overlay_name##_end[];           \
> +                                                               \
> +       __of_overlay_apply_kunit((test),                        \
> +                       __dtbo_##overlay_name##_begin,          \
> +                       __dtbo_##overlay_name##_end);           \
> +})
> +
> +#endif
> --
> https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
> https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
>

Quoting David Gow (2024-05-01 00:55:10)
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote:
> > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > index 2d8f756aab56..282befa17edf 100644
> > --- a/Documentation/dev-tools/kunit/api/index.rst
> > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > @@ -9,11 +9,15 @@ API Reference
> >         test
> >         resource
> >         functionredirection
> > +       of
> >
> >
> >  This page documents the KUnit kernel testing API. It is divided into the
> >  following sections:
> >
> > +Core KUnit API
> > +==============
> > +
> >  Documentation/dev-tools/kunit/api/test.rst
> >
> >   - Documents all of the standard testing API
> > @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
> >  Documentation/dev-tools/kunit/api/functionredirection.rst
> >
> >   - Documents the KUnit Function Redirection API
> > +
> > +Driver KUnit API
> > +================
> 
> If we're adding a separate 'Driver' section here, it's probably
> sensible to move the existing device/driver helper documentation here,
> rather than leaving it in resource.rst as-is. I'm happy to do that in
> a follow-up patch, though.

To clarify, you're talking about "Managed Devices"? Looks like that can
be a follow-up to split it into a new file and then put it here. If
you're happy to do that then I'll leave it to you.

> 
> > +
> > +Documentation/dev-tools/kunit/api/of.rst
> > +
> > + - Documents the KUnit device tree (OF) API
> > diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> > new file mode 100644
> > index 000000000000..8587591c3e78
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/of.rst
> > @@ -0,0 +1,13 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +====================
> > +Device Tree (OF) API
> > +====================
> > +
> > +The KUnit device tree API is used to test device tree (of_*) dependent code.
> > +
> > +.. kernel-doc:: include/kunit/of.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/of/of_kunit.c
> > +   :export:
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index 251d33532148..0dfd05079313 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -19,6 +19,7 @@ obj-y += kexec.o
> >  endif
> >  endif
> >
> > +obj-$(CONFIG_KUNIT) += of_kunit.o
> 
> I'm tempted to have this either live in lib/kunit, or be behind a
> separate Kconfig option, particularly since this will end up as a
> separate module, as-is.

Is the idea to have a single module that has all the kunit "stuff" in it
so we can just load one module and be done? Is there any discussion on
the list I can read to see the argument for this?

> 
> >  obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> >
> >  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> > new file mode 100644
> > index 000000000000..f63527268a51
> > --- /dev/null
> > +++ b/drivers/of/of_kunit.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test managed device tree APIs
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +
> > +#include <kunit/of.h>
> > +#include <kunit/test.h>
> > +#include <kunit/resource.h>
> > +
> > +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> > +{
> > +       of_overlay_remove(ovcs_id);
> > +}
> > +
> > +/**
> > + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> > + * @test: test context
> > + * @overlay_fdt: device tree overlay to apply
> > + * @overlay_fdt_size: size in bytes of @overlay_fdt
> > + * @ovcs_id: identifier of overlay, used to remove the overlay
> > + *
> > + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> > + * case and is automatically removed with of_overlay_remove() after the test
> > + * case concludes.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + */
> > +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> > +                              u32 overlay_fdt_size, int *ovcs_id)
> 
> We're using kunit_ as a prefix for the device helpers (e.g.
> kunit_device_register()), so it may make sense to do that here, too.
> It's not as important as with the platform_device helpers, which are
> very similar to the existing device ones, but if we want to treat
> these as "part of KUnit which deals with of_overlays", rather than
> "part of "of_overlay which deals with KUnit", this may fit better.
> 
> Thoughts?

I'm fine either way with the name. I recall that last time we put a
kunit postfix to make it easier to tab complete or something like that.

I find it hard to understand the distinction you're trying to make
though. I guess you're saying the difference is what subsystem maintains
the code, kunit or of. When they're simple wrappers it is easier to
extract them out to lib/kunit and thus they can (should?) have the kunit
prefix. Maybe that always holds true, because kunit wrappers are
typically another API consumer, and if the API is exported either in a
linux/ header or as an exported symbol it can be wrapped in lib/kunit
easily. Did I follow correctly? When would of_overlay ever deal with
KUnit?

> > diff --git a/include/kunit/of.h b/include/kunit/of.h
> > new file mode 100644
> > index 000000000000..9981442ba578
> > --- /dev/null
> > +++ b/include/kunit/of.h
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _KUNIT_OF_H
> > +#define _KUNIT_OF_H
> > +
> > +#include <kunit/test.h>
> > +
> > +struct device_node;
> > +
> > +#ifdef CONFIG_OF
> 
> Do we also need to check for CONFIG_OF_OVERLAY here?
> 
> Also, how useful is it to compile but skip tests without
> CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
> which may make it more obvious that these are disabled if it's
> unexpected.
> 
> Thoughts?

I've tried to make it so that tests skip if an option isn't enabled. I
suppose the CONFIG_OF_OVERLAY check can be hoisted up here as well so
that the skip isn't buried in lower levels.

On Fri, 3 May 2024 at 08:36, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting David Gow (2024-05-01 00:55:10)
> > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote:
> > > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > > index 2d8f756aab56..282befa17edf 100644
> > > --- a/Documentation/dev-tools/kunit/api/index.rst
> > > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > > @@ -9,11 +9,15 @@ API Reference
> > >         test
> > >         resource
> > >         functionredirection
> > > +       of
> > >
> > >
> > >  This page documents the KUnit kernel testing API. It is divided into the
> > >  following sections:
> > >
> > > +Core KUnit API
> > > +==============
> > > +
> > >  Documentation/dev-tools/kunit/api/test.rst
> > >
> > >   - Documents all of the standard testing API
> > > @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
> > >  Documentation/dev-tools/kunit/api/functionredirection.rst
> > >
> > >   - Documents the KUnit Function Redirection API
> > > +
> > > +Driver KUnit API
> > > +================
> >
> > If we're adding a separate 'Driver' section here, it's probably
> > sensible to move the existing device/driver helper documentation here,
> > rather than leaving it in resource.rst as-is. I'm happy to do that in
> > a follow-up patch, though.
>
> To clarify, you're talking about "Managed Devices"? Looks like that can
> be a follow-up to split it into a new file and then put it here. If
> you're happy to do that then I'll leave it to you.
>
Yeah, this is "Managed Devices". I'll send out a follow-up patch to
the documentation once this has landed so we don't conflict.

> >
> > > +
> > > +Documentation/dev-tools/kunit/api/of.rst
> > > +
> > > + - Documents the KUnit device tree (OF) API
> > > diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> > > new file mode 100644
> > > index 000000000000..8587591c3e78
> > > --- /dev/null
> > > +++ b/Documentation/dev-tools/kunit/api/of.rst
> > > @@ -0,0 +1,13 @@
> > > +.. SPDX-License-Identifier: GPL-2.0
> > > +
> > > +====================
> > > +Device Tree (OF) API
> > > +====================
> > > +
> > > +The KUnit device tree API is used to test device tree (of_*) dependent code.
> > > +
> > > +.. kernel-doc:: include/kunit/of.h
> > > +   :internal:
> > > +
> > > +.. kernel-doc:: drivers/of/of_kunit.c
> > > +   :export:
> > > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > > index 251d33532148..0dfd05079313 100644
> > > --- a/drivers/of/Makefile
> > > +++ b/drivers/of/Makefile
> > > @@ -19,6 +19,7 @@ obj-y += kexec.o
> > >  endif
> > >  endif
> > >
> > > +obj-$(CONFIG_KUNIT) += of_kunit.o
> >
> > I'm tempted to have this either live in lib/kunit, or be behind a
> > separate Kconfig option, particularly since this will end up as a
> > separate module, as-is.
>
> Is the idea to have a single module that has all the kunit "stuff" in it
> so we can just load one module and be done? Is there any discussion on
> the list I can read to see the argument for this?

I don't think there's been any specific discussion around making sure
KUnit lives in one module: this is just the first patch which would
make CONFIG_KUNIT build several separate ones.
Personally, I'd prefer to have the CONFIG_KUNIT option only build one
module itself, and otherwise keep the corresponding code in lib/kunit,
just so it's clearer what side effects enabling / disabling it has.

But ultimately, this really is just another side effect of the
discussion below about whether this is integrated as "part of KUnit",
in which case it can live in lib/kunit and be under CONFIG_KUNIT, or
if it's a part of of, in which case this is fine (though I'd rather it
be behind a CONFIG_OF_KUNIT_HELPERS or similar, personally).


> >
> > >  obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> > >
> > >  obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > > diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> > > new file mode 100644
> > > index 000000000000..f63527268a51
> > > --- /dev/null
> > > +++ b/drivers/of/of_kunit.c
> > > @@ -0,0 +1,99 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Test managed device tree APIs
> > > + */
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_fdt.h>
> > > +
> > > +#include <kunit/of.h>
> > > +#include <kunit/test.h>
> > > +#include <kunit/resource.h>
> > > +
> > > +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> > > +{
> > > +       of_overlay_remove(ovcs_id);
> > > +}
> > > +
> > > +/**
> > > + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> > > + * @test: test context
> > > + * @overlay_fdt: device tree overlay to apply
> > > + * @overlay_fdt_size: size in bytes of @overlay_fdt
> > > + * @ovcs_id: identifier of overlay, used to remove the overlay
> > > + *
> > > + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> > > + * case and is automatically removed with of_overlay_remove() after the test
> > > + * case concludes.
> > > + *
> > > + * Return: 0 on success, negative errno on failure
> > > + */
> > > +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> > > +                              u32 overlay_fdt_size, int *ovcs_id)
> >
> > We're using kunit_ as a prefix for the device helpers (e.g.
> > kunit_device_register()), so it may make sense to do that here, too.
> > It's not as important as with the platform_device helpers, which are
> > very similar to the existing device ones, but if we want to treat
> > these as "part of KUnit which deals with of_overlays", rather than
> > "part of "of_overlay which deals with KUnit", this may fit better.
> >
> > Thoughts?
>
> I'm fine either way with the name. I recall that last time we put a
> kunit postfix to make it easier to tab complete or something like that.
>
> I find it hard to understand the distinction you're trying to make
> though. I guess you're saying the difference is what subsystem maintains
> the code, kunit or of. When they're simple wrappers it is easier to
> extract them out to lib/kunit and thus they can (should?) have the kunit
> prefix. Maybe that always holds true, because kunit wrappers are
> typically another API consumer, and if the API is exported either in a
> linux/ header or as an exported symbol it can be wrapped in lib/kunit
> easily. Did I follow correctly? When would of_overlay ever deal with
> KUnit?

Yeah, it's about what subsystem is maintaining the code, which impacts
a bit of the naming, and depends a bit on the intended use-case.

If these helpers are intended to test a particular subsystem, and are
of no use outside it, it seems clear that they should be a part of
that subsystem. For instance, the drm_kunit_helpers.
If they're exposing kunit-specific wrappers around core APIs, it makes
sense for them to be a part of KUnit. (The managed devices stuff, for
instance, as the device model is used by pretty much everything. It
also requires a KUnit-managed struct kunit_bus, which is hooked into
KUnit at a lower level, so needs to be a part of kunit.)

It gets more complicated for cases like of, where the helpers are both
used for testing of itself, and for testing drivers which rely on it.
So I think it could go either way. My gut instinct is that
platform_device is generic enough to be a part of KUnit (to match the
existing managed device stuff). For of_overlay, I could go either way,
and just leaned to having it be part of KUnit as that's a bit more
common, and it matches, e.g., the headers and documentation being
under include/kunit and dev-tools/kunit respectively.

> > > diff --git a/include/kunit/of.h b/include/kunit/of.h
> > > new file mode 100644
> > > index 000000000000..9981442ba578
> > > --- /dev/null
> > > +++ b/include/kunit/of.h
> > > @@ -0,0 +1,94 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _KUNIT_OF_H
> > > +#define _KUNIT_OF_H
> > > +
> > > +#include <kunit/test.h>
> > > +
> > > +struct device_node;
> > > +
> > > +#ifdef CONFIG_OF
> >
> > Do we also need to check for CONFIG_OF_OVERLAY here?
> >
> > Also, how useful is it to compile but skip tests without
> > CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
> > which may make it more obvious that these are disabled if it's
> > unexpected.
> >
> > Thoughts?
>
> I've tried to make it so that tests skip if an option isn't enabled. I
> suppose the CONFIG_OF_OVERLAY check can be hoisted up here as well so
> that the skip isn't buried in lower levels.

Yeah, my feeling here is that if we're going to declare functions
which interact with of_overlay, we should have the 'skip' fallbacks
occur for either both CONFIG_OF and CONFIG_OF_OVERLAY here, or neither
(and require the test use its own #include guards). Having CONFIG_OF
checked here, and CONFIG_OF_OVERLAY checked elsewhere seems confusing
to me.

Cheers,
-- David