[1/4] kunit: Add APIs for managing devices

Commit Message

David Gow Dec. 5, 2023, 7:31 a.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Matti Vaittinen Dec. 5, 2023, 8:30 a.m. UTC | #1

On 12/5/23 09:31, [e-mail] 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 <[e-mail]>

> --- /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 <[e-mail]>
> + */
> +
> +#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 <[e-mail]>
> + */
> +

...

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

...

Maxime Ripard Dec. 5, 2023, 9 a.m. UTC | #2

Hi David,

Thanks a lot for working on this.

On Tue, Dec 05, 2023 at 03:31:33PM +0800, [e-mail] 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 <[e-mail]>
> ---
>  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 <[e-mail]>
> + */
> +
> +#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 <[e-mail]>
> + */
> +
> +#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

Amadeusz Sławiński Dec. 5, 2023, 9:02 a.m. UTC | #3

On 12/5/2023 8:31 AM, [e-mail] 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 <[e-mail]>
> ---
>   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

kernel test robot Dec. 5, 2023, 1:53 p.m. UTC | #4

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]

url:    https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base:   c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link:    https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: powerpc-randconfig-r081-20231205 (https://download.01.org/0day-ci/archive/20231205/[e-mail]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

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

kernel test robot Dec. 5, 2023, 2:59 p.m. UTC | #5

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]

url:    https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base:   c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link:    https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: i386-randconfig-141-20231205 (https://download.01.org/0day-ci/archive/20231205/[e-mail]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

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

kernel test robot Dec. 5, 2023, 3:10 p.m. UTC | #6

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c8613be119892ccceffbc550b9b9d7d68b995c9e]

url:    https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base:   c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link:    https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: x86_64-randconfig-122-20231205 (https://download.01.org/0day-ci/archive/20231205/[e-mail]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

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

kernel test robot Dec. 5, 2023, 4:05 p.m. UTC | #7

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on c8613be119892ccceffbc550b9b9d7d68b995c9e]

url:    https://github.com/intel-lab-lkp/linux/commits/davidgow-google-com/kunit-Add-APIs-for-managing-devices/20231205-153349
base:   c8613be119892ccceffbc550b9b9d7d68b995c9e
patch link:    https://lore.kernel.org/r/20231205-kunit_bus-v1-1-635036d3bc13%40google.com
patch subject: [PATCH 1/4] kunit: Add APIs for managing devices
config: x86_64-buildonly-randconfig-001-20231205 (https://download.01.org/0day-ci/archive/20231205/[e-mail]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231205/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

   ld: lib/kunit/device.o: in function `kunit_bus_init':
>> device.c:(.text+0x40): multiple definition of `init_module'; lib/kunit/test.o:test.c:(.init.text+0x0): first defined here

Greg Kroah-Hartman Dec. 5, 2023, 5:30 p.m. UTC | #8

On Tue, Dec 05, 2023 at 03:31:33PM +0800, [e-mail] 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 <[e-mail]>
> + */
> +
> +#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

David Gow Dec. 6, 2023, 7:43 a.m. UTC | #9

On Tue, 5 Dec 2023 at 16:30, Matti Vaittinen <[e-mail]> wrote:
>
> On 12/5/23 09:31, [e-mail] 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 <[e-mail]>
>
> > --- /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 <[e-mail]>
> > + */
> > +
> > +#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 <[e-mail]>
> > + */
> > +
>
> ...
>
> > +
> > +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

David Gow Dec. 6, 2023, 7:44 a.m. UTC | #10

Hey Greg,

On Wed, 6 Dec 2023 at 01:31, Greg Kroah-Hartman
<[e-mail]> wrote:
>
> On Tue, Dec 05, 2023 at 03:31:33PM +0800, [e-mail] wrote:
> > Tests for drivers often require a struct device to pass to other
> > functions. While it's possible to create these with
> > root_device_register(), or to use something like a platform device, this
> > is both a misuse of those APIs, and can be difficult to clean up after,
> > for example, a failed assertion.
> >
> > Add some KUnit-specific functions for registering and unregistering a
> > struct device:
> > - kunit_device_register()
> > - kunit_device_register_with_driver()
> > - kunit_device_unregister()
> >
> > These helpers allocate a on a 'kunit' bus which will either probe the
> > driver passed in (kunit_device_register_with_driver), or will create a
> > stub driver (kunit_device_register) which is cleaned up on test shutdown.
> >
> > Devices are automatically unregistered on test shutdown, but can be
> > manually unregistered earlier with kunit_device_unregister() in order
> > to, for example, test device release code.
>
> At first glance, nice work.  But looks like 0-day doesn't like it that
> much, so I'll wait for the next version to review it properly.

Thanks very much for taking a look. I'll send v2 with the 0-day (and
other) issues fixed sometime tomorrow.

