clk: Add kunit tests for fixed rate and parent data

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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Saravana Kannan <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 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

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 <[email protected]>

Cc Rob Herring <[email protected]>

Cc Saravana Kannan <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

 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

Documentation/dev-tools/kunit/api/index.rst +4 -0

index 2d8f756aab56..282befa17edf 100644
@@ -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

Documentation/dev-tools/kunit/api/index.rst +4 -0

+
+Documentation/dev-tools/kunit/api/of.rst
+
+ - Documents the KUnit device tree (OF) API

Documentation/dev-tools/kunit/api/of.rst new file +13 -0

new file mode 100644
index 000000000000..8587591c3e78
@@ -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:

drivers/of/Makefile +0 -0

index 251d33532148..0dfd05079313 100644

drivers/of/Makefile +0 -0

 obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
 
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/of_kunit.c new file +22 -0

new file mode 100644
index 000000000000..f63527268a51
@@ -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

drivers/of/of_kunit.c new file +67 -0

+{
+       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);

include/kunit/of.h new file +0 -0

new file mode 100644
index 000000000000..9981442ba578

include/kunit/of.h new file +85 -1

+
+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

Quoted 23 lines

Documentation/dev-tools/kunit/api/index.rst +4 -0

index 2d8f756aab56..282befa17edf 100644
@@ -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
 

Documentation/dev-tools/kunit/api/index.rst Documentation/dev-tools/kunit/api/resource.rst · line 34

Quoted 4 lines

Documentation/dev-tools/kunit/api/index.rst +3 -0

  - 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.

Quoted 32 lines

Documentation/dev-tools/kunit/api/index.rst +4 -0

+
+Documentation/dev-tools/kunit/api/of.rst
+
+ - Documents the KUnit device tree (OF) API

Documentation/dev-tools/kunit/api/of.rst new file +13 -0

new file mode 100644
index 000000000000..8587591c3e78
@@ -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:

drivers/of/Makefile +0 -0

index 251d33532148..0dfd05079313 100644
@@ -19,6 +19,7 @@ obj-y += kexec.o
 endif
 endif
 

drivers/of/Makefile obj-y += kexec.o · line 22

Quoted 1 line

drivers/of/Makefile +1 -0

+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.

Quoted 32 lines

drivers/of/Makefile +0 -0

 obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
 
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/of_kunit.c new file +22 -0

new file mode 100644
index 000000000000..f63527268a51
@@ -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

drivers/of/of_kunit.c line 32

Quoted 10 lines

drivers/of/of_kunit.c new file +10 -0

+ * @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?

Quoted 6 lines

include/kunit/of.h new file +0 -0

new file mode 100644
index 000000000000..9981442ba578

include/kunit/of.h line 9

Quoted 9 lines

include/kunit/of.h new file +9 -0

@@ -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?

Quoting David Gow (2024-05-01 00:55:10)

Quoted 24 lines · Stephen Boyd

Quoted 23 lines

Documentation/dev-tools/kunit/api/index.rst +4 -0

index 2d8f756aab56..282befa17edf 100644
@@ -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
 

Quoted 9 lines

Documentation/dev-tools/kunit/api/index.rst Documentation/dev-tools/kunit/api/resource.rst · line 34

Quoted 4 lines

Documentation/dev-tools/kunit/api/index.rst +3 -0

  - 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.

Quoted 32 lines

Quoted 32 lines

Documentation/dev-tools/kunit/api/index.rst +4 -0

+
+Documentation/dev-tools/kunit/api/of.rst
+
+ - Documents the KUnit device tree (OF) API

Documentation/dev-tools/kunit/api/of.rst new file +13 -0

new file mode 100644
index 000000000000..8587591c3e78
@@ -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:

drivers/of/Makefile +0 -0

index 251d33532148..0dfd05079313 100644
@@ -19,6 +19,7 @@ obj-y += kexec.o
 endif
 endif
 

Quoted 5 lines

drivers/of/Makefile obj-y += kexec.o · line 22

Quoted 1 line

drivers/of/Makefile +1 -0

+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?

Quoted 50 lines

Quoted 32 lines

drivers/of/Makefile +0 -0

 obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
 
 obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/of_kunit.c new file +22 -0

new file mode 100644
index 000000000000..f63527268a51
@@ -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

drivers/of/of_kunit.c line 32

Quoted 10 lines

drivers/of/of_kunit.c new file +10 -0

+ * @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.

Quoted 1 line

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?

Quoted 23 lines

Quoted 6 lines

include/kunit/of.h new file +0 -0

new file mode 100644
index 000000000000..9981442ba578

include/kunit/of.h line 9

Quoted 9 lines

include/kunit/of.h new file +9 -0

@@ -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.

Quoted 1 line

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.

Add the vendor prefix "test" to reserve a vendor prefix for bindings that are purely for testing device tree code. This allows test code to write bindings that can be checked by the schema validator.

Reviewed-by Rob Herring <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Conor Dooley <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -1

index b97d298b3eb6..e590f5ab539f 100644
@@ -1448,6 +1448,8 @@ patternProperties:
     description: Terasic Inc.
   "^tesla,.*":
     description: Tesla, Inc.
+  "^test,.*":
+    description: Reserved for use by tests. For example, KUnit.
   "^tfc,.*":
     description: Three Five Corp
   "^thead,.*":
--

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Describe a binding for an empty device node used by KUnit tests to confirm overlays load properly.

Reviewed-by Rob Herring <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc David Gow <[email protected]>

Cc Rae Moar <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

 .../devicetree/bindings/test/test,empty.yaml  | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml

Documentation/devicetree/bindings/test/test,empty.yaml new file +1 -0

new file mode 100644
index 000000000000..20dc83b15bbf
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

Documentation/devicetree/bindings/test/test,empty.yaml new file +19 -1

+
+description:
+  An empty node to confirm tests can load device tree overlays.
+
+properties:
+  compatible:
+    const: test,empty
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    kunit-node {
+      compatible = "test,empty";
+    };
+...
--

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Quoted 4 lines · Stephen Boyd

Describe a binding for an empty device node used by KUnit tests to confirm overlays load properly.

Quoted 8 lines

Reviewed-by Rob Herring <[email protected]>

Cc Krzysztof Kozlowski <[email protected]>

Cc Conor Dooley <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc David Gow <[email protected]>

Cc Rae Moar <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

Looks good to me.

Reviewed-by David Gow <[email protected]>

-- David

Quoted 15 lines

 .../devicetree/bindings/test/test,empty.yaml  | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml

Documentation/devicetree/bindings/test/test,empty.yaml new file +5 -0

new file mode 100644
index 000000000000..20dc83b15bbf
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/test/test,empty.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#

Documentation/devicetree/bindings/test/test,empty.yaml line 11

Quoted 6 lines

Documentation/devicetree/bindings/test/test,empty.yaml new file +6 -0

+
+title: Empty node
+
+maintainers:
+  - David Gow <[email protected]>
+  - Brendan Higgins <[email protected]>

Brendan: Do you want to use your linux.dev address here?

Test the KUnit test managed overlay APIs. Confirm that platform devices are created and destroyed properly. This provides us confidence that the test managed APIs work correctly and can be relied upon to provide tests with fake platform devices and device nodes via overlays compiled into the kernel image.

