Message-ID: <CABVgOSm_Qce1AZQjH8wE4t238hxLYTNswNf1AkEDbWtqDTK7Ow@mail.gmail.com>
Date: Wed, 1 May 2024 15:55:10 +0800
From: David Gow <davidgow@...gle.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, 
	linux-clk@...r.kernel.org, patches@...ts.linux.dev, 
	kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org, 
	devicetree@...r.kernel.org, Brendan Higgins <brendan.higgins@...ux.dev>, 
	Rae Moar <rmoar@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>, 
	Saravana Kannan <saravanak@...gle.com>, Daniel Latypov <dlatypov@...gle.com>, 
	Christian Marangi <ansuelsmth@...il.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Maxime Ripard <maxime@...no.tech>
Subject: Re: [PATCH v4 01/10] of: Add test managed wrappers for of_overlay_apply()/of_node_put()

On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@...nel.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@...nel.org>
> Cc: Rob Herring <robh@...nel.org>
> Cc: Saravana Kannan <saravanak@...gle.com>
> Signed-off-by: Stephen Boyd <sboyd@...nel.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@...gle.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
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)

Powered by blists - more mailing lists