[PATCH 0/4] kunit: Add helpers for creating test-managed devices

[davi...@google.com]

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/20230325043104.3...@google.com/

(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 <davi...@google.com>
---
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 <davi...@google.com>

[davi...@google.com]

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 <davi...@google.com>
---

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 <davi...@google.com>
+ */
+
+#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 <davi...@google.com>
+ */
+
+#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

[davi...@google.com]

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 <davi...@google.com>
---

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

[davi...@google.com]

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 <davi...@google.com>
---

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

[davi...@google.com]

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 <davi...@google.com>
---

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

[Matti Vaittinen]

On 12/5/23 09:31, davi...@google.com 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 <mazzies...@gmail.com>

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 <davi...@google.com>
> + */
> +

...

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

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

...

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

[Matti Vaittinen]

On 12/5/23 09:31, davi...@google.com 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 <davi...@google.com>

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

[Matti Vaittinen]

On 12/5/23 09:31, davi...@google.com 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 <davi...@google.com>

[Maxime Ripard]

Hi David,

Thanks a lot for working on this.

^ 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 preferred syntax is sizeof(*kunit_dev) here

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

[Amadeusz Sławiński]

Typo: Devices

[Amadeusz Sławiński]

On 12/5/2023 8:31 AM, davi...@google.com 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 <davi...@google.com>
> ---
> 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?

[Maxime Ripard]

Kunit recently gained helpers to create test managed devices. This means
that we no longer have to roll our own helpers in KMS and we can reuse
them.

Signed-off-by: Maxime Ripard <mri...@kernel.org>

---

David, feel free to integrate that patch into your series and merge it
whenever and wherever you see fit.
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 66 ++---------------------
1 file changed, 3 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index c251e6b34de0..ca4f8e4c5d5d 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -5,6 +5,7 @@
#include <drm/drm_kunit_helpers.h>
#include <drm/drm_managed.h>

+#include <kunit/device.h>
#include <kunit/resource.h>

#include <linux/device.h>
@@ -15,28 +16,6 @@
static const struct drm_mode_config_funcs drm_mode_config_funcs = {
};

-static int fake_probe(struct platform_device *pdev)
-{
- return 0;
-}
-
-static struct platform_driver fake_platform_driver = {
- .probe = fake_probe,
- .driver = {
- .name = KUNIT_DEVICE_NAME,
- },
-};
-
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_driver_unregister,
- platform_driver_unregister,
- struct platform_driver *);
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_put,
- platform_device_put,
- struct platform_device *);
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_del,
- platform_device_del,
- struct platform_device *);
-
/**
* drm_kunit_helper_alloc_device - Allocate a mock device for a KUnit test

* @test: The test context object

@@ -54,34 +33,7 @@ KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_del,
*/
struct device *drm_kunit_helper_alloc_device(struct kunit *test)
{
- struct platform_device *pdev;
- int ret;
-
- ret = platform_driver_register(&fake_platform_driver);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_driver_unregister,
- &fake_platform_driver);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_device_put,
- pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = platform_device_add(pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_device_del,
- pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- return &pdev->dev;
+ return kunit_device_register(test, KUNIT_DEVICE_NAME);
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);

@@ -94,19 +46,7 @@ EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
*/
void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
-
- kunit_release_action(test,
- kunit_action_platform_device_del,
- pdev);
-
- kunit_release_action(test,
- kunit_action_platform_device_put,
- pdev);
-
- kunit_release_action(test,
- kunit_action_platform_driver_unregister,
- &fake_platform_driver);
+ kunit_device_unregister(test, dev);
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);

--
2.43.0

[Mark Brown]

On Tue, Dec 05, 2023 at 03:31:36PM +0800, davi...@google.com 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 <bro...@kernel.org>

[kernel test robot]

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/202312052114...@intel.com/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/202312052114...@intel.com/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 <l...@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312052114...@intel.com/

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

[kernel test robot]

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/202312052230...@intel.com/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/202312052230...@intel.com/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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202312052230...@intel.com/

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]

[kernel test robot]

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/202312052210...@intel.com/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/202312052210...@intel.com/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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202312052210...@intel.com/

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?