Cc Rob Herring <[email protected]>

Cc Saravana Kannan <[email protected]>

Cc Daniel Latypov <[email protected]>

Cc Brendan Higgins <[email protected]>

Cc David Gow <[email protected]>

Cc Rae Moar <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/of/.kunitconfig            |   1 +
 drivers/of/Kconfig                 |  10 +++
 drivers/of/Makefile                |   1 +
 drivers/of/kunit_overlay_test.dtso |   9 +++
 drivers/of/overlay_test.c          | 115 +++++++++++++++++++++++++++++
 5 files changed, 136 insertions(+)
 create mode 100644 drivers/of/kunit_overlay_test.dtso
 create mode 100644 drivers/of/overlay_test.c

drivers/base/test/platform_kunit.c new file +5 -0

new file mode 100644
index 000000000000..54af6db2a6d8
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test managed platform driver
+ */
+

drivers/base/test/platform_kunit.c new file +59 -0

+
+/**
+ * platform_driver_register_kunit() - Register a KUnit test managed platform driver
+ * @test: test context
+ * @drv: platform driver to register
+ *
+ * Register a test managed platform driver. This allows callers to embed the
+ * @drv in a container structure and use container_of() in the probe function
+ * to pass information to KUnit tests. It can be assumed that the driver has
+ * probed when this function returns.
+ *
+ * Example
+ *
+ * .. code-block:: c
+ *
+ *     struct kunit_test_context {
+ *             struct platform_driver pdrv;
+ *             const char *data;
+ *     };
+ *
+ *     static inline struct kunit_test_context *
+ *     to_test_context(struct platform_device *pdev)
+ *     {
+ *             return container_of(to_platform_driver(pdev->dev.driver),
+ *                                 struct kunit_test_context,
+ *                                 pdrv);
+ *     }
+ *
+ *     static int kunit_platform_driver_probe(struct platform_device *pdev)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = to_test_context(pdev);
+ *             ctx->data = "test data";
+ *
+ *             return 0;
+ *     }
+ *
+ *     static void kunit_platform_driver_test(struct kunit *test)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ *             KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ *
+ *             ctx->pdrv.probe = kunit_platform_driver_probe;
+ *             ctx->pdrv.driver.name = "kunit-platform";
+ *             ctx->pdrv.driver.owner = THIS_MODULE;
+ *
+ *             KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv));
+ *             KUNIT_EXPECT_STREQ(test, ctx->data, "test data");
+ *     }
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int platform_driver_register_kunit(struct kunit *test,
+                                  struct platform_driver *drv)
+{
+       int ret;

drivers/base/test/platform_kunit.c new file +6 -0

+
+       return kunit_add_action_or_reset(test,
+                                        (kunit_action_t *)&platform_driver_unregister,
+                                        drv);
+}
+EXPORT_SYMBOL_GPL(platform_driver_register_kunit);

Quoting Stephen Boyd (2024-04-22 16:23:58)

Quoted 1 line

drivers/base/test/platform_kunit.c +0 -0

drivers/base/test/platform_kunit.c line 168

Quoted 6 lines

drivers/base/test/platform_kunit.c +6 -0

+
+       /*
+        * Wait for the driver to probe (or at least flush out of the deferred
+        * workqueue)
+        */
+       wait_for_device_probe();

Should this be removed? I was thinking that this isn't a pure wrapper around platform_driver_register() because it has this wait call. Maybe it's better to have some other kunit API that can wait for a specific device to probe and timeout if it doesn't happen in that amount of time. That API would use the bus notifiers and look for BUS_NOTIFY_BOUND_DRIVER. Or maybe that function could setup a completion that the test can wait on.

Introduce KUnit resource wrappers around platform_driver_register(), platform_device_alloc(), and platform_device_add() so that test authors can register platform drivers/devices from their tests and have the drivers/devices automatically be unregistered when the test is done.

This makes test setup code simpler when a platform driver or platform device is needed. Add a few test cases at the same time to make sure the APIs work as intended.

Cc Brendan Higgins <[email protected]>

Cc David Gow <[email protected]>

Cc Rae Moar <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

 Documentation/dev-tools/kunit/api/index.rst   |   5 +
 .../dev-tools/kunit/api/platformdevice.rst    |  10 +
 drivers/base/test/Makefile                    |   3 +
 drivers/base/test/platform_kunit-test.c       | 140 ++++++++++++++
 drivers/base/test/platform_kunit.c            | 174 ++++++++++++++++++
 include/kunit/platform_device.h               |  15 ++
 6 files changed, 347 insertions(+)
 create mode 100644 Documentation/dev-tools/kunit/api/platformdevice.rst
 create mode 100644 drivers/base/test/platform_kunit-test.c
 create mode 100644 drivers/base/test/platform_kunit.c
 create mode 100644 include/kunit/platform_device.h

Documentation/dev-tools/kunit/api/index.rst +0 -0

index 282befa17edf..02b26f5e8750 100644

Documentation/dev-tools/kunit/api/index.rst +4 -0

 This page documents the KUnit kernel testing API. It is divided into the

@@ -36,3 +37,7 @@ Driver KUnit API
 Documentation/dev-tools/kunit/api/of.rst
 
  - Documents the KUnit device tree (OF) API
+
+Documentation/dev-tools/kunit/api/platformdevice.rst
+
+ - Documents the KUnit platform device API

Documentation/dev-tools/kunit/api/platformdevice.rst new file +10 -0

new file mode 100644
index 000000000000..b228fb6558c2
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+Platform Device API
+===================
+
+The KUnit platform device API is used to test platform devices.
+
+.. kernel-doc:: drivers/base/test/platform_kunit.c
+   :export:

drivers/base/test/Makefile +0 -0

index e321dfc7e922..740aef267fbe 100644

drivers/base/test/Makefile +1 -0

 obj-$(CONFIG_DM_KUNIT_TEST)    += root-device-test.o
 obj-$(CONFIG_DM_KUNIT_TEST)    += platform-device-test.o
+obj-$(CONFIG_DM_KUNIT_TEST)    += platform_kunit-test.o
 
 obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o
 CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN)

drivers/base/test/platform_kunit-test.c new file +140 -0