In the meantime, I've tried to explain some of the weirder decisions
below -- it mostly boils down to the existing use-cases only wanting
an opaque 'struct device *' they can pass around, and my attempt to
find a minimal (but still sensible) implementation of that. I'm
definitely happy to tweak this to make it a more 'normal' use of the
device model where that makes sense, though, especially if it doesn't
require too much boilerplate on the part of test authors.

> One nit I did notice:
>
> > +// For internal use only -- registers the kunit_bus.
> > +int kunit_bus_init(void);
>
> Put stuff like this in a local .h file, don't pollute the include/linux/
> files for things that you do not want any other part of the kernel to
> call.
>

v2 will have this in lib/kunit/device-impl.h

> > +/**
> > + * kunit_device_register_with_driver() - Create a struct device for use in KUnit tests
> > + * @test: The test context object.
> > + * @name: The name to give the created device.
> > + * @drv: The struct device_driver to associate with the device.
> > + *
> > + * Creates a struct kunit_device (which is a struct device) with the given
> > + * name, and driver. The device will be cleaned up on test exit, or when
> > + * kunit_device_unregister is called. See also kunit_device_register, if you
> > + * wish KUnit to create and manage a driver for you
> > + */
> > +struct device *kunit_device_register_with_driver(struct kunit *test,
> > +                                              const char *name,
> > +                                              struct device_driver *drv);
>
> Shouldn't "struct device_driver *" be a constant pointer?

Done (and for the internal functions) for v2.
>
> But really, why is this a "raw" device_driver pointer and not a pointer
> to the driver type for your bus?

So, this is where the more difficult questions start (and where my
knowledge of the driver model gets a bit shakier).

At the moment, there's no struct kunit_driver; the goal was to have
whatever the minimal amount of infrastructure needed to get a 'struct
device *' that could be plumbed through existing code which accepts
it. (Read: mostly devres resource management stuff, get_device(),
etc.)

So, in this version, there's no:
- struct kunit_driver: we've no extra data to store / function
pointers other than what's in struct device_driver.
- The kunit_bus is as minimal as I could get it: each device matches
exactly one driver pointer (which is passed as struct
kunit_device->driver).
- The 'struct kunit_device' type is kept private, and 'struct device'
is used instead, as this is supposed to only be passed to generic
device functions (KUnit is just managing its lifecycle).

I've no problem adding these extra types to flesh this out into a more
'normal' setup, though I'd rather keep the boilerplate on the user
side minimal if possible. I suspect if we were to return a struct
kunit_device, everyone would be quickly grabbing and passing around a
raw 'struct device *' anyway, which is what the existing tests with
fake devices do (via root_device_register, which returns struct
device, or by returning &platform_device->dev from a helper).

Similarly, the kunit_bus is not ever exposed to test code, nor really
is the driver (except via kunit_device_register_with_driver(), which
isn't actually being used anywhere yet, so it may make sense to cut it
out from the next version). So, ideally tests won't even be aware that
their devices are attached to the kunit_bus, nor that they have
drivers attached: it's mostly just to make these normal enough that
they show up nicely in sysfs and play well with the devm_ resource
management functions.

>
> Oh heck, let's point out the other issues as I'm already here...
>
> > @@ -7,7 +7,8 @@ kunit-objs +=                         test.o \
> >                                       assert.o \
> >                                       try-catch.o \
> >                                       executor.o \
> > -                                     attributes.o
> > +                                     attributes.o \
> > +                                     device.o
>
> Shouldn't this file be "bus.c" as you are creating a kunit bus?
>

I've sort-of grouped this as "device helpers", as it handles KUnit
devices, drivers, and the kunit_bus, but devices are the most
user-facing part. Indeed, the bus feels like an 'implementation
detail'. Happy to rename it if that makes things more consistent,
though.