[kernel test robot]

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/202312052341...@intel.com/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/202312052341...@intel.com/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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202312052341...@intel.com/

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

[Greg Kroah-Hartman]

On Tue, Dec 05, 2023 at 03:31:33PM +0800, davi...@google.com 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 <davi...@google.com>
> + */
> +
> +#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?

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

[David Gow]

I'll add these for v2.

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.

Fair enough, I've tentatively got rid of the underscores for v2.

Thanks,
-- David

[David Gow]

Hey Greg,

On Wed, 6 Dec 2023 at 01:31, Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
>
> On Tue, Dec 05, 2023 at 03:31:33PM +0800, davi...@google.com 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?

Will do. Would root_device_register() make more sense 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.

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

[kernel test robot]

Hi Maxime,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20231205]
[cannot apply to drm-misc/drm-misc-next v6.7-rc4 v6.7-rc3 v6.7-rc2 linus/master v6.7-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/drm-tests-Switch-to-kunit-devices/20231205-170508
base: next-20231205
patch link: https://lore.kernel.org/r/20231205090405.153140-1-mripard%40kernel.org
patch subject: [PATCH] drm/tests: Switch to kunit devices
config: i386-buildonly-randconfig-004-20231206 (https://download.01.org/0day-ci/archive/20231207/202312070011...@intel.com/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/20231207/202312070011...@intel.com/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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202312070011...@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/tests/drm_kunit_helpers.c:8:10: fatal error: 'kunit/device.h' file not found
#include <kunit/device.h>
^~~~~~~~~~~~~~~~
1 error generated.


vim +8 drivers/gpu/drm/tests/drm_kunit_helpers.c

7
> 8 #include <kunit/device.h>
9 #include <kunit/resource.h>
10

[kernel test robot]

Hi Maxime,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20231205]
[cannot apply to drm-misc/drm-misc-next v6.7-rc4 v6.7-rc3 v6.7-rc2 linus/master v6.7-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/drm-tests-Switch-to-kunit-devices/20231205-170508
base: next-20231205
patch link: https://lore.kernel.org/r/20231205090405.153140-1-mripard%40kernel.org
patch subject: [PATCH] drm/tests: Switch to kunit devices

config: i386-randconfig-012-20231206 (https://download.01.org/0day-ci/archive/20231207/202312070010...@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231207/202312070010...@intel.com/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 <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202312070010...@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/tests/drm_kunit_helpers.c:8:10: fatal error: kunit/device.h: No such file or directory
#include <kunit/device.h>
^~~~~~~~~~~~~~~~
compilation terminated.

[Kees Cook]

Acked-by: Kees Cook <kees...@chromium.org>

(As an aside; shouldn't this get automatically cleaned up like other
kunit resources, though?)

--
Kees Cook

[Greg Kroah-Hartman]

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.

Yes.

As it's fixed, no need for this, so let's not be confused going forward :)

thanks,

greg k-h

[David Gow]

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

[davi...@google.com]

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/20230325043104.3...@google.com/

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

I'd really appreciate any extra scrutiny that can be given to this;
particularly around the device refcounts and whether we can guarantee
that the device will be released at the correct point in the test
cleanup. I've seen a few crashes in kunit_cleanup, but only on some
already flaky/fragile UML/clang/alltests setups, which seem to go away
if I remove the devm_add_action() call (or if I enable any debugging
features / symbols, annoyingly).

Cheers,
-- David

Signed-off-by: David Gow <davi...@google.com>
---

Changes in v2:
- Simplify device/driver/bus matching, removing the no-longer-required
kunit_bus_match function. (Thanks, Greg)
- The return values are both more consistent (kunit_device_register now
returns an explicit error pointer, rather than failing the test), and
better documented.
- Add some basic documentation to the implementations as well as the
headers. The documentation in the headers is still more complete, and
is now properly compiled into the HTML documentation (under
dev-tools/kunit/api/resources.html). (Thanks, Matti)
- Moved the internal-only kunit_bus_init() function to a private header,
lib/kunit/device-impl.h to avoid polluting the public headers, and
match other internal-only headers. (Thanks, Greg)
- Alphabetise KUnit includes in other test modules. (Thanks, Amadeusz.)
- Several code cleanups, particularly around error handling and
allocation. (Thanks Greg, Maxime)
- Several const-correctness and casting improvements. (Thanks, Greg)
- Added a new test to verify KUnit cleanup triggers device cleanup.
(Thanks, Maxime).
- Improved the user-specified device test to verify that probe/remove
hooks are called correctly. (Thanks, Maxime).
- The overflow test no-longer needlessly calls
kunit_device_unregister().
- Several other minor cleanups and documentation improvements, which
hopefully make this a bit clearer and more robust.
- Link to v1: https://lore.kernel.org/r/20231205-kunit_bus...@google.com

---
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/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++
include/kunit/device.h | 80 +++++++++++

lib/fortify_kunit.c | 5 +-
lib/kunit/Makefile | 3 +-

lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-

lib/kunit/test.c | 3 +
lib/overflow_kunit.c | 5 +-

sound/soc/soc-topology-test.c | 10 +-
10 files changed, 465 insertions(+), 15 deletions(-)

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

Signed-off-by: David Gow <davi...@google.com>
---

Documentation/dev-tools/kunit/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++
include/kunit/device.h | 80 +++++++++++

lib/kunit/Makefile | 3 +-
lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-
lib/kunit/test.c | 3 +

7 files changed, 458 insertions(+), 2 deletions(-)

diff --git a/Documentation/dev-tools/kunit/api/resource.rst b/Documentation/dev-tools/kunit/api/resource.rst
index 0a94f831259e..ec6002a6b0db 100644
--- a/Documentation/dev-tools/kunit/api/resource.rst
+++ b/Documentation/dev-tools/kunit/api/resource.rst
@@ -11,3 +11,12 @@ state on a per-test basis, register custom cleanup actions, and more.

.. kernel-doc:: include/kunit/resource.h
:internal:
+
+Managed Devices
+---------------
+
+Functions for using KUnit-managed struct device and struct device_driver.
+Include ``kunit/device.h`` to use these.
+
+.. kernel-doc:: include/kunit/device.h
+ :internal:
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 9db12e91668e..53c6f7dc8a42 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -797,3 +797,53 @@ 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 Devices 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");

+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_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..2450110ad64e
--- /dev/null
+++ b/include/kunit/device.h
@@ -0,0 +1,80 @@
+/* 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 <davi...@google.com>
+ */
+

+#ifndef _KUNIT_DEVICE_H
+#define _KUNIT_DEVICE_H
+
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test.h>
+

+struct device;
+struct device_driver;

+
+/**
+ * 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.
+ *
+ * Return: a stub struct device_driver, managed by KUnit, with the name @name.
+ */

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

+ * Return: a pointer to a struct device which will be cleaned up when the test
+ * exits, or an error pointer if the device could not be allocated or registered.
+ */
+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.
+ *
+ * Return: a pointer to a struct device which will be cleaned up when the test
+ * exits, or an error pointer if the device could not be allocated or registered.

+ */
+struct device *kunit_device_register_with_driver(struct kunit *test,
+ const char *name,

+ const 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..1db4305b615a
--- /dev/null
+++ b/lib/kunit/device.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit-managed device implementation
+ *
+ * Implementation of struct kunit_device helpers for fake devices whose
+ * lifecycle is managed by KUnit.

+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+
+#include <linux/device.h>
+
+#include <kunit/test.h>
+#include <kunit/device.h>
+#include <kunit/resource.h>
+

+#include "device-impl.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 *);
+

+/* The root device for the KUnit bus, parent of all kunit_devices. */
+static struct device *kunit_bus_device;

+
+/* A device owned by a KUnit test. */
+struct kunit_device {
+ struct device dev;

+ /* The KUnit test which owns this device. */
+ struct kunit *owner;
+ /* If the driver is managed by KUnit and unique to this device. */
+ const struct device_driver *driver;
+};
+
+#define to_kunit_device(d) container_of_const(d, struct kunit_device, dev)

+
+static struct bus_type kunit_bus_type = {
+ .name = "kunit",

+};
+
+/* Register the 'kunit_bus' used for fake devices. */

+int kunit_bus_init(void)
+{
+ int error;
+

+ kunit_bus_device = root_device_register("kunit");
+ if (!kunit_bus_device)
+ return -ENOMEM;

+
+ error = bus_register(&kunit_bus_type);

+ if (error)
+ bus_unregister(&kunit_bus_type);
+ return error;
+}
+
+/* Release a 'fake' KUnit device. */

+static void kunit_device_release(struct device *d)
+{
+ kfree(to_kunit_device(d));
+}
+

+/**
+ * Create and register a KUnit-managed struct device_driver on the kunit_bus.
+ * Returns an error pointer on failure.
+ */

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

+/* Helper which creates a kunit_device, attaches it to the kunit_bus*/
+static struct kunit_device *kunit_device_register_internal(struct kunit *test,
+ const char *name,
+ const struct device_driver *drv)
+{

+ struct kunit_device *kunit_dev;
+ int err = -ENOMEM;
+

+ kunit_dev = kzalloc(sizeof(*kunit_dev), 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);
+ }
+

+ kunit_dev->dev.release = kunit_device_release;
+ kunit_dev->dev.bus = &kunit_bus_type;

+ kunit_dev->dev.parent = kunit_bus_device;

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

+/**
+ * Create and register a new KUnit-managed device, using the user-supplied device_driver.
+ * On failure, returns an error pointer.

+ */
+struct device *kunit_device_register_with_driver(struct kunit *test,
+ const char *name,

+ const struct device_driver *drv)
+{
+ struct kunit_device *kunit_dev = kunit_device_register_internal(test, name, drv);
+
+ if (IS_ERR_OR_NULL(kunit_dev))
+ return ERR_CAST(kunit_dev);

+
+ return &kunit_dev->dev;

+}
+EXPORT_SYMBOL_GPL(kunit_device_register_with_driver);
+
+/**
+ * Create and register a new KUnit-managed device, including a matching device_driver.
+ * On failure, returns an error pointer.
+ */
+struct device *kunit_device_register(struct kunit *test, const char *name)
+{
+ struct device_driver *drv;

+ struct kunit_device *dev;
+

+ drv = kunit_driver_create(test, name);
+ if (IS_ERR(drv))
+ return ERR_CAST(drv);
+
+ dev = kunit_device_register_internal(test, name, drv);
+ if (IS_ERR(dev)) {
+ kunit_release_action(test, driver_unregister_wrapper, (void *)drv);
+ return ERR_CAST(dev);
+ }
+
+ /* Request the driver be freed. */
+ dev->driver = drv;
+
+
+ return &dev->dev;
+}
+EXPORT_SYMBOL_GPL(kunit_device_register);
+
+/* Unregisters a KUnit-managed device early (including the driver, if automatically created). */

+void kunit_device_unregister(struct kunit *test, struct device *dev)

+{
+ const struct device_driver *driver = to_kunit_device(dev)->driver;

+
+ kunit_release_action(test, device_unregister_wrapper, dev);

+ if (driver)
+ kunit_release_action(test, driver_unregister_wrapper, (void *)driver);

+}
+EXPORT_SYMBOL_GPL(kunit_device_unregister);
+
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c

index 3e9c5192d095..6c666fbdd6ad 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,135 @@ 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;

+ long action_was_run = 0;

+
+ test_device = kunit_device_register(test, "my_device");

+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device);

+
+ // Add an action to verify cleanup.

+ devm_add_action(test_device, test_dev_action, &action_was_run);
+
+ KUNIT_EXPECT_EQ(test, action_was_run, 0);

+
+ kunit_device_unregister(test, test_device);
+

+ KUNIT_EXPECT_EQ(test, action_was_run, 1);
+}
+
+static void kunit_device_cleanup_test(struct kunit *test)

+{
+ struct device *test_device;

+ long action_was_run = 0;

+
+ 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, &action_was_run);
+
+ KUNIT_EXPECT_EQ(test, action_was_run, 0);
+
+ /* Force KUnit to run cleanup early. */
+ kunit_cleanup(test);
+
+ KUNIT_EXPECT_EQ(test, action_was_run, 1);
+}
+
+struct driver_test_state {
+ bool driver_device_probed;
+ bool driver_device_removed;
+ long action_was_run;
+};
+
+static int driver_probe_hook(struct device *dev)
+{
+ struct kunit *test = kunit_get_current_test();
+ struct driver_test_state *state = (struct driver_test_state *)test->priv;
+
+ state->driver_device_probed = true;

+ return 0;
+}
+