new file mode 100644
index 000000000000..ce545532d209
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for platform driver infrastructure.
+ */
+
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+#include <kunit/test.h>
+
+static const char * const kunit_devname = "kunit-platform";
+
+/*
+ * Test that platform_device_alloc_kunit() creates a platform device.
+ */
+static void platform_device_alloc_kunit_test(struct kunit *test)
+{
+       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+                       platform_device_alloc_kunit(test, kunit_devname, 1));
+}
+
+/*
+ * Test that platform_device_add_kunit() registers a platform device on the
+ * platform bus with the proper name and id.
+ */
+static void platform_device_add_kunit_test(struct kunit *test)
+{
+       struct platform_device *pdev;
+       const char *name = kunit_devname;
+       const int id = -1;
+
+       pdev = platform_device_alloc_kunit(test, name, id);
+       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+       KUNIT_EXPECT_EQ(test, 0, platform_device_add_kunit(test, pdev));
+       KUNIT_EXPECT_TRUE(test, dev_is_platform(&pdev->dev));
+       KUNIT_EXPECT_STREQ(test, pdev->name, name);
+       KUNIT_EXPECT_EQ(test, pdev->id, id);
+}
+
+/*
+ * Test that platform_device_add_kunit() called twice with the same device name
+ * and id fails the second time and properly cleans up.
+ */
+static void platform_device_add_kunit_twice_fails_test(struct kunit *test)
+{
+       struct platform_device *pdev;
+       const char *name = kunit_devname;
+       const int id = -1;
+
+       pdev = platform_device_alloc_kunit(test, name, id);
+       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+       KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev));
+
+       pdev = platform_device_alloc_kunit(test, name, id);
+       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+       KUNIT_EXPECT_NE(test, 0, platform_device_add_kunit(test, pdev));
+}
+
+/*
+ * Test suite for struct platform_device kunit APIs
+ */
+static struct kunit_case platform_device_kunit_test_cases[] = {
+       KUNIT_CASE(platform_device_alloc_kunit_test),
+       KUNIT_CASE(platform_device_add_kunit_test),
+       KUNIT_CASE(platform_device_add_kunit_twice_fails_test),
+       {}
+};
+
+static struct kunit_suite platform_device_kunit_suite = {
+       .name = "platform_device_kunit",
+       .test_cases = platform_device_kunit_test_cases,
+};
+
+struct kunit_platform_driver_test_context {
+       struct platform_driver pdrv;
+       const char *data;
+};
+
+static const char * const test_data = "test data";
+
+static inline struct kunit_platform_driver_test_context *
+to_test_context(struct platform_device *pdev)
+{
+       return container_of(to_platform_driver(pdev->dev.driver),
+                           struct kunit_platform_driver_test_context,
+                           pdrv);
+}
+
+static int kunit_platform_driver_probe(struct platform_device *pdev)
+{
+       struct kunit_platform_driver_test_context *ctx;
+
+       ctx = to_test_context(pdev);
+       ctx->data = test_data;
+
+       return 0;
+}
+
+/* Test that platform_driver_register_kunit() registers a driver that probes. */
+static void platform_driver_register_kunit_test(struct kunit *test)
+{
+       struct platform_device *pdev;
+       struct kunit_platform_driver_test_context *ctx;
+
+       ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+       pdev = platform_device_alloc_kunit(test, kunit_devname, -1);
+       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+       KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev));
+
+       ctx->pdrv.probe = kunit_platform_driver_probe;
+       ctx->pdrv.driver.name = kunit_devname;
+       ctx->pdrv.driver.owner = THIS_MODULE;
+
+       KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv));
+       KUNIT_EXPECT_STREQ(test, ctx->data, test_data);
+}
+
+static struct kunit_case platform_driver_kunit_test_cases[] = {
+       KUNIT_CASE(platform_driver_register_kunit_test),
+       {}
+};
+
+/*
+ * Test suite for struct platform_driver kunit APIs
+ */
+static struct kunit_suite platform_driver_kunit_suite = {
+       .name = "platform_driver_kunit",
+       .test_cases = platform_driver_kunit_test_cases,
+};
+
+kunit_test_suites(
+       &platform_device_kunit_suite,
+       &platform_driver_kunit_suite,
+);
+
+MODULE_LICENSE("GPL");

drivers/base/test/platform_kunit.c new file +13 -0

new file mode 100644
index 000000000000..54af6db2a6d8
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test managed platform driver
+ */
+
+#include <linux/device/driver.h>
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+#include <kunit/resource.h>
+
+/**
+ * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device

drivers/base/test/platform_kunit.c new file +23 -0

+               return NULL;
+
+       return pdev;
+}
+EXPORT_SYMBOL_GPL(platform_device_alloc_kunit);
+
+static void platform_device_add_kunit_exit(struct kunit_resource *res)
+{
+       struct platform_device *pdev = res->data;
+
+       platform_device_unregister(pdev);
+}
+
+static bool
+platform_device_alloc_kunit_match(struct kunit *test,
+                                 struct kunit_resource *res, void *match_data)
+{
+       struct platform_device *pdev = match_data;
+
+       return res->data == pdev;
+}
+
+/**

drivers/base/test/platform_kunit.c new file +19 -0

+{
+       struct kunit_resource *res;
+       int ret;
+
+       ret = platform_device_add(pdev);
+       if (ret)
+               return ret;
+
+       res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev);
+       if (res) {
+               /*
+                * Transfer the reference count of the platform device if it was
+                * allocated with platform_device_alloc_kunit(). In that case,
+                * calling platform_device_put() leads to reference count
+                * underflow because platform_device_unregister() does it for
+                * us and we call platform_device_unregister() from
+                * platform_device_add_kunit_exit().
+                *
+                * Usually callers transfer the refcount from

drivers/base/test/platform_kunit.c new file +52 -0

+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(platform_device_add_kunit);
+
+/**
+ * platform_driver_register_kunit() - Register a KUnit test managed platform driver
+ * @test: test context
+ * @drv: platform driver to register
+ *
+ * Register a test managed platform driver. This allows callers to embed the
+ * @drv in a container structure and use container_of() in the probe function
+ * to pass information to KUnit tests. It can be assumed that the driver has
+ * probed when this function returns.
+ *
+ * Example
+ *
+ * .. code-block:: c
+ *
+ *     struct kunit_test_context {
+ *             struct platform_driver pdrv;
+ *             const char *data;
+ *     };
+ *
+ *     static inline struct kunit_test_context *
+ *     to_test_context(struct platform_device *pdev)
+ *     {
+ *             return container_of(to_platform_driver(pdev->dev.driver),
+ *                                 struct kunit_test_context,
+ *                                 pdrv);
+ *     }
+ *
+ *     static int kunit_platform_driver_probe(struct platform_device *pdev)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = to_test_context(pdev);
+ *             ctx->data = "test data";
+ *
+ *             return 0;
+ *     }
+ *
+ *     static void kunit_platform_driver_test(struct kunit *test)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ *             KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ *
+ *             ctx->pdrv.probe = kunit_platform_driver_probe;
+ *             ctx->pdrv.driver.name = "kunit-platform";

drivers/base/test/platform_kunit.c new file +6 -0

+
+       return kunit_add_action_or_reset(test,
+                                        (kunit_action_t *)&platform_driver_unregister,
+                                        drv);
+}
+EXPORT_SYMBOL_GPL(platform_driver_register_kunit);

include/kunit/platform_device.h new file +15 -1

new file mode 100644
index 000000000000..28d28abf15a4
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _KUNIT_PLATFORM_DRIVER_H
+#define _KUNIT_PLATFORM_DRIVER_H
+
+struct kunit;
+struct platform_device;
+struct platform_driver;
+
+struct platform_device *
+platform_device_alloc_kunit(struct kunit *test, const char *name, int id);
+int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev);
+
+int platform_driver_register_kunit(struct kunit *test, struct platform_driver *drv);
+
+#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

--
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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240422232404.213174-6-sboyd%40kernel.org.

Quoted 24 lines

Documentation/dev-tools/kunit/api/platformdevice.rst new file +10 -0

new file mode 100644
index 000000000000..b228fb6558c2
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+Platform Device API
+===================
+
+The KUnit platform device API is used to test platform devices.
+
+.. kernel-doc:: drivers/base/test/platform_kunit.c
+   :export:

drivers/base/test/Makefile +0 -0

index e321dfc7e922..740aef267fbe 100644
@@ -1,8 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
 

drivers/base/test/Makefile line 5

Quoted 2 lines

drivers/base/test/Makefile +2 -0

+obj-$(CONFIG_KUNIT) += platform_kunit.o
+

Do we want this to be part of the kunit.ko module (and hence, probably, under lib/kunit), or to keep this as a separate module. I'm tempted, personally, to treat this as a part of KUnit, and have it be part of the same module. There are a couple of reasons for this:
- It's nice to have CONFIG_KUNIT produce only one module. If we want this to be separate, I'd be tempted to put it behind its own kconfig entry.
- The name platform_kunit.ko suggests (to me, at least) that this is the test for platform devices, not the implementation of the helper.

I probably can be persuaded otherwise if you've got a strong preference for it to stay as-is, though.

Quoted 19 lines

drivers/base/test/platform_kunit.c new file +13 -0

new file mode 100644
index 000000000000..54af6db2a6d8
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test managed platform driver
+ */
+
+#include <linux/device/driver.h>
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+#include <kunit/resource.h>
+
+/**
+ * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device

drivers/base/test/platform_kunit.c line 23

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ * @test: test context
+ * @name: device name of platform device to alloc
+ * @id: identifier of platform device to alloc.
+ *
+ * Allocate a test managed platform device. The device is put when the test completes.
+ *
+ * Return: Allocated platform device on success, NULL on failure.
+ */
+struct platform_device *
+platform_device_alloc_kunit(struct kunit *test, const char *name, int id)

