Message-ID: <ddb65f28-e900-44ef-aad3-568093540c38@gmail.com>
Date:   Tue, 5 Dec 2023 10:30:07 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     davidgow@...gle.com, Rae Moar <rmoar@...gle.com>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Maxime Ripard <mripard@...nel.org>
Cc:     linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH 1/4] kunit: Add APIs for managing devices

On 12/5/23 09:31, davidgow@...gle.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@...il.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@...gle.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@...gle.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! ~~

Powered by blists - more mailing lists