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.3761770-1-davidgow@go...
(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 davidgow@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,
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 davidgow@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 davidgow@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 davidgow@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
On 12/5/23 09:31, davidgow@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 mazziesaccount@gmail.com
--- /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 davidgow@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);
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 davidgow@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(?)
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name) +{
- struct device_driver *driver;
- int err = -ENOMEM;
- driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
- if (!driver)
return ERR_PTR(err);- driver->name = name;
- driver->bus = &kunit_bus_type;
- driver->owner = THIS_MODULE;
- err = driver_register(driver);
- if (err) {
kunit_kfree(test, driver);return ERR_PTR(err);- }
- kunit_add_action(test, driver_unregister_wrapper, driver);
- return driver;
+} +EXPORT_SYMBOL_GPL(kunit_driver_create);
+struct kunit_device *__kunit_device_register_internal(struct kunit *test,
const char *name,struct device_driver *drv)
Very much nitpicking only - but do you think either the "__"-prefix or the "_internal"-suffix would be enough and not both? (Just to make function a tad shorter, not that it matters much though).
+{
- struct kunit_device *kunit_dev;
- int err = -ENOMEM;
- kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
- if (!kunit_dev)
return ERR_PTR(err);- kunit_dev->owner = test;
- err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
- if (err) {
kfree(kunit_dev);return ERR_PTR(err);- }
- /* Set the expected driver pointer, so we match. */
- kunit_dev->driver = drv;
- kunit_dev->dev.release = kunit_device_release;
- kunit_dev->dev.bus = &kunit_bus_type;
- kunit_dev->dev.parent = &kunit_bus;
- err = device_register(&kunit_dev->dev);
- if (err) {
put_device(&kunit_dev->dev);return ERR_PTR(err);- }
- kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
- return kunit_dev;
+}
...
On Tue, 5 Dec 2023 at 16:30, Matti Vaittinen mazziesaccount@gmail.com wrote:
On 12/5/23 09:31, davidgow@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 mazziesaccount@gmail.com
--- /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 davidgow@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);
I wish the return values for error case(s) were also mentioned. But please, see my next comment as well.
I'll add these for v2.
+#endif
+#endif
...
diff --git a/lib/kunit/device.c b/lib/kunit/device.c new file mode 100644 index 000000000000..93ace1a2297d --- /dev/null +++ b/lib/kunit/device.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- KUnit basic device implementation
- Implementation of struct kunit_device helpers.
- Copyright (C) 2023, Google LLC.
- Author: David Gow davidgow@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(?)
I'll add at least something to the implementations, too.
We've mostly kept the full documentation in the headers so they can be found by people who only have headers installed, but also because the headers tend to be smaller, and sphinx runs slowly enough as it is without needing a bigger file to parse.
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name) +{
struct device_driver *driver;int err = -ENOMEM;driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);if (!driver)return ERR_PTR(err);driver->name = name;driver->bus = &kunit_bus_type;driver->owner = THIS_MODULE;err = driver_register(driver);if (err) {kunit_kfree(test, driver);return ERR_PTR(err);}kunit_add_action(test, driver_unregister_wrapper, driver);return driver;+} +EXPORT_SYMBOL_GPL(kunit_driver_create);
+struct kunit_device *__kunit_device_register_internal(struct kunit *test,
const char *name,struct device_driver *drv)Very much nitpicking only - but do you think either the "__"-prefix or the "_internal"-suffix would be enough and not both? (Just to make function a tad shorter, not that it matters much though).
Fair enough, I've tentatively got rid of the underscores for v2.
+{
struct kunit_device *kunit_dev;int err = -ENOMEM;kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);if (!kunit_dev)return ERR_PTR(err);kunit_dev->owner = test;err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);if (err) {kfree(kunit_dev);return ERR_PTR(err);}/* Set the expected driver pointer, so we match. */kunit_dev->driver = drv;kunit_dev->dev.release = kunit_device_release;kunit_dev->dev.bus = &kunit_bus_type;kunit_dev->dev.parent = &kunit_bus;err = device_register(&kunit_dev->dev);if (err) {put_device(&kunit_dev->dev);return ERR_PTR(err);}kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);return kunit_dev;+}
Thanks, -- David
Hi David,
Thanks a lot for working on this.
On Tue, Dec 05, 2023 at 03:31:33PM +0800, davidgow@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.
Signed-off-by: David Gow davidgow@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
^ Devices
+----------------------------------
+When testing drivers or code which interacts with drivers, many functions will +require a ``struct device`` or ``struct device_driver``. In many cases, setting +up a real device is not required to test any given function, so a fake device +can be used instead.
+KUnit provides helper functions to create and manage these fake devices, which +are internally of type ``struct kunit_device``, and are attached to a special +``kunit_bus``. These devices support managed device resources (devres), as +described in Documentation/driver-api/driver-model/devres.rst
+To create a KUnit-managed ``struct device_driver``, use ``kunit_driver_create()``, +which will create a driver with the given name, on the ``kunit_bus``. This driver +will automatically be destroyed when the corresponding test finishes, but can also +be manually destroyed with ``driver_unregister()``.
+To create a fake device, use the ``kunit_device_register()``, which will create +and register a device, using a new KUnit-managed driver created with ``kunit_driver_create()``. +To provide a specific, non-KUnit-managed driver, use ``kunit_device_register_with_driver()`` +instead. Like with managed drivers, KUnit-managed fake devices are automatically +cleaned up when the test finishes, but can be manually cleaned up early with +``kunit_device_unregister()``.
I think we should add a test for that, just like we did for root and platform devices. We've been bitten by that before :)
+The KUnit devices should be used in preference to ``root_device_register()``, and +instead of ``platform_device_register()`` in cases where the device is not otherwise +a platform device.
+For example:
+.. code-block:: c
- #include <kunit/device.h>
- static void test_my_device(struct kunit *test)
- {
struct device *fake_device;const char *dev_managed_string;// Create a fake device.fake_device = kunit_device_register(test, "my_device");// Pass it to functions which need a device.dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");// Everything is cleaned up automatically when the test ends.- }
\ No newline at end of file diff --git a/include/kunit/device.h b/include/kunit/device.h new file mode 100644 index 000000000000..fd2193bc55f1 --- /dev/null +++ b/include/kunit/device.h @@ -0,0 +1,76 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- KUnit basic device implementation
- Helpers for creating and managing fake devices for KUnit tests.
- Copyright (C) 2023, Google LLC.
- Author: David Gow davidgow@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.oifeq ($(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 davidgow@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);
The preferred syntax is sizeof(*kunit_dev) here
- if (!kunit_dev)
return ERR_PTR(err);- kunit_dev->owner = test;
- err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
- if (err) {
kfree(kunit_dev);return ERR_PTR(err);- }
- /* Set the expected driver pointer, so we match. */
- kunit_dev->driver = drv;
- kunit_dev->dev.release = kunit_device_release;
- kunit_dev->dev.bus = &kunit_bus_type;
- kunit_dev->dev.parent = &kunit_bus;
- err = device_register(&kunit_dev->dev);
- if (err) {
put_device(&kunit_dev->dev);return ERR_PTR(err);- }
- kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
- return kunit_dev;
+}
+struct device *kunit_device_register_with_driver(struct kunit *test,
const char *name,struct device_driver *drv)+{
- struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
- if (IS_ERR_OR_NULL(kunit_dev))
return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */- return &kunit_dev->dev;
+} +EXPORT_SYMBOL_GPL(kunit_device_register_with_driver);
+struct device *kunit_device_register(struct kunit *test, const char *name) +{
- struct device_driver *drv = kunit_driver_create(test, name);
- struct kunit_device *dev;
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, drv);
- dev = __kunit_device_register_internal(test, name, drv);
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
- dev->cleanup_driver = true;
- return (struct device *)dev;
+} +EXPORT_SYMBOL_GPL(kunit_device_register);
+void kunit_device_unregister(struct kunit *test, struct device *dev) +{
- bool cleanup_driver = ((struct kunit_device *)dev)->cleanup_driver;
- struct device_driver *driver = ((struct kunit_device *)dev)->driver;
- kunit_release_action(test, device_unregister_wrapper, dev);
- if (cleanup_driver)
kunit_release_action(test, driver_unregister_wrapper, driver);+} +EXPORT_SYMBOL_GPL(kunit_device_unregister);
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 3e9c5192d095..a4007158bf36 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -8,6 +8,9 @@ #include <kunit/test.h> #include <kunit/test-bug.h> +#include <linux/device.h> +#include <kunit/device.h>
#include "string-stream.h" #include "try-catch-impl.h" @@ -687,6 +690,69 @@ static struct kunit_case kunit_current_test_cases[] = { {} }; +static void test_dev_action(void *priv) +{
- *(void **)priv = (void *)1;
+}
+static void kunit_device_test(struct kunit *test) +{
- struct device *test_device;
- test_device = kunit_device_register(test, "my_device");
- KUNIT_ASSERT_NOT_NULL(test, test_device);
- // Add an action to verify cleanup.
- devm_add_action(test_device, test_dev_action, &test->priv);
- KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)0);
- kunit_device_unregister(test, test_device);
- KUNIT_EXPECT_PTR_EQ(test, test->priv, (void *)1);
+}
Oh, it looks like you do check there, sorry.
I guess I would have expected an explicit test for that, but that works :)
+static void kunit_device_driver_test(struct kunit *test) +{
- struct device_driver *test_driver;
- struct device *test_device;
- test_driver = kunit_driver_create(test, "my_driver");
- KUNIT_ASSERT_NOT_NULL(test, test_driver);
- test_device = kunit_device_register_with_driver(test, "my_device", test_driver);
- KUNIT_ASSERT_NOT_NULL(test, test_device);
Should we test that the probe (and remove) hooks has been called too?
Maxime
On 12/5/2023 8:31 AM, davidgow@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.
Signed-off-by: David Gow davidgow@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 +----------------------------------
Typo: Devices
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... 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.VQD1viY5-lkp@i...) 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.VQD1viY5-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312052114.VQD1viY5-lkp@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
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... 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.ic1pg0uo-lkp@i...) 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.ic1pg0uo-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312052230.ic1pg0uo-lkp@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]
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
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... 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.IGXJP7wU-lkp@i...) 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.IGXJP7wU-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312052210.IGXJP7wU-lkp@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?
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
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... 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.fEujgbbC-lkp@i...) 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.fEujgbbC-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312052341.fEujgbbC-lkp@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
On Tue, Dec 05, 2023 at 03:31:33PM +0800, davidgow@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 davidgow@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?
+}
+static struct bus_type kunit_bus_type = {
- .name = "kunit",
- .match = kunit_bus_match
+};
+int kunit_bus_init(void) +{
- int error;
- error = bus_register(&kunit_bus_type);
- if (!error) {
error = device_register(&kunit_bus);if (error)bus_unregister(&kunit_bus_type);- }
- return error;
+} +late_initcall(kunit_bus_init);
+static void kunit_device_release(struct device *d) +{
- kfree(to_kunit_device(d));
+}
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name) +{
- struct device_driver *driver;
- int err = -ENOMEM;
- driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
- if (!driver)
return ERR_PTR(err);- driver->name = name;
- driver->bus = &kunit_bus_type;
- driver->owner = THIS_MODULE;
- err = driver_register(driver);
- if (err) {
kunit_kfree(test, driver);return ERR_PTR(err);- }
- kunit_add_action(test, driver_unregister_wrapper, driver);
- return driver;
+} +EXPORT_SYMBOL_GPL(kunit_driver_create);
+struct kunit_device *__kunit_device_register_internal(struct kunit *test,
const char *name,struct device_driver *drv)+{
- struct kunit_device *kunit_dev;
- int err = -ENOMEM;
- kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);
- if (!kunit_dev)
return ERR_PTR(err);- kunit_dev->owner = test;
- err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);
- if (err) {
kfree(kunit_dev);return ERR_PTR(err);- }
- /* Set the expected driver pointer, so we match. */
- kunit_dev->driver = drv;
Ah, so this is the match function to pass above? If so, why do you need it at all?
- kunit_dev->dev.release = kunit_device_release;
- kunit_dev->dev.bus = &kunit_bus_type;
- kunit_dev->dev.parent = &kunit_bus;
- err = device_register(&kunit_dev->dev);
- if (err) {
put_device(&kunit_dev->dev);return ERR_PTR(err);- }
- kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);
- return kunit_dev;
+}
+struct device *kunit_device_register_with_driver(struct kunit *test,
const char *name,struct device_driver *drv)+{
- struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);
- if (IS_ERR_OR_NULL(kunit_dev))
This is almost always a sign that something is wrong with the api.
return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */
Ick, the cast is odd, are you sure you need it? Why would you return a struct device and not a kunit_device() anyway?
- return &kunit_dev->dev;
Again, why this type, why not use the real type you have?
thanks,
greg k-h
Hey Greg,
On Wed, 6 Dec 2023 at 01:31, Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Tue, Dec 05, 2023 at 03:31:33PM +0800, davidgow@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.oShouldn't this file be "bus.c" as you are creating a kunit bus?
I've sort-of grouped this as "device helpers", as it handles KUnit devices, drivers, and the kunit_bus, but devices are the most user-facing part. Indeed, the bus feels like an 'implementation detail'. Happy to rename it if that makes things more consistent, though.
ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o diff --git a/lib/kunit/device.c b/lib/kunit/device.c new file mode 100644 index 000000000000..93ace1a2297d --- /dev/null +++ b/lib/kunit/device.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- KUnit basic device implementation
"basic bus/driver implementation", not device, right?
Given that the users of this really only care about getting their "device", and the bus/driver are more implementation details, I'd rather go with something like "KUnit-managed device implementation" or "KUnit device-model helpers". How do those sound?
- Implementation of struct kunit_device helpers.
- Copyright (C) 2023, Google LLC.
- Author: David Gow davidgow@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.
Will do. Would root_device_register() make more sense here?
+/* A device owned by a KUnit test. */ +struct kunit_device {
struct device dev;struct kunit *owner;/* Force binding to a specific driver. */struct device_driver *driver;/* The driver is managed by KUnit and unique to this device. */bool cleanup_driver;+};
Wait, why isn't your "kunit" device above a struct kunit_device structure? Why is it ok to be a "raw" struct device (hint, that's almost never a good idea.)
+static inline struct kunit_device *to_kunit_device(struct device *d) +{
return container_of(d, struct kunit_device, dev);container_of_const()? And to use that properly, why not make this a #define?
+}
+static int kunit_bus_match(struct device *dev, struct device_driver *driver) +{
struct kunit_device *kunit_dev = to_kunit_device(dev);if (kunit_dev->driver == driver)return 1;return 0;I don't understand, what are you trying to match here?
This is just to make sure that the match function will use whatever driver is passed through. When I originally started writing this, there were some resource cleanup problems if there was no driver associated with a device, though that's fixed now.
+}
+static struct bus_type kunit_bus_type = {
.name = "kunit",.match = kunit_bus_match+};
+int kunit_bus_init(void) +{
int error;error = bus_register(&kunit_bus_type);if (!error) {error = device_register(&kunit_bus);if (error)bus_unregister(&kunit_bus_type);}return error;+} +late_initcall(kunit_bus_init);
+static void kunit_device_release(struct device *d) +{
kfree(to_kunit_device(d));+}
+struct device_driver *kunit_driver_create(struct kunit *test, const char *name) +{
struct device_driver *driver;int err = -ENOMEM;driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);if (!driver)return ERR_PTR(err);driver->name = name;driver->bus = &kunit_bus_type;driver->owner = THIS_MODULE;err = driver_register(driver);if (err) {kunit_kfree(test, driver);return ERR_PTR(err);}kunit_add_action(test, driver_unregister_wrapper, driver);return driver;+} +EXPORT_SYMBOL_GPL(kunit_driver_create);
+struct kunit_device *__kunit_device_register_internal(struct kunit *test,
const char *name,struct device_driver *drv)+{
struct kunit_device *kunit_dev;int err = -ENOMEM;kunit_dev = kzalloc(sizeof(struct kunit_device), GFP_KERNEL);if (!kunit_dev)return ERR_PTR(err);kunit_dev->owner = test;err = dev_set_name(&kunit_dev->dev, "%s.%s", test->name, name);if (err) {kfree(kunit_dev);return ERR_PTR(err);}/* Set the expected driver pointer, so we match. */kunit_dev->driver = drv;Ah, so this is the match function to pass above? If so, why do you need it at all?
This is just to make sure there's a driver attached, so that driver_detach() eventually gets called on the device, which used to be required to clean up resources in some circumstances. This has since been fixed in 699fb50d9903 ("drivers: base: Free devm resources when unregistering a device"), but it seemed worth keeping it both to make these devices seem "more normal", and potentially to facilitate users providing a struct device_driver themselves later on, should that one day be useful.
kunit_dev->dev.release = kunit_device_release;kunit_dev->dev.bus = &kunit_bus_type;kunit_dev->dev.parent = &kunit_bus;err = device_register(&kunit_dev->dev);if (err) {put_device(&kunit_dev->dev);return ERR_PTR(err);}kunit_add_action(test, device_unregister_wrapper, &kunit_dev->dev);return kunit_dev;+}
+struct device *kunit_device_register_with_driver(struct kunit *test,
const char *name,struct device_driver *drv)+{
struct kunit_device *kunit_dev = __kunit_device_register_internal(test, name, drv);if (IS_ERR_OR_NULL(kunit_dev))This is almost always a sign that something is wrong with the api.
This can probably just be IS_ERR().
return (struct device *)kunit_dev; /* This is an error or NULL, so is compatible */Ick, the cast is odd, are you sure you need it? Why would you return a struct device and not a kunit_device() anyway?
All the users of this only want the struct device, so it's more convenient if that's all we return (and it lets us keep struct kunit_device private). But it's a minor inconvenience at worst, so I don't mind changing it.
return &kunit_dev->dev;Again, why this type, why not use the real type you have?
As above, to keep 'struct kunit_device' private.
Thanks again for looking at this; I'd definitely appreciate any further input you may have on the design.
Cheers, -- David
On Wed, Dec 06, 2023 at 03:44:08PM +0800, David Gow wrote:
But really, why is this a "raw" device_driver pointer and not a pointer to the driver type for your bus?
So, this is where the more difficult questions start (and where my knowledge of the driver model gets a bit shakier).
At the moment, there's no struct kunit_driver; the goal was to have whatever the minimal amount of infrastructure needed to get a 'struct device *' that could be plumbed through existing code which accepts it. (Read: mostly devres resource management stuff, get_device(), etc.)
So, in this version, there's no:
- struct kunit_driver: we've no extra data to store / function
pointers other than what's in struct device_driver.
- The kunit_bus is as minimal as I could get it: each device matches
exactly one driver pointer (which is passed as struct kunit_device->driver).
- The 'struct kunit_device' type is kept private, and 'struct device'
is used instead, as this is supposed to only be passed to generic device functions (KUnit is just managing its lifecycle).
I've no problem adding these extra types to flesh this out into a more 'normal' setup, though I'd rather keep the boilerplate on the user side minimal if possible. I suspect if we were to return a struct kunit_device, everyone would be quickly grabbing and passing around a raw 'struct device *' anyway, which is what the existing tests with fake devices do (via root_device_register, which returns struct device, or by returning &platform_device->dev from a helper).
Similarly, the kunit_bus is not ever exposed to test code, nor really is the driver (except via kunit_device_register_with_driver(), which isn't actually being used anywhere yet, so it may make sense to cut it out from the next version). So, ideally tests won't even be aware that their devices are attached to the kunit_bus, nor that they have drivers attached: it's mostly just to make these normal enough that they show up nicely in sysfs and play well with the devm_ resource management functions.
Ok, this makes more sense, thank you for the detailed explaination. Making this "generic" like you have done here seems reasonable for now.
attributes.o
attributes.o \device.oShouldn't this file be "bus.c" as you are creating a kunit bus?
I've sort-of grouped this as "device helpers", as it handles KUnit devices, drivers, and the kunit_bus, but devices are the most user-facing part. Indeed, the bus feels like an 'implementation detail'. Happy to rename it if that makes things more consistent, though.
Nah, device.o makes sense for now, thanks.
ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o diff --git a/lib/kunit/device.c b/lib/kunit/device.c new file mode 100644 index 000000000000..93ace1a2297d --- /dev/null +++ b/lib/kunit/device.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- KUnit basic device implementation
"basic bus/driver implementation", not device, right?
Given that the users of this really only care about getting their "device", and the bus/driver are more implementation details, I'd rather go with something like "KUnit-managed device implementation" or "KUnit device-model helpers". How do those sound?
Either would be good, thanks.
- Implementation of struct kunit_device helpers.
- Copyright (C) 2023, Google LLC.
- Author: David Gow davidgow@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.
Will do. Would root_device_register() make more sense here?
Yes.
+/* A device owned by a KUnit test. */ +struct kunit_device {
struct device dev;struct kunit *owner;/* Force binding to a specific driver. */struct device_driver *driver;/* The driver is managed by KUnit and unique to this device. */bool cleanup_driver;+};
Wait, why isn't your "kunit" device above a struct kunit_device structure? Why is it ok to be a "raw" struct device (hint, that's almost never a good idea.)
+static inline struct kunit_device *to_kunit_device(struct device *d) +{
return container_of(d, struct kunit_device, dev);container_of_const()? And to use that properly, why not make this a #define?
+}
+static int kunit_bus_match(struct device *dev, struct device_driver *driver) +{
struct kunit_device *kunit_dev = to_kunit_device(dev);if (kunit_dev->driver == driver)return 1;return 0;I don't understand, what are you trying to match here?
This is just to make sure that the match function will use whatever driver is passed through. When I originally started writing this, there were some resource cleanup problems if there was no driver associated with a device, though that's fixed now.
As it's fixed, no need for this, so let's not be confused going forward :)
thanks,
greg k-h
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 davidgow@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)
On 12/5/23 09:31, davidgow@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 davidgow@google.com
Reviewed-by: Matti Vaittinen mazziesaccount@gmail.com
On Tue, Dec 05, 2023 at 03:31:34PM +0800, davidgow@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 davidgow@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)
Acked-by: Kees Cook keescook@chromium.org
(As an aside; shouldn't this get automatically cleaned up like other kunit resources, though?)
On Thu, 7 Dec 2023 at 05:07, Kees Cook keescook@chromium.org wrote:
On Tue, Dec 05, 2023 at 03:31:34PM +0800, davidgow@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 davidgow@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)
Acked-by: Kees Cook keescook@chromium.org
(As an aside; shouldn't this get automatically cleaned up like other kunit resources, though?)
We can't just get rid of the {kunit_,}device_unregister() here, because otherwise we'd have several devices with the same name during the test.
So, yes, these get automatically cleaned up, but the test would have to be restructured to either give each device a different name, or split the tests up further.
-- David
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 davidgow@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
On 12/5/23 09:31, davidgow@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 davidgow@google.com
Reviewed-by: Matti Vaittinen mazziesaccount@gmail.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 davidgow@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); }
/*
On 12/5/2023 8:31 AM, davidgow@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 davidgow@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?
/* ===== HELPER FUNCTIONS =================================================== */ @@ -21,26 +22,20 @@ */ static struct device *test_dev; -static struct device_driver test_drv = {
- .name = "sound-soc-topology-test-driver",
-};
- static int snd_soc_tplg_test_init(struct kunit *test) {
- test_dev = root_device_register("sound-soc-topology-test");
- test_dev = kunit_device_register(test, "sound-soc-topology-test"); test_dev = get_device(test_dev); if (!test_dev) return -ENODEV;
- test_dev->driver = &test_drv;
- return 0; }
static void snd_soc_tplg_test_exit(struct kunit *test) { put_device(test_dev);
- root_device_unregister(test_dev);
- kunit_device_unregister(test, test_dev); }
/*
On Tue, Dec 05, 2023 at 03:31:36PM +0800, davidgow@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 broonie@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 mripard@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);
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-Switc... 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.fDjYhKoz-lkp@i...) 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.fDjYhKoz-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312070011.fDjYhKoz-lkp@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
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-Switc... 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.HAfMuYFE-lkp@i...) 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.HAfMuYFE-lkp@i...)
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 lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312070010.HAfMuYFE-lkp@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.
vim +8 drivers/gpu/drm/tests/drm_kunit_helpers.c
7
8 #include <kunit/device.h>
9 #include <kunit/resource.h> 10
linux-kselftest-mirror@lists.linaro.org
-
Amadeusz Sławiński -
David Gow -
davidgow＠google.com
-
Greg Kroah-Hartman -
Kees Cook -
kernel test robot -
Mark Brown -
Matti Vaittinen -
Maxime Ripard