> >
> >  ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> >  kunit-objs +=                                debugfs.o
> > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > new file mode 100644
> > index 000000000000..93ace1a2297d
> > --- /dev/null
> > +++ b/lib/kunit/device.c
> > @@ -0,0 +1,176 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit basic device implementation
>
> "basic bus/driver implementation", not device, right?
>

Given that the users of this really only care about getting their
"device", and the bus/driver are more implementation details, I'd
rather go with something like "KUnit-managed device implementation" or
"KUnit device-model helpers". How do those sound?

> > + *
> > + * Implementation of struct kunit_device helpers.
> > + *
> > + * Copyright (C) 2023, Google LLC.
> > + * Author: David Gow <[e-mail]>
> > + */
> > +
> > +#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

Greg Kroah-Hartman Dec. 7, 2023, 2:29 a.m. UTC | #11

On Wed, Dec 06, 2023 at 03:44:08PM +0800, David Gow wrote:
> > But really, why is this a "raw" device_driver pointer and not a pointer
> > to the driver type for your bus?
> 
> So, this is where the more difficult questions start (and where my
> knowledge of the driver model gets a bit shakier).
> 
> At the moment, there's no struct kunit_driver; the goal was to have
> whatever the minimal amount of infrastructure needed to get a 'struct
> device *' that could be plumbed through existing code which accepts
> it. (Read: mostly devres resource management stuff, get_device(),
> etc.)
> 
> So, in this version, there's no:
> - struct kunit_driver: we've no extra data to store / function
> pointers other than what's in struct device_driver.
> - The kunit_bus is as minimal as I could get it: each device matches
> exactly one driver pointer (which is passed as struct
> kunit_device->driver).
> - The 'struct kunit_device' type is kept private, and 'struct device'
> is used instead, as this is supposed to only be passed to generic
> device functions (KUnit is just managing its lifecycle).
> 
> I've no problem adding these extra types to flesh this out into a more
> 'normal' setup, though I'd rather keep the boilerplate on the user
> side minimal if possible. I suspect if we were to return a struct
> kunit_device, everyone would be quickly grabbing and passing around a
> raw 'struct device *' anyway, which is what the existing tests with
> fake devices do (via root_device_register, which returns struct
> device, or by returning &platform_device->dev from a helper).
> 
> Similarly, the kunit_bus is not ever exposed to test code, nor really
> is the driver (except via kunit_device_register_with_driver(), which
> isn't actually being used anywhere yet, so it may make sense to cut it
> out from the next version). So, ideally tests won't even be aware that
> their devices are attached to the kunit_bus, nor that they have
> drivers attached: it's mostly just to make these normal enough that
> they show up nicely in sysfs and play well with the devm_ resource
> management functions.

Ok, this makes more sense, thank you for the detailed explaination.
Making this "generic" like you have done here seems reasonable for now.

> > > -                                     attributes.o
> > > +                                     attributes.o \
> > > +                                     device.o
> >
> > Shouldn't this file be "bus.c" as you are creating a kunit bus?
> >
> 
> I've sort-of grouped this as "device helpers", as it handles KUnit
> devices, drivers, and the kunit_bus, but devices are the most
> user-facing part. Indeed, the bus feels like an 'implementation
> detail'. Happy to rename it if that makes things more consistent,
> though.

Nah, device.o makes sense for now, thanks.

> > >  ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> > >  kunit-objs +=                                debugfs.o
> > > diff --git a/lib/kunit/device.c b/lib/kunit/device.c
> > > new file mode 100644
> > > index 000000000000..93ace1a2297d
> > > --- /dev/null
> > > +++ b/lib/kunit/device.c
> > > @@ -0,0 +1,176 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * KUnit basic device implementation
> >
> > "basic bus/driver implementation", not device, right?
> >
> 
> Given that the users of this really only care about getting their
> "device", and the bus/driver are more implementation details, I'd
> rather go with something like "KUnit-managed device implementation" or
> "KUnit device-model helpers". How do those sound?

Either would be good, thanks.

> > > + *
> > > + * Implementation of struct kunit_device helpers.
> > > + *
> > > + * Copyright (C) 2023, Google LLC.
> > > + * Author: David Gow <[e-mail]>
> > > + */
> > > +
> > > +#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

Patch

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 <[e-mail]>
+ */
+
+#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 <[e-mail]>
+ */
+
+#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