I'd prefer, personally, this be named something like kunit_platform_device_alloc(), to match the existing kunit_device_register() functions.

drivers/base/test/platform_kunit.c line 31

Quoted 8 lines

drivers/base/test/platform_kunit.c new file +8 -0

+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_alloc(name, id);
+       if (!pdev)
+               return NULL;
+
+       if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev))

Alas, casting function pointers to kunit_action_t* breaks CFI. It's worth using a wrapper, which can be created with the KUNIT_DEFINE_ACTION_WRAPPER() macro, e.g.

KUNIT_DEFINE_ACTION_WRAPPER(platform_device_put_wrapper, platform_device_put, struct platform_device *);

Quoted 24 lines

drivers/base/test/platform_kunit.c new file +23 -0

+               return NULL;
+
+       return pdev;
+}
+EXPORT_SYMBOL_GPL(platform_device_alloc_kunit);
+
+static void platform_device_add_kunit_exit(struct kunit_resource *res)
+{
+       struct platform_device *pdev = res->data;
+
+       platform_device_unregister(pdev);
+}
+
+static bool
+platform_device_alloc_kunit_match(struct kunit *test,
+                                 struct kunit_resource *res, void *match_data)
+{
+       struct platform_device *pdev = match_data;
+
+       return res->data == pdev;
+}
+
+/**

drivers/base/test/platform_kunit.c line 64

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ * platform_device_add_kunit() - Register a KUnit test managed platform device
+ * @test: test context
+ * @pdev: platform device to add
+ *
+ * Register a test managed platform device. The device is unregistered when the
+ * test completes.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev)

As above, I'd lean towards naming this kunit_platform_device_add() for consistency with the other KUnit device helpers.

Quoted 19 lines

drivers/base/test/platform_kunit.c new file +19 -0

+{
+       struct kunit_resource *res;
+       int ret;
+
+       ret = platform_device_add(pdev);
+       if (ret)
+               return ret;
+
+       res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev);
+       if (res) {
+               /*
+                * Transfer the reference count of the platform device if it was
+                * allocated with platform_device_alloc_kunit(). In that case,
+                * calling platform_device_put() leads to reference count
+                * underflow because platform_device_unregister() does it for
+                * us and we call platform_device_unregister() from
+                * platform_device_add_kunit_exit().
+                *
+                * Usually callers transfer the refcount from

drivers/base/test/platform_kunit.c line 93

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+                * platform_device_alloc() to platform_device_add() and simply
+                * call platform_device_unregister() when done, but with kunit
+                * we have to keep this straight by redirecting the free
+                * routine for the resource.
+                */
+               res->free = platform_device_add_kunit_exit;
+               kunit_put_resource(res);
+       } else if (kunit_add_action_or_reset(test,
+                                            (kunit_action_t *)&platform_device_unregister,
+                                            pdev)) {

Nit: We don't want to cast directly to kunit_action_t *, as that breaks CFI. Can we use KUNIT_DEFINE_ACTION_WRAPPER()?

drivers/base/test/platform_kunit.c line 94

Quoted 1 line

drivers/base/test/platform_kunit.c new file +1 -0

+               return -ENOMEM;

Nit: This is fine, as kunit_add_action_or_reset() only returns 0 or -ENOMEM at the moment, but it could cause problems down the line if we ever want to return a different error. I don't think that's particularly likely, but it might be nicer to properly propagate the error.

Quoted 53 lines

drivers/base/test/platform_kunit.c new file +52 -0

+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(platform_device_add_kunit);
+
+/**
+ * platform_driver_register_kunit() - Register a KUnit test managed platform driver
+ * @test: test context
+ * @drv: platform driver to register
+ *
+ * Register a test managed platform driver. This allows callers to embed the
+ * @drv in a container structure and use container_of() in the probe function
+ * to pass information to KUnit tests. It can be assumed that the driver has
+ * probed when this function returns.
+ *
+ * Example
+ *
+ * .. code-block:: c
+ *
+ *     struct kunit_test_context {
+ *             struct platform_driver pdrv;
+ *             const char *data;
+ *     };
+ *
+ *     static inline struct kunit_test_context *
+ *     to_test_context(struct platform_device *pdev)
+ *     {
+ *             return container_of(to_platform_driver(pdev->dev.driver),
+ *                                 struct kunit_test_context,
+ *                                 pdrv);
+ *     }
+ *
+ *     static int kunit_platform_driver_probe(struct platform_device *pdev)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = to_test_context(pdev);
+ *             ctx->data = "test data";
+ *
+ *             return 0;
+ *     }
+ *
+ *     static void kunit_platform_driver_test(struct kunit *test)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ *             KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ *
+ *             ctx->pdrv.probe = kunit_platform_driver_probe;
+ *             ctx->pdrv.driver.name = "kunit-platform";

drivers/base/test/platform_kunit.c line 156

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ *             ctx->pdrv.driver.owner = THIS_MODULE;
+ *
+ *             KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv));
+ *             KUNIT_EXPECT_STREQ(test, ctx->data, "test data");
+ *     }
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int platform_driver_register_kunit(struct kunit *test,
+                                  struct platform_driver *drv)

As above, I'd prefer kunit_platform_driver_register()

drivers/base/test/platform_kunit.c line 168

Quoted 12 lines

drivers/base/test/platform_kunit.c new file +12 -0

+{
+       int ret;
+
+       ret = platform_driver_register(drv);
+       if (ret)
+               return ret;
+
+       /*
+        * Wait for the driver to probe (or at least flush out of the deferred
+        * workqueue)
+        */
+       wait_for_device_probe();

Personally, I don't mind if this wrapper waits here (even if it makes it less of a 'pure' wrapper), so long as we document it. Can you think of any cases where we explicitly want _not_ to wait in a test?

Quoting David Gow (2024-05-01 00:55:46)

Quoted 25 lines · Stephen Boyd

Quoted 24 lines

Documentation/dev-tools/kunit/api/platformdevice.rst new file +10 -0

new file mode 100644
index 000000000000..b228fb6558c2
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+Platform Device API
+===================
+
+The KUnit platform device API is used to test platform devices.
+
+.. kernel-doc:: drivers/base/test/platform_kunit.c
+   :export:

drivers/base/test/Makefile +0 -0

index e321dfc7e922..740aef267fbe 100644
@@ -1,8 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
 

Quoted 12 lines

drivers/base/test/Makefile line 5

Quoted 2 lines

drivers/base/test/Makefile +2 -0

+obj-$(CONFIG_KUNIT) += platform_kunit.o
+

Do we want this to be part of the kunit.ko module (and hence, probably, under lib/kunit), or to keep this as a separate module. I'm tempted, personally, to treat this as a part of KUnit, and have it be part of the same module. There are a couple of reasons for this:
- It's nice to have CONFIG_KUNIT produce only one module. If we want this to be separate, I'd be tempted to put it behind its own kconfig entry.
- The name platform_kunit.ko suggests (to me, at least) that this is the test for platform devices, not the implementation of the helper.

I was following *_kunit as "helpers" and *_test as the test. Only loosely based on the documentation that mentions to use _test or _kunit for test files. Maybe it should have _kunit_helpers postfix?

Following the single module design should I merge the tests for this code into kunit-test.c? And do the same sort of thing for clk helpers? That sounds like it won't scale very well if everything is in one module.

Shouldn't the wrapper code for subsystems live in those subsystems like drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should be moved out to drivers/base/? lib/kunit can stay focused on providing pure kunit code then.

Quoted 52 lines

I probably can be persuaded otherwise if you've got a strong preference for it to stay as-is, though.

Quoted 19 lines

drivers/base/test/platform_kunit.c new file +13 -0

new file mode 100644
index 000000000000..54af6db2a6d8
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test managed platform driver
+ */
+
+#include <linux/device/driver.h>
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+#include <kunit/resource.h>
+
+/**
+ * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device

drivers/base/test/platform_kunit.c line 23

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ * @test: test context
+ * @name: device name of platform device to alloc
+ * @id: identifier of platform device to alloc.
+ *
+ * Allocate a test managed platform device. The device is put when the test completes.
+ *
+ * Return: Allocated platform device on success, NULL on failure.
+ */
+struct platform_device *
+platform_device_alloc_kunit(struct kunit *test, const char *name, int id)

I'd prefer, personally, this be named something like kunit_platform_device_alloc(), to match the existing kunit_device_register() functions.

drivers/base/test/platform_kunit.c line 31

Quoted 8 lines

drivers/base/test/platform_kunit.c new file +8 -0

+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_alloc(name, id);
+       if (!pdev)
+               return NULL;
+
+       if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev))