+static int driver_remove_hook(struct device *dev)
+{
+ struct kunit *test = kunit_get_current_test();
+ struct driver_test_state *state = (struct driver_test_state *)test->priv;
+
+ state->driver_device_removed = true;

+ return 0;
+}
+

+
+static void kunit_device_driver_test(struct kunit *test)
+{
+ struct device_driver *test_driver;
+ struct device *test_device;

+ struct driver_test_state test_state = {};
+
+ test->priv = &test_state;

+ test_driver = kunit_driver_create(test, "my_driver");
+

+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_driver);
+
+ test_driver->probe = driver_probe_hook;
+ test_driver->remove = driver_remove_hook;

+
+ test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
+

+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device);
+
+ // Make sure the probe function was called.
+ KUNIT_ASSERT_TRUE(test, test_state.driver_device_probed);

+
+ // Add an action to verify cleanup.

+ devm_add_action(test_device, test_dev_action, &test_state.action_was_run);
+
+ KUNIT_EXPECT_EQ(test, test_state.action_was_run, 0);
+
+ kunit_device_unregister(test, test_device);

+ test_device = NULL;
+

+ // Make sure the remove hook was called.
+ KUNIT_ASSERT_TRUE(test, test_state.driver_device_removed);
+
+ // We're going to test this again.
+ test_state.driver_device_probed = false;

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

