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]
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
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
diff --git a/Documentation/devicetree/bindings/test/test,empty.yaml b/Documentation/devicetree/bindings/test/test,empty.yaml
new file mode 100644
index 000000000000..20dc83b15bbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/test/test,empty.yaml
@@ -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#
+
+title: Empty node
+
+maintainers:
+ - David Gow <[email protected]>
+ - Brendan Higgins <[email protected]>
+
+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
Describe a binding for a device that consumes a fixed rate clk in DT so
that a KUnit test can get the clk registered by of_fixed_clk_setup() and
test that it is setup 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]>
---
.../bindings/test/test,clk-fixed-rate.yaml | 35 +++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
diff --git a/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml b/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
new file mode 100644
index 000000000000..b9f58cba944c
--- /dev/null
+++ b/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/test/test,clk-fixed-rate.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: KUnit clk fixed rate test clk consumer
+
+maintainers:
+ - Stephen Boyd <[email protected]>
+
+description:
+ A clk consumer of a fixed rate clk used to test the fixed rate clk
+ implementation.
+
+properties:
+ compatible:
+ const: test,clk-fixed-rate
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - clocks
+
+additionalProperties: false
+
+examples:
+ - |
+ clock-consumer {
+ compatible = "test,clk-fixed-rate";
+ clocks = <&fixed_clk>;
+ };
+...
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
Unit tests are more ergonomic and simpler to understand if they don't
have to hoist a bunch of code into the test harness init and exit
functions. Add some test managed wrappers for the clk APIs so that clk
unit tests can write more code in the actual test and less code in the
harness.
Only add APIs that are used for now. More wrappers can be added in the
future as necessary.
Cc: Brendan Higgins <[email protected]>
Cc: David Gow <[email protected]>
Cc: Rae Moar <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
Documentation/dev-tools/kunit/api/clk.rst | 10 +
Documentation/dev-tools/kunit/api/index.rst | 5 +
drivers/clk/Makefile | 5 +
drivers/clk/clk_kunit.c | 198 ++++++++++++++++++++
include/kunit/clk.h | 28 +++
5 files changed, 246 insertions(+)
create mode 100644 Documentation/dev-tools/kunit/api/clk.rst
create mode 100644 drivers/clk/clk_kunit.c
create mode 100644 include/kunit/clk.h
diff --git a/Documentation/dev-tools/kunit/api/clk.rst b/Documentation/dev-tools/kunit/api/clk.rst
new file mode 100644
index 000000000000..badd87a35f9e
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/clk.rst
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========
+Clk API
+========
+
+The KUnit clk API is used to test clk providers and clk consumers.
+
+.. kernel-doc:: drivers/clk/clk_kunit.c
+ :export:
diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
index 02b26f5e8750..5cdb552a0808 100644
--- a/Documentation/dev-tools/kunit/api/index.rst
+++ b/Documentation/dev-tools/kunit/api/index.rst
@@ -9,6 +9,7 @@ API Reference
test
resource
functionredirection
+ clk
of
platformdevice
@@ -34,6 +35,10 @@ Documentation/dev-tools/kunit/api/functionredirection.rst
Driver KUnit API
================
+Documentation/dev-tools/kunit/api/clk.rst
+
+ - Documents the KUnit clk API
+
Documentation/dev-tools/kunit/api/of.rst
- Documents the KUnit device tree (OF) API
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 14fa8d4ecc1f..1fd74658a801 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -18,6 +18,11 @@ ifeq ($(CONFIG_OF), y)
obj-$(CONFIG_COMMON_CLK) += clk-conf.o
endif
+# KUnit specific helpers
+ifeq ($(CONFIG_COMMON_CLK), y)
+obj-$(CONFIG_KUNIT) += clk_kunit.o
+endif
+
# hardware specific clock types
# please keep this section sorted lexicographically by file path name
obj-$(CONFIG_COMMON_CLK_APPLE_NCO) += clk-apple-nco.o
diff --git a/drivers/clk/clk_kunit.c b/drivers/clk/clk_kunit.c
new file mode 100644
index 000000000000..ba14f7f5908d
--- /dev/null
+++ b/drivers/clk/clk_kunit.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit helpers for clk providers and consumers
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include <kunit/clk.h>
+#include <kunit/resource.h>
+
+/**
+ * clk_prepare_enable_kunit() - Test managed clk_prepare_enable()
+ * @test: The test context
+ * @clk: clk to prepare and enable
+ *
+ * Return: 0 on success, or negative errno on failure.
+ */
+int clk_prepare_enable_kunit(struct kunit *test, struct clk *clk)
+{
+ int ret;
+
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ return ret;
+
+ return kunit_add_action_or_reset(test,
+ (kunit_action_t *)&clk_disable_unprepare,
+ clk);
+}
+EXPORT_SYMBOL_GPL(clk_prepare_enable_kunit);
+
+static struct clk *__clk_get_kunit(struct kunit *test, struct clk *clk)
+{
+ int ret;
+
+ if (IS_ERR(clk))
+ return clk;
+
+ ret = kunit_add_action_or_reset(test, (kunit_action_t *)&clk_put, clk);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return clk;
+}
+
+/**
+ * clk_get_kunit() - Test managed clk_get()
+ * @test: The test context
+ * @dev: device for clock "consumer"
+ * @con_id: clock consumer ID
+ *
+ * Just like clk_get(), except the clk is managed by the test case and is
+ * automatically put with clk_put() after the test case concludes.
+ *
+ * Return: new clk consumer or ERR_PTR on failure.
+ */
+struct clk *
+clk_get_kunit(struct kunit *test, struct device *dev, const char *con_id)
+{
+ struct clk *clk;
+
+ clk = clk_get(dev, con_id);
+
+ return __clk_get_kunit(test, clk);
+}
+EXPORT_SYMBOL_GPL(clk_get_kunit);
+
+/**
+ * of_clk_get_kunit() - Test managed of_clk_get()
+ * @test: The test context
+ * @np: device_node for clock "consumer"
+ * @index: index in 'clocks' property of @np
+ *
+ * Just like of_clk_get(), except the clk is managed by the test case and is
+ * automatically put with clk_put() after the test case concludes.
+ *
+ * Return: new clk consumer or ERR_PTR on failure.
+ */
+struct clk *
+of_clk_get_kunit(struct kunit *test, struct device_node *np, int index)
+{
+ struct clk *clk;
+
+ clk = of_clk_get(np, index);
+
+ return __clk_get_kunit(test, clk);
+}
+EXPORT_SYMBOL_GPL(of_clk_get_kunit);
+
+/**
+ * clk_hw_get_clk_kunit() - Test managed clk_hw_get_clk()
+ * @test: The test context
+ * @hw: clk_hw associated with the clk being consumed
+ * @con_id: connection ID string on device
+ *
+ * Just like clk_hw_get_clk(), except the clk is managed by the test case and
+ * is automatically put with clk_put() after the test case concludes.
+ *
+ * Return: new clk consumer or ERR_PTR on failure.
+ */
+struct clk *
+clk_hw_get_clk_kunit(struct kunit *test, struct clk_hw *hw, const char *con_id)
+{
+ struct clk *clk;
+
+ clk = clk_hw_get_clk(hw, con_id);
+
+ return __clk_get_kunit(test, clk);
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_clk_kunit);
+
+/**
+ * clk_hw_get_clk_prepared_enabled_kunit() - Test managed clk_hw_get_clk() + clk_prepare_enable()
+ * @test: The test context
+ * @hw: clk_hw associated with the clk being consumed
+ * @con_id: connection ID string on device
+ *
+ * Just like
+ *
+ * .. code-block:: c
+ *
+ * struct clk *clk = clk_hw_get_clk(...);
+ * clk_prepare_enable(clk);
+ *
+ * except the clk is managed by the test case and is automatically disabled and
+ * unprepared with clk_disable_unprepare() and put with clk_put() after the
+ * test case concludes.
+ *
+ * Return: new clk consumer that is prepared and enabled or ERR_PTR on failure.
+ */
+struct clk *
+clk_hw_get_clk_prepared_enabled_kunit(struct kunit *test, struct clk_hw *hw,
+ const char *con_id)
+{
+ int ret;
+ struct clk *clk;
+
+ clk = clk_hw_get_clk_kunit(test, hw, con_id);
+ if (IS_ERR(clk))
+ return clk;
+
+ ret = clk_prepare_enable_kunit(test, clk);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return clk;
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_clk_prepared_enabled_kunit);
+
+/**
+ * clk_hw_register_kunit() - Test managed clk_hw_register()
+ * @test: The test context
+ * @dev: device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * Just like clk_hw_register(), except the clk registration is managed by the
+ * test case and is automatically unregistered after the test case concludes.
+ *
+ * Return: 0 on success or a negative errno value on failure.
+ */
+int clk_hw_register_kunit(struct kunit *test, struct device *dev, struct clk_hw *hw)
+{
+ int ret;
+
+ ret = clk_hw_register(dev, hw);
+ if (ret)
+ return ret;
+
+ return kunit_add_action_or_reset(test, (kunit_action_t *)&clk_hw_unregister, hw);
+}
+EXPORT_SYMBOL_GPL(clk_hw_register_kunit);
+
+/**
+ * of_clk_hw_register_kunit() - Test managed of_clk_hw_register()
+ * @test: The test context
+ * @node: device_node of device that is registering this clock
+ * @hw: link to hardware-specific clock data
+ *
+ * Just like of_clk_hw_register(), except the clk registration is managed by
+ * the test case and is automatically unregistered after the test case
+ * concludes.
+ *
+ * Return: 0 on success or a negative errno value on failure.
+ */
+int of_clk_hw_register_kunit(struct kunit *test, struct device_node *node, struct clk_hw *hw)
+{
+ int ret;
+
+ ret = of_clk_hw_register(node, hw);
+ if (ret)
+ return ret;
+
+ return kunit_add_action_or_reset(test, (kunit_action_t *)&clk_hw_unregister, hw);
+}
+EXPORT_SYMBOL_GPL(of_clk_hw_register_kunit);
diff --git a/include/kunit/clk.h b/include/kunit/clk.h
new file mode 100644
index 000000000000..73bc99cefe7b
--- /dev/null
+++ b/include/kunit/clk.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _CLK_KUNIT_H
+#define _CLK_KUNIT_H
+
+struct clk;
+struct clk_hw;
+struct device;
+struct device_node;
+struct kunit;
+
+struct clk *
+clk_get_kunit(struct kunit *test, struct device *dev, const char *con_id);
+struct clk *
+of_clk_get_kunit(struct kunit *test, struct device_node *np, int index);
+
+struct clk *
+clk_hw_get_clk_kunit(struct kunit *test, struct clk_hw *hw, const char *con_id);
+struct clk *
+clk_hw_get_clk_prepared_enabled_kunit(struct kunit *test, struct clk_hw *hw,
+ const char *con_id);
+
+int clk_prepare_enable_kunit(struct kunit *test, struct clk *clk);
+
+int clk_hw_register_kunit(struct kunit *test, struct device *dev, struct clk_hw *hw);
+int of_clk_hw_register_kunit(struct kunit *test, struct device_node *node,
+ struct clk_hw *hw);
+
+#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
Test that the fixed rate basic type clk works as intended.
Cc: Brendan Higgins <[email protected]>
Cc: David Gow <[email protected]>
Cc: Rae Moar <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/.kunitconfig | 2 +
drivers/clk/Kconfig | 8 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-fixed-rate_test.c | 377 +++++++++++++++++++++
drivers/clk/clk-fixed-rate_test.h | 8 +
drivers/clk/kunit_clk_fixed_rate_test.dtso | 19 ++
6 files changed, 415 insertions(+)
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/kunit_clk_fixed_rate_test.dtso
diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig
index efa12ac2b3f2..54ece9207055 100644
--- a/drivers/clk/.kunitconfig
+++ b/drivers/clk/.kunitconfig
@@ -1,6 +1,8 @@
CONFIG_KUNIT=y
+CONFIG_OF=y
CONFIG_COMMON_CLK=y
CONFIG_CLK_KUNIT_TEST=y
+CONFIG_CLK_FIXED_RATE_KUNIT_TEST=y
CONFIG_CLK_GATE_KUNIT_TEST=y
CONFIG_CLK_FD_KUNIT_TEST=y
CONFIG_UML_PCI_OVER_VIRTIO=n
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 50af5fc7f570..6e66d04dd11d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -510,6 +510,14 @@ config CLK_KUNIT_TEST
help
Kunit tests for the common clock framework.
+config CLK_FIXED_RATE_KUNIT_TEST
+ tristate "Basic fixed rate clk type KUnit test" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ select OF_OVERLAY if OF
+ help
+ KUnit tests for the basic fixed rate clk type.
+
config CLK_GATE_KUNIT_TEST
tristate "Basic gate type Kunit test" if !KUNIT_ALL_TESTS
depends on KUNIT
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 1fd74658a801..816abac45fbd 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_CLK_KUNIT_TEST) += clk_test.o
obj-$(CONFIG_COMMON_CLK) += clk-divider.o
obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
+obj-$(CONFIG_CLK_FIXED_RATE_KUNIT_TEST) += clk-fixed-rate_test.o kunit_clk_fixed_rate_test.dtbo.o
obj-$(CONFIG_COMMON_CLK) += clk-gate.o
obj-$(CONFIG_CLK_GATE_KUNIT_TEST) += clk-gate_test.o
obj-$(CONFIG_COMMON_CLK) += clk-multiplier.o
diff --git a/drivers/clk/clk-fixed-rate_test.c b/drivers/clk/clk-fixed-rate_test.c
new file mode 100644
index 000000000000..d1dd7c1568f2
--- /dev/null
+++ b/drivers/clk/clk-fixed-rate_test.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for clk fixed rate basic type
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <kunit/clk.h>
+#include <kunit/of.h>
+#include <kunit/platform_device.h>
+#include <kunit/resource.h>
+#include <kunit/test.h>
+
+#include "clk-fixed-rate_test.h"
+
+/**
+ * struct clk_hw_fixed_rate_kunit_params - Parameters to pass to __clk_hw_register_fixed_rate()
+ * @dev: device registering clk
+ * @np: device_node of device registering clk
+ * @name: name of clk
+ * @parent_name: parent name of clk
+ * @parent_hw: clk_hw pointer to parent of clk
+ * @parent_data: parent_data describing parent of clk
+ * @flags: clk framework flags
+ * @fixed_rate: frequency of clk
+ * @fixed_accuracy: accuracy of clk
+ * @clk_fixed_flags: fixed rate specific clk flags
+ */
+struct clk_hw_fixed_rate_kunit_params {
+ struct device *dev;
+ struct device_node *np;
+ const char *name;
+ const char *parent_name;
+ const struct clk_hw *parent_hw;
+ const struct clk_parent_data *parent_data;
+ unsigned long flags;
+ unsigned long fixed_rate;
+ unsigned long fixed_accuracy;
+ unsigned long clk_fixed_flags;
+};
+
+static int
+clk_hw_register_fixed_rate_kunit_init(struct kunit_resource *res, void *context)
+{
+ struct clk_hw_fixed_rate_kunit_params *params = context;
+ struct clk_hw *hw;
+
+ hw = __clk_hw_register_fixed_rate(params->dev, params->np,
+ params->name,
+ params->parent_name,
+ params->parent_hw,
+ params->parent_data,
+ params->flags,
+ params->fixed_rate,
+ params->fixed_accuracy,
+ params->clk_fixed_flags,
+ false);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
+
+ res->data = hw;
+
+ return 0;
+}
+
+static void clk_hw_register_fixed_rate_kunit_exit(struct kunit_resource *res)
+{
+ struct clk_hw *hw = res->data;
+
+ clk_hw_unregister_fixed_rate(hw);
+}
+
+/**
+ * clk_hw_register_fixed_rate_kunit() - Test managed __clk_hw_register_fixed_rate()
+ * @test: The test context
+ * @params: Arguments to __clk_hw_register_fixed_rate()
+ *
+ * Return: Registered fixed rate clk_hw or ERR_PTR on failure
+ */
+static struct clk_hw *
+clk_hw_register_fixed_rate_kunit(struct kunit *test,
+ struct clk_hw_fixed_rate_kunit_params *params)
+{
+ struct clk_hw *hw;
+
+ hw = kunit_alloc_resource(test,
+ clk_hw_register_fixed_rate_kunit_init,
+ clk_hw_register_fixed_rate_kunit_exit,
+ GFP_KERNEL, params);
+ if (!hw)
+ return ERR_PTR(-EINVAL);
+
+ return hw;
+}
+
+/**
+ * clk_hw_unregister_fixed_rate_kunit() - Test managed clk_hw_unregister_fixed_rate()
+ * @test: The test context
+ * @hw: fixed rate clk to unregister upon test completion
+ *
+ * Automatically unregister @hw when @test is complete via
+ * clk_hw_unregister_fixed_rate().
+ *
+ * Return: 0 on success or negative errno on failure
+ */
+static int clk_hw_unregister_fixed_rate_kunit(struct kunit *test, struct clk_hw *hw)
+{
+ if (!kunit_alloc_resource(test, NULL,
+ clk_hw_register_fixed_rate_kunit_exit,
+ GFP_KERNEL, hw))
+ return -ENOMEM;
+
+ return 0;
+}
+
+/*
+ * Test that clk_get_rate() on a fixed rate clk registered with
+ * clk_hw_register_fixed_rate() gets the proper frequency.
+ */
+static void clk_fixed_rate_rate_test(struct kunit *test)
+{
+ struct clk_hw *hw;
+ struct clk *clk;
+ const unsigned long fixed_rate = 230000;
+
+ hw = clk_hw_register_fixed_rate(NULL, "test-fixed-rate", NULL, 0, fixed_rate);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_fixed_rate_kunit(test, hw));
+
+ clk = clk_hw_get_clk_prepared_enabled_kunit(test, hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_EXPECT_EQ(test, fixed_rate, clk_get_rate(clk));
+}
+
+/*
+ * Test that clk_get_accuracy() on a fixed rate clk registered via
+ * clk_hw_register_fixed_rate_with_accuracy() gets the proper accuracy.
+ */
+static void clk_fixed_rate_accuracy_test(struct kunit *test)
+{
+ struct clk_hw *hw;
+ struct clk *clk;
+ const unsigned long fixed_accuracy = 5000;
+
+ hw = clk_hw_register_fixed_rate_with_accuracy(NULL, "test-fixed-rate",
+ NULL, 0, 0,
+ fixed_accuracy);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_fixed_rate_kunit(test, hw));
+
+ clk = clk_hw_get_clk_kunit(test, hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_EXPECT_EQ(test, fixed_accuracy, clk_get_accuracy(clk));
+}
+
+/* Test suite for a fixed rate clk without any parent */
+static struct kunit_case clk_fixed_rate_test_cases[] = {
+ KUNIT_CASE(clk_fixed_rate_rate_test),
+ KUNIT_CASE(clk_fixed_rate_accuracy_test),
+ {}
+};
+
+static struct kunit_suite clk_fixed_rate_suite = {
+ .name = "clk_fixed_rate",
+ .test_cases = clk_fixed_rate_test_cases,
+};
+
+/*
+ * Test that clk_get_parent() on a fixed rate clk gets the proper parent.
+ */
+static void clk_fixed_rate_parent_test(struct kunit *test)
+{
+ struct clk_hw *hw, *parent_hw;
+ struct clk *expected_parent, *actual_parent;
+ struct clk *clk;
+ const char *parent_name = "test-fixed-rate-parent";
+ struct clk_hw_fixed_rate_kunit_params parent_params = {
+ .name = parent_name,
+ };
+
+ parent_hw = clk_hw_register_fixed_rate_kunit(test, &parent_params);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+ KUNIT_ASSERT_STREQ(test, parent_name, clk_hw_get_name(parent_hw));
+
+ expected_parent = clk_hw_get_clk_kunit(test, parent_hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected_parent);
+
+ hw = clk_hw_register_fixed_rate(NULL, "test-fixed-rate", parent_name, 0, 0);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_fixed_rate_kunit(test, hw));
+
+ clk = clk_hw_get_clk_kunit(test, hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ actual_parent = clk_get_parent(clk);
+ KUNIT_EXPECT_TRUE(test, clk_is_match(expected_parent, actual_parent));
+}
+
+/*
+ * Test that clk_get_rate() on a fixed rate clk ignores the parent rate.
+ */
+static void clk_fixed_rate_parent_rate_test(struct kunit *test)
+{
+ struct clk_hw *hw, *parent_hw;
+ struct clk *clk;
+ const unsigned long expected_rate = 1405;
+ const unsigned long parent_rate = 90402;
+ const char *parent_name = "test-fixed-rate-parent";
+ struct clk_hw_fixed_rate_kunit_params parent_params = {
+ .name = parent_name,
+ .fixed_rate = parent_rate,
+ };
+
+ parent_hw = clk_hw_register_fixed_rate_kunit(test, &parent_params);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+ KUNIT_ASSERT_STREQ(test, parent_name, clk_hw_get_name(parent_hw));
+
+ hw = clk_hw_register_fixed_rate(NULL, "test-fixed-rate", parent_name, 0,
+ expected_rate);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_fixed_rate_kunit(test, hw));
+
+ clk = clk_hw_get_clk_prepared_enabled_kunit(test, hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_EXPECT_EQ(test, expected_rate, clk_get_rate(clk));
+}
+
+/*
+ * Test that clk_get_accuracy() on a fixed rate clk ignores the parent accuracy.
+ */
+static void clk_fixed_rate_parent_accuracy_test(struct kunit *test)
+{
+ struct clk_hw *hw, *parent_hw;
+ struct clk *clk;
+ const unsigned long expected_accuracy = 900;
+ const unsigned long parent_accuracy = 24000;
+ const char *parent_name = "test-fixed-rate-parent";
+ struct clk_hw_fixed_rate_kunit_params parent_params = {
+ .name = parent_name,
+ .fixed_accuracy = parent_accuracy,
+ };
+
+ parent_hw = clk_hw_register_fixed_rate_kunit(test, &parent_params);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+ KUNIT_ASSERT_STREQ(test, parent_name, clk_hw_get_name(parent_hw));
+
+ hw = clk_hw_register_fixed_rate_with_accuracy(NULL, "test-fixed-rate",
+ parent_name, 0, 0,
+ expected_accuracy);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_fixed_rate_kunit(test, hw));
+
+ clk = clk_hw_get_clk_kunit(test, hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_EXPECT_EQ(test, expected_accuracy, clk_get_accuracy(clk));
+}
+
+/* Test suite for a fixed rate clk with a parent */
+static struct kunit_case clk_fixed_rate_parent_test_cases[] = {
+ KUNIT_CASE(clk_fixed_rate_parent_test),
+ KUNIT_CASE(clk_fixed_rate_parent_rate_test),
+ KUNIT_CASE(clk_fixed_rate_parent_accuracy_test),
+ {}
+};
+
+static struct kunit_suite clk_fixed_rate_parent_suite = {
+ .name = "clk_fixed_rate_parent",
+ .test_cases = clk_fixed_rate_parent_test_cases,
+};
+
+struct clk_fixed_rate_of_test_context {
+ struct device *dev;
+ struct platform_driver pdrv;
+};
+
+static inline struct clk_fixed_rate_of_test_context *
+pdev_to_clk_fixed_rate_of_test_context(struct platform_device *pdev)
+{
+ return container_of(to_platform_driver(pdev->dev.driver),
+ struct clk_fixed_rate_of_test_context,
+ pdrv);
+}
+
+/*
+ * Test that of_fixed_clk_setup() registers a fixed rate clk with the proper
+ * rate.
+ */
+static void clk_fixed_rate_of_probe_test(struct kunit *test)
+{
+ struct clk_fixed_rate_of_test_context *ctx = test->priv;
+ struct device *dev = ctx->dev;
+ struct clk *clk;
+
+ clk = clk_get_kunit(test, dev, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_ASSERT_EQ(test, 0, clk_prepare_enable_kunit(test, clk));
+ KUNIT_EXPECT_EQ(test, TEST_FIXED_FREQUENCY, clk_get_rate(clk));
+}
+
+/*
+ * Test that of_fixed_clk_setup() registers a fixed rate clk with the proper
+ * accuracy.
+ */
+static void clk_fixed_rate_of_accuracy_test(struct kunit *test)
+{
+ struct clk_fixed_rate_of_test_context *ctx = test->priv;
+ struct device *dev = ctx->dev;
+ struct clk *clk;
+
+ clk = clk_get_kunit(test, dev, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+
+ KUNIT_EXPECT_EQ(test, TEST_FIXED_ACCURACY, clk_get_accuracy(clk));
+}
+
+static struct kunit_case clk_fixed_rate_of_cases[] = {
+ KUNIT_CASE(clk_fixed_rate_of_probe_test),
+ KUNIT_CASE(clk_fixed_rate_of_accuracy_test),
+ {}
+};
+
+static int clk_fixed_rate_of_test_probe(struct platform_device *pdev)
+{
+ struct clk_fixed_rate_of_test_context *ctx;
+
+ ctx = pdev_to_clk_fixed_rate_of_test_context(pdev);
+ ctx->dev = &pdev->dev;
+
+ return 0;
+}
+
+static int clk_fixed_rate_of_init(struct kunit *test)
+{
+ struct clk_fixed_rate_of_test_context *ctx;
+ static const struct of_device_id match_table[] = {
+ { .compatible = "test,clk-fixed-rate" },
+ { }
+ };
+
+ KUNIT_ASSERT_EQ(test, 0,
+ of_overlay_apply_kunit(test, kunit_clk_fixed_rate_test));
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ test->priv = ctx;
+
+ ctx->pdrv.probe = clk_fixed_rate_of_test_probe;
+ ctx->pdrv.driver.of_match_table = match_table;
+ ctx->pdrv.driver.name = __func__;
+ ctx->pdrv.driver.owner = THIS_MODULE;
+
+ KUNIT_ASSERT_EQ(test, 0,
+ platform_driver_register_kunit(test, &ctx->pdrv));
+
+ return 0;
+}
+
+static struct kunit_suite clk_fixed_rate_of_suite = {
+ .name = "clk_fixed_rate_of",
+ .init = clk_fixed_rate_of_init,
+ .test_cases = clk_fixed_rate_of_cases,
+};
+
+kunit_test_suites(
+ &clk_fixed_rate_suite,
+ &clk_fixed_rate_of_suite,
+ &clk_fixed_rate_parent_suite,
+);
+MODULE_LICENSE("GPL");
diff --git a/drivers/clk/clk-fixed-rate_test.h b/drivers/clk/clk-fixed-rate_test.h
new file mode 100644
index 000000000000..e0d28e5b6081
--- /dev/null
+++ b/drivers/clk/clk-fixed-rate_test.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _CLK_FIXED_RATE_TEST_H
+#define _CLK_FIXED_RATE_TEST_H
+
+#define TEST_FIXED_FREQUENCY 50000000
+#define TEST_FIXED_ACCURACY 300
+
+#endif
diff --git a/drivers/clk/kunit_clk_fixed_rate_test.dtso b/drivers/clk/kunit_clk_fixed_rate_test.dtso
new file mode 100644
index 000000000000..10989b07e5b3
--- /dev/null
+++ b/drivers/clk/kunit_clk_fixed_rate_test.dtso
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+#include "clk-fixed-rate_test.h"
+
+&{/} {
+ fixed_50MHz: kunit-clock {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <TEST_FIXED_FREQUENCY>;
+ clock-accuracy = <TEST_FIXED_ACCURACY>;
+ };
+
+ kunit-clock-consumer {
+ compatible = "test,clk-fixed-rate";
+ clocks = <&fixed_50MHz>;
+ };
+};
--
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 a device that provides and consumes clks in DT so
that a KUnit test can register clks based on the device node and test
clk_hw_register() with clk_parent_data.
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]>
---
.../bindings/clock/test,clk-parent-data.yaml | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml
diff --git a/Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml b/Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml
new file mode 100644
index 000000000000..a2f927526405
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/test,clk-parent-data.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/test,clk-parent-data.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Fake clk provider for clk_parent_data unit tests
+
+maintainers:
+ - Stephen Boyd <[email protected]>
+
+description:
+ A clk provider to test the struct clk_parent_data implementation in the Linux
+ kernel.
+
+properties:
+ compatible:
+ const: test,clk-parent-data
+
+ clocks:
+ items:
+ - description: Fixed parent
+ - description: 50 MHz fixed parent
+
+ clock-names:
+ items:
+ - const: parent_fwname
+ - const: "50"
+
+ "#clock-cells":
+ const: 1
+
+required:
+ - compatible
+ - "#clock-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ clock-controller {
+ compatible = "test,clk-parent-data";
+ #clock-cells = <1>;
+ clocks = <&fixed_parent>, <&fixed_50MHz>;
+ clock-names = "parent_fwname", "50";
+ };
+...
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
Test that clks registered with 'struct clk_parent_data' work as
intended and can find their parents.
Cc: Christian Marangi <[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/clk/Kconfig | 1 +
drivers/clk/Makefile | 3 +-
drivers/clk/clk_parent_data_test.h | 10 +
drivers/clk/clk_test.c | 451 +++++++++++++++++++-
drivers/clk/kunit_clk_parent_data_test.dtso | 28 ++
5 files changed, 491 insertions(+), 2 deletions(-)
create mode 100644 drivers/clk/clk_parent_data_test.h
create mode 100644 drivers/clk/kunit_clk_parent_data_test.dtso
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 6e66d04dd11d..755333c77298 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -507,6 +507,7 @@ config CLK_KUNIT_TEST
tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS
depends on KUNIT
default KUNIT_ALL_TESTS
+ select OF_OVERLAY if OF
help
Kunit tests for the common clock framework.
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 816abac45fbd..990e87ee4f92 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -2,7 +2,8 @@
# common clock types
obj-$(CONFIG_HAVE_CLK) += clk-devres.o clk-bulk.o clkdev.o
obj-$(CONFIG_COMMON_CLK) += clk.o
-obj-$(CONFIG_CLK_KUNIT_TEST) += clk_test.o
+obj-$(CONFIG_CLK_KUNIT_TEST) += clk_test.o \
+ kunit_clk_parent_data_test.dtbo.o
obj-$(CONFIG_COMMON_CLK) += clk-divider.o
obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
diff --git a/drivers/clk/clk_parent_data_test.h b/drivers/clk/clk_parent_data_test.h
new file mode 100644
index 000000000000..eedd53ae910d
--- /dev/null
+++ b/drivers/clk/clk_parent_data_test.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _CLK_PARENT_DATA_TEST_H
+#define _CLK_PARENT_DATA_TEST_H
+
+#define CLK_PARENT_DATA_1MHZ_NAME "1mhz_fixed_legacy"
+#define CLK_PARENT_DATA_PARENT1 "parent_fwname"
+#define CLK_PARENT_DATA_PARENT2 "50"
+#define CLK_PARENT_DATA_50MHZ_NAME "50_clk"
+
+#endif
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 39e2b5ff4f51..655bf0909fb7 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -4,12 +4,19 @@
*/
#include <linux/clk.h>
#include <linux/clk-provider.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
/* Needed for clk_hw_get_clk() */
#include "clk.h"
+#include <kunit/clk.h>
+#include <kunit/of.h>
+#include <kunit/platform_device.h>
#include <kunit/test.h>
+#include "clk_parent_data_test.h"
+
static const struct clk_ops empty_clk_ops = { };
#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000)
@@ -2659,6 +2666,446 @@ static struct kunit_suite clk_mux_no_reparent_test_suite = {
.test_cases = clk_mux_no_reparent_test_cases,
};
+struct clk_register_clk_parent_data_test_case {
+ const char *desc;
+ struct clk_parent_data pdata;
+};
+
+static void
+clk_register_clk_parent_data_test_case_to_desc(
+ const struct clk_register_clk_parent_data_test_case *t, char *desc)
+{
+ strcpy(desc, t->desc);
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_of_cases[] = {
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct clk_parent_data::index.
+ */
+ .desc = "clk_parent_data_of_index_test",
+ .pdata.index = 0,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct clk_parent_data::fwname.
+ */
+ .desc = "clk_parent_data_of_fwname_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct clk_parent_data::name.
+ */
+ .desc = "clk_parent_data_of_name_test",
+ /* The index must be negative to indicate firmware not used */
+ .pdata.index = -1,
+ .pdata.name = CLK_PARENT_DATA_1MHZ_NAME,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct
+ * clk_parent_data::{fw_name,name}.
+ */
+ .desc = "clk_parent_data_of_fwname_name_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+ .pdata.name = "not_matching",
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct clk_parent_data::{index,name}.
+ * Index takes priority.
+ */
+ .desc = "clk_parent_data_of_index_name_priority_test",
+ .pdata.index = 0,
+ .pdata.name = "not_matching",
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device_node can
+ * find a parent based on struct
+ * clk_parent_data::{index,fwname,name}. The fw_name takes
+ * priority over index and name.
+ */
+ .desc = "clk_parent_data_of_index_fwname_name_priority_test",
+ .pdata.index = 1,
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT1,
+ .pdata.name = "not_matching",
+ },
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_of_test, clk_register_clk_parent_data_of_cases,
+ clk_register_clk_parent_data_test_case_to_desc)
+
+/**
+ * struct clk_register_clk_parent_data_of_ctx - Context for clk_parent_data OF tests
+ * @np: device node of clk under test
+ * @hw: clk_hw for clk under test
+ */
+struct clk_register_clk_parent_data_of_ctx {
+ struct device_node *np;
+ struct clk_hw hw;
+};
+
+static int clk_register_clk_parent_data_of_test_init(struct kunit *test)
+{
+ struct clk_register_clk_parent_data_of_ctx *ctx;
+
+ KUNIT_ASSERT_EQ(test, 0,
+ of_overlay_apply_kunit(test, kunit_clk_parent_data_test));
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ test->priv = ctx;
+
+ ctx->np = of_find_compatible_node(NULL, NULL, "test,clk-parent-data");
+ if (!ctx->np)
+ return -ENODEV;
+
+ return kunit_add_action_or_reset(test, (kunit_action_t *)&of_node_put, ctx->np);
+}
+
+/*
+ * Test that a clk registered with a struct device_node can find a parent based on
+ * struct clk_parent_data when the hw member isn't set.
+ */
+static void clk_register_clk_parent_data_of_test(struct kunit *test)
+{
+ struct clk_register_clk_parent_data_of_ctx *ctx = test->priv;
+ struct clk_hw *parent_hw;
+ const struct clk_register_clk_parent_data_test_case *test_param;
+ struct clk_init_data init = { };
+ struct clk *expected_parent, *actual_parent;
+
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->np);
+
+ expected_parent = of_clk_get_kunit(test, ctx->np, 0);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected_parent);
+
+ test_param = test->param_value;
+ init.parent_data = &test_param->pdata;
+ init.num_parents = 1;
+ init.name = "parent_data_of_test_clk";
+ init.ops = &clk_dummy_single_parent_ops;
+ ctx->hw.init = &init;
+ KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->np, &ctx->hw));
+
+ parent_hw = clk_hw_get_parent(&ctx->hw);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+
+ actual_parent = clk_hw_get_clk_kunit(test, parent_hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, actual_parent);
+
+ KUNIT_EXPECT_TRUE(test, clk_is_match(expected_parent, actual_parent));
+}
+
+static struct kunit_case clk_register_clk_parent_data_of_test_cases[] = {
+ KUNIT_CASE_PARAM(clk_register_clk_parent_data_of_test,
+ clk_register_clk_parent_data_of_test_gen_params),
+ {}
+};
+
+/*
+ * Test suite for registering clks with struct clk_parent_data and a struct
+ * device_node.
+ */
+static struct kunit_suite clk_register_clk_parent_data_of_suite = {
+ .name = "clk_register_clk_parent_data_of",
+ .init = clk_register_clk_parent_data_of_test_init,
+ .test_cases = clk_register_clk_parent_data_of_test_cases,
+};
+
+/**
+ * struct clk_register_clk_parent_data_device_ctx - Context for clk_parent_data device tests
+ * @dev: device of clk under test
+ * @hw: clk_hw for clk under test
+ * @pdrv: driver to attach to find @dev
+ */
+struct clk_register_clk_parent_data_device_ctx {
+ struct device *dev;
+ struct clk_hw hw;
+ struct platform_driver pdrv;
+};
+
+static inline struct clk_register_clk_parent_data_device_ctx *
+clk_register_clk_parent_data_driver_to_test_context(struct platform_device *pdev)
+{
+ return container_of(to_platform_driver(pdev->dev.driver),
+ struct clk_register_clk_parent_data_device_ctx, pdrv);
+}
+
+static int clk_register_clk_parent_data_device_probe(struct platform_device *pdev)
+{
+ struct clk_register_clk_parent_data_device_ctx *ctx;
+
+ ctx = clk_register_clk_parent_data_driver_to_test_context(pdev);
+ ctx->dev = &pdev->dev;
+
+ return 0;
+}
+
+static void clk_register_clk_parent_data_device_driver(struct kunit *test)
+{
+ struct clk_register_clk_parent_data_device_ctx *ctx = test->priv;
+ static const struct of_device_id match_table[] = {
+ { .compatible = "test,clk-parent-data" },
+ { }
+ };
+
+ ctx->pdrv.probe = clk_register_clk_parent_data_device_probe;
+ ctx->pdrv.driver.of_match_table = match_table;
+ ctx->pdrv.driver.name = __func__;
+ ctx->pdrv.driver.owner = THIS_MODULE;
+
+ KUNIT_ASSERT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_device_cases[] = {
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::index.
+ */
+ .desc = "clk_parent_data_device_index_test",
+ .pdata.index = 1,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::fwname.
+ */
+ .desc = "clk_parent_data_device_fwname_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::name.
+ */
+ .desc = "clk_parent_data_device_name_test",
+ /* The index must be negative to indicate firmware not used */
+ .pdata.index = -1,
+ .pdata.name = CLK_PARENT_DATA_50MHZ_NAME,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::{fw_name,name}.
+ */
+ .desc = "clk_parent_data_device_fwname_name_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ .pdata.name = "not_matching",
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::{index,name}. Index
+ * takes priority.
+ */
+ .desc = "clk_parent_data_device_index_name_priority_test",
+ .pdata.index = 1,
+ .pdata.name = "not_matching",
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::{index,fwname,name}.
+ * The fw_name takes priority over index and name.
+ */
+ .desc = "clk_parent_data_device_index_fwname_name_priority_test",
+ .pdata.index = 0,
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ .pdata.name = "not_matching",
+ },
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_device_test,
+ clk_register_clk_parent_data_device_cases,
+ clk_register_clk_parent_data_test_case_to_desc)
+
+/*
+ * Test that a clk registered with a struct device can find a parent based on
+ * struct clk_parent_data when the hw member isn't set.
+ */
+static void clk_register_clk_parent_data_device_test(struct kunit *test)
+{
+ struct clk_register_clk_parent_data_device_ctx *ctx;
+ const struct clk_register_clk_parent_data_test_case *test_param;
+ struct clk_hw *parent_hw;
+ struct clk_init_data init = { };
+ struct clk *expected_parent, *actual_parent;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ test->priv = ctx;
+
+ clk_register_clk_parent_data_device_driver(test);
+
+ expected_parent = clk_get_kunit(test, ctx->dev, "50");
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, expected_parent);
+
+ test_param = test->param_value;
+ init.parent_data = &test_param->pdata;
+ init.num_parents = 1;
+ init.name = "parent_data_device_test_clk";
+ init.ops = &clk_dummy_single_parent_ops;
+ ctx->hw.init = &init;
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, &ctx->hw));
+
+ parent_hw = clk_hw_get_parent(&ctx->hw);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_hw);
+
+ actual_parent = clk_hw_get_clk_kunit(test, parent_hw, __func__);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, actual_parent);
+
+ KUNIT_EXPECT_TRUE(test, clk_is_match(expected_parent, actual_parent));
+}
+
+static const struct clk_register_clk_parent_data_test_case
+clk_register_clk_parent_data_device_hw_cases[] = {
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw.
+ */
+ .desc = "clk_parent_data_device_hw_index_test",
+ /* The index must be negative to indicate firmware not used */
+ .pdata.index = -1,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw when
+ * struct clk_parent_data::fw_name is set.
+ */
+ .desc = "clk_parent_data_device_hw_fwname_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw when struct
+ * clk_parent_data::name is set.
+ */
+ .desc = "clk_parent_data_device_hw_name_test",
+ /* The index must be negative to indicate firmware not used */
+ .pdata.index = -1,
+ .pdata.name = CLK_PARENT_DATA_50MHZ_NAME,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw when struct
+ * clk_parent_data::{fw_name,name} are set.
+ */
+ .desc = "clk_parent_data_device_hw_fwname_name_test",
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ .pdata.name = "not_matching",
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw when struct
+ * clk_parent_data::index is set. The hw pointer takes
+ * priority.
+ */
+ .desc = "clk_parent_data_device_hw_index_priority_test",
+ .pdata.index = 0,
+ },
+ {
+ /*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw when
+ * struct clk_parent_data::{index,fwname,name} are set.
+ * The hw pointer takes priority over everything else.
+ */
+ .desc = "clk_parent_data_device_hw_index_fwname_name_priority_test",
+ .pdata.index = 0,
+ .pdata.fw_name = CLK_PARENT_DATA_PARENT2,
+ .pdata.name = "not_matching",
+ },
+};
+
+KUNIT_ARRAY_PARAM(clk_register_clk_parent_data_device_hw_test,
+ clk_register_clk_parent_data_device_hw_cases,
+ clk_register_clk_parent_data_test_case_to_desc)
+
+/*
+ * Test that a clk registered with a struct device can find a
+ * parent based on struct clk_parent_data::hw.
+ */
+static void clk_register_clk_parent_data_device_hw_test(struct kunit *test)
+{
+ struct clk_register_clk_parent_data_device_ctx *ctx;
+ const struct clk_register_clk_parent_data_test_case *test_param;
+ struct clk_dummy_context *parent;
+ struct clk_hw *parent_hw;
+ struct clk_parent_data pdata = { };
+ struct clk_init_data init = { };
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+ test->priv = ctx;
+
+ clk_register_clk_parent_data_device_driver(test);
+
+ parent = kunit_kzalloc(test, sizeof(*parent), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+ parent_hw = &parent->hw;
+ parent_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk",
+ &clk_dummy_rate_ops, 0);
+
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, parent_hw));
+
+ test_param = test->param_value;
+ memcpy(&pdata, &test_param->pdata, sizeof(pdata));
+ pdata.hw = parent_hw;
+ init.parent_data = &pdata;
+ init.num_parents = 1;
+ init.ops = &clk_dummy_single_parent_ops;
+ init.name = "parent_data_device_hw_test_clk";
+ ctx->hw.init = &init;
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, ctx->dev, &ctx->hw));
+
+ KUNIT_EXPECT_PTR_EQ(test, parent_hw, clk_hw_get_parent(&ctx->hw));
+}
+
+static struct kunit_case clk_register_clk_parent_data_device_test_cases[] = {
+ KUNIT_CASE_PARAM(clk_register_clk_parent_data_device_test,
+ clk_register_clk_parent_data_device_test_gen_params),
+ KUNIT_CASE_PARAM(clk_register_clk_parent_data_device_hw_test,
+ clk_register_clk_parent_data_device_hw_test_gen_params),
+ {}
+};
+
+static int clk_register_clk_parent_data_device_init(struct kunit *test)
+{
+ KUNIT_ASSERT_EQ(test, 0,
+ of_overlay_apply_kunit(test, kunit_clk_parent_data_test));
+
+ return 0;
+}
+
+/*
+ * Test suite for registering clks with struct clk_parent_data and a struct
+ * device.
+ */
+static struct kunit_suite clk_register_clk_parent_data_device_suite = {
+ .name = "clk_register_clk_parent_data_device",
+ .init = clk_register_clk_parent_data_device_init,
+ .test_cases = clk_register_clk_parent_data_device_test_cases,
+};
+
kunit_test_suites(
&clk_leaf_mux_set_rate_parent_test_suite,
&clk_test_suite,
@@ -2671,7 +3118,9 @@ kunit_test_suites(
&clk_range_test_suite,
&clk_range_maximize_test_suite,
&clk_range_minimize_test_suite,
+ &clk_register_clk_parent_data_of_suite,
+ &clk_register_clk_parent_data_device_suite,
&clk_single_parent_mux_test_suite,
- &clk_uncached_test_suite
+ &clk_uncached_test_suite,
);
MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/kunit_clk_parent_data_test.dtso b/drivers/clk/kunit_clk_parent_data_test.dtso
new file mode 100644
index 000000000000..7d3ed9a5a2e8
--- /dev/null
+++ b/drivers/clk/kunit_clk_parent_data_test.dtso
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+#include "clk_parent_data_test.h"
+
+&{/} {
+ fixed_50: kunit-clock-50MHz {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <50000000>;
+ clock-output-names = CLK_PARENT_DATA_50MHZ_NAME;
+ };
+
+ fixed_parent: kunit-clock-1MHz {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <1000000>;
+ clock-output-names = CLK_PARENT_DATA_1MHZ_NAME;
+ };
+
+ kunit-clock-controller {
+ compatible = "test,clk-parent-data";
+ clocks = <&fixed_parent>, <&fixed_50>;
+ clock-names = CLK_PARENT_DATA_PARENT1, CLK_PARENT_DATA_PARENT2;
+ #clock-cells = <1>;
+ };
+};
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
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
diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
index 282befa17edf..02b26f5e8750 100644
--- a/Documentation/dev-tools/kunit/api/index.rst
+++ b/Documentation/dev-tools/kunit/api/index.rst
@@ -10,6 +10,7 @@ API Reference
resource
functionredirection
of
+ platformdevice
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
diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst
new file mode 100644
index 000000000000..b228fb6558c2
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/platformdevice.rst
@@ -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:
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index e321dfc7e922..740aef267fbe 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,8 +1,11 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
+obj-$(CONFIG_KUNIT) += platform_kunit.o
+
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)
diff --git a/drivers/base/test/platform_kunit-test.c b/drivers/base/test/platform_kunit-test.c
new file mode 100644
index 000000000000..ce545532d209
--- /dev/null
+++ b/drivers/base/test/platform_kunit-test.c
@@ -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");
diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
new file mode 100644
index 000000000000..54af6db2a6d8
--- /dev/null
+++ b/drivers/base/test/platform_kunit.c
@@ -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
+ * @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)
+{
+ 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.
+ */
+int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev)
+{
+ 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
+ * 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)) {
+ return -ENOMEM;
+ }
+
+ 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";
+ * 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;
+
+ 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();
+
+ return kunit_add_action_or_reset(test,
+ (kunit_action_t *)&platform_driver_unregister,
+ drv);
+}
+EXPORT_SYMBOL_GPL(platform_driver_register_kunit);
diff --git a/include/kunit/platform_device.h b/include/kunit/platform_device.h
new file mode 100644
index 000000000000..28d28abf15a4
--- /dev/null
+++ b/include/kunit/platform_device.h
@@ -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
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
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
+================
+
+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
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)
+{
+ 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
+
+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
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(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index b97d298b3eb6..e590f5ab539f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -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
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
diff --git a/drivers/of/.kunitconfig b/drivers/of/.kunitconfig
index 5a8fee11978c..4c53d2c7a275 100644
--- a/drivers/of/.kunitconfig
+++ b/drivers/of/.kunitconfig
@@ -1,3 +1,4 @@
CONFIG_KUNIT=y
CONFIG_OF=y
CONFIG_OF_KUNIT_TEST=y
+CONFIG_OF_OVERLAY_KUNIT_TEST=y
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dd726c7056bf..0e2d608c3e20 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -107,6 +107,16 @@ config OF_OVERLAY
While this option is selected automatically when needed, you can
enable it manually to improve device tree unit test coverage.
+config OF_OVERLAY_KUNIT_TEST
+ tristate "Device Tree overlay KUnit tests" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ select OF_OVERLAY
+ help
+ This option builds KUnit unit tests for the device tree overlay code.
+
+ If unsure, say N here, but this option is safe to enable.
+
config OF_NUMA
bool
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 0dfd05079313..88588ffb9de6 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -21,5 +21,6 @@ endif
obj-$(CONFIG_KUNIT) += of_kunit.o
obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
+obj-$(CONFIG_OF_OVERLAY_KUNIT_TEST) += overlay_test.o kunit_overlay_test.dtbo.o
obj-$(CONFIG_OF_UNITTEST) += unittest-data/
diff --git a/drivers/of/kunit_overlay_test.dtso b/drivers/of/kunit_overlay_test.dtso
new file mode 100644
index 000000000000..85f20b4b4c16
--- /dev/null
+++ b/drivers/of/kunit_overlay_test.dtso
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+&{/} {
+ kunit-test {
+ compatible = "test,empty";
+ };
+};
diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
new file mode 100644
index 000000000000..223e5a5c23c5
--- /dev/null
+++ b/drivers/of/overlay_test.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for device tree overlays
+ */
+#include <linux/device/bus.h>
+#include <linux/kconfig.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include <kunit/of.h>
+#include <kunit/test.h>
+
+static const char * const kunit_node_name = "kunit-test";
+static const char * const kunit_compatible = "test,empty";
+
+/* Test that of_overlay_apply_kunit() adds a node to the live tree */
+static void of_overlay_apply_kunit_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, kunit_node_name);
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, np);
+ of_node_put(np);
+}
+
+/*
+ * Test that of_overlay_apply_kunit() creates platform devices with the
+ * expected device_node
+ */
+static void of_overlay_apply_kunit_platform_device(struct kunit *test)
+{
+ struct platform_device *pdev;
+ struct device_node *np;
+
+ KUNIT_ASSERT_EQ(test, 0,
+ of_overlay_apply_kunit(test, kunit_overlay_test));
+
+ np = of_find_node_by_name(NULL, kunit_node_name);
+ of_node_put_kunit(test, np);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
+
+ pdev = of_find_device_by_node(np);
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev);
+ put_device(&pdev->dev);
+}
+
+static int of_overlay_bus_match_compatible(struct device *dev, const void *data)
+{
+ return of_device_is_compatible(dev->of_node, data);
+}
+
+/* Test that of_overlay_apply_kunit() cleans up after the test is finished */
+static void of_overlay_apply_kunit_cleanup(struct kunit *test)
+{
+ struct kunit fake;
+ struct platform_device *pdev;
+ struct device *dev;
+ struct device_node *np;
+
+ 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");
+
+ kunit_init_test(&fake, "fake test", NULL);
+ KUNIT_ASSERT_EQ(test, fake.status, KUNIT_SUCCESS);
+
+ KUNIT_ASSERT_EQ(test, 0,
+ of_overlay_apply_kunit(&fake, kunit_overlay_test));
+
+ np = of_find_node_by_name(NULL, kunit_node_name);
+ of_node_put(np); /* Not derefing 'np' after this */
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
+
+ pdev = of_find_device_by_node(np);
+ put_device(&pdev->dev); /* Not derefing 'pdev' after this */
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+ /* Remove overlay */
+ kunit_cleanup(&fake);
+
+ /* The node and device should be removed */
+ np = of_find_node_by_name(NULL, kunit_node_name);
+ KUNIT_EXPECT_PTR_EQ(test, NULL, np);
+ of_node_put(np);
+
+ 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);
+}
+
+static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
+ KUNIT_CASE(of_overlay_apply_kunit_apply),
+ KUNIT_CASE(of_overlay_apply_kunit_platform_device),
+ KUNIT_CASE(of_overlay_apply_kunit_cleanup),
+ {}
+};
+
+/*
+ * Test suite for test managed device tree overlays.
+ */
+static struct kunit_suite of_overlay_apply_kunit_suite = {
+ .name = "of_overlay_apply_kunit",
+ .test_cases = of_overlay_apply_kunit_test_cases,
+};
+
+kunit_test_suites(
+ &of_overlay_apply_kunit_suite,
+);
+MODULE_LICENSE("GPL");
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
On Mon, 22 Apr 2024 16:23:54 -0700, Stephen Boyd wrote:
> Add test managed wrappers for of_overlay_apply() that automatically
> removes the overlay when the test is finished. This API is intended for
> use by KUnit tests that test code which relies on 'struct device_node's
> and of_*() APIs.
>
> KUnit tests will call of_overlay_apply_kunit() to load an overlay that's
> been built into the kernel image. When the test is complete, the overlay
> will be removed.
>
> This has a few benefits:
>
> 1) It keeps the tests hermetic because the overlay is removed when the
> test is complete. Tests won't even be aware that an overlay was
> loaded in another test.
>
> 2) The overlay code can live right next to the unit test that loads it.
> The overlay and the unit test can be compiled into one kernel module
> if desired.
>
> 3) We can test different device tree configurations by loading
> different overlays. The overlays can be written for a specific test,
> and there can be many of them loaded per-test without needing to jam
> all possible combinations into one DTB.
>
> 4) It also allows KUnit to test device tree dependent code on any
> architecture, not just UML. This allows KUnit tests to test
> architecture specific device tree code.
>
> There are some potential pitfalls though. Test authors need to be
> careful to not overwrite properties in the live tree. The easiest way to
> do this is to add and remove nodes with a 'kunit-' prefix, almost
> guaranteeing that the same node won't be present in the tree loaded at
> boot.
>
> Suggested-by: Rob Herring <[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
>
Reviewed-by: Rob Herring (Arm) <[email protected]>
On Mon, 22 Apr 2024 16:23:57 -0700, Stephen Boyd wrote:
> 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
>
Reviewed-by: Rob Herring (Arm) <[email protected]>
Quoting Stephen Boyd (2024-04-22 16:23:58)
> diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
> new file mode 100644
> index 000000000000..54af6db2a6d8
> --- /dev/null
> +++ b/drivers/base/test/platform_kunit.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test managed platform driver
> + */
> +
[...]
> +
> +/**
> + * 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;
> +
> + 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();
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.
> +
> + 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-24 11:11:21)
> Quoting Stephen Boyd (2024-04-22 16:23:58)
> > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
[...]
> > +
> > + /*
> > + * 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.
I have an implementation that does this that I'll send.
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Saravana Kannan <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
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 <[email protected]>
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
>
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
>
> 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]>
> ---
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index b97d298b3eb6..e590f5ab539f 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -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
>
>
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
>
> 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]>
> ---
I really like this: I think it'll definitely help make platform
devices easier to use in tests. (And the handling of the unregistering
is particularly much nicer than trying to do it by hand, IMO.)
I've got a few suggestions below, mostly around naming and that it's
probably best to put this in lib/kunit/, given that the header is in
include/kunit, and there's already the generic kunit_device
functionality there. There's also a control-flow-integrity issue or
two, as casting function pointers will trigger that.
Otherwise, this looks good.
-- David
> 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
>
> diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> index 282befa17edf..02b26f5e8750 100644
> --- a/Documentation/dev-tools/kunit/api/index.rst
> +++ b/Documentation/dev-tools/kunit/api/index.rst
> @@ -10,6 +10,7 @@ API Reference
> resource
> functionredirection
> of
> + platformdevice
A note (to myself, as much as anything): for the other device
wrappers, we considered them 'resources' and so bundled the
documentation in with the 'resource' documentation.
Maybe it'd make sense to split it out into its own device.rst file. We
could optionally include the platformdevice stuff in the same file if
we wanted to consolidate documentation for "device helpers", though
I'm not sure if it's worthwhile.
>
>
> 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
> diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst
> new file mode 100644
> index 000000000000..b228fb6558c2
> --- /dev/null
> +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst
> @@ -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:
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index e321dfc7e922..740aef267fbe 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -1,8 +1,11 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
>
> +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.
> 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)
> diff --git a/drivers/base/test/platform_kunit-test.c b/drivers/base/test/platform_kunit-test.c
> new file mode 100644
> index 000000000000..ce545532d209
> --- /dev/null
> +++ b/drivers/base/test/platform_kunit-test.c
> @@ -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");
> diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
> new file mode 100644
> index 000000000000..54af6db2a6d8
> --- /dev/null
> +++ b/drivers/base/test/platform_kunit.c
> @@ -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
> + * @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.
> +{
> + 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 *);
> + 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.
> + */
> +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.
> +{
> + 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
> + * 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()?
> + 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.
> + }
> +
> + 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";
> + * 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()
> +{
> + 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?
> +
> + return kunit_add_action_or_reset(test,
> + (kunit_action_t *)&platform_driver_unregister,
> + drv);
> +}
> +EXPORT_SYMBOL_GPL(platform_driver_register_kunit);
> diff --git a/include/kunit/platform_device.h b/include/kunit/platform_device.h
> new file mode 100644
> index 000000000000..28d28abf15a4
> --- /dev/null
> +++ b/include/kunit/platform_device.h
> @@ -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.
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
>
> 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]>
> ---
Looks good to me.
Reviewed-by: David Gow <[email protected]>
-- David
> .../devicetree/bindings/test/test,empty.yaml | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml
>
> diff --git a/Documentation/devicetree/bindings/test/test,empty.yaml b/Documentation/devicetree/bindings/test/test,empty.yaml
> new file mode 100644
> index 000000000000..20dc83b15bbf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/test/test,empty.yaml
> @@ -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#
> +
> +title: Empty node
> +
> +maintainers:
> + - David Gow <[email protected]>
> + - Brendan Higgins <[email protected]>
Brendan: Do you want to use your linux.dev address here?
> +
> +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
>
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
>
> 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]
>
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:
> [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
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.
Otherwise, I think this would be fine to take via either the clk or DT
and clk trees: there are no conflicts with the current KUnit changes
for 6.10. At worst, we might hit some conflicts in the documentation,
but there's nothing scheduled yet.
Cheers,
-- David
> 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
>
On Wed, May 1, 2024 at 3:55 AM 'David Gow' via KUnit Development
<[email protected]> wrote:
>
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> >
> > 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]>
> > ---
>
> Looks good to me.
>
> Reviewed-by: David Gow <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
>
> -- David
>
> > .../devicetree/bindings/test/test,empty.yaml | 30 +++++++++++++++++++
> > 1 file changed, 30 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/test/test,empty.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/test/test,empty.yaml b/Documentation/devicetree/bindings/test/test,empty.yaml
> > new file mode 100644
> > index 000000000000..20dc83b15bbf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/test/test,empty.yaml
> > @@ -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#
> > +
> > +title: Empty node
> > +
> > +maintainers:
> > + - David Gow <[email protected]>
> > + - Brendan Higgins <[email protected]>
>
> Brendan: Do you want to use your linux.dev address here?
Ah, thanks for pointing that out, David. Yes, please use "Brendan
Higgins <[email protected]>" here.
Otherwise it looks good to me.
Quoting David Gow (2024-05-01 00:55:10)
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > index 2d8f756aab56..282befa17edf 100644
> > --- a/Documentation/dev-tools/kunit/api/index.rst
> > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > @@ -9,11 +9,15 @@ API Reference
> > test
> > resource
> > functionredirection
> > + of
> >
> >
> > This page documents the KUnit kernel testing API. It is divided into the
> > following sections:
> >
> > +Core KUnit API
> > +==============
> > +
> > Documentation/dev-tools/kunit/api/test.rst
> >
> > - Documents all of the standard testing API
> > @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
> > Documentation/dev-tools/kunit/api/functionredirection.rst
> >
> > - Documents the KUnit Function Redirection API
> > +
> > +Driver KUnit API
> > +================
>
> If we're adding a separate 'Driver' section here, it's probably
> sensible to move the existing device/driver helper documentation here,
> rather than leaving it in resource.rst as-is. I'm happy to do that in
> a follow-up patch, though.
To clarify, you're talking about "Managed Devices"? Looks like that can
be a follow-up to split it into a new file and then put it here. If
you're happy to do that then I'll leave it to you.
>
> > +
> > +Documentation/dev-tools/kunit/api/of.rst
> > +
> > + - Documents the KUnit device tree (OF) API
> > diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> > new file mode 100644
> > index 000000000000..8587591c3e78
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/of.rst
> > @@ -0,0 +1,13 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +====================
> > +Device Tree (OF) API
> > +====================
> > +
> > +The KUnit device tree API is used to test device tree (of_*) dependent code.
> > +
> > +.. kernel-doc:: include/kunit/of.h
> > + :internal:
> > +
> > +.. kernel-doc:: drivers/of/of_kunit.c
> > + :export:
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index 251d33532148..0dfd05079313 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -19,6 +19,7 @@ obj-y += kexec.o
> > endif
> > endif
> >
> > +obj-$(CONFIG_KUNIT) += of_kunit.o
>
> I'm tempted to have this either live in lib/kunit, or be behind a
> separate Kconfig option, particularly since this will end up as a
> separate module, as-is.
Is the idea to have a single module that has all the kunit "stuff" in it
so we can just load one module and be done? Is there any discussion on
the list I can read to see the argument for this?
>
> > obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> >
> > obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> > new file mode 100644
> > index 000000000000..f63527268a51
> > --- /dev/null
> > +++ b/drivers/of/of_kunit.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test managed device tree APIs
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_fdt.h>
> > +
> > +#include <kunit/of.h>
> > +#include <kunit/test.h>
> > +#include <kunit/resource.h>
> > +
> > +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> > +{
> > + of_overlay_remove(ovcs_id);
> > +}
> > +
> > +/**
> > + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> > + * @test: test context
> > + * @overlay_fdt: device tree overlay to apply
> > + * @overlay_fdt_size: size in bytes of @overlay_fdt
> > + * @ovcs_id: identifier of overlay, used to remove the overlay
> > + *
> > + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> > + * case and is automatically removed with of_overlay_remove() after the test
> > + * case concludes.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + */
> > +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> > + u32 overlay_fdt_size, int *ovcs_id)
>
> We're using kunit_ as a prefix for the device helpers (e.g.
> kunit_device_register()), so it may make sense to do that here, too.
> It's not as important as with the platform_device helpers, which are
> very similar to the existing device ones, but if we want to treat
> these as "part of KUnit which deals with of_overlays", rather than
> "part of "of_overlay which deals with KUnit", this may fit better.
>
> Thoughts?
I'm fine either way with the name. I recall that last time we put a
kunit postfix to make it easier to tab complete or something like that.
I find it hard to understand the distinction you're trying to make
though. I guess you're saying the difference is what subsystem maintains
the code, kunit or of. When they're simple wrappers it is easier to
extract them out to lib/kunit and thus they can (should?) have the kunit
prefix. Maybe that always holds true, because kunit wrappers are
typically another API consumer, and if the API is exported either in a
linux/ header or as an exported symbol it can be wrapped in lib/kunit
easily. Did I follow correctly? When would of_overlay ever deal with
KUnit?
> > diff --git a/include/kunit/of.h b/include/kunit/of.h
> > new file mode 100644
> > index 000000000000..9981442ba578
> > --- /dev/null
> > +++ b/include/kunit/of.h
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _KUNIT_OF_H
> > +#define _KUNIT_OF_H
> > +
> > +#include <kunit/test.h>
> > +
> > +struct device_node;
> > +
> > +#ifdef CONFIG_OF
>
> Do we also need to check for CONFIG_OF_OVERLAY here?
>
> Also, how useful is it to compile but skip tests without
> CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
> which may make it more obvious that these are disabled if it's
> unexpected.
>
> Thoughts?
I've tried to make it so that tests skip if an option isn't enabled. I
suppose the CONFIG_OF_OVERLAY check can be hoisted up here as well so
that the skip isn't buried in lower levels.
Quoting David Gow (2024-05-01 00:55:46)
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > new file mode 100644
> > index 000000000000..b228fb6558c2
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > @@ -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:
> > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> > index e321dfc7e922..740aef267fbe 100644
> > --- a/drivers/base/test/Makefile
> > +++ b/drivers/base/test/Makefile
> > @@ -1,8 +1,11 @@
> > # SPDX-License-Identifier: GPL-2.0
> > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
> >
> > +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.
>
> I probably can be persuaded otherwise if you've got a strong
> preference for it to stay as-is, though.
>
> > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
> > new file mode 100644
> > index 000000000000..54af6db2a6d8
> > --- /dev/null
> > +++ b/drivers/base/test/platform_kunit.c
> > @@ -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
> > + * @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.
>
>
> > +{
> > + 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 *);
Thanks. I missed that.
>
> > + 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.
> > + */
> > +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.
>
> > +{
> > + 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
> > + * 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()?
>
> > + 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.
>
> > + }
> > +
> > + 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";
> > + * 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()
>
> > +{
> > + 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.
Quoting David Gow (2024-05-01 01:08:11)
>
> 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?
Let's discuss on one of the patches.
>
> 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:
> > [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<---
diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
index 223e5a5c23c5..85cfbe6bb132 100644
--- a/drivers/of/overlay_test.c
+++ b/drivers/of/overlay_test.c
@@ -45,7 +45,8 @@ static void of_overlay_apply_kunit_platform_device(struct kunit *test)
pdev = of_find_device_by_node(np);
KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev);
- put_device(&pdev->dev);
+ if (pdev)
+ put_device(&pdev->dev);
}
static int of_overlay_bus_match_compatible(struct device *dev, const void *data)
@@ -77,8 +78,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
pdev = of_find_device_by_node(np);
- put_device(&pdev->dev); /* Not derefing 'pdev' after this */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+ put_device(&pdev->dev); /* Not derefing 'pdev' after this */
/* Remove overlay */
kunit_cleanup(&fake);
@@ -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);
}
static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
> > [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
>
> 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.
On Fri, 3 May 2024 at 08:36, Stephen Boyd <[email protected]> wrote:
>
> Quoting David Gow (2024-05-01 00:55:10)
> > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> > > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > > index 2d8f756aab56..282befa17edf 100644
> > > --- a/Documentation/dev-tools/kunit/api/index.rst
> > > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > > @@ -9,11 +9,15 @@ API Reference
> > > test
> > > resource
> > > functionredirection
> > > + of
> > >
> > >
> > > This page documents the KUnit kernel testing API. It is divided into the
> > > following sections:
> > >
> > > +Core KUnit API
> > > +==============
> > > +
> > > Documentation/dev-tools/kunit/api/test.rst
> > >
> > > - Documents all of the standard testing API
> > > @@ -25,3 +29,10 @@ Documentation/dev-tools/kunit/api/resource.rst
> > > Documentation/dev-tools/kunit/api/functionredirection.rst
> > >
> > > - Documents the KUnit Function Redirection API
> > > +
> > > +Driver KUnit API
> > > +================
> >
> > If we're adding a separate 'Driver' section here, it's probably
> > sensible to move the existing device/driver helper documentation here,
> > rather than leaving it in resource.rst as-is. I'm happy to do that in
> > a follow-up patch, though.
>
> To clarify, you're talking about "Managed Devices"? Looks like that can
> be a follow-up to split it into a new file and then put it here. If
> you're happy to do that then I'll leave it to you.
>
Yeah, this is "Managed Devices". I'll send out a follow-up patch to
the documentation once this has landed so we don't conflict.
> >
> > > +
> > > +Documentation/dev-tools/kunit/api/of.rst
> > > +
> > > + - Documents the KUnit device tree (OF) API
> > > diff --git a/Documentation/dev-tools/kunit/api/of.rst b/Documentation/dev-tools/kunit/api/of.rst
> > > new file mode 100644
> > > index 000000000000..8587591c3e78
> > > --- /dev/null
> > > +++ b/Documentation/dev-tools/kunit/api/of.rst
> > > @@ -0,0 +1,13 @@
> > > +.. SPDX-License-Identifier: GPL-2.0
> > > +
> > > +====================
> > > +Device Tree (OF) API
> > > +====================
> > > +
> > > +The KUnit device tree API is used to test device tree (of_*) dependent code.
> > > +
> > > +.. kernel-doc:: include/kunit/of.h
> > > + :internal:
> > > +
> > > +.. kernel-doc:: drivers/of/of_kunit.c
> > > + :export:
> > > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > > index 251d33532148..0dfd05079313 100644
> > > --- a/drivers/of/Makefile
> > > +++ b/drivers/of/Makefile
> > > @@ -19,6 +19,7 @@ obj-y += kexec.o
> > > endif
> > > endif
> > >
> > > +obj-$(CONFIG_KUNIT) += of_kunit.o
> >
> > I'm tempted to have this either live in lib/kunit, or be behind a
> > separate Kconfig option, particularly since this will end up as a
> > separate module, as-is.
>
> Is the idea to have a single module that has all the kunit "stuff" in it
> so we can just load one module and be done? Is there any discussion on
> the list I can read to see the argument for this?
I don't think there's been any specific discussion around making sure
KUnit lives in one module: this is just the first patch which would
make CONFIG_KUNIT build several separate ones.
Personally, I'd prefer to have the CONFIG_KUNIT option only build one
module itself, and otherwise keep the corresponding code in lib/kunit,
just so it's clearer what side effects enabling / disabling it has.
But ultimately, this really is just another side effect of the
discussion below about whether this is integrated as "part of KUnit",
in which case it can live in lib/kunit and be under CONFIG_KUNIT, or
if it's a part of of, in which case this is fine (though I'd rather it
be behind a CONFIG_OF_KUNIT_HELPERS or similar, personally).
> >
> > > obj-$(CONFIG_OF_KUNIT_TEST) += of_test.o
> > >
> > > obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> > > diff --git a/drivers/of/of_kunit.c b/drivers/of/of_kunit.c
> > > new file mode 100644
> > > index 000000000000..f63527268a51
> > > --- /dev/null
> > > +++ b/drivers/of/of_kunit.c
> > > @@ -0,0 +1,99 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Test managed device tree APIs
> > > + */
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_fdt.h>
> > > +
> > > +#include <kunit/of.h>
> > > +#include <kunit/test.h>
> > > +#include <kunit/resource.h>
> > > +
> > > +static void of_overlay_fdt_apply_kunit_exit(void *ovcs_id)
> > > +{
> > > + of_overlay_remove(ovcs_id);
> > > +}
> > > +
> > > +/**
> > > + * of_overlay_fdt_apply_kunit() - Test managed of_overlay_fdt_apply()
> > > + * @test: test context
> > > + * @overlay_fdt: device tree overlay to apply
> > > + * @overlay_fdt_size: size in bytes of @overlay_fdt
> > > + * @ovcs_id: identifier of overlay, used to remove the overlay
> > > + *
> > > + * Just like of_overlay_fdt_apply(), except the overlay is managed by the test
> > > + * case and is automatically removed with of_overlay_remove() after the test
> > > + * case concludes.
> > > + *
> > > + * Return: 0 on success, negative errno on failure
> > > + */
> > > +int of_overlay_fdt_apply_kunit(struct kunit *test, void *overlay_fdt,
> > > + u32 overlay_fdt_size, int *ovcs_id)
> >
> > We're using kunit_ as a prefix for the device helpers (e.g.
> > kunit_device_register()), so it may make sense to do that here, too.
> > It's not as important as with the platform_device helpers, which are
> > very similar to the existing device ones, but if we want to treat
> > these as "part of KUnit which deals with of_overlays", rather than
> > "part of "of_overlay which deals with KUnit", this may fit better.
> >
> > Thoughts?
>
> I'm fine either way with the name. I recall that last time we put a
> kunit postfix to make it easier to tab complete or something like that.
>
> I find it hard to understand the distinction you're trying to make
> though. I guess you're saying the difference is what subsystem maintains
> the code, kunit or of. When they're simple wrappers it is easier to
> extract them out to lib/kunit and thus they can (should?) have the kunit
> prefix. Maybe that always holds true, because kunit wrappers are
> typically another API consumer, and if the API is exported either in a
> linux/ header or as an exported symbol it can be wrapped in lib/kunit
> easily. Did I follow correctly? When would of_overlay ever deal with
> KUnit?
Yeah, it's about what subsystem is maintaining the code, which impacts
a bit of the naming, and depends a bit on the intended use-case.
If these helpers are intended to test a particular subsystem, and are
of no use outside it, it seems clear that they should be a part of
that subsystem. For instance, the drm_kunit_helpers.
If they're exposing kunit-specific wrappers around core APIs, it makes
sense for them to be a part of KUnit. (The managed devices stuff, for
instance, as the device model is used by pretty much everything. It
also requires a KUnit-managed struct kunit_bus, which is hooked into
KUnit at a lower level, so needs to be a part of kunit.)
It gets more complicated for cases like of, where the helpers are both
used for testing of itself, and for testing drivers which rely on it.
So I think it could go either way. My gut instinct is that
platform_device is generic enough to be a part of KUnit (to match the
existing managed device stuff). For of_overlay, I could go either way,
and just leaned to having it be part of KUnit as that's a bit more
common, and it matches, e.g., the headers and documentation being
under include/kunit and dev-tools/kunit respectively.
> > > diff --git a/include/kunit/of.h b/include/kunit/of.h
> > > new file mode 100644
> > > index 000000000000..9981442ba578
> > > --- /dev/null
> > > +++ b/include/kunit/of.h
> > > @@ -0,0 +1,94 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _KUNIT_OF_H
> > > +#define _KUNIT_OF_H
> > > +
> > > +#include <kunit/test.h>
> > > +
> > > +struct device_node;
> > > +
> > > +#ifdef CONFIG_OF
> >
> > Do we also need to check for CONFIG_OF_OVERLAY here?
> >
> > Also, how useful is it to compile but skip tests without
> > CONFIG_OF{,_OVERLAY} enabled? The other option is a compile error,
> > which may make it more obvious that these are disabled if it's
> > unexpected.
> >
> > Thoughts?
>
> I've tried to make it so that tests skip if an option isn't enabled. I
> suppose the CONFIG_OF_OVERLAY check can be hoisted up here as well so
> that the skip isn't buried in lower levels.
Yeah, my feeling here is that if we're going to declare functions
which interact with of_overlay, we should have the 'skip' fallbacks
occur for either both CONFIG_OF and CONFIG_OF_OVERLAY here, or neither
(and require the test use its own #include guards). Having CONFIG_OF
checked here, and CONFIG_OF_OVERLAY checked elsewhere seems confusing
to me.
Cheers,
-- David
On Fri, 3 May 2024 at 09:04, Stephen Boyd <[email protected]> wrote:
>
> Quoting David Gow (2024-05-01 00:55:46)
> > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> > > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > > new file mode 100644
> > > index 000000000000..b228fb6558c2
> > > --- /dev/null
> > > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > > @@ -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:
> > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> > > index e321dfc7e922..740aef267fbe 100644
> > > --- a/drivers/base/test/Makefile
> > > +++ b/drivers/base/test/Makefile
> > > @@ -1,8 +1,11 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
> > >
> > > +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?
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.
> 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.
>
> 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.
> >
> > I probably can be persuaded otherwise if you've got a strong
> > preference for it to stay as-is, though.
> >
> > > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
> > > new file mode 100644
> > > index 000000000000..54af6db2a6d8
> > > --- /dev/null
> > > +++ b/drivers/base/test/platform_kunit.c
> > > @@ -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
> > > + * @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.
> >
> >
> > > +{
> > > + 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 *);
>
> Thanks. I missed that.
>
> >
> > > + 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.
> > > + */
> > > +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.
> >
> > > +{
> > > + 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
> > > + * 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()?
> >
> > > + 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.
>
> >
> > > + }
> > > +
> > > + 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";
> > > + * 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()
> >
> > > +{
> > > + 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.
Sounds good!
Cheers,
-- David
Quoting David Gow (2024-05-04 01:30:34)
> On Fri, 3 May 2024 at 09:04, Stephen Boyd <[email protected]> wrote:
> >
> > Quoting David Gow (2024-05-01 00:55:46)
> > > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <[email protected]> wrote:
> > > > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > > > new file mode 100644
> > > > index 000000000000..b228fb6558c2
> > > > --- /dev/null
> > > > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst
> > > > @@ -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:
> > > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> > > > index e321dfc7e922..740aef267fbe 100644
> > > > --- a/drivers/base/test/Makefile
> > > > +++ b/drivers/base/test/Makefile
> > > > @@ -1,8 +1,11 @@
> > > > # SPDX-License-Identifier: GPL-2.0
> > > > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o
> > > >
> > > > +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?
>
> 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.
Alright, I'll rename the files.
>
> > 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.
Alright.
>
> >
> > 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.
Ok, thanks. The kzalloc wrappers look like the best example here. They
are so essential that they are in lib/kunit. The platform bus is built
into the kernel all the time, similar to mm, so I can see it being
essential and desired to have the wrappers in lib/kunit.
>
> 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.
Sounds good. I'll follow this route.
>
> 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.
>
Yeah. My final hesitation is that it will be "too easy" to make devices
that live on the platform_bus when they should really be on the
kunit_bus. I guess we'll have to watch out for folks making platform
devices that don't use any other platform device APIs like IO resources,
etc.
Quoting Stephen Boyd (2024-04-22 16:23:58)
> diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c
> new file mode 100644
> index 000000000000..54af6db2a6d8
> --- /dev/null
> +++ b/drivers/base/test/platform_kunit.c
> @@ -0,0 +1,174 @@
[...]
> +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.
> + */
> +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev)
> +{
> + 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);
This doesn't work because platform_device_alloc_kunit() used
kunit_add_action_or_reset() which has a chained free routine and data
pointer. I've added a test to make sure the platform device is removed
from the bus. It's not super great though because when this code fails
to find a match it will still remove the device by calling
platform_device_unregister() when the test ends. It will follow that up
with a call to platform_device_put(), which is the problem as that
causes an underflow and operates on an already freed device.
I couldn't come up with anything better than searching the platform bus.
Maybe if there was a way to allocate the memory or redirect where
platform_device_alloc_kunit() got memory from we could hold the device
memory around after it should have been freed and make sure the kref for
the device kobject is 0. That seems pretty invasive to do though so I'm
just going to leave it for now and add this test to make sure it cleans
up.
Quoting Stephen Boyd (2024-05-02 18:27:42)
> Quoting David Gow (2024-05-01 01:08:11)
> >
> > 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:
> > > [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<---
> diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
> index 223e5a5c23c5..85cfbe6bb132 100644
> --- a/drivers/of/overlay_test.c
> +++ b/drivers/of/overlay_test.c
> @@ -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.
>
> static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
>
> > > [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
> >
> > 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<----
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..fa7b439e9402 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
of_platform_device_create(node, buf, NULL);
}
+ node = of_find_node_by_path("/");
+ if (node)
+ of_node_set_flag(node, OF_POPULATED_BUS);
+ 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?
On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Stephen Boyd (2024-05-02 18:27:42)
> > Quoting David Gow (2024-05-01 01:08:11)
> > >
> > > 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:
> > > > [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<---
> > diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
> > index 223e5a5c23c5..85cfbe6bb132 100644
> > --- a/drivers/of/overlay_test.c
> > +++ b/drivers/of/overlay_test.c
> > @@ -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.
>
> >
> > static struct kunit_case of_overlay_apply_kunit_test_cases[] = {
> >
> > > > [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
> > >
> > > 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<----
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 389d4ea6bfc1..fa7b439e9402 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
> of_platform_device_create(node, buf, NULL);
> }
>
> + 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.
> + 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?
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.
Rob
Quoting Rob Herring (2024-05-15 06:06:09)
> On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <[email protected]> wrote:
> >
> > 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<----
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 389d4ea6bfc1..fa7b439e9402 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
> > of_platform_device_create(node, buf, NULL);
> > }
> >
> > + 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<---
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..acecefcfdba7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root,
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);
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.
---8<---
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 389d4ea6bfc1..38dfafc25d86 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -735,10 +735,6 @@ static int of_platform_notify(struct notifier_block *nb,
switch (of_reconfig_get_state_change(action, rd)) {
case OF_RECONFIG_CHANGE_ADD:
- /* verify that the parent is a bus */
- if (!of_node_check_flag(rd->dn->parent, OF_POPULATED_BUS))
- return NOTIFY_OK; /* not for us */
-
/* already populated? (driver using of_populate manually) */
if (of_node_check_flag(rd->dn, OF_POPULATED))
return NOTIFY_OK;
>
> > + 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?
>
> Overlays adding things to the root node might be suspect, but probably
> there are some valid reasons to do so.
In this case we're using it to add nodes without a reg property to the
root node.
> 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.
>
Ok. Sounds like adding these nodes to the root node is the right way
then.
I wonder if we can make MMIO devices appear on the kunit bus by adding
DT support to the bus and then letting those nodes have reg properties
that we "sinkhole" by making those iomem resources point to something
else in the ioremap code. Then we can work in MMIO kunit emulation that
way to let tests check code that works with readl/writel, e.g. the clk
gate tests.
On Wed, May 15, 2024 at 4:15 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Rob Herring (2024-05-15 06:06:09)
> > On Tue, May 14, 2024 at 4:29 PM Stephen Boyd <[email protected]> wrote:
> > >
> > > 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<----
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 389d4ea6bfc1..fa7b439e9402 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -565,6 +565,10 @@ static int __init of_platform_default_populate_init(void)
> > > of_platform_device_create(node, buf, NULL);
> > > }
> > >
> > > + 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<---
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 389d4ea6bfc1..acecefcfdba7 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root,
> 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...
> 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.
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).
> 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.
Rob
Quoting Rob Herring (2024-05-15 15:08:47)
> On Wed, May 15, 2024 at 4:15 PM Stephen Boyd <[email protected]> wrote:
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 389d4ea6bfc1..acecefcfdba7 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -421,6 +421,7 @@ int of_platform_bus_probe(struct device_node *root,
> > 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...
Ok. I'm not intending to send this patch.
>
> > 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.
>
> Huh. Maybe pseries doesn't have any platform devices?
Looks like it.
>
> 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).
>
> > 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.
>
Alright. I've added a check to see if the root node is the parent to
allow it. That works well enough, so I'll send that in v5.
Quoting Stephen Boyd (2024-04-22 16:23:59)
> diff --git a/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml b/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
> new file mode 100644
> index 000000000000..b9f58cba944c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/test/test,clk-fixed-rate.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/test/test,clk-fixed-rate.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: KUnit clk fixed rate test clk consumer
Removed KUnit from the title.
> +
> +maintainers:
> + - Stephen Boyd <[email protected]>
> +
> +description:
> + A clk consumer of a fixed rate clk used to test the fixed rate clk
> + implementation.
> +
> +properties:
> + compatible:
> + const: test,clk-fixed-rate
None of this is really fixed rate clk specific. I'm going to rename this
to "test,single-clk-consumer" so that it can be used anywhere we want to
consume a single clock cell. Someone can introduce a double/multiple
binding as needed.
> +
> + clocks:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + clock-consumer {
> + compatible = "test,clk-fixed-rate";
> + clocks = <&fixed_clk>;
> + };
> +...