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