Alas, casting function pointers to kunit_action_t* breaks CFI. It's worth using a wrapper, which can be created with the KUNIT_DEFINE_ACTION_WRAPPER() macro, e.g.

Quoted 2 lines

KUNIT_DEFINE_ACTION_WRAPPER(platform_device_put_wrapper, platform_device_put, struct platform_device *);

Thanks. I missed that.

Quoted 71 lines

Quoted 24 lines

drivers/base/test/platform_kunit.c new file +23 -0

+               return NULL;
+
+       return pdev;
+}
+EXPORT_SYMBOL_GPL(platform_device_alloc_kunit);
+
+static void platform_device_add_kunit_exit(struct kunit_resource *res)
+{
+       struct platform_device *pdev = res->data;
+
+       platform_device_unregister(pdev);
+}
+
+static bool
+platform_device_alloc_kunit_match(struct kunit *test,
+                                 struct kunit_resource *res, void *match_data)
+{
+       struct platform_device *pdev = match_data;
+
+       return res->data == pdev;
+}
+
+/**

drivers/base/test/platform_kunit.c line 64

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ * platform_device_add_kunit() - Register a KUnit test managed platform device
+ * @test: test context
+ * @pdev: platform device to add
+ *
+ * Register a test managed platform device. The device is unregistered when the
+ * test completes.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev)

As above, I'd lean towards naming this kunit_platform_device_add() for consistency with the other KUnit device helpers.

Quoted 19 lines

drivers/base/test/platform_kunit.c new file +19 -0

+{
+       struct kunit_resource *res;
+       int ret;
+
+       ret = platform_device_add(pdev);
+       if (ret)
+               return ret;
+
+       res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev);
+       if (res) {
+               /*
+                * Transfer the reference count of the platform device if it was
+                * allocated with platform_device_alloc_kunit(). In that case,
+                * calling platform_device_put() leads to reference count
+                * underflow because platform_device_unregister() does it for
+                * us and we call platform_device_unregister() from
+                * platform_device_add_kunit_exit().
+                *
+                * Usually callers transfer the refcount from

drivers/base/test/platform_kunit.c line 93

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+                * platform_device_alloc() to platform_device_add() and simply
+                * call platform_device_unregister() when done, but with kunit
+                * we have to keep this straight by redirecting the free
+                * routine for the resource.
+                */
+               res->free = platform_device_add_kunit_exit;
+               kunit_put_resource(res);
+       } else if (kunit_add_action_or_reset(test,
+                                            (kunit_action_t *)&platform_device_unregister,
+                                            pdev)) {

Nit: We don't want to cast directly to kunit_action_t *, as that breaks CFI. Can we use KUNIT_DEFINE_ACTION_WRAPPER()?

Quoted 7 lines

drivers/base/test/platform_kunit.c line 94

Quoted 1 line

drivers/base/test/platform_kunit.c new file +1 -0

+               return -ENOMEM;

Nit: This is fine, as kunit_add_action_or_reset() only returns 0 or -ENOMEM at the moment, but it could cause problems down the line if we ever want to return a different error. I don't think that's particularly likely, but it might be nicer to properly propagate the error.

I will propagate the return value.

Quoted 66 lines

Quoted 53 lines

drivers/base/test/platform_kunit.c new file +52 -0

+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(platform_device_add_kunit);
+
+/**
+ * platform_driver_register_kunit() - Register a KUnit test managed platform driver
+ * @test: test context
+ * @drv: platform driver to register
+ *
+ * Register a test managed platform driver. This allows callers to embed the
+ * @drv in a container structure and use container_of() in the probe function
+ * to pass information to KUnit tests. It can be assumed that the driver has
+ * probed when this function returns.
+ *
+ * Example
+ *
+ * .. code-block:: c
+ *
+ *     struct kunit_test_context {
+ *             struct platform_driver pdrv;
+ *             const char *data;
+ *     };
+ *
+ *     static inline struct kunit_test_context *
+ *     to_test_context(struct platform_device *pdev)
+ *     {
+ *             return container_of(to_platform_driver(pdev->dev.driver),
+ *                                 struct kunit_test_context,
+ *                                 pdrv);
+ *     }
+ *
+ *     static int kunit_platform_driver_probe(struct platform_device *pdev)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = to_test_context(pdev);
+ *             ctx->data = "test data";
+ *
+ *             return 0;
+ *     }
+ *
+ *     static void kunit_platform_driver_test(struct kunit *test)
+ *     {
+ *             struct kunit_test_context *ctx;
+ *
+ *             ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ *             KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ *
+ *             ctx->pdrv.probe = kunit_platform_driver_probe;
+ *             ctx->pdrv.driver.name = "kunit-platform";

drivers/base/test/platform_kunit.c line 156

Quoted 10 lines

drivers/base/test/platform_kunit.c new file +10 -0

+ *             ctx->pdrv.driver.owner = THIS_MODULE;
+ *
+ *             KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv));
+ *             KUNIT_EXPECT_STREQ(test, ctx->data, "test data");
+ *     }
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int platform_driver_register_kunit(struct kunit *test,
+                                  struct platform_driver *drv)

As above, I'd prefer kunit_platform_driver_register()

Quoted 17 lines

drivers/base/test/platform_kunit.c line 168

Quoted 12 lines

drivers/base/test/platform_kunit.c new file +12 -0

+{
+       int ret;
+
+       ret = platform_driver_register(drv);
+       if (ret)
+               return ret;
+
+       /*
+        * Wait for the driver to probe (or at least flush out of the deferred
+        * workqueue)
+        */
+       wait_for_device_probe();

Personally, I don't mind if this wrapper waits here (even if it makes it less of a 'pure' wrapper), so long as we document it. Can you think of any cases where we explicitly want _not_ to wait in a test?

I don't like it because it's not deterministic. The function doesn't take any struct device to wait for. I've already written the code to use a completion, and it works well enough so I'll just do that. Then we don't have to worry if this API goes away, or that it doesn't actually determine if the driver has probed the device.

Quoted 40 lines

Quoting David Gow (2024-05-01 00:55:46)

Quoted 25 lines · Stephen Boyd

Quoted 24 lines

Documentation/dev-tools/kunit/api/platformdevice.rst new file +10 -0

new file mode 100644
index 000000000000..b228fb6558c2
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===================
+Platform Device API
+===================
+
+The KUnit platform device API is used to test platform devices.
+
+.. kernel-doc:: drivers/base/test/platform_kunit.c
+   :export:

drivers/base/test/Makefile +0 -0

index e321dfc7e922..740aef267fbe 100644
@@ -1,8 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
 

Quoted 12 lines

drivers/base/test/Makefile line 5

Quoted 2 lines

drivers/base/test/Makefile +2 -0

+obj-$(CONFIG_KUNIT) += platform_kunit.o
+

Do we want this to be part of the kunit.ko module (and hence, probably, under lib/kunit), or to keep this as a separate module. I'm tempted, personally, to treat this as a part of KUnit, and have it be part of the same module. There are a couple of reasons for this:
- It's nice to have CONFIG_KUNIT produce only one module. If we want this to be separate, I'd be tempted to put it behind its own kconfig entry.
- The name platform_kunit.ko suggests (to me, at least) that this is the test for platform devices, not the implementation of the helper.

Quoted 3 lines

I was following *_kunit as "helpers" and *_test as the test. Only loosely based on the documentation that mentions to use _test or _kunit for test files. Maybe it should have _kunit_helpers postfix?

Yeah, the style guide currently suggests that *_test is the default for tests, but that _kunit may also be used for tests if _test is already used for non-KUnit tests:
https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-names

DRM has drm_kunit_helpers, so _kunit_helpers seems like a good suffix to settle on.

Quoted 4 lines

Following the single module design should I merge the tests for this code into kunit-test.c? And do the same sort of thing for clk helpers? That sounds like it won't scale very well if everything is in one module.

I don't think it's as important that the tests live in the same module. It's nice from an ergonomic point-of-view to only have to modprobe the one thing, but we've already let that ship sail somewhat with string-stream-test.

Either way, splitting up kunit-test.c is something we'll almost certainly want to do at some point, and we can always put them into the same module even if they're different source files if we have to.

Quoted 6 lines

Shouldn't the wrapper code for subsystems live in those subsystems like drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should be moved out to drivers/base/? lib/kunit can stay focused on providing pure kunit code then.

I tend to agree that wrapper code for subsystems should live in those subsystems, especially if the subsystems are relatively self-contained (i.e., the helpers are used to test that subsystem itself, rather than exported for other parts of the kernel to use to test interactions with said subsystem). For 'core' parts of the kernel, I think it makes it easier to make these obviously part of KUnit (e.g. kunit_kzalloc() is easier to have within KUnit, rather than as a part of the allocators).

The struct device wrappers have the problem that they rely on the kunit_bus being registered, which is currently done when the kunit module is loaded. So it hooks more deeply into KUnit than is comfortable to do from drivers/base. So we've treated it as a 'core' part of the kernel.

Ultimately, it's a grey area, so I can live with this going either way, depending on the actual helpers, so long as we don't end up with lots of half-in/half-out helpers, which behave a bit like both. (For example, at the moment, helpers which live outside lib/kunit are documented and have headers in the respective subsystems' directories.)

FWIW, my gut feeling for what's "most consistent" with what we've done so far is:
1. platform_device helpers should live alongside the current managed device stuff, which is currently in lib/kunit
2. clk helpers should probably live in clk
3. of/of_overlay sits a bit in the middle, but having thought more about it, it'd probably lean towards having it be part of 'of', not 'kunit.

But all of this is, to some extent, just bikeshedding, so as long as we pick somewhere to put them, and don't mix things up too much, I don't think it matters exactly what side of this fuzzy line they end up on.

drivers/base/test/platform_kunit.c new file +0 -0

new file mode 100644
index 000000000000..54af6db2a6d8
@@ -0,0 +1,174 @@

drivers/base/test/platform_kunit.c new file +42 -0

+struct platform_device *
+platform_device_alloc_kunit(struct kunit *test, const char *name, int id)
+{
+       struct platform_device *pdev;
+
+       pdev = platform_device_alloc(name, id);
+       if (!pdev)
+               return NULL;
+
+       if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev))
+               return NULL;
+
+       return pdev;
+}
+EXPORT_SYMBOL_GPL(platform_device_alloc_kunit);
+
+static void platform_device_add_kunit_exit(struct kunit_resource *res)
+{
+       struct platform_device *pdev = res->data;
+
+       platform_device_unregister(pdev);
+}
+
+static bool
+platform_device_alloc_kunit_match(struct kunit *test,
+                                 struct kunit_resource *res, void *match_data)
+{
+       struct platform_device *pdev = match_data;
+
+       return res->data == pdev;
+}
+
+/**
+ * platform_device_add_kunit() - Register a KUnit test managed platform device
+ * @test: test context
+ * @pdev: platform device to add
+ *
+ * Register a test managed platform device. The device is unregistered when the
+ * test completes.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */

Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml new file +17 -0