+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device);
+
+ // Probe was called again.
+ KUNIT_ASSERT_TRUE(test, test_state.driver_device_probed);
+

+ // Everything is automatically freed here.
+}
+
+static struct kunit_case kunit_device_test_cases[] = {
+ KUNIT_CASE(kunit_device_test),

+ KUNIT_CASE(kunit_device_cleanup_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 +826,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..c457593e4913 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -18,6 +18,7 @@
#include <linux/sched.h>

#include "debugfs.h"
+#include "device-impl.h"
#include "hooks-impl.h"
#include "string-stream.h"
#include "try-catch-impl.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.472.g3155946c3a-goog

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
Acked-by: Kees Cook <kees...@chromium.org>

Signed-off-by: David Gow <davi...@google.com>
---

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..2e4fedc81621 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c
@@ -15,6 +15,7 @@
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>
#include <linux/slab.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.472.g3155946c3a-goog

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

Signed-off-by: David Gow <davi...@google.com>
---

lib/overflow_kunit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index 34db0b3aa502..c527f6b75789 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c
@@ -6,6 +6,7 @@

*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>

#include <linux/kernel.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,8 +635,6 @@ static void overflow_allocation_test(struct kunit *test)
check_allocation_overflow(devm_kmalloc);
check_allocation_overflow(devm_kzalloc);

- device_unregister(dev);
-

kunit_info(test, "%d allocation overflow tests finished\n", count);
#undef check_allocation_overflow
}

--

2.43.0.472.g3155946c3a-goog

[davi...@google.com]

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 <bro...@kernel.org>

Signed-off-by: David Gow <davi...@google.com>
---

sound/soc/soc-topology-test.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/sound/soc/soc-topology-test.c b/sound/soc/soc-topology-test.c
index 2cd3540cec04..70cbccc42a42 100644
--- a/sound/soc/soc-topology-test.c
+++ b/sound/soc/soc-topology-test.c
@@ -9,6 +9,7 @@
#include <sound/core.h>
#include <sound/soc.h>
#include <sound/soc-topology.h>
+#include <kunit/device.h>
#include <kunit/test.h>

/* ===== HELPER FUNCTIONS =================================================== */

@@ -21,26 +22,19 @@

*/
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);
}

