KUnit tests often need to provide a struct device, and thus far have
mostly been using root_device_register() or platform devices to create
a 'fake device' for use with, e.g., code which uses device-managed
resources. This has several disadvantages, including not being designed
for test use, scattering files in sysfs, and requiring manual teardown
on test exit, which may not always be possible in case of failure.
Instead, introduce a set of helper functions which allow devices
(internally a struct kunit_device) to be created and managed by KUnit --
i.e., they will be automatically unregistered on test exit. These
helpers can either use a user-provided struct device_driver, or have one
automatically created and managed by KUnit. In both cases, the device
lives on a new kunit_bus.
This is a follow-up to a previous proposal here:
https://lore.kernel.org/linux-kselftest/[email protected]/
(The kunit_defer() function in the first patch there has since been
merged as the 'deferred actions' feature.)
My intention is to take this whole series in via the kselftest/kunit
branch, but I'm equally okay with splitting up the later patches which
use this to go via the various subsystem trees in case there are merge
conflicts.
Cheers,
-- David
Signed-off-by: David Gow <[email protected]>
---
David Gow (4):
kunit: Add APIs for managing devices
fortify: test: Use kunit_device
overflow: Replace fake root_device with kunit_device
ASoC: topology: Replace fake root_device with kunit_device in tests
Documentation/dev-tools/kunit/usage.rst | 49 +++++++++
include/kunit/device.h | 76 ++++++++++++++
lib/fortify_kunit.c | 5 +-
lib/kunit/Makefile | 3 +-
lib/kunit/device.c | 176 ++++++++++++++++++++++++++++++++
lib/kunit/kunit-test.c | 68 +++++++++++-
lib/kunit/test.c | 3 +
lib/overflow_kunit.c | 5 +-
sound/soc/soc-topology-test.c | 11 +-
9 files changed, 382 insertions(+), 14 deletions(-)
---
base-commit: c8613be119892ccceffbc550b9b9d7d68b995c9e
change-id: 20230718-kunit_bus-ab19c4ef48dc
Best regards,
--
David Gow <[email protected]>
Tests for drivers often require a struct device to pass to other
functions. While it's possible to create these with
root_device_register(), or to use something like a platform device, this
is both a misuse of those APIs, and can be difficult to clean up after,
for example, a failed assertion.
Add some KUnit-specific functions for registering and unregistering a
struct device:
- kunit_device_register()
- kunit_device_register_with_driver()
- kunit_device_unregister()
These helpers allocate a on a 'kunit' bus which will either probe the
driver passed in (kunit_device_register_with_driver), or will create a
stub driver (kunit_device_register) which is cleaned up on test shutdown.
Devices are automatically unregistered on test shutdown, but can be
manually unregistered earlier with kunit_device_unregister() in order
to, for example, test device release code.
Signed-off-by: David Gow <[email protected]>
---
Documentation/dev-tools/kunit/usage.rst | 49 +++++++++
include/kunit/device.h | 76 ++++++++++++++
lib/kunit/Makefile | 3 +-
lib/kunit/device.c | 176 ++++++++++++++++++++++++++++++++
lib/kunit/kunit-test.c | 68 +++++++++++-
lib/kunit/test.c | 3 +
6 files changed, 373 insertions(+), 2 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 9db12e91668e..a222a98edceb 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -797,3 +797,52 @@ structures as shown below:
KUnit is not enabled, or if no test is running in the current task, it will do
nothing. This compiles down to either a no-op or a static key check, so will
have a negligible performance impact when no test is running.
+
+Managing Fake Devcices and Drivers
+----------------------------------
+
+When testing drivers or code which interacts with drivers, many functions will
+require a ``struct device`` or ``struct device_driver``. In many cases, setting
+up a real device is not required to test any given function, so a fake device
+can be used instead.
+
+KUnit provides helper functions to create and manage these fake devices, which
+are internally of type ``struct kunit_device``, and are attached to a special
+``kunit_bus``. These devices support managed device resources (devres), as
+described in Documentation/driver-api/driver-model/devres.rst
+
+To create a KUnit-managed ``struct device_driver``, use ``kunit_driver_create()``,
+which will create a driver with the given name, on the ``kunit_bus``. This driver
+will automatically be destroyed when the corresponding test finishes, but can also
+be manually destroyed with ``driver_unregister()``.
+
+To create a fake device, use the ``kunit_device_register()``, which will create
+and register a device, using a new KUnit-managed driver created with ``kunit_driver_create()``.
+To provide a specific, non-KUnit-managed driver, use ``kunit_device_register_with_driver()``
+instead. Like with managed drivers, KUnit-managed fake devices are automatically
+cleaned up when the test finishes, but can be manually cleaned up early with
+``kunit_device_unregister()``.
+
+The KUnit devices should be used in preference to ``root_device_register()``, and
+instead of ``platform_device_register()`` in cases where the device is not otherwise
+a platform device.
+
+For example:
+
+.. code-block:: c
+
+ #include <kunit/device.h>
+
+ static void test_my_device(struct kunit *test)
+ {
+ struct device *fake_device;
+ const char *dev_managed_string;
+
+ // Create a fake device.
+ fake_device = kunit_device_register(test, "my_device");
+
+ // Pass it to functions which need a device.
+ dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");
+
+ // Everything is cleaned up automatically when the test ends.
+ }
\ No newline at end of file
diff --git a/include/kunit/device.h b/include/kunit/device.h
new file mode 100644
index 000000000000..fd2193bc55f1
--- /dev/null
+++ b/include/kunit/device.h
@@ -0,0 +1,76 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit basic device implementation
+ *
+ * Helpers for creating and managing fake devices for KUnit tests.
+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <[email protected]>
+ */
+
+#ifndef _KUNIT_DEVICE_H
+#define _KUNIT_DEVICE_H
+
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test.h>
+
+struct kunit_device;
+struct device;
+struct device_driver;
+
+// For internal use only -- registers the kunit_bus.
+int kunit_bus_init(void);
+
+/**
+ * kunit_driver_create() - Create a struct device_driver attached to the kunit_bus
+ * @test: The test context object.
+ * @name: The name to give the created driver.
+ *
+ * Creates a struct device_driver attached to the kunit_bus, with the name @name.
+ * This driver will automatically be cleaned up on test exit.
+ */
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name);
+
+/**
+ * kunit_device_register() - Create a struct device for use in KUnit tests
+ * @test: The test context object.
+ * @name: The name to give the created device.
+ *
+ * Creates a struct kunit_device (which is a struct device) with the given name,
+ * and a corresponding driver. The device and driver will be cleaned up on test
+ * exit, or when kunit_device_unregister is called. See also
+ * kunit_device_register_with_driver, if you wish to provide your own
+ * struct device_driver.
+ */
+struct device *kunit_device_register(struct kunit *test, const char *name);
+
+/**
+ * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
+ * @test: The test context object.
+ * @name: The name to give the created device.
+ * @drv: The struct device_driver to associate with the device.
+ *
+ * Creates a struct kunit_device (which is a struct device) with the given
+ * name, and driver. The device will be cleaned up on test exit, or when
+ * kunit_device_unregister is called. See also kunit_device_register, if you
+ * wish KUnit to create and manage a driver for you
+ */
+struct device *kunit_device_register_with_driver(struct kunit *test,
+ const char *name,
+ struct device_driver *drv);
+
+/**
+ * kunit_device_unregister() - Unregister a KUnit-managed device
+ * @test: The test context object which created the device
+ * @dev: The device.
+ *
+ * Unregisters and destroys a struct device which was created with
+ * kunit_device_register or kunit_device_register_with_driver. If KUnit created
+ * a driver, cleans it up as well.
+ */
+void kunit_device_unregister(struct kunit *test, struct device *dev);
+
+#endif
+
+#endif
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 46f75f23dfe4..309659a32a78 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -7,7 +7,8 @@ kunit-objs += test.o \
assert.o \
try-catch.o \
executor.o \
- attributes.o
+ attributes.o \
+ device.o
ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
kunit-objs += debugfs.o
diff --git a/lib/kunit/device.c b/lib/kunit/device.c
new file mode 100644
index 000000000000..93ace1a2297d
--- /dev/null
+++ b/lib/kunit/device.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit basic device implementation
+ *
+ * Implementation of struct kunit_device helpers.
+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <[email protected]>
+ */
+
+#include <linux/device.h>
+
+#include <kunit/test.h>
+#include <kunit/device.h>
+#include <kunit/resource.h>
+
+
+/* Wrappers for use with kunit_add_action() */
+KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
+KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
+
+static struct device kunit_bus = {
+ .init_name = "kunit"
+};
+
+/* A device owned by a KUnit test. */
+struct kunit_device {
+ struct device dev;
+ struct kunit *owner;
+ /* Force binding to a specific driver. */
+ struct device_driver *driver;
+ /* The driver is managed by KUnit and unique to this device. */
+ bool cleanup_driver;
+};
+
+static inline struct kunit_device *to_kunit_device(struct device *d)
+{
+ return container_of(d, struct kunit_device, dev);
+}
+
+static int kunit_bus_match(struct device *dev, struct device_driver *driver)
+{
+ struct kunit_device *kunit_dev = to_kunit_device(dev);
+
+ if (kunit_dev->driver == driver)
+ return 1;
+
+ return 0;
+}
+
+static struct bus_type kunit_bus_type = {
+ .name = "kunit",
+ .match = kunit_bus_match
+};
+
+int kunit_bus_init(void)
+{
+ int error;
+
+ error = bus_register(&kunit_bus_type);
+ if (!error) {
+ error = device_register(&kunit_bus);
+ if (error)
+ bus_unregister(&kunit_bus_type);
+ }
+ return error;
+}
+late_initcall(kunit_bus_init);
+
+static void kunit_device_release(struct device *d)
+{
+ kfree(to_kunit_device(d));
+}
+
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
+{
+ struct device_driver *driver;
+ int err = -ENOMEM;
+
+ driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
+
+ if (!driver)
+ return ERR_PTR(err);
+
+ driver->name = name;
+ driver->bus = &kunit_bus_type;
+ driver->owner = THIS_MODULE;
+
+ err = driver_register(driver);
+ if (err) {
+ kunit_kfree(test, driver);
+ return ERR_PTR(err);
+ }
+
+ kunit_add_action(test, driver_unregister_wrapper, driver);
+ return driver;
+}
+EXPORT_SYMBOL_GPL(kunit_driver_create);
+
+struct kunit_device *__kunit_device_register_internal(struct kunit *test,
+ const char *name,
+ struct device_driver *drv)
+{
+ struct kunit_device *kunit_dev;
+ int err = -ENOMEM;
+
+ kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
+ if (!kunit_dev)
+ return ERR_PTR(err);
+
+ kunit_dev->owner = test;
+
+ err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
+ if (err) {
+ kfree(kunit_dev);
+ return ERR_PTR(err);
+ }
+
+ /* Set the expected driver pointer, so we match. */
+ kunit_dev->driver = drv;
+
+ kunit_dev->dev.release = kunit_device_release;
+ kunit_dev->dev.bus = &kunit_bus_type;
+ kunit_dev->dev.parent = &kunit_bus;
+
+ err = device_register(&kunit_dev->dev);
+ if (err) {
+ put_device(&kunit_dev->dev);
+ return ERR_PTR(err);
+ }
+
+ kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
+
+ return kunit_dev;
+}
+
+struct device *kunit_device_register_with_driver(struct kunit *test,
+ const char *name,
+ struct device_driver *drv)
+{
+ struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
+
+ if (IS_ERR_OR_NULL(kunit_dev))
+ return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
+
+ return &kunit_dev->dev;
+}
+EXPORT_SYMBOL_GPL(kunit_device_register_with_driver);
+
+struct device *kunit_device_register(struct kunit *test, const char *name)
+{
+ struct device_driver *drv = kunit_driver_create(test, name);
+ struct kunit_device *dev;
+
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drv);
+
+ dev = __kunit_device_register_internal(test, name, drv);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+ dev->cleanup_driver = true;
+
+ return (struct device *)dev;
+}
+EXPORT_SYMBOL_GPL(kunit_device_register);
+
+void kunit_device_unregister(struct kunit *test, struct device *dev)
+{
+ bool cleanup_driver = ((struct kunit_device *)dev)->cleanup_driver;
+ struct device_driver *driver = ((struct kunit_device *)dev)->driver;
+
+ kunit_release_action(test, device_unregister_wrapper, dev);
+ if (cleanup_driver)
+ kunit_release_action(test, driver_unregister_wrapper, driver);
+}
+EXPORT_SYMBOL_GPL(kunit_device_unregister);
+
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 3e9c5192d095..a4007158bf36 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -8,6 +8,9 @@
#include <kunit/test.h>
#include <kunit/test-bug.h>
+#include <linux/device.h>
+#include <kunit/device.h>
+
#include "string-stream.h"
#include "try-catch-impl.h"
@@ -687,6 +690,69 @@ static struct kunit_case kunit_current_test_cases[] = {
{}
};
+static void test_dev_action(void *priv)
+{
+ *(void **)priv = (void *)1;
+}
+
+static void kunit_device_test(struct kunit *test)
+{
+ struct device *test_device;
+
+ test_device = kunit_device_register(test, "my_device");
+
+ KUNIT_ASSERT_NOT_NULL(test, test_device);
+
+ // Add an action to verify cleanup.
+ devm_add_action(test_device, test_dev_action, &test->priv);
+
+ KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)0);
+
+ kunit_device_unregister(test, test_device);
+
+ KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)1);
+}
+
+static void kunit_device_driver_test(struct kunit *test)
+{
+ struct device_driver *test_driver;
+ struct device *test_device;
+
+ test_driver = kunit_driver_create(test, "my_driver");
+
+ KUNIT_ASSERT_NOT_NULL(test, test_driver);
+
+ test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
+
+ KUNIT_ASSERT_NOT_NULL(test, test_device);
+
+ // Add an action to verify cleanup.
+ devm_add_action(test_device, test_dev_action, &test->priv);
+
+ KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)0);
+
+ kunit_device_unregister(test, test_device);
+ test_device = NULL;
+
+ // The driver should not automatically be destroyed by
+ // kunit_device_unregister, so we can re-use it.
+ test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
+ KUNIT_ASSERT_NOT_NULL(test, test_device);
+
+ // Everything is automatically freed here.
+}
+
+static struct kunit_case kunit_device_test_cases[] = {
+ KUNIT_CASE(kunit_device_test),
+ KUNIT_CASE(kunit_device_driver_test),
+ {}
+};
+
+static struct kunit_suite kunit_device_test_suite = {
+ .name = "kunit_device",
+ .test_cases = kunit_device_test_cases,
+};
+
static struct kunit_suite kunit_current_test_suite = {
.name = "kunit_current",
.test_cases = kunit_current_test_cases,
@@ -694,6 +760,6 @@ static struct kunit_suite kunit_current_test_suite = {
kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
&kunit_log_test_suite, &kunit_status_test_suite,
- &kunit_current_test_suite);
+ &kunit_current_test_suite, &kunit_device_test_suite);
MODULE_LICENSE("GPL v2");
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 0308865194bb..144c8e7be197 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -10,6 +10,7 @@
#include <kunit/test.h>
#include <kunit/test-bug.h>
#include <kunit/attributes.h>
+#include <kunit/device.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
@@ -840,6 +841,8 @@ static int __init kunit_init(void)
kunit_install_hooks();
kunit_debugfs_init();
+
+ kunit_bus_init();
#ifdef CONFIG_MODULES
return register_module_notifier(&kunit_mod_nb);
#else
--
2.43.0.rc2.451.g8631bc7472-goog
Using struct root_device to create fake devices for tests is something
of a hack. The new struct kunit_device is meant for this purpose, so use
it instead.
Signed-off-by: David Gow <[email protected]>
---
lib/fortify_kunit.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
index c8c33cbaae9e..f7a1fce8849b 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c
@@ -16,6 +16,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <kunit/test.h>
+#include <kunit/device.h>
#include <linux/device.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -269,7 +270,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
size_t len; \
\
/* Create dummy device for devm_kmalloc()-family tests. */ \
- dev = root_device_register(dev_name); \
+ dev = kunit_device_register(test, dev_name); \
KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), \
"Cannot register test device\n"); \
\
@@ -303,7 +304,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
checker(len, devm_kmemdup(dev, "Ohai", len, gfp), \
devm_kfree(dev, p)); \
\
- device_unregister(dev); \
+ kunit_device_unregister(test, dev); \
} while (0)
DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
--
2.43.0.rc2.451.g8631bc7472-goog
Using struct root_device to create fake devices for tests is something
of a hack. The new struct kunit_device is meant for this purpose, so use
it instead.
Signed-off-by: David Gow <[email protected]>
---
lib/overflow_kunit.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index 34db0b3aa502..91b03217c462 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c
@@ -7,6 +7,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <kunit/test.h>
+#include <kunit/device.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/mm.h>
@@ -618,7 +619,7 @@ static void overflow_allocation_test(struct kunit *test)
} while (0)
/* Create dummy device for devm_kmalloc()-family tests. */
- dev = root_device_register(device_name);
+ dev = kunit_device_register(test, device_name);
KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev),
"Cannot register test device\n");
@@ -634,7 +635,7 @@ static void overflow_allocation_test(struct kunit *test)
check_allocation_overflow(devm_kmalloc);
check_allocation_overflow(devm_kzalloc);
- device_unregister(dev);
+ kunit_device_unregister(test, dev);
kunit_info(test, "%d allocation overflow tests finished\n", count);
#undef check_allocation_overflow
--
2.43.0.rc2.451.g8631bc7472-goog
Using struct root_device to create fake devices for tests is something
of a hack. The new struct kunit_device is meant for this purpose, so use
it instead.
Signed-off-by: David Gow <[email protected]>
---
sound/soc/soc-topology-test.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-topology-test.c b/sound/soc/soc-topology-test.c
index 2cd3540cec04..1d7696e5bffc 100644
--- a/sound/soc/soc-topology-test.c
+++ b/sound/soc/soc-topology-test.c
@@ -10,6 +10,7 @@
#include <sound/soc.h>
#include <sound/soc-topology.h>
#include <kunit/test.h>
+#include <kunit/device.h>
/* ===== HELPER FUNCTIONS =================================================== */
@@ -21,26 +22,20 @@
*/
static struct device *test_dev;
-static struct device_driver test_drv = {
- .name = "sound-soc-topology-test-driver",
-};
-
static int snd_soc_tplg_test_init(struct kunit *test)
{
- test_dev = root_device_register("sound-soc-topology-test");
+ test_dev = kunit_device_register(test, "sound-soc-topology-test");
test_dev = get_device(test_dev);
if (!test_dev)
return -ENODEV;
- test_dev->driver = &test_drv;
-
return 0;
}
static void snd_soc_tplg_test_exit(struct kunit *test)
{
put_device(test_dev);
- root_device_unregister(test_dev);
+ kunit_device_unregister(test, test_dev);
}
/*
--
2.43.0.rc2.451.g8631bc7472-goog
On 12/5/23 09:31, [email protected] wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
>
> Add some KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_register_with_driver()
> - kunit_device_unregister()
Thanks a lot David! I have been missing these!
I love the explanation you added under Documentation. Very helpful I'd
say. I only have very minor comments which you can ignore if they don't
make sense to you or the kunit-subsystem.
With or without the suggested changes:
Reviewed-by: Matti Vaittinen <[email protected]>
> --- /dev/null
> +++ b/include/kunit/device.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit basic device implementation
> + *
> + * Helpers for creating and managing fake devices for KUnit tests.
> + *
> + * Copyright (C) 2023, Google LLC.
> + * Author: David Gow <[email protected]>
> + */
> +
> +#ifndef _KUNIT_DEVICE_H
> +#define _KUNIT_DEVICE_H
> +
> +#if IS_ENABLED(CONFIG_KUNIT)
> +
> +#include <kunit/test.h>
> +
> +struct kunit_device;
> +struct device;
> +struct device_driver;
> +
> +// For internal use only -- registers the kunit_bus.
> +int kunit_bus_init(void);
> +
> +/**
> + * kunit_driver_create() - Create a struct device_driver attached to the kunit_bus
> + * @test: The test context object.
> + * @name: The name to give the created driver.
> + *
> + * Creates a struct device_driver attached to the kunit_bus, with the name @name.
> + * This driver will automatically be cleaned up on test exit.
> + */
> +struct device_driver *kunit_driver_create(struct kunit *test, const char *name);
> +
> +/**
> + * kunit_device_register() - Create a struct device for use in KUnit tests
> + * @test: The test context object.
> + * @name: The name to give the created device.
> + *
> + * Creates a struct kunit_device (which is a struct device) with the given name,
> + * and a corresponding driver. The device and driver will be cleaned up on test
> + * exit, or when kunit_device_unregister is called. See also
> + * kunit_device_register_with_driver, if you wish to provide your own
> + * struct device_driver.
> + */
> +struct device *kunit_device_register(struct kunit *test, const char *name);
> +
> +/**
> + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> + * @test: The test context object.
> + * @name: The name to give the created device.
> + * @drv: The struct device_driver to associate with the device.
> + *
> + * Creates a struct kunit_device (which is a struct device) with the given
> + * name, and driver. The device will be cleaned up on test exit, or when
> + * kunit_device_unregister is called. See also kunit_device_register, if you
> + * wish KUnit to create and manage a driver for you
> + */
> +struct device *kunit_device_register_with_driver(struct kunit *test,
> + const char *name,
> + struct device_driver *drv);
> +
> +/**
> + * kunit_device_unregister() - Unregister a KUnit-managed device
> + * @test: The test context object which created the device
> + * @dev: The device.
> + *
> + * Unregisters and destroys a struct device which was created with
> + * kunit_device_register or kunit_device_register_with_driver. If KUnit created
> + * a driver, cleans it up as well.
> + */
> +void kunit_device_unregister(struct kunit *test, struct device *dev);
I wish the return values for error case(s) were also mentioned. But
please, see my next comment as well.
> +
> +#endif
> +
> +#endif
...
> diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> new file mode 100644
> index 000000000000..93ace1a2297d
> --- /dev/null
> +++ b/lib/kunit/device.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit basic device implementation
> + *
> + * Implementation of struct kunit_device helpers.
> + *
> + * Copyright (C) 2023, Google LLC.
> + * Author: David Gow <[email protected]>
> + */
> +
...
> +
> +static void kunit_device_release(struct device *d)
> +{
> + kfree(to_kunit_device(d));
> +}
I see you added the function documentation to the header. I assume this
is the kunit style(?) I may be heretical, but I'd love to see at least a
very short documentation for (all) exported functions here. I think the
arguments are mostly self-explatonary, but at least for me the return
values aren't that obvious. Whether they are kerneldoc or not is not
that important to me.
I think you did a great job adding docs under Documentation/ (and the
header) - but at least I tend to just jump to function implementation
when I need to figure out how it behaves. Having doc (or pointer to doc)
also here helps. I don't think it's that widely spread practice to add
docs to the headers(?)
> +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> +{
> + struct device_driver *driver;
> + int err = -ENOMEM;
> +
> + driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> +
> + if (!driver)
> + return ERR_PTR(err);
> +
> + driver->name = name;
> + driver->bus = &kunit_bus_type;
> + driver->owner = THIS_MODULE;
> +
> + err = driver_register(driver);
> + if (err) {
> + kunit_kfree(test, driver);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, driver_unregister_wrapper, driver);
> + return driver;
> +}
> +EXPORT_SYMBOL_GPL(kunit_driver_create);
> +
> +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> + const char *name,
> + struct device_driver *drv)
Very much nitpicking only - but do you think either the "__"-prefix or
the "_internal"-suffix would be enough and not both? (Just to make
function a tad shorter, not that it matters much though).
> +{
> + struct kunit_device *kunit_dev;
> + int err = -ENOMEM;
> +
> + kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
> + if (!kunit_dev)
> + return ERR_PTR(err);
> +
> + kunit_dev->owner = test;
> +
> + err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> + if (err) {
> + kfree(kunit_dev);
> + return ERR_PTR(err);
> + }
> +
> + /* Set the expected driver pointer, so we match. */
> + kunit_dev->driver = drv;
> +
> + kunit_dev->dev.release = kunit_device_release;
> + kunit_dev->dev.bus = &kunit_bus_type;
> + kunit_dev->dev.parent = &kunit_bus;
> +
> + err = device_register(&kunit_dev->dev);
> + if (err) {
> + put_device(&kunit_dev->dev);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> +
> + return kunit_dev;
> +}
...
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 12/5/23 09:31, [email protected] wrote:
> Using struct root_device to create fake devices for tests is something
> of a hack. The new struct kunit_device is meant for this purpose, so use
> it instead.
>
> Signed-off-by: David Gow <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 12/5/23 09:31, [email protected] wrote:
> Using struct root_device to create fake devices for tests is something
> of a hack. The new struct kunit_device is meant for this purpose, so use
> it instead.
>
> Signed-off-by: David Gow <[email protected]>
Reviewed-by: Matti Vaittinen <[email protected]>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Hi David,
Thanks a lot for working on this.
On Tue, Dec 05, 2023 at 03:31:33PM +0800, [email protected] wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
>
> Add some KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_register_with_driver()
> - kunit_device_unregister()
>
> These helpers allocate a on a 'kunit' bus which will either probe the
> driver passed in (kunit_device_register_with_driver), or will create a
> stub driver (kunit_device_register) which is cleaned up on test shutdown.
>
> Devices are automatically unregistered on test shutdown, but can be
> manually unregistered earlier with kunit_device_unregister() in order
> to, for example, test device release code.
>
> Signed-off-by: David Gow <[email protected]>
> ---
> Documentation/dev-tools/kunit/usage.rst | 49 +++++++++
> include/kunit/device.h | 76 ++++++++++++++
> lib/kunit/Makefile | 3 +-
> lib/kunit/device.c | 176 ++++++++++++++++++++++++++++++++
> lib/kunit/kunit-test.c | 68 +++++++++++-
> lib/kunit/test.c | 3 +
> 6 files changed, 373 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 9db12e91668e..a222a98edceb 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -797,3 +797,52 @@ structures as shown below:
> KUnit is not enabled, or if no test is running in the current task, it will do
> nothing. This compiles down to either a no-op or a static key check, so will
> have a negligible performance impact when no test is running.
> +
> +Managing Fake Devcices and Drivers
^ Devices
> +----------------------------------
> +
> +When testing drivers or code which interacts with drivers, many functions will
> +require a ``struct device`` or ``struct device_driver``. In many cases, setting
> +up a real device is not required to test any given function, so a fake device
> +can be used instead.
> +
> +KUnit provides helper functions to create and manage these fake devices, which
> +are internally of type ``struct kunit_device``, and are attached to a special
> +``kunit_bus``. These devices support managed device resources (devres), as
> +described in Documentation/driver-api/driver-model/devres.rst
> +
> +To create a KUnit-managed ``struct device_driver``, use ``kunit_driver_create()``,
> +which will create a driver with the given name, on the ``kunit_bus``. This driver
> +will automatically be destroyed when the corresponding test finishes, but can also
> +be manually destroyed with ``driver_unregister()``.
> +
> +To create a fake device, use the ``kunit_device_register()``, which will create
> +and register a device, using a new KUnit-managed driver created with ``kunit_driver_create()``.
> +To provide a specific, non-KUnit-managed driver, use ``kunit_device_register_with_driver()``
> +instead. Like with managed drivers, KUnit-managed fake devices are automatically
> +cleaned up when the test finishes, but can be manually cleaned up early with
> +``kunit_device_unregister()``.
I think we should add a test for that, just like we did for root and
platform devices. We've been bitten by that before :)
> +The KUnit devices should be used in preference to ``root_device_register()``, and
> +instead of ``platform_device_register()`` in cases where the device is not otherwise
> +a platform device.
> +
> +For example:
> +
> +.. code-block:: c
> +
> + #include <kunit/device.h>
> +
> + static void test_my_device(struct kunit *test)
> + {
> + struct device *fake_device;
> + const char *dev_managed_string;
> +
> + // Create a fake device.
> + fake_device = kunit_device_register(test, "my_device");
> +
> + // Pass it to functions which need a device.
> + dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");
> +
> + // Everything is cleaned up automatically when the test ends.
> + }
> \ No newline at end of file
> diff --git a/include/kunit/device.h b/include/kunit/device.h
> new file mode 100644
> index 000000000000..fd2193bc55f1
> --- /dev/null
> +++ b/include/kunit/device.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit basic device implementation
> + *
> + * Helpers for creating and managing fake devices for KUnit tests.
> + *
> + * Copyright (C) 2023, Google LLC.
> + * Author: David Gow <[email protected]>
> + */
> +
> +#ifndef _KUNIT_DEVICE_H
> +#define _KUNIT_DEVICE_H
> +
> +#if IS_ENABLED(CONFIG_KUNIT)
> +
> +#include <kunit/test.h>
> +
> +struct kunit_device;
> +struct device;
> +struct device_driver;
> +
> +// For internal use only -- registers the kunit_bus.
> +int kunit_bus_init(void);
> +
> +/**
> + * kunit_driver_create() - Create a struct device_driver attached to the kunit_bus
> + * @test: The test context object.
> + * @name: The name to give the created driver.
> + *
> + * Creates a struct device_driver attached to the kunit_bus, with the name @name.
> + * This driver will automatically be cleaned up on test exit.
> + */
> +struct device_driver *kunit_driver_create(struct kunit *test, const char *name);
> +
> +/**
> + * kunit_device_register() - Create a struct device for use in KUnit tests
> + * @test: The test context object.
> + * @name: The name to give the created device.
> + *
> + * Creates a struct kunit_device (which is a struct device) with the given name,
> + * and a corresponding driver. The device and driver will be cleaned up on test
> + * exit, or when kunit_device_unregister is called. See also
> + * kunit_device_register_with_driver, if you wish to provide your own
> + * struct device_driver.
> + */
> +struct device *kunit_device_register(struct kunit *test, const char *name);
> +
> +/**
> + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> + * @test: The test context object.
> + * @name: The name to give the created device.
> + * @drv: The struct device_driver to associate with the device.
> + *
> + * Creates a struct kunit_device (which is a struct device) with the given
> + * name, and driver. The device will be cleaned up on test exit, or when
> + * kunit_device_unregister is called. See also kunit_device_register, if you
> + * wish KUnit to create and manage a driver for you
> + */
> +struct device *kunit_device_register_with_driver(struct kunit *test,
> + const char *name,
> + struct device_driver *drv);
> +
> +/**
> + * kunit_device_unregister() - Unregister a KUnit-managed device
> + * @test: The test context object which created the device
> + * @dev: The device.
> + *
> + * Unregisters and destroys a struct device which was created with
> + * kunit_device_register or kunit_device_register_with_driver. If KUnit created
> + * a driver, cleans it up as well.
> + */
> +void kunit_device_unregister(struct kunit *test, struct device *dev);
> +
> +#endif
> +
> +#endif
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 46f75f23dfe4..309659a32a78 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -7,7 +7,8 @@ kunit-objs += test.o \
> assert.o \
> try-catch.o \
> executor.o \
> - attributes.o
> + attributes.o \
> + device.o
>
> ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> kunit-objs += debugfs.o
> diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> new file mode 100644
> index 000000000000..93ace1a2297d
> --- /dev/null
> +++ b/lib/kunit/device.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit basic device implementation
> + *
> + * Implementation of struct kunit_device helpers.
> + *
> + * Copyright (C) 2023, Google LLC.
> + * Author: David Gow <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +
> +#include <kunit/test.h>
> +#include <kunit/device.h>
> +#include <kunit/resource.h>
> +
> +
> +/* Wrappers for use with kunit_add_action() */
> +KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
> +KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
> +
> +static struct device kunit_bus = {
> + .init_name = "kunit"
> +};
> +
> +/* A device owned by a KUnit test. */
> +struct kunit_device {
> + struct device dev;
> + struct kunit *owner;
> + /* Force binding to a specific driver. */
> + struct device_driver *driver;
> + /* The driver is managed by KUnit and unique to this device. */
> + bool cleanup_driver;
> +};
> +
> +static inline struct kunit_device *to_kunit_device(struct device *d)
> +{
> + return container_of(d, struct kunit_device, dev);
> +}
> +
> +static int kunit_bus_match(struct device *dev, struct device_driver *driver)
> +{
> + struct kunit_device *kunit_dev = to_kunit_device(dev);
> +
> + if (kunit_dev->driver == driver)
> + return 1;
> +
> + return 0;
> +}
> +
> +static struct bus_type kunit_bus_type = {
> + .name = "kunit",
> + .match = kunit_bus_match
> +};
> +
> +int kunit_bus_init(void)
> +{
> + int error;
> +
> + error = bus_register(&kunit_bus_type);
> + if (!error) {
> + error = device_register(&kunit_bus);
> + if (error)
> + bus_unregister(&kunit_bus_type);
> + }
> + return error;
> +}
> +late_initcall(kunit_bus_init);
> +
> +static void kunit_device_release(struct device *d)
> +{
> + kfree(to_kunit_device(d));
> +}
> +
> +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> +{
> + struct device_driver *driver;
> + int err = -ENOMEM;
> +
> + driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> +
> + if (!driver)
> + return ERR_PTR(err);
> +
> + driver->name = name;
> + driver->bus = &kunit_bus_type;
> + driver->owner = THIS_MODULE;
> +
> + err = driver_register(driver);
> + if (err) {
> + kunit_kfree(test, driver);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, driver_unregister_wrapper, driver);
> + return driver;
> +}
> +EXPORT_SYMBOL_GPL(kunit_driver_create);
> +
> +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> + const char *name,
> + struct device_driver *drv)
> +{
> + struct kunit_device *kunit_dev;
> + int err = -ENOMEM;
> +
> + kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
The preferred syntax is sizeof(*kunit_dev) here
> + if (!kunit_dev)
> + return ERR_PTR(err);
> +
> + kunit_dev->owner = test;
> +
> + err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> + if (err) {
> + kfree(kunit_dev);
> + return ERR_PTR(err);
> + }
> +
> + /* Set the expected driver pointer, so we match. */
> + kunit_dev->driver = drv;
> +
> + kunit_dev->dev.release = kunit_device_release;
> + kunit_dev->dev.bus = &kunit_bus_type;
> + kunit_dev->dev.parent = &kunit_bus;
> +
> + err = device_register(&kunit_dev->dev);
> + if (err) {
> + put_device(&kunit_dev->dev);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> +
> + return kunit_dev;
> +}
> +
> +struct device *kunit_device_register_with_driver(struct kunit *test,
> + const char *name,
> + struct device_driver *drv)
> +{
> + struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
> +
> + if (IS_ERR_OR_NULL(kunit_dev))
> + return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
> +
> + return &kunit_dev->dev;
> +}
> +EXPORT_SYMBOL_GPL(kunit_device_register_with_driver);
> +
> +struct device *kunit_device_register(struct kunit *test, const char *name)
> +{
> + struct device_driver *drv = kunit_driver_create(test, name);
> + struct kunit_device *dev;
> +
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drv);
> +
> + dev = __kunit_device_register_internal(test, name, drv);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
> +
> + dev->cleanup_driver = true;
> +
> + return (struct device *)dev;
> +}
> +EXPORT_SYMBOL_GPL(kunit_device_register);
> +
> +void kunit_device_unregister(struct kunit *test, struct device *dev)
> +{
> + bool cleanup_driver = ((struct kunit_device *)dev)->cleanup_driver;
> + struct device_driver *driver = ((struct kunit_device *)dev)->driver;
> +
> + kunit_release_action(test, device_unregister_wrapper, dev);
> + if (cleanup_driver)
> + kunit_release_action(test, driver_unregister_wrapper, driver);
> +}
> +EXPORT_SYMBOL_GPL(kunit_device_unregister);
> +
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 3e9c5192d095..a4007158bf36 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -8,6 +8,9 @@
> #include <kunit/test.h>
> #include <kunit/test-bug.h>
>
> +#include <linux/device.h>
> +#include <kunit/device.h>
> +
> #include "string-stream.h"
> #include "try-catch-impl.h"
>
> @@ -687,6 +690,69 @@ static struct kunit_case kunit_current_test_cases[] = {
> {}
> };
>
> +static void test_dev_action(void *priv)
> +{
> + *(void **)priv = (void *)1;
> +}
> +
> +static void kunit_device_test(struct kunit *test)
> +{
> + struct device *test_device;
> +
> + test_device = kunit_device_register(test, "my_device");
> +
> + KUNIT_ASSERT_NOT_NULL(test, test_device);
> +
> + // Add an action to verify cleanup.
> + devm_add_action(test_device, test_dev_action, &test->priv);
> +
> + KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)0);
> +
> + kunit_device_unregister(test, test_device);
> +
> + KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)1);
> +}
Oh, it looks like you do check there, sorry.
I guess I would have expected an explicit test for that, but that works
:)
> +
> +static void kunit_device_driver_test(struct kunit *test)
> +{
> + struct device_driver *test_driver;
> + struct device *test_device;
> +
> + test_driver = kunit_driver_create(test, "my_driver");
> +
> + KUNIT_ASSERT_NOT_NULL(test, test_driver);
> +
> + test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
> +
> + KUNIT_ASSERT_NOT_NULL(test, test_device);
Should we test that the probe (and remove) hooks has been called too?
Maxime
On 12/5/2023 8:31 AM, [email protected] wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
>
> Add some KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_register_with_driver()
> - kunit_device_unregister()
>
> These helpers allocate a on a 'kunit' bus which will either probe the
> driver passed in (kunit_device_register_with_driver), or will create a
> stub driver (kunit_device_register) which is cleaned up on test shutdown.
>
> Devices are automatically unregistered on test shutdown, but can be
> manually unregistered earlier with kunit_device_unregister() in order
> to, for example, test device release code.
>
> Signed-off-by: David Gow <[email protected]>
> ---
> Documentation/dev-tools/kunit/usage.rst | 49 +++++++++
> include/kunit/device.h | 76 ++++++++++++++
> lib/kunit/Makefile | 3 +-
> lib/kunit/device.c | 176 ++++++++++++++++++++++++++++++++
> lib/kunit/kunit-test.c | 68 +++++++++++-
> lib/kunit/test.c | 3 +
> 6 files changed, 373 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 9db12e91668e..a222a98edceb 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -797,3 +797,52 @@ structures as shown below:
> KUnit is not enabled, or if no test is running in the current task, it will do
> nothing. This compiles down to either a no-op or a static key check, so will
> have a negligible performance impact when no test is running.
> +
> +Managing Fake Devcices and Drivers
> +----------------------------------
Typo: Devices
On 12/5/2023 8:31 AM, [email protected] wrote:
> Using struct root_device to create fake devices for tests is something
> of a hack. The new struct kunit_device is meant for this purpose, so use
> it instead.
>
> Signed-off-by: David Gow <[email protected]>
> ---
> sound/soc/soc-topology-test.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/sound/soc/soc-topology-test.c b/sound/soc/soc-topology-test.c
> index 2cd3540cec04..1d7696e5bffc 100644
> --- a/sound/soc/soc-topology-test.c
> +++ b/sound/soc/soc-topology-test.c
> @@ -10,6 +10,7 @@
> #include <sound/soc.h>
> #include <sound/soc-topology.h>
> #include <kunit/test.h>
> +#include <kunit/device.h>
Nitpick:
Can we add device.h before test.h, to keep it in alphabetical order?
>
> /* ===== HELPER FUNCTIONS =================================================== */
>
> @@ -21,26 +22,20 @@
> */
> static struct device *test_dev;
>
> -static struct device_driver test_drv = {
> - .name = "sound-soc-topology-test-driver",
> -};
> -
> static int snd_soc_tplg_test_init(struct kunit *test)
> {
> - test_dev = root_device_register("sound-soc-topology-test");
> + test_dev = kunit_device_register(test, "sound-soc-topology-test");
> test_dev = get_device(test_dev);
> if (!test_dev)
> return -ENODEV;
>
> - test_dev->driver = &test_drv;
> -
> return 0;
> }
>
> static void snd_soc_tplg_test_exit(struct kunit *test)
> {
> put_device(test_dev);
> - root_device_unregister(test_dev);
> + kunit_device_unregister(test, test_dev);
> }
>
> /*
>
On Tue, Dec 05, 2023 at 03:31:36PM +0800, [email protected] wrote:
> Using struct root_device to create fake devices for tests is something
> of a hack. The new struct kunit_device is meant for this purpose, so use
> it instead.
Acked-by: Mark Brown <[email protected]>
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]
url: https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base: c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link: https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: powerpc-randconfig-r081-20231205 (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> lib/kunit/device.c:100:22: warning: no previous prototype for function '__kunit_device_register_internal' [-Wmissing-prototypes]
100 | struct kunit_device *__kunit_device_register_internal(struct kunit *test,
| ^
lib/kunit/device.c:100:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
100 | struct kunit_device *__kunit_device_register_internal(struct kunit *test,
| ^
| static
1 warning generated.
vim +/__kunit_device_register_internal +100 lib/kunit/device.c
99
> 100 struct kunit_device *__kunit_device_register_internal(struct kunit *test,
101 const char *name,
102 struct device_driver *drv)
103 {
104 struct kunit_device *kunit_dev;
105 int err = -ENOMEM;
106
107 kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
108 if (!kunit_dev)
109 return ERR_PTR(err);
110
111 kunit_dev->owner = test;
112
113 err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
114 if (err) {
115 kfree(kunit_dev);
116 return ERR_PTR(err);
117 }
118
119 /* Set the expected driver pointer, so we match. */
120 kunit_dev->driver = drv;
121
122 kunit_dev->dev.release = kunit_device_release;
123 kunit_dev->dev.bus = &kunit_bus_type;
124 kunit_dev->dev.parent = &kunit_bus;
125
126 err = device_register(&kunit_dev->dev);
127 if (err) {
128 put_device(&kunit_dev->dev);
129 return ERR_PTR(err);
130 }
131
132 kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
133
134 return kunit_dev;
135 }
136
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]
url: https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base: c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link: https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: i386-randconfig-141-20231205 (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> lib/kunit/device.c:100:22: warning: no previous prototype for '__kunit_device_register_internal' [-Wmissing-prototypes]
100 | struct kunit_device *__kunit_device_register_internal(struct kunit *test,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for DRM_I915_DEBUG_GEM
Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=m] && EXPERT [=y] && DRM_I915_WERROR [=n]
Selected by [m]:
- DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && DRM_I915 [=m] && EXPERT [=y] && !COMPILE_TEST [=n]
vim +/__kunit_device_register_internal +100 lib/kunit/device.c
99
> 100 struct kunit_device *__kunit_device_register_internal(struct kunit *test,
101 const char *name,
102 struct device_driver *drv)
103 {
104 struct kunit_device *kunit_dev;
105 int err = -ENOMEM;
106
107 kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
108 if (!kunit_dev)
109 return ERR_PTR(err);
110
111 kunit_dev->owner = test;
112
113 err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
114 if (err) {
115 kfree(kunit_dev);
116 return ERR_PTR(err);
117 }
118
119 /* Set the expected driver pointer, so we match. */
120 kunit_dev->driver = drv;
121
122 kunit_dev->dev.release = kunit_device_release;
123 kunit_dev->dev.bus = &kunit_bus_type;
124 kunit_dev->dev.parent = &kunit_bus;
125
126 err = device_register(&kunit_dev->dev);
127 if (err) {
128 put_device(&kunit_dev->dev);
129 return ERR_PTR(err);
130 }
131
132 kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
133
134 return kunit_dev;
135 }
136
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]
url: https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base: c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link: https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: x86_64-randconfig-122-20231205 (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> lib/kunit/device.c:100:21: sparse: sparse: symbol '__kunit_device_register_internal' was not declared. Should it be static?
vim +/__kunit_device_register_internal +100 lib/kunit/device.c
99
> 100 struct kunit_device *__kunit_device_register_internal(struct kunit *test,
101 const char *name,
102 struct device_driver *drv)
103 {
104 struct kunit_device *kunit_dev;
105 int err = -ENOMEM;
106
107 kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
108 if (!kunit_dev)
109 return ERR_PTR(err);
110
111 kunit_dev->owner = test;
112
113 err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
114 if (err) {
115 kfree(kunit_dev);
116 return ERR_PTR(err);
117 }
118
119 /* Set the expected driver pointer, so we match. */
120 kunit_dev->driver = drv;
121
122 kunit_dev->dev.release = kunit_device_release;
123 kunit_dev->dev.bus = &kunit_bus_type;
124 kunit_dev->dev.parent = &kunit_bus;
125
126 err = device_register(&kunit_dev->dev);
127 if (err) {
128 put_device(&kunit_dev->dev);
129 return ERR_PTR(err);
130 }
131
132 kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
133
134 return kunit_dev;
135 }
136
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
kernel test robot noticed the following build errors:
[auto build test ERROR on c8613be119892ccceffbc550b9b9d7d68b995c9e]
url: https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base: c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link: https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: x86_64-buildonly-randconfig-001-20231205 (https://download.01.org/0day-ci/archive/20231205/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
ld: lib/kunit/device.o: in function `kunit_bus_init':
>> device.c:(.text+0x40): multiple definition of `init_module'; lib/kunit/test.o:test.c:(.init.text+0x0): first defined here
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, Dec 05, 2023 at 03:31:33PM +0800, [email protected] wrote:
> Tests for drivers often require a struct device to pass to other
> functions. While it's possible to create these with
> root_device_register(), or to use something like a platform device, this
> is both a misuse of those APIs, and can be difficult to clean up after,
> for example, a failed assertion.
>
> Add some KUnit-specific functions for registering and unregistering a
> struct device:
> - kunit_device_register()
> - kunit_device_register_with_driver()
> - kunit_device_unregister()
>
> These helpers allocate a on a 'kunit' bus which will either probe the
> driver passed in (kunit_device_register_with_driver), or will create a
> stub driver (kunit_device_register) which is cleaned up on test shutdown.
>
> Devices are automatically unregistered on test shutdown, but can be
> manually unregistered earlier with kunit_device_unregister() in order
> to, for example, test device release code.
At first glance, nice work. But looks like 0-day doesn't like it that
much, so I'll wait for the next version to review it properly.
One nit I did notice:
> +// For internal use only -- registers the kunit_bus.
> +int kunit_bus_init(void);
Put stuff like this in a local .h file, don't pollute the include/linux/
files for things that you do not want any other part of the kernel to
call.
> +/**
> + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> + * @test: The test context object.
> + * @name: The name to give the created device.
> + * @drv: The struct device_driver to associate with the device.
> + *
> + * Creates a struct kunit_device (which is a struct device) with the given
> + * name, and driver. The device will be cleaned up on test exit, or when
> + * kunit_device_unregister is called. See also kunit_device_register, if you
> + * wish KUnit to create and manage a driver for you
> + */
> +struct device *kunit_device_register_with_driver(struct kunit *test,
> + const char *name,
> + struct device_driver *drv);
Shouldn't "struct device_driver *" be a constant pointer?
But really, why is this a "raw" device_driver pointer and not a pointer
to the driver type for your bus?
Oh heck, let's point out the other issues as I'm already here...
> @@ -7,7 +7,8 @@ kunit-objs += test.o \
> assert.o \
> try-catch.o \
> executor.o \
> - attributes.o
> + attributes.o \
> + device.o
Shouldn't this file be "bus.c" as you are creating a kunit bus?
>
> ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> kunit-objs += debugfs.o
> diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> new file mode 100644
> index 000000000000..93ace1a2297d
> --- /dev/null
> +++ b/lib/kunit/device.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit basic device implementation
"basic bus/driver implementation", not device, right?
> + *
> + * Implementation of struct kunit_device helpers.
> + *
> + * Copyright (C) 2023, Google LLC.
> + * Author: David Gow <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +
> +#include <kunit/test.h>
> +#include <kunit/device.h>
> +#include <kunit/resource.h>
> +
> +
> +/* Wrappers for use with kunit_add_action() */
> +KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
> +KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
> +
> +static struct device kunit_bus = {
> + .init_name = "kunit"
> +};
A static device as a bus? This feels wrong, what is it for? And where
does this live? If you _REALLY_ want a single device for the root of
your bus (which is a good idea), then make it a dynamic variable (as it
is reference counted), NOT a static struct device which should not be
done if at all possible.
> +
> +/* A device owned by a KUnit test. */
> +struct kunit_device {
> + struct device dev;
> + struct kunit *owner;
> + /* Force binding to a specific driver. */
> + struct device_driver *driver;
> + /* The driver is managed by KUnit and unique to this device. */
> + bool cleanup_driver;
> +};
Wait, why isn't your "kunit" device above a struct kunit_device
structure? Why is it ok to be a "raw" struct device (hint, that's
almost never a good idea.)
> +static inline struct kunit_device *to_kunit_device(struct device *d)
> +{
> + return container_of(d, struct kunit_device, dev);
container_of_const()? And to use that properly, why not make this a #define?
> +}
> +
> +static int kunit_bus_match(struct device *dev, struct device_driver *driver)
> +{
> + struct kunit_device *kunit_dev = to_kunit_device(dev);
> +
> + if (kunit_dev->driver == driver)
> + return 1;
> +
> + return 0;
I don't understand, what are you trying to match here?
> +}
> +
> +static struct bus_type kunit_bus_type = {
> + .name = "kunit",
> + .match = kunit_bus_match
> +};
> +
> +int kunit_bus_init(void)
> +{
> + int error;
> +
> + error = bus_register(&kunit_bus_type);
> + if (!error) {
> + error = device_register(&kunit_bus);
> + if (error)
> + bus_unregister(&kunit_bus_type);
> + }
> + return error;
> +}
> +late_initcall(kunit_bus_init);
> +
> +static void kunit_device_release(struct device *d)
> +{
> + kfree(to_kunit_device(d));
> +}
> +
> +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> +{
> + struct device_driver *driver;
> + int err = -ENOMEM;
> +
> + driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> +
> + if (!driver)
> + return ERR_PTR(err);
> +
> + driver->name = name;
> + driver->bus = &kunit_bus_type;
> + driver->owner = THIS_MODULE;
> +
> + err = driver_register(driver);
> + if (err) {
> + kunit_kfree(test, driver);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, driver_unregister_wrapper, driver);
> + return driver;
> +}
> +EXPORT_SYMBOL_GPL(kunit_driver_create);
> +
> +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> + const char *name,
> + struct device_driver *drv)
> +{
> + struct kunit_device *kunit_dev;
> + int err = -ENOMEM;
> +
> + kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
> + if (!kunit_dev)
> + return ERR_PTR(err);
> +
> + kunit_dev->owner = test;
> +
> + err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> + if (err) {
> + kfree(kunit_dev);
> + return ERR_PTR(err);
> + }
> +
> + /* Set the expected driver pointer, so we match. */
> + kunit_dev->driver = drv;
Ah, so this is the match function to pass above? If so, why do you need
it at all?
> +
> + kunit_dev->dev.release = kunit_device_release;
> + kunit_dev->dev.bus = &kunit_bus_type;
> + kunit_dev->dev.parent = &kunit_bus;
> +
> + err = device_register(&kunit_dev->dev);
> + if (err) {
> + put_device(&kunit_dev->dev);
> + return ERR_PTR(err);
> + }
> +
> + kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> +
> + return kunit_dev;
> +}
> +
> +struct device *kunit_device_register_with_driver(struct kunit *test,
> + const char *name,
> + struct device_driver *drv)
> +{
> + struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
> +
> + if (IS_ERR_OR_NULL(kunit_dev))
This is almost always a sign that something is wrong with the api.
> + return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
Ick, the cast is odd, are you sure you need it? Why would you return a
struct device and not a kunit_device() anyway?
> +
> + return &kunit_dev->dev;
Again, why this type, why not use the real type you have?
thanks,
greg k-h
Hey Greg,
On Wed, 6 Dec 2023 at 01:31, Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Tue, Dec 05, 2023 at 03:31:33PM +0800, [email protected] wrote:
> > Tests for drivers often require a struct device to pass to other
> > functions. While it's possible to create these with
> > root_device_register(), or to use something like a platform device, this
> > is both a misuse of those APIs, and can be difficult to clean up after,
> > for example, a failed assertion.
> >
> > Add some KUnit-specific functions for registering and unregistering a
> > struct device:
> > - kunit_device_register()
> > - kunit_device_register_with_driver()
> > - kunit_device_unregister()
> >
> > These helpers allocate a on a 'kunit' bus which will either probe the
> > driver passed in (kunit_device_register_with_driver), or will create a
> > stub driver (kunit_device_register) which is cleaned up on test shutdown.
> >
> > Devices are automatically unregistered on test shutdown, but can be
> > manually unregistered earlier with kunit_device_unregister() in order
> > to, for example, test device release code.
>
> At first glance, nice work. But looks like 0-day doesn't like it that
> much, so I'll wait for the next version to review it properly.
Thanks very much for taking a look. I'll send v2 with the 0-day (and
other) issues fixed sometime tomorrow.
In the meantime, I've tried to explain some of the weirder decisions
below -- it mostly boils down to the existing use-cases only wanting
an opaque 'struct device *' they can pass around, and my attempt to
find a minimal (but still sensible) implementation of that. I'm
definitely happy to tweak this to make it a more 'normal' use of the
device model where that makes sense, though, especially if it doesn't
require too much boilerplate on the part of test authors.
> One nit I did notice:
>
> > +// For internal use only -- registers the kunit_bus.
> > +int kunit_bus_init(void);
>
> Put stuff like this in a local .h file, don't pollute the include/linux/
> files for things that you do not want any other part of the kernel to
> call.
>
v2 will have this in lib/kunit/device-impl.h
> > +/**
> > + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> > + * @test: The test context object.
> > + * @name: The name to give the created device.
> > + * @drv: The struct device_driver to associate with the device.
> > + *
> > + * Creates a struct kunit_device (which is a struct device) with the given
> > + * name, and driver. The device will be cleaned up on test exit, or when
> > + * kunit_device_unregister is called. See also kunit_device_register, if you
> > + * wish KUnit to create and manage a driver for you
> > + */
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > + const char *name,
> > + struct device_driver *drv);
>
> Shouldn't "struct device_driver *" be a constant pointer?
Done (and for the internal functions) for v2.
>
> But really, why is this a "raw" device_driver pointer and not a pointer
> to the driver type for your bus?
So, this is where the more difficult questions start (and where my
knowledge of the driver model gets a bit shakier).
At the moment, there's no struct kunit_driver; the goal was to have
whatever the minimal amount of infrastructure needed to get a 'struct
device *' that could be plumbed through existing code which accepts
it. (Read: mostly devres resource management stuff, get_device(),
etc.)
So, in this version, there's no:
- struct kunit_driver: we've no extra data to store / function
pointers other than what's in struct device_driver.
- The kunit_bus is as minimal as I could get it: each device matches
exactly one driver pointer (which is passed as struct
kunit_device->driver).
- The 'struct kunit_device' type is kept private, and 'struct device'
is used instead, as this is supposed to only be passed to generic
device functions (KUnit is just managing its lifecycle).
I've no problem adding these extra types to flesh this out into a more
'normal' setup, though I'd rather keep the boilerplate on the user
side minimal if possible. I suspect if we were to return a struct
kunit_device, everyone would be quickly grabbing and passing around a
raw 'struct device *' anyway, which is what the existing tests with
fake devices do (via root_device_register, which returns struct
device, or by returning &platform_device->dev from a helper).
Similarly, the kunit_bus is not ever exposed to test code, nor really
is the driver (except via kunit_device_register_with_driver(), which
isn't actually being used anywhere yet, so it may make sense to cut it
out from the next version). So, ideally tests won't even be aware that
their devices are attached to the kunit_bus, nor that they have
drivers attached: it's mostly just to make these normal enough that
they show up nicely in sysfs and play well with the devm_ resource
management functions.
>
> Oh heck, let's point out the other issues as I'm already here...
>
> > @@ -7,7 +7,8 @@ kunit-objs += test.o \
> > assert.o \
> > try-catch.o \
> > executor.o \
> > - attributes.o
> > + attributes.o \
> > + device.o
>
> Shouldn't this file be "bus.c" as you are creating a kunit bus?
>
I've sort-of grouped this as "device helpers", as it handles KUnit
devices, drivers, and the kunit_bus, but devices are the most
user-facing part. Indeed, the bus feels like an 'implementation
detail'. Happy to rename it if that makes things more consistent,
though.
> >
> > ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> > kunit-objs += debugfs.o
> > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > new file mode 100644
> > index 000000000000..93ace1a2297d
> > --- /dev/null
> > +++ b/lib/kunit/device.c
> > @@ -0,0 +1,176 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit basic device implementation
>
> "basic bus/driver implementation", not device, right?
>
Given that the users of this really only care about getting their
"device", and the bus/driver are more implementation details, I'd
rather go with something like "KUnit-managed device implementation" or
"KUnit device-model helpers". How do those sound?
> > + *
> > + * Implementation of struct kunit_device helpers.
> > + *
> > + * Copyright (C) 2023, Google LLC.
> > + * Author: David Gow <[email protected]>
> > + */
> > +
> > +#include <linux/device.h>
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/device.h>
> > +#include <kunit/resource.h>
> > +
> > +
> > +/* Wrappers for use with kunit_add_action() */
> > +KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
> > +KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
> > +
> > +static struct device kunit_bus = {
> > + .init_name = "kunit"
> > +};
>
> A static device as a bus? This feels wrong, what is it for? And where
> does this live? If you _REALLY_ want a single device for the root of
> your bus (which is a good idea), then make it a dynamic variable (as it
> is reference counted), NOT a static struct device which should not be
> done if at all possible.
Will do. Would root_device_register() make more sense here?
>
> > +
> > +/* A device owned by a KUnit test. */
> > +struct kunit_device {
> > + struct device dev;
> > + struct kunit *owner;
> > + /* Force binding to a specific driver. */
> > + struct device_driver *driver;
> > + /* The driver is managed by KUnit and unique to this device. */
> > + bool cleanup_driver;
> > +};
>
> Wait, why isn't your "kunit" device above a struct kunit_device
> structure? Why is it ok to be a "raw" struct device (hint, that's
> almost never a good idea.)
>
> > +static inline struct kunit_device *to_kunit_device(struct device *d)
> > +{
> > + return container_of(d, struct kunit_device, dev);
>
> container_of_const()? And to use that properly, why not make this a #define?
>
> > +}
> > +
> > +static int kunit_bus_match(struct device *dev, struct device_driver *driver)
> > +{
> > + struct kunit_device *kunit_dev = to_kunit_device(dev);
> > +
> > + if (kunit_dev->driver == driver)
> > + return 1;
> > +
> > + return 0;
>
> I don't understand, what are you trying to match here?
>
This is just to make sure that the match function will use whatever
driver is passed through. When I originally started writing this,
there were some resource cleanup problems if there was no driver
associated with a device, though that's fixed now.
> > +}
> > +
> > +static struct bus_type kunit_bus_type = {
> > + .name = "kunit",
> > + .match = kunit_bus_match
> > +};
> > +
> > +int kunit_bus_init(void)
> > +{
> > + int error;
> > +
> > + error = bus_register(&kunit_bus_type);
> > + if (!error) {
> > + error = device_register(&kunit_bus);
> > + if (error)
> > + bus_unregister(&kunit_bus_type);
> > + }
> > + return error;
> > +}
> > +late_initcall(kunit_bus_init);
> > +
> > +static void kunit_device_release(struct device *d)
> > +{
> > + kfree(to_kunit_device(d));
> > +}
> > +
> > +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> > +{
> > + struct device_driver *driver;
> > + int err = -ENOMEM;
> > +
> > + driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> > +
> > + if (!driver)
> > + return ERR_PTR(err);
> > +
> > + driver->name = name;
> > + driver->bus = &kunit_bus_type;
> > + driver->owner = THIS_MODULE;
> > +
> > + err = driver_register(driver);
> > + if (err) {
> > + kunit_kfree(test, driver);
> > + return ERR_PTR(err);
> > + }
> > +
> > + kunit_add_action(test, driver_unregister_wrapper, driver);
> > + return driver;
> > +}
> > +EXPORT_SYMBOL_GPL(kunit_driver_create);
> > +
> > +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> > + const char *name,
> > + struct device_driver *drv)
> > +{
> > + struct kunit_device *kunit_dev;
> > + int err = -ENOMEM;
> > +
> > + kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
> > + if (!kunit_dev)
> > + return ERR_PTR(err);
> > +
> > + kunit_dev->owner = test;
> > +
> > + err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> > + if (err) {
> > + kfree(kunit_dev);
> > + return ERR_PTR(err);
> > + }
> > +
> > + /* Set the expected driver pointer, so we match. */
> > + kunit_dev->driver = drv;
>
> Ah, so this is the match function to pass above? If so, why do you need
> it at all?
This is just to make sure there's a driver attached, so that
driver_detach() eventually gets called on the device, which used to be
required to clean up resources in some circumstances.
This has since been fixed in 699fb50d9903 ("drivers: base: Free devm
resources when unregistering a device"), but it seemed worth keeping
it both to make these devices seem "more normal", and potentially to
facilitate users providing a struct device_driver themselves later on,
should that one day be useful.
> > +
> > + kunit_dev->dev.release = kunit_device_release;
> > + kunit_dev->dev.bus = &kunit_bus_type;
> > + kunit_dev->dev.parent = &kunit_bus;
> > +
> > + err = device_register(&kunit_dev->dev);
> > + if (err) {
> > + put_device(&kunit_dev->dev);
> > + return ERR_PTR(err);
> > + }
> > +
> > + kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> > +
> > + return kunit_dev;
> > +}
> > +
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > + const char *name,
> > + struct device_driver *drv)
> > +{
> > + struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
> > +
> > + if (IS_ERR_OR_NULL(kunit_dev))
>
> This is almost always a sign that something is wrong with the api.
This can probably just be IS_ERR().
>
> > + return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
>
> Ick, the cast is odd, are you sure you need it? Why would you return a
> struct device and not a kunit_device() anyway?
>
All the users of this only want the struct device, so it's more
convenient if that's all we return (and it lets us keep struct
kunit_device private). But it's a minor inconvenience at worst, so I
don't mind changing it.
> > +
> > + return &kunit_dev->dev;
>
> Again, why this type, why not use the real type you have?
As above, to keep 'struct kunit_device' private.
Thanks again for looking at this; I'd definitely appreciate any
further input you may have on the design.
Cheers,
-- David
On Tue, 5 Dec 2023 at 16:30, Matti Vaittinen <[email protected]> wrote:
>
> On 12/5/23 09:31, [email protected] wrote:
> > Tests for drivers often require a struct device to pass to other
> > functions. While it's possible to create these with
> > root_device_register(), or to use something like a platform device, this
> > is both a misuse of those APIs, and can be difficult to clean up after,
> > for example, a failed assertion.
> >
> > Add some KUnit-specific functions for registering and unregistering a
> > struct device:
> > - kunit_device_register()
> > - kunit_device_register_with_driver()
> > - kunit_device_unregister()
>
> Thanks a lot David! I have been missing these!
>
> I love the explanation you added under Documentation. Very helpful I'd
> say. I only have very minor comments which you can ignore if they don't
> make sense to you or the kunit-subsystem.
>
> With or without the suggested changes:
>
> Reviewed-by: Matti Vaittinen <[email protected]>
>
> > --- /dev/null
> > +++ b/include/kunit/device.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * KUnit basic device implementation
> > + *
> > + * Helpers for creating and managing fake devices for KUnit tests.
> > + *
> > + * Copyright (C) 2023, Google LLC.
> > + * Author: David Gow <[email protected]>
> > + */
> > +
> > +#ifndef _KUNIT_DEVICE_H
> > +#define _KUNIT_DEVICE_H
> > +
> > +#if IS_ENABLED(CONFIG_KUNIT)
> > +
> > +#include <kunit/test.h>
> > +
> > +struct kunit_device;
> > +struct device;
> > +struct device_driver;
> > +
> > +// For internal use only -- registers the kunit_bus.
> > +int kunit_bus_init(void);
> > +
> > +/**
> > + * kunit_driver_create() - Create a struct device_driver attached to the kunit_bus
> > + * @test: The test context object.
> > + * @name: The name to give the created driver.
> > + *
> > + * Creates a struct device_driver attached to the kunit_bus, with the name @name.
> > + * This driver will automatically be cleaned up on test exit.
> > + */
> > +struct device_driver *kunit_driver_create(struct kunit *test, const char *name);
> > +
> > +/**
> > + * kunit_device_register() - Create a struct device for use in KUnit tests
> > + * @test: The test context object.
> > + * @name: The name to give the created device.
> > + *
> > + * Creates a struct kunit_device (which is a struct device) with the given name,
> > + * and a corresponding driver. The device and driver will be cleaned up on test
> > + * exit, or when kunit_device_unregister is called. See also
> > + * kunit_device_register_with_driver, if you wish to provide your own
> > + * struct device_driver.
> > + */
> > +struct device *kunit_device_register(struct kunit *test, const char *name);
> > +
> > +/**
> > + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> > + * @test: The test context object.
> > + * @name: The name to give the created device.
> > + * @drv: The struct device_driver to associate with the device.
> > + *
> > + * Creates a struct kunit_device (which is a struct device) with the given
> > + * name, and driver. The device will be cleaned up on test exit, or when
> > + * kunit_device_unregister is called. See also kunit_device_register, if you
> > + * wish KUnit to create and manage a driver for you
> > + */
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > + const char *name,
> > + struct device_driver *drv);
> > +
> > +/**
> > + * kunit_device_unregister() - Unregister a KUnit-managed device
> > + * @test: The test context object which created the device
> > + * @dev: The device.
> > + *
> > + * Unregisters and destroys a struct device which was created with
> > + * kunit_device_register or kunit_device_register_with_driver. If KUnit created
> > + * a driver, cleans it up as well.
> > + */
> > +void kunit_device_unregister(struct kunit *test, struct device *dev);
>
> I wish the return values for error case(s) were also mentioned. But
> please, see my next comment as well.
>
I'll add these for v2.
> > +
> > +#endif
> > +
> > +#endif
>
> ...
>
> > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > new file mode 100644
> > index 000000000000..93ace1a2297d
> > --- /dev/null
> > +++ b/lib/kunit/device.c
> > @@ -0,0 +1,176 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit basic device implementation
> > + *
> > + * Implementation of struct kunit_device helpers.
> > + *
> > + * Copyright (C) 2023, Google LLC.
> > + * Author: David Gow <[email protected]>
> > + */
> > +
>
> ...
>
> > +
> > +static void kunit_device_release(struct device *d)
> > +{
> > + kfree(to_kunit_device(d));
> > +}
>
> I see you added the function documentation to the header. I assume this
> is the kunit style(?) I may be heretical, but I'd love to see at least a
> very short documentation for (all) exported functions here. I think the
> arguments are mostly self-explatonary, but at least for me the return
> values aren't that obvious. Whether they are kerneldoc or not is not
> that important to me.
>
> I think you did a great job adding docs under Documentation/ (and the
> header) - but at least I tend to just jump to function implementation
> when I need to figure out how it behaves. Having doc (or pointer to doc)
> also here helps. I don't think it's that widely spread practice to add
> docs to the headers(?)
>
I'll add at least something to the implementations, too.
We've mostly kept the full documentation in the headers so they can be
found by people who only have headers installed, but also because the
headers tend to be smaller, and sphinx runs slowly enough as it is
without needing a bigger file to parse.
> > +struct device_driver *kunit_driver_create(struct kunit *test, const char *name)
> > +{
> > + struct device_driver *driver;
> > + int err = -ENOMEM;
> > +
> > + driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> > +
> > + if (!driver)
> > + return ERR_PTR(err);
> > +
> > + driver->name = name;
> > + driver->bus = &kunit_bus_type;
> > + driver->owner = THIS_MODULE;
> > +
> > + err = driver_register(driver);
> > + if (err) {
> > + kunit_kfree(test, driver);
> > + return ERR_PTR(err);
> > + }
> > +
> > + kunit_add_action(test, driver_unregister_wrapper, driver);
> > + return driver;
> > +}
> > +EXPORT_SYMBOL_GPL(kunit_driver_create);
> > +
> > +struct kunit_device *__kunit_device_register_internal(struct kunit *test,
> > + const char *name,
> > + struct device_driver *drv)
>
> Very much nitpicking only - but do you think either the "__"-prefix or
> the "_internal"-suffix would be enough and not both? (Just to make
> function a tad shorter, not that it matters much though).
>
Fair enough, I've tentatively got rid of the underscores for v2.
> > +{
> > + struct kunit_device *kunit_dev;
> > + int err = -ENOMEM;
> > +
> > + kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
> > + if (!kunit_dev)
> > + return ERR_PTR(err);
> > +
> > + kunit_dev->owner = test;
> > +
> > + err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
> > + if (err) {
> > + kfree(kunit_dev);
> > + return ERR_PTR(err);
> > + }
> > +
> > + /* Set the expected driver pointer, so we match. */
> > + kunit_dev->driver = drv;
> > +
> > + kunit_dev->dev.release = kunit_device_release;
> > + kunit_dev->dev.bus = &kunit_bus_type;
> > + kunit_dev->dev.parent = &kunit_bus;
> > +
> > + err = device_register(&kunit_dev->dev);
> > + if (err) {
> > + put_device(&kunit_dev->dev);
> > + return ERR_PTR(err);
> > + }
> > +
> > + kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
> > +
> > + return kunit_dev;
> > +}
>
Thanks,
-- David
On Tue, Dec 05, 2023 at 03:31:34PM +0800, [email protected] wrote:
> Using struct root_device to create fake devices for tests is something
> of a hack. The new struct kunit_device is meant for this purpose, so use
> it instead.
>
> Signed-off-by: David Gow <[email protected]>
> ---
> lib/fortify_kunit.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
> index c8c33cbaae9e..f7a1fce8849b 100644
> --- a/lib/fortify_kunit.c
> +++ b/lib/fortify_kunit.c
> @@ -16,6 +16,7 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <kunit/test.h>
> +#include <kunit/device.h>
> #include <linux/device.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> @@ -269,7 +270,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
> size_t len; \
> \
> /* Create dummy device for devm_kmalloc()-family tests. */ \
> - dev = root_device_register(dev_name); \
> + dev = kunit_device_register(test, dev_name); \
> KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), \
> "Cannot register test device\n"); \
> \
> @@ -303,7 +304,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
> checker(len, devm_kmemdup(dev, "Ohai", len, gfp), \
> devm_kfree(dev, p)); \
> \
> - device_unregister(dev); \
> + kunit_device_unregister(test, dev); \
> } while (0)
> DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
Acked-by: Kees Cook <[email protected]>
(As an aside; shouldn't this get automatically cleaned up like other
kunit resources, though?)
--
Kees Cook
On Wed, Dec 06, 2023 at 03:44:08PM +0800, David Gow wrote:
> > But really, why is this a "raw" device_driver pointer and not a pointer
> > to the driver type for your bus?
>
> So, this is where the more difficult questions start (and where my
> knowledge of the driver model gets a bit shakier).
>
> At the moment, there's no struct kunit_driver; the goal was to have
> whatever the minimal amount of infrastructure needed to get a 'struct
> device *' that could be plumbed through existing code which accepts
> it. (Read: mostly devres resource management stuff, get_device(),
> etc.)
>
> So, in this version, there's no:
> - struct kunit_driver: we've no extra data to store / function
> pointers other than what's in struct device_driver.
> - The kunit_bus is as minimal as I could get it: each device matches
> exactly one driver pointer (which is passed as struct
> kunit_device->driver).
> - The 'struct kunit_device' type is kept private, and 'struct device'
> is used instead, as this is supposed to only be passed to generic
> device functions (KUnit is just managing its lifecycle).
>
> I've no problem adding these extra types to flesh this out into a more
> 'normal' setup, though I'd rather keep the boilerplate on the user
> side minimal if possible. I suspect if we were to return a struct
> kunit_device, everyone would be quickly grabbing and passing around a
> raw 'struct device *' anyway, which is what the existing tests with
> fake devices do (via root_device_register, which returns struct
> device, or by returning &platform_device->dev from a helper).
>
> Similarly, the kunit_bus is not ever exposed to test code, nor really
> is the driver (except via kunit_device_register_with_driver(), which
> isn't actually being used anywhere yet, so it may make sense to cut it
> out from the next version). So, ideally tests won't even be aware that
> their devices are attached to the kunit_bus, nor that they have
> drivers attached: it's mostly just to make these normal enough that
> they show up nicely in sysfs and play well with the devm_ resource
> management functions.
Ok, this makes more sense, thank you for the detailed explaination.
Making this "generic" like you have done here seems reasonable for now.
> > > - attributes.o
> > > + attributes.o \
> > > + device.o
> >
> > Shouldn't this file be "bus.c" as you are creating a kunit bus?
> >
>
> I've sort-of grouped this as "device helpers", as it handles KUnit
> devices, drivers, and the kunit_bus, but devices are the most
> user-facing part. Indeed, the bus feels like an 'implementation
> detail'. Happy to rename it if that makes things more consistent,
> though.
Nah, device.o makes sense for now, thanks.
> > > ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> > > kunit-objs += debugfs.o
> > > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > > new file mode 100644
> > > index 000000000000..93ace1a2297d
> > > --- /dev/null
> > > +++ b/lib/kunit/device.c
> > > @@ -0,0 +1,176 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * KUnit basic device implementation
> >
> > "basic bus/driver implementation", not device, right?
> >
>
> Given that the users of this really only care about getting their
> "device", and the bus/driver are more implementation details, I'd
> rather go with something like "KUnit-managed device implementation" or
> "KUnit device-model helpers". How do those sound?
Either would be good, thanks.
> > > + *
> > > + * Implementation of struct kunit_device helpers.
> > > + *
> > > + * Copyright (C) 2023, Google LLC.
> > > + * Author: David Gow <[email protected]>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +
> > > +#include <kunit/test.h>
> > > +#include <kunit/device.h>
> > > +#include <kunit/resource.h>
> > > +
> > > +
> > > +/* Wrappers for use with kunit_add_action() */
> > > +KUNIT_DEFINE_ACTION_WRAPPER(device_unregister_wrapper, device_unregister, struct device *);
> > > +KUNIT_DEFINE_ACTION_WRAPPER(driver_unregister_wrapper, driver_unregister, struct device_driver *);
> > > +
> > > +static struct device kunit_bus = {
> > > + .init_name = "kunit"
> > > +};
> >
> > A static device as a bus? This feels wrong, what is it for? And where
> > does this live? If you _REALLY_ want a single device for the root of
> > your bus (which is a good idea), then make it a dynamic variable (as it
> > is reference counted), NOT a static struct device which should not be
> > done if at all possible.
>
> Will do. Would root_device_register() make more sense here?
Yes.
> > > +
> > > +/* A device owned by a KUnit test. */
> > > +struct kunit_device {
> > > + struct device dev;
> > > + struct kunit *owner;
> > > + /* Force binding to a specific driver. */
> > > + struct device_driver *driver;
> > > + /* The driver is managed by KUnit and unique to this device. */
> > > + bool cleanup_driver;
> > > +};
> >
> > Wait, why isn't your "kunit" device above a struct kunit_device
> > structure? Why is it ok to be a "raw" struct device (hint, that's
> > almost never a good idea.)
> >
> > > +static inline struct kunit_device *to_kunit_device(struct device *d)
> > > +{
> > > + return container_of(d, struct kunit_device, dev);
> >
> > container_of_const()? And to use that properly, why not make this a #define?
> >
> > > +}
> > > +
> > > +static int kunit_bus_match(struct device *dev, struct device_driver *driver)
> > > +{
> > > + struct kunit_device *kunit_dev = to_kunit_device(dev);
> > > +
> > > + if (kunit_dev->driver == driver)
> > > + return 1;
> > > +
> > > + return 0;
> >
> > I don't understand, what are you trying to match here?
> >
>
> This is just to make sure that the match function will use whatever
> driver is passed through. When I originally started writing this,
> there were some resource cleanup problems if there was no driver
> associated with a device, though that's fixed now.
As it's fixed, no need for this, so let's not be confused going forward :)
thanks,
greg k-h
On Thu, 7 Dec 2023 at 05:07, Kees Cook <[email protected]> wrote:
>
> On Tue, Dec 05, 2023 at 03:31:34PM +0800, [email protected] wrote:
> > Using struct root_device to create fake devices for tests is something
> > of a hack. The new struct kunit_device is meant for this purpose, so use
> > it instead.
> >
> > Signed-off-by: David Gow <[email protected]>
> > ---
> > lib/fortify_kunit.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
> > index c8c33cbaae9e..f7a1fce8849b 100644
> > --- a/lib/fortify_kunit.c
> > +++ b/lib/fortify_kunit.c
> > @@ -16,6 +16,7 @@
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > #include <kunit/test.h>
> > +#include <kunit/device.h>
> > #include <linux/device.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > @@ -269,7 +270,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
> > size_t len; \
> > \
> > /* Create dummy device for devm_kmalloc()-family tests. */ \
> > - dev = root_device_register(dev_name); \
> > + dev = kunit_device_register(test, dev_name); \
> > KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), \
> > "Cannot register test device\n"); \
> > \
> > @@ -303,7 +304,7 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc)
> > checker(len, devm_kmemdup(dev, "Ohai", len, gfp), \
> > devm_kfree(dev, p)); \
> > \
> > - device_unregister(dev); \
> > + kunit_device_unregister(test, dev); \
> > } while (0)
> > DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc)
>
> Acked-by: Kees Cook <[email protected]>
>
> (As an aside; shouldn't this get automatically cleaned up like other
> kunit resources, though?)
>
We can't just get rid of the {kunit_,}device_unregister() here,
because otherwise we'd have several devices with the same name during
the test.
So, yes, these get automatically cleaned up, but the test would have
to be restructured to either give each device a different name, or
split the tests up further.
-- David