+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-consumer {
+      compatible = "test,clk-fixed-rate";
+      clocks = <&fixed_clk>;
+    };
+...

This patch series adds unit tests for the clk fixed rate basic type and the clk registration functions that use struct clk_parent_data. To get there, we add support for loading device tree overlays onto the live DTB along with probing platform drivers to bind to device nodes in the overlays. With this series, we're able to exercise some of the code in the common clk framework that uses devicetree lookups to find parents and the fixed rate clk code that scans device tree directly and creates clks. Please review.

I Cced everyone to all the patches so they get the full context. I'm hoping I can take the whole pile through the clk tree as they all build upon each other. Or the DT part can be merged through the DT tree to reduce the dependencies.

Changes from v3 (https://lore.kernel.org/r/[email protected]):
* No longer depend on Frank's series[1] because it was merged upstream[2]
* Use kunit_add_action_or_reset() to shorten code
* Skip tests properly when CONFIG_OF_OVERLAY isn't set

Changes from v2 (https://lore.kernel.org/r/[email protected]):
* Overlays don't depend on __symbols__ node
* Depend on Frank's always create root node if CONFIG_OF series[1]
* Added kernel-doc to KUnit API doc
* Fixed some kernel-doc on functions
* More test cases for fixed rate clk

Changes from v1 (https://lore.kernel.org/r/[email protected]):
* Don't depend on UML, use unittest data approach to attach nodes
* Introduce overlay loading API for KUnit
* Move platform_device KUnit code to drivers/base/test
* Use #define macros for constants shared between unit tests and overlays
* Settle on "test" as a vendor prefix
* Make KUnit wrappers have "_kunit" postfix

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]

[15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47
[15:18:51] Expected pdev is not null, but is
[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c

[15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99
[15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_platform_device

[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c [15:18:51] note: kunit_try_catch[698] exited with irqs disabled
[15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77
[15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_cleanup

Stephen Boyd (10):
of: Add test managed wrappers for of_overlay_apply()/of_node_put()
dt-bindings: vendor-prefixes: Add "test" vendor for KUnit and friends
dt-bindings: test: Add KUnit empty node binding
of: Add a KUnit test for overlays and test managed APIs
platform: Add test managed platform_device/driver APIs
dt-bindings: kunit: Add fixed rate clk consumer test
clk: Add test managed clk provider/consumer APIs
clk: Add KUnit tests for clk fixed rate basic type
dt-bindings: clk: Add KUnit clk_parent_data test
clk: Add KUnit tests for clks registered with struct clk_parent_data

 Documentation/dev-tools/kunit/api/clk.rst     |  10 +
 Documentation/dev-tools/kunit/api/index.rst   |  21 +
 Documentation/dev-tools/kunit/api/of.rst      |  13 +
 .../dev-tools/kunit/api/platformdevice.rst    |  10 +
 .../bindings/clock/test,clk-parent-data.yaml  |  47 ++
 .../bindings/test/test,clk-fixed-rate.yaml    |  35 ++
 .../devicetree/bindings/test/test,empty.yaml  |  30 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/base/test/Makefile                    |   3 +
 drivers/base/test/platform_kunit-test.c       | 140 ++++++
 drivers/base/test/platform_kunit.c            | 174 +++++++
 drivers/clk/.kunitconfig                      |   2 +
 drivers/clk/Kconfig                           |   9 +
 drivers/clk/Makefile                          |   9 +-
 drivers/clk/clk-fixed-rate_test.c             | 377 +++++++++++++++
 drivers/clk/clk-fixed-rate_test.h             |   8 +
 drivers/clk/clk_kunit.c                       | 198 ++++++++
 drivers/clk/clk_parent_data_test.h            |  10 +
 drivers/clk/clk_test.c                        | 451 +++++++++++++++++-
 drivers/clk/kunit_clk_fixed_rate_test.dtso    |  19 +
 drivers/clk/kunit_clk_parent_data_test.dtso   |  28 ++
 drivers/of/.kunitconfig                       |   1 +
 drivers/of/Kconfig                            |  10 +
 drivers/of/Makefile                           |   2 +
 drivers/of/kunit_overlay_test.dtso            |   9 +
 drivers/of/of_kunit.c                         |  99 ++++
 drivers/of/overlay_test.c                     | 115 +++++
 include/kunit/clk.h                           |  28 ++
 include/kunit/of.h                            |  94 ++++
 include/kunit/platform_device.h               |  15 +
 30 files changed, 1967 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/dev-tools/kunit/api/clk.rst
 create mode 100644 Documentation/dev-tools/kunit/api/of.rst
 create mode 100644 Documentation/dev-tools/kunit/api/platformdevice.rst
 create mode 100644 Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml
 create mode 100644 Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
 create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml
 create mode 100644 drivers/base/test/platform_kunit-test.c
 create mode 100644 drivers/base/test/platform_kunit.c
 create mode 100644 drivers/clk/clk-fixed-rate_test.c
 create mode 100644 drivers/clk/clk-fixed-rate_test.h
 create mode 100644 drivers/clk/clk_kunit.c
 create mode 100644 drivers/clk/clk_parent_data_test.h
 create mode 100644 drivers/clk/kunit_clk_fixed_rate_test.dtso
 create mode 100644 drivers/clk/kunit_clk_parent_data_test.dtso
 create mode 100644 drivers/of/kunit_overlay_test.dtso
 create mode 100644 drivers/of/of_kunit.c
 create mode 100644 drivers/of/overlay_test.c
 create mode 100644 include/kunit/clk.h
 create mode 100644 include/kunit/of.h
 create mode 100644 include/kunit/platform_device.h

base-commit: 4cece764965020c22cff7665b18a012006359095 --
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git

Thanks very much. I'm about halfway through reviewing these, and I like them a lot so far.

Most of my thoughts are just naming ideas. I fear some of them may be the reverse of previous suggestions, as we've since landed the KUnit device wrappers in include/kunit/device.h, which we decided would live as part of KUnit, not as part of the device infrastructure. I don't enormously mind if we make the opposite decision for these, though it does seem a bit inconsistent if we do 'devices' differently from 'platform_devices'. Thoughts?

The other thing I've noted so far is that the of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup tests fail (and BUG() with a NULL pointer) on powerpc:

Quoted 3 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47
[15:18:51] Expected pdev is not null, but is
[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c

Quoted 3 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99
[15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_platform_device

Quoted 5 lines

[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c [15:18:51] note: kunit_try_catch[698] exited with irqs disabled
[15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77
[15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_cleanup

I've not had a chance to dig into it any further, yet, but it appears to work on all of the other architectures I tried.

Quoting David Gow (2024-05-01 01:08:11)

Quoted 7 lines

The other thing I've noted so far is that the of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup tests fail (and BUG() with a NULL pointer) on powerpc:

Quoted 3 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47
[15:18:51] Expected pdev is not null, but is
[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c

This seems to be because pdev is NULL and we call put_device(&pdev->dev) on it. We could be nicer and have an 'if (pdev)' check there. I wonder if that fixes the other two below?

---8<---

drivers/of/overlay_test.c +0 -0

index 223e5a5c23c5..85cfbe6bb132 100644

drivers/of/overlay_test.c +0 -0

 static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
 

Quoted 10 lines

Quoted 9 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99
[15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_platform_device

[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c [15:18:51] note: kunit_try_catch[698] exited with irqs disabled
[15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77
[15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_cleanup

Quoted 2 lines

I've not had a chance to dig into it any further, yet, but it appears to work on all of the other architectures I tried.

Cool. I don't know why powerpc doesn't make devices. Maybe it has a similar design to sparc to create resources. I'll check it out.

Quoting Stephen Boyd (2024-05-02 18:27:42)

Quoted 19 lines

Quoting David Gow (2024-05-01 01:08:11)

Quoted 7 lines

The other thing I've noted so far is that the of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup tests fail (and BUG() with a NULL pointer) on powerpc:

Quoted 3 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47
[15:18:51] Expected pdev is not null, but is
[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c

This seems to be because pdev is NULL and we call put_device(&pdev->dev) on it. We could be nicer and have an 'if (pdev)' check there. I wonder if that fixes the other two below?

---8<---

drivers/of/overlay_test.c +0 -0

index 223e5a5c23c5..85cfbe6bb132 100644

drivers/of/overlay_test.c static void of_overlay_apply_kunit_cleanup(struct kunit *test) · line 97

Quoted 7 lines

drivers/of/overlay_test.c +2 -1

@@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
        dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible,
                              of_overlay_bus_match_compatible);
        KUNIT_EXPECT_PTR_EQ(test, NULL, dev);
-       put_device(dev);
+       if (dev)
+               put_device(dev);
 }

This last hunk isn't needed.

Quoted 13 lines

drivers/of/overlay_test.c +0 -0

 static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
 

Quoted 10 lines

Quoted 9 lines

[15:18:51] # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99
[15:18:51] # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_platform_device

[15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c [15:18:51] note: kunit_try_catch[698] exited with irqs disabled
[15:18:51] # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77
[15:18:51] # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4
[15:18:51] [FAILED] of_overlay_apply_kunit_cleanup

Quoted 6 lines

Quoted 2 lines

I've not had a chance to dig into it any further, yet, but it appears to work on all of the other architectures I tried.

Cool. I don't know why powerpc doesn't make devices. Maybe it has a similar design to sparc to create resources. I'll check it out.

powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that in of_platform_default_populate_init() then the overlays can be applied.

---8<----

drivers/of/platform.c +0 -0

index 389d4ea6bfc1..fa7b439e9402 100644

drivers/of/platform.c +1 -0

+               of_node_put(node);
        } else {
                /*
                 * Handle certain compatibles explicitly, since we don't want to create

I'm guessing this is wrong though, because I see bunch of powerpc specific code calling of_platform_bus_probe() which will set the flag on the actual platform bus nodes. Maybe we should just allow overlays to create devices at the root node regardless? Of course, the flag doc says "platform bus created for children" and if we never populated the root then that isn't entirely accurate.

Rob, can you point me in the right direction? Do we need to use simple-bus in the test overlays and teach overlay code to populate that bus?

Quoted 13 lines

powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that in of_platform_default_populate_init() then the overlays can be applied.

---8<----

drivers/of/platform.c +0 -0

index 389d4ea6bfc1..fa7b439e9402 100644
@@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
                                of_platform_device_create(node, buf, NULL);
                }
 

drivers/of/platform.c static int __init of_platform_default_populate_init(void) · line 570

Quoted 3 lines

drivers/of/platform.c +3 -0

+               node = of_find_node_by_path("/");
+               if (node)
+                       of_node_set_flag(node, OF_POPULATED_BUS);

I think you want to do this in of_platform_bus_probe() instead to mirror of_platform_populate(). These are supposed to be the same except that 'populate' only creates devices for nodes with compatible while 'probe' will create devices for all child nodes. Looks like we are missing some devlink stuff too. There may have been some issue for PPC with it.

Quoted 12 lines

drivers/of/platform.c +1 -0

+               of_node_put(node);
        } else {
                /*
                 * Handle certain compatibles explicitly, since we don't want to create

I'm guessing this is wrong though, because I see bunch of powerpc specific code calling of_platform_bus_probe() which will set the flag on the actual platform bus nodes. Maybe we should just allow overlays to create devices at the root node regardless? Of course, the flag doc says "platform bus created for children" and if we never populated the root then that isn't entirely accurate.

Quoted 2 lines

Rob, can you point me in the right direction? Do we need to use simple-bus in the test overlays and teach overlay code to populate that bus?

Overlays adding things to the root node might be suspect, but probably there are some valid reasons to do so.

If simple-bus makes sense here,
then yes, you should use it. But if what's on it is not MMIO devices, don't. That's a warning in the schema now.

Quoting Rob Herring (2024-05-15 06:06:09)

Quoted 14 lines · Stephen Boyd

Quoted 13 lines

powerpc doesn't mark the root node with OF_POPULATED_BUS. If I set that in of_platform_default_populate_init() then the overlays can be applied.

---8<----

drivers/of/platform.c +0 -0

index 389d4ea6bfc1..fa7b439e9402 100644
@@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
                                of_platform_device_create(node, buf, NULL);
                }
 

Quoted 10 lines

drivers/of/platform.c static int __init of_platform_default_populate_init(void) · line 570

Quoted 3 lines

drivers/of/platform.c +3 -0

+               node = of_find_node_by_path("/");
+               if (node)
+                       of_node_set_flag(node, OF_POPULATED_BUS);

I think you want to do this in of_platform_bus_probe() instead to mirror of_platform_populate(). These are supposed to be the same except that 'populate' only creates devices for nodes with compatible while 'probe' will create devices for all child nodes. Looks like we are missing some devlink stuff too. There may have been some issue for PPC with it.

Got it. So this patch?

---8<---

drivers/of/platform.c +0 -0

index 389d4ea6bfc1..acecefcfdba7 100644

drivers/of/platform.c +0 -0

                if (!of_match_node(matches, child))
                        continue;
                rc = of_platform_bus_create(child, matches, NULL, parent, false);

This doesn't work though. I see that prom_init() is called, which constructs a DTB and flattens it to be unflattened by unflatten_device_tree(). The powerpc machine type used by qemu is PLATFORM_PSERIES_LPAR. It looks like it never calls of_platform_bus_probe() from the pseries platform code.

What about skipping the OF_POPULATED_BUS check, or skipping the check when the parent is the root node? This is the if condition that's giving the headache.

Quoted 5 lines

drivers/of/platform.c +0 -0

index 389d4ea6bfc1..acecefcfdba7 100644
@@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root,

drivers/of/platform.c int of_platform_bus_probe(struct device_node *root, · line 424

Quoted 4 lines

drivers/of/platform.c +1 -0

        if (of_match_node(matches, root)) {
                rc = of_platform_bus_create(root, matches, NULL, parent, false);
        } else for_each_child_of_node(root, child) {
+               of_node_set_flag(root, OF_POPULATED_BUS);

No, the same spot as of_platform_populate has it. I guess this would be the same, but no reason to do this in the for_each_child_of_node loop...

Quoted 6 lines

drivers/of/platform.c +0 -0

                if (!of_match_node(matches, child))
                        continue;
                rc = of_platform_bus_create(child, matches, NULL, parent, false);

Quoted 5 lines

This doesn't work though. I see that prom_init() is called, which constructs a DTB and flattens it to be unflattened by unflatten_device_tree(). The powerpc machine type used by qemu is PLATFORM_PSERIES_LPAR. It looks like it never calls of_platform_bus_probe() from the pseries platform code.

Huh. Maybe pseries doesn't have any platform devices?

Ideally, we'd still do it in of_platform_default_populate_init(), but if you look at the history, you'll see that broke some PPC boards (damn initcall ordering).

Quoted 3 lines

What about skipping the OF_POPULATED_BUS check, or skipping the check when the parent is the root node? This is the if condition that's giving the headache.

I don't think we should just remove it, but a root node check seems fine.