/*

--
2.43.0.472.g3155946c3a-goog

[Maxime Ripard]

On Fri, 8 Dec 2023 18:09:29 +0800, davi...@google.com 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.
>

> [ ... ]

Reviewed-by: Maxime Ripard <mri...@kernel.org>

Thanks!
Maxime

[Maxime Ripard]

Hi David,

On Fri, Dec 08, 2023 at 06:09:28PM +0800, davi...@google.com wrote:
> 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/20230325043104.3...@google.com/
>
> (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.

Could you take (and apply eventually) that patch as part of your series?
https://lore.kernel.org/linux-kselftest/20231205090405....@kernel.org/

Thanks
Maxime

[davi...@google.com]

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/20230325043104.3...@google.com/

(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 <davi...@google.com>
---

Changes in v3:
- Port the DRM tests to these new helpers (Thanks, Maxime!)
- Include the lib/kunit/device-impl.h file, which was missing from the
previous revision.
- Fix a use-after-free bug in kunit_device_driver_test, which resulted
in memory corruption on some clang-built UML builds.
- The 'test_state' is now allocated with kunit_kzalloc(), not on the
stack, as the stack will be gone when cleanup occurs.
- Link to v2: https://lore.kernel.org/r/20231208-kunit_bus...@google.com

Maxime Ripard (1):

drm/tests: Switch to kunit devices

Documentation/dev-tools/kunit/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++

drivers/gpu/drm/tests/drm_kunit_helpers.c | 66 +--------

include/kunit/device.h | 80 +++++++++++
lib/fortify_kunit.c | 5 +-
lib/kunit/Makefile | 3 +-

lib/kunit/device-impl.h | 17 +++

lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-
lib/kunit/test.c | 3 +

lib/overflow_kunit.c | 5 +-
sound/soc/soc-topology-test.c | 10 +-

12 files changed, 485 insertions(+), 78 deletions(-)
---
base-commit: b285ba6f8cc1b2bfece0b4350fdb92c8780bc698

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

Reviewed-by: Maxime Ripard <mri...@kernel.org>

Signed-off-by: David Gow <davi...@google.com>
---

Documentation/dev-tools/kunit/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++

include/kunit/device.h | 80 +++++++++++

lib/kunit/Makefile | 3 +-
lib/kunit/device-impl.h | 17 +++
lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-
lib/kunit/test.c | 3 +

8 files changed, 475 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/device-impl.h b/lib/kunit/device-impl.h
new file mode 100644
index 000000000000..54bd55836405
--- /dev/null
+++ b/lib/kunit/device-impl.h
@@ -0,0 +1,17 @@

+/* SPDX-License-Identifier: GPL-2.0 */
+/*

+ * KUnit internal header for device helpers
+ *
+ * Header for KUnit-internal driver / bus management.

+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+

+#ifndef _KUNIT_DEVICE_IMPL_H
+#define _KUNIT_DEVICE_IMPL_H
+

+// For internal use only -- registers the kunit_bus.
+int kunit_bus_init(void);

+
+#endif //_KUNIT_DEVICE_IMPL_H

index ee6927c60979..c4259d910356 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -5,9 +5,13 @@
* Copyright (C) 2019, Google LLC.
* Author: Brendan Higgins <brendan...@google.com>
*/
+#include "linux/gfp_types.h"

