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

Matti Vaittinen
2023-12-05T08:30:37Z

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

...

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

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