#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 +691,134 @@ static struct kunit_case kunit_current_test_cases[] = {

+static void kunit_device_driver_test(struct kunit *test)
+{
+ struct device_driver *test_driver;
+ struct device *test_device;

+ struct driver_test_state *test_state = kunit_kzalloc(test, sizeof(*test_state), GFP_KERNEL);
+
+ test->priv = test_state;

+ test_driver = kunit_driver_create(test, "my_driver");
+
+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_driver);
+
+ test_driver->probe = driver_probe_hook;
+ test_driver->remove = driver_remove_hook;
+
+ test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
+
+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device);
+
+ // Make sure the probe function was called.

+ KUNIT_ASSERT_TRUE(test, test_state->driver_device_probed);

+
+ // Add an action to verify cleanup.

+ devm_add_action(test_device, test_dev_action, &test_state->action_was_run);
+
+ KUNIT_EXPECT_EQ(test, test_state->action_was_run, 0);
+
+ kunit_device_unregister(test, test_device);

+ test_device = NULL;
+
+ // Make sure the remove hook was called.

+ KUNIT_ASSERT_TRUE(test, test_state->driver_device_removed);

+
+ // We're going to test this again.

+ test_state->driver_device_probed = false;

+
+ // 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);
+
+ // This can fail with an error pointer.
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_device);
+
+ // Probe was called again.

+ KUNIT_ASSERT_TRUE(test, test_state->driver_device_probed);

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
Acked-by: Kees Cook <kees...@chromium.org>

Signed-off-by: David Gow <davi...@google.com>
---

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..2e4fedc81621 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c

@@ -15,6 +15,7 @@

*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>

#include <linux/slab.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"); \
\

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

Signed-off-by: David Gow <davi...@google.com>
---

lib/overflow_kunit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index 34db0b3aa502..c527f6b75789 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c

@@ -6,6 +6,7 @@

*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>

#include <linux/kernel.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");

[davi...@google.com]

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 <bro...@kernel.org>

Signed-off-by: David Gow <davi...@google.com>
---

[davi...@google.com]

From: Maxime Ripard <mri...@kernel.org>

Kunit recently gained helpers to create test managed devices. This means
that we no longer have to roll our own helpers in KMS and we can reuse
them.

Signed-off-by: Maxime Ripard <mri...@kernel.org>
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 66 ++-----------------------------
1 file changed, 3 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index c251e6b34de0..ca4f8e4c5d5d 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -5,6 +5,7 @@
#include <drm/drm_kunit_helpers.h>
#include <drm/drm_managed.h>

+#include <kunit/device.h>
#include <kunit/resource.h>

#include <linux/device.h>
@@ -15,28 +16,6 @@
static const struct drm_mode_config_funcs drm_mode_config_funcs = {
};

-static int fake_probe(struct platform_device *pdev)
-{
- return 0;
-}
-
-static struct platform_driver fake_platform_driver = {
- .probe = fake_probe,
- .driver = {
- .name = KUNIT_DEVICE_NAME,
- },
-};
-
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_driver_unregister,
- platform_driver_unregister,
- struct platform_driver *);
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_put,
- platform_device_put,
- struct platform_device *);
-KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_del,
- platform_device_del,
- struct platform_device *);
-
/**
* drm_kunit_helper_alloc_device - Allocate a mock device for a KUnit test

* @test: The test context object

@@ -54,34 +33,7 @@ KUNIT_DEFINE_ACTION_WRAPPER(kunit_action_platform_device_del,
*/
struct device *drm_kunit_helper_alloc_device(struct kunit *test)
{
- struct platform_device *pdev;
- int ret;
-
- ret = platform_driver_register(&fake_platform_driver);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_driver_unregister,
- &fake_platform_driver);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_device_put,
- pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = platform_device_add(pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- ret = kunit_add_action_or_reset(test,
- kunit_action_platform_device_del,
- pdev);
- KUNIT_ASSERT_EQ(test, ret, 0);
-
- return &pdev->dev;
+ return kunit_device_register(test, KUNIT_DEVICE_NAME);
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);

@@ -94,19 +46,7 @@ EXPORT_SYMBOL_GPL(drm_kunit_helper_alloc_device);
*/
void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
-
- kunit_release_action(test,
- kunit_action_platform_device_del,
- pdev);
-
- kunit_release_action(test,
- kunit_action_platform_device_put,
- pdev);
-
- kunit_release_action(test,
- kunit_action_platform_driver_unregister,
- &fake_platform_driver);
+ kunit_device_unregister(test, dev);
}
EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);


--
2.43.0.472.g3155946c3a-goog

[David Gow]

Thanks -- I've included it in v3 (which fixes a few other issues), and
will take it along with the rest of the series:
https://lore.kernel.org/linux-kselftest/20231214-kunit_bus...@google.com/T/

Cheers,
-- David

[David Gow]

On Thu, 14 Dec 2023 at 16:49, <davi...@google.com> wrote:
>
> From: Maxime Ripard <mri...@kernel.org>
>
> Kunit recently gained helpers to create test managed devices. This means
> that we no longer have to roll our own helpers in KMS and we can reuse
> them.
>
> Signed-off-by: Maxime Ripard <mri...@kernel.org>
> ---

I've tested this over a few different architectures and configs, and
it works fine.

Tested-by: David Gow <davi...@google.com>

And whoops -- forgot to add my Signed-off-by above.

Signed-off-by: David Gow <davi...@google.com>

It's identical to the version here, anyway -- I didn't make any
changes, just included it in this series to have everything in one
place:
https://lore.kernel.org/linux-kselftest/20231205090405....@kernel.org/

-- David

[Kees Cook]

On Thu, Dec 14, 2023 at 04:49:17PM +0800, davi...@google.com 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.
>
> Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
> Signed-off-by: David Gow <davi...@google.com>

Acked-by: Kees Cook <kees...@chromium.org>

--
Kees Cook

[davi...@google.com]

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/20230325043104.3...@google.com/

(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 <davi...@google.com>
---

Changes in v4:
- Update tags, fix a missing Signed-off-by.
- Link to v3: https://lore.kernel.org/r/20231214-kunit_bus...@google.com

Documentation/dev-tools/kunit/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++

drivers/gpu/drm/tests/drm_kunit_helpers.c | 66 +--------
include/kunit/device.h | 80 +++++++++++
lib/fortify_kunit.c | 5 +-

lib/kunit/Makefile | 3 +-
lib/kunit/device-impl.h | 17 +++
lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-
lib/kunit/test.c | 3 +

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
Reviewed-by: Maxime Ripard <mri...@kernel.org>

Signed-off-by: David Gow <davi...@google.com>
---

Documentation/dev-tools/kunit/api/resource.rst | 9 ++
Documentation/dev-tools/kunit/usage.rst | 50 +++++++

include/kunit/device.h | 80 +++++++++++

lib/kunit/Makefile | 3 +-
lib/kunit/device-impl.h | 17 +++
lib/kunit/device.c | 181 +++++++++++++++++++++++++
lib/kunit/kunit-test.c | 134 +++++++++++++++++-
lib/kunit/test.c | 3 +

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>

Acked-by: Kees Cook <kees...@chromium.org>

Signed-off-by: David Gow <davi...@google.com>
---

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..2e4fedc81621 100644
--- a/lib/fortify_kunit.c
+++ b/lib/fortify_kunit.c

@@ -15,6 +15,7 @@

*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>

#include <linux/slab.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"); \
\

[davi...@google.com]

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.

Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
Acked-by: Kees Cook <kees...@chromium.org>

Signed-off-by: David Gow <davi...@google.com>
---

lib/overflow_kunit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
index 34db0b3aa502..c527f6b75789 100644
--- a/lib/overflow_kunit.c
+++ b/lib/overflow_kunit.c

@@ -6,6 +6,7 @@

*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <kunit/device.h>
#include <kunit/test.h>
#include <linux/device.h>

#include <linux/kernel.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");

[davi...@google.com]

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 <bro...@kernel.org>

Signed-off-by: David Gow <davi...@google.com>
---

[davi...@google.com]

From: Maxime Ripard <mri...@kernel.org>

Kunit recently gained helpers to create test managed devices. This means
that we no longer have to roll our own helpers in KMS and we can reuse
them.

Signed-off-by: Maxime Ripard <mri...@kernel.org>

Tested-by: David Gow <davi...@google.com>

Signed-off-by: David Gow <davi...@google.com>
---

* @test: The test context object

[Greg Kroah-Hartman]

On Fri, Dec 15, 2023 at 03:39:08PM +0800, davi...@google.com 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.
>
> Reviewed-by: Matti Vaittinen <mazzies...@gmail.com>
> Reviewed-by: Maxime Ripard <mri...@kernel.org>
> Signed-off-by: David Gow <davi...@google.com>

Nice work!

Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

[Rae Moar]

On Fri, Dec 15, 2023 at 2:39 AM <davi...@google.com> wrote:
>
> From: Maxime Ripard <mri...@kernel.org>
>
> Kunit recently gained helpers to create test managed devices. This means
> that we no longer have to roll our own helpers in KMS and we can reuse
> them.

Hello!

This looks good to me. Thanks!

Reviewed-by: Rae Moar <rm...@google.com>

-Rae