| Message ID | 20240422232404.213174-6-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: Add kunit tests for fixed rate and parent data | expand |
Quoting Stephen Boyd (2024-04-22 16:23:58) > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c > new file mode 100644 > index 000000000000..54af6db2a6d8 > --- /dev/null > +++ b/drivers/base/test/platform_kunit.c > @@ -0,0 +1,174 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test managed platform driver > + */ > + [...] > + > +/** > + * platform_driver_register_kunit() - Register a KUnit test managed platform driver > + * @test: test context > + * @drv: platform driver to register > + * > + * Register a test managed platform driver. This allows callers to embed the > + * @drv in a container structure and use container_of() in the probe function > + * to pass information to KUnit tests. It can be assumed that the driver has > + * probed when this function returns. > + * > + * Example > + * > + * .. code-block:: c > + * > + * struct kunit_test_context { > + * struct platform_driver pdrv; > + * const char *data; > + * }; > + * > + * static inline struct kunit_test_context * > + * to_test_context(struct platform_device *pdev) > + * { > + * return container_of(to_platform_driver(pdev->dev.driver), > + * struct kunit_test_context, > + * pdrv); > + * } > + * > + * static int kunit_platform_driver_probe(struct platform_device *pdev) > + * { > + * struct kunit_test_context *ctx; > + * > + * ctx = to_test_context(pdev); > + * ctx->data = "test data"; > + * > + * return 0; > + * } > + * > + * static void kunit_platform_driver_test(struct kunit *test) > + * { > + * struct kunit_test_context *ctx; > + * > + * ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); > + * KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + * > + * ctx->pdrv.probe = kunit_platform_driver_probe; > + * ctx->pdrv.driver.name = "kunit-platform"; > + * ctx->pdrv.driver.owner = THIS_MODULE; > + * > + * KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); > + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > + * } > + * > + * Return: 0 on success, negative errno on failure. > + */ > +int platform_driver_register_kunit(struct kunit *test, > + struct platform_driver *drv) > +{ > + int ret; > + > + ret = platform_driver_register(drv); > + if (ret) > + return ret; > + > + /* > + * Wait for the driver to probe (or at least flush out of the deferred > + * workqueue) > + */ > + wait_for_device_probe(); Should this be removed? I was thinking that this isn't a pure wrapper around platform_driver_register() because it has this wait call. Maybe it's better to have some other kunit API that can wait for a specific device to probe and timeout if it doesn't happen in that amount of time. That API would use the bus notifiers and look for BUS_NOTIFY_BOUND_DRIVER. Or maybe that function could setup a completion that the test can wait on. > + > + return kunit_add_action_or_reset(test, > + (kunit_action_t *)&platform_driver_unregister, > + drv); > +} > +EXPORT_SYMBOL_GPL(platform_driver_register_kunit);
Quoting Stephen Boyd (2024-04-24 11:11:21) > Quoting Stephen Boyd (2024-04-22 16:23:58) > > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c [...] > > + > > + /* > > + * Wait for the driver to probe (or at least flush out of the deferred > > + * workqueue) > > + */ > > + wait_for_device_probe(); > > Should this be removed? I was thinking that this isn't a pure wrapper > around platform_driver_register() because it has this wait call. Maybe > it's better to have some other kunit API that can wait for a specific > device to probe and timeout if it doesn't happen in that amount of time. > That API would use the bus notifiers and look for > BUS_NOTIFY_BOUND_DRIVER. Or maybe that function could setup a completion > that the test can wait on. I have an implementation that does this that I'll send.
On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote: > > Introduce KUnit resource wrappers around platform_driver_register(), > platform_device_alloc(), and platform_device_add() so that test authors > can register platform drivers/devices from their tests and have the > drivers/devices automatically be unregistered when the test is done. > > This makes test setup code simpler when a platform driver or platform > device is needed. Add a few test cases at the same time to make sure the > APIs work as intended. > > Cc: Brendan Higgins <brendan.higgins@linux.dev> > Cc: David Gow <davidgow@google.com> > Cc: Rae Moar <rmoar@google.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- I really like this: I think it'll definitely help make platform devices easier to use in tests. (And the handling of the unregistering is particularly much nicer than trying to do it by hand, IMO.) I've got a few suggestions below, mostly around naming and that it's probably best to put this in lib/kunit/, given that the header is in include/kunit, and there's already the generic kunit_device functionality there. There's also a control-flow-integrity issue or two, as casting function pointers will trigger that. Otherwise, this looks good. -- David > Documentation/dev-tools/kunit/api/index.rst | 5 + > .../dev-tools/kunit/api/platformdevice.rst | 10 + > drivers/base/test/Makefile | 3 + > drivers/base/test/platform_kunit-test.c | 140 ++++++++++++++ > drivers/base/test/platform_kunit.c | 174 ++++++++++++++++++ > include/kunit/platform_device.h | 15 ++ > 6 files changed, 347 insertions(+) > create mode 100644 Documentation/dev-tools/kunit/api/platformdevice.rst > create mode 100644 drivers/base/test/platform_kunit-test.c > create mode 100644 drivers/base/test/platform_kunit.c > create mode 100644 include/kunit/platform_device.h > > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst > index 282befa17edf..02b26f5e8750 100644 > --- a/Documentation/dev-tools/kunit/api/index.rst > +++ b/Documentation/dev-tools/kunit/api/index.rst > @@ -10,6 +10,7 @@ API Reference > resource > functionredirection > of > + platformdevice A note (to myself, as much as anything): for the other device wrappers, we considered them 'resources' and so bundled the documentation in with the 'resource' documentation. Maybe it'd make sense to split it out into its own device.rst file. We could optionally include the platformdevice stuff in the same file if we wanted to consolidate documentation for "device helpers", though I'm not sure if it's worthwhile. > > > This page documents the KUnit kernel testing API. It is divided into the > @@ -36,3 +37,7 @@ Driver KUnit API > Documentation/dev-tools/kunit/api/of.rst > > - Documents the KUnit device tree (OF) API > + > +Documentation/dev-tools/kunit/api/platformdevice.rst > + > + - Documents the KUnit platform device API > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst > new file mode 100644 > index 000000000000..b228fb6558c2 > --- /dev/null > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst > @@ -0,0 +1,10 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +=================== > +Platform Device API > +=================== > + > +The KUnit platform device API is used to test platform devices. > + > +.. kernel-doc:: drivers/base/test/platform_kunit.c > + :export: > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile > index e321dfc7e922..740aef267fbe 100644 > --- a/drivers/base/test/Makefile > +++ b/drivers/base/test/Makefile > @@ -1,8 +1,11 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o > > +obj-$(CONFIG_KUNIT) += platform_kunit.o > + Do we want this to be part of the kunit.ko module (and hence, probably, under lib/kunit), or to keep this as a separate module. I'm tempted, personally, to treat this as a part of KUnit, and have it be part of the same module. There are a couple of reasons for this: - It's nice to have CONFIG_KUNIT produce only one module. If we want this to be separate, I'd be tempted to put it behind its own kconfig entry. - The name platform_kunit.ko suggests (to me, at least) that this is the test for platform devices, not the implementation of the helper. I probably can be persuaded otherwise if you've got a strong preference for it to stay as-is, though. > obj-$(CONFIG_DM_KUNIT_TEST) += root-device-test.o > obj-$(CONFIG_DM_KUNIT_TEST) += platform-device-test.o > +obj-$(CONFIG_DM_KUNIT_TEST) += platform_kunit-test.o > > obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o > CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN) > diff --git a/drivers/base/test/platform_kunit-test.c b/drivers/base/test/platform_kunit-test.c > new file mode 100644 > index 000000000000..ce545532d209 > --- /dev/null > +++ b/drivers/base/test/platform_kunit-test.c > @@ -0,0 +1,140 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KUnit test for platform driver infrastructure. > + */ > + > +#include <linux/platform_device.h> > + > +#include <kunit/platform_device.h> > +#include <kunit/test.h> > + > +static const char * const kunit_devname = "kunit-platform"; > + > +/* > + * Test that platform_device_alloc_kunit() creates a platform device. > + */ > +static void platform_device_alloc_kunit_test(struct kunit *test) > +{ > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > + platform_device_alloc_kunit(test, kunit_devname, 1)); > +} > + > +/* > + * Test that platform_device_add_kunit() registers a platform device on the > + * platform bus with the proper name and id. > + */ > +static void platform_device_add_kunit_test(struct kunit *test) > +{ > + struct platform_device *pdev; > + const char *name = kunit_devname; > + const int id = -1; > + > + pdev = platform_device_alloc_kunit(test, name, id); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + > + KUNIT_EXPECT_EQ(test, 0, platform_device_add_kunit(test, pdev)); > + KUNIT_EXPECT_TRUE(test, dev_is_platform(&pdev->dev)); > + KUNIT_EXPECT_STREQ(test, pdev->name, name); > + KUNIT_EXPECT_EQ(test, pdev->id, id); > +} > + > +/* > + * Test that platform_device_add_kunit() called twice with the same device name > + * and id fails the second time and properly cleans up. > + */ > +static void platform_device_add_kunit_twice_fails_test(struct kunit *test) > +{ > + struct platform_device *pdev; > + const char *name = kunit_devname; > + const int id = -1; > + > + pdev = platform_device_alloc_kunit(test, name, id); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev)); > + > + pdev = platform_device_alloc_kunit(test, name, id); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + > + KUNIT_EXPECT_NE(test, 0, platform_device_add_kunit(test, pdev)); > +} > + > +/* > + * Test suite for struct platform_device kunit APIs > + */ > +static struct kunit_case platform_device_kunit_test_cases[] = { > + KUNIT_CASE(platform_device_alloc_kunit_test), > + KUNIT_CASE(platform_device_add_kunit_test), > + KUNIT_CASE(platform_device_add_kunit_twice_fails_test), > + {} > +}; > + > +static struct kunit_suite platform_device_kunit_suite = { > + .name = "platform_device_kunit", > + .test_cases = platform_device_kunit_test_cases, > +}; > + > +struct kunit_platform_driver_test_context { > + struct platform_driver pdrv; > + const char *data; > +}; > + > +static const char * const test_data = "test data"; > + > +static inline struct kunit_platform_driver_test_context * > +to_test_context(struct platform_device *pdev) > +{ > + return container_of(to_platform_driver(pdev->dev.driver), > + struct kunit_platform_driver_test_context, > + pdrv); > +} > + > +static int kunit_platform_driver_probe(struct platform_device *pdev) > +{ > + struct kunit_platform_driver_test_context *ctx; > + > + ctx = to_test_context(pdev); > + ctx->data = test_data; > + > + return 0; > +} > + > +/* Test that platform_driver_register_kunit() registers a driver that probes. */ > +static void platform_driver_register_kunit_test(struct kunit *test) > +{ > + struct platform_device *pdev; > + struct kunit_platform_driver_test_context *ctx; > + > + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + > + pdev = platform_device_alloc_kunit(test, kunit_devname, -1); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev)); > + > + ctx->pdrv.probe = kunit_platform_driver_probe; > + ctx->pdrv.driver.name = kunit_devname; > + ctx->pdrv.driver.owner = THIS_MODULE; > + > + KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); > + KUNIT_EXPECT_STREQ(test, ctx->data, test_data); > +} > + > +static struct kunit_case platform_driver_kunit_test_cases[] = { > + KUNIT_CASE(platform_driver_register_kunit_test), > + {} > +}; > + > +/* > + * Test suite for struct platform_driver kunit APIs > + */ > +static struct kunit_suite platform_driver_kunit_suite = { > + .name = "platform_driver_kunit", > + .test_cases = platform_driver_kunit_test_cases, > +}; > + > +kunit_test_suites( > + &platform_device_kunit_suite, > + &platform_driver_kunit_suite, > +); > + > +MODULE_LICENSE("GPL"); > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c > new file mode 100644 > index 000000000000..54af6db2a6d8 > --- /dev/null > +++ b/drivers/base/test/platform_kunit.c > @@ -0,0 +1,174 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test managed platform driver > + */ > + > +#include <linux/device/driver.h> > +#include <linux/platform_device.h> > + > +#include <kunit/platform_device.h> > +#include <kunit/resource.h> > + > +/** > + * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device > + * @test: test context > + * @name: device name of platform device to alloc > + * @id: identifier of platform device to alloc. > + * > + * Allocate a test managed platform device. The device is put when the test completes. > + * > + * Return: Allocated platform device on success, NULL on failure. > + */ > +struct platform_device * > +platform_device_alloc_kunit(struct kunit *test, const char *name, int id) I'd prefer, personally, this be named something like kunit_platform_device_alloc(), to match the existing kunit_device_register() functions. > +{ > + struct platform_device *pdev; > + > + pdev = platform_device_alloc(name, id); > + if (!pdev) > + return NULL; > + > + if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev)) Alas, casting function pointers to kunit_action_t* breaks CFI. It's worth using a wrapper, which can be created with the KUNIT_DEFINE_ACTION_WRAPPER() macro, e.g. KUNIT_DEFINE_ACTION_WRAPPER(platform_device_put_wrapper, platform_device_put, struct platform_device *); > + return NULL; > + > + return pdev; > +} > +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit); > + > +static void platform_device_add_kunit_exit(struct kunit_resource *res) > +{ > + struct platform_device *pdev = res->data; > + > + platform_device_unregister(pdev); > +} > + > +static bool > +platform_device_alloc_kunit_match(struct kunit *test, > + struct kunit_resource *res, void *match_data) > +{ > + struct platform_device *pdev = match_data; > + > + return res->data == pdev; > +} > + > +/** > + * platform_device_add_kunit() - Register a KUnit test managed platform device > + * @test: test context > + * @pdev: platform device to add > + * > + * Register a test managed platform device. The device is unregistered when the > + * test completes. > + * > + * Return: 0 on success, negative errno on failure. > + */ > +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev) As above, I'd lean towards naming this kunit_platform_device_add() for consistency with the other KUnit device helpers. > +{ > + struct kunit_resource *res; > + int ret; > + > + ret = platform_device_add(pdev); > + if (ret) > + return ret; > + > + res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev); > + if (res) { > + /* > + * Transfer the reference count of the platform device if it was > + * allocated with platform_device_alloc_kunit(). In that case, > + * calling platform_device_put() leads to reference count > + * underflow because platform_device_unregister() does it for > + * us and we call platform_device_unregister() from > + * platform_device_add_kunit_exit(). > + * > + * Usually callers transfer the refcount from > + * platform_device_alloc() to platform_device_add() and simply > + * call platform_device_unregister() when done, but with kunit > + * we have to keep this straight by redirecting the free > + * routine for the resource. > + */ > + res->free = platform_device_add_kunit_exit; > + kunit_put_resource(res); > + } else if (kunit_add_action_or_reset(test, > + (kunit_action_t *)&platform_device_unregister, > + pdev)) { Nit: We don't want to cast directly to kunit_action_t *, as that breaks CFI. Can we use KUNIT_DEFINE_ACTION_WRAPPER()? > + return -ENOMEM; Nit: This is fine, as kunit_add_action_or_reset() only returns 0 or -ENOMEM at the moment, but it could cause problems down the line if we ever want to return a different error. I don't think that's particularly likely, but it might be nicer to properly propagate the error. > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(platform_device_add_kunit); > + > +/** > + * platform_driver_register_kunit() - Register a KUnit test managed platform driver > + * @test: test context > + * @drv: platform driver to register > + * > + * Register a test managed platform driver. This allows callers to embed the > + * @drv in a container structure and use container_of() in the probe function > + * to pass information to KUnit tests. It can be assumed that the driver has > + * probed when this function returns. > + * > + * Example > + * > + * .. code-block:: c > + * > + * struct kunit_test_context { > + * struct platform_driver pdrv; > + * const char *data; > + * }; > + * > + * static inline struct kunit_test_context * > + * to_test_context(struct platform_device *pdev) > + * { > + * return container_of(to_platform_driver(pdev->dev.driver), > + * struct kunit_test_context, > + * pdrv); > + * } > + * > + * static int kunit_platform_driver_probe(struct platform_device *pdev) > + * { > + * struct kunit_test_context *ctx; > + * > + * ctx = to_test_context(pdev); > + * ctx->data = "test data"; > + * > + * return 0; > + * } > + * > + * static void kunit_platform_driver_test(struct kunit *test) > + * { > + * struct kunit_test_context *ctx; > + * > + * ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); > + * KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + * > + * ctx->pdrv.probe = kunit_platform_driver_probe; > + * ctx->pdrv.driver.name = "kunit-platform"; > + * ctx->pdrv.driver.owner = THIS_MODULE; > + * > + * KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); > + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > + * } > + * > + * Return: 0 on success, negative errno on failure. > + */ > +int platform_driver_register_kunit(struct kunit *test, > + struct platform_driver *drv) As above, I'd prefer kunit_platform_driver_register() > +{ > + int ret; > + > + ret = platform_driver_register(drv); > + if (ret) > + return ret; > + > + /* > + * Wait for the driver to probe (or at least flush out of the deferred > + * workqueue) > + */ > + wait_for_device_probe(); Personally, I don't mind if this wrapper waits here (even if it makes it less of a 'pure' wrapper), so long as we document it. Can you think of any cases where we explicitly want _not_ to wait in a test? > + > + return kunit_add_action_or_reset(test, > + (kunit_action_t *)&platform_driver_unregister, > + drv); > +} > +EXPORT_SYMBOL_GPL(platform_driver_register_kunit); > diff --git a/include/kunit/platform_device.h b/include/kunit/platform_device.h > new file mode 100644 > index 000000000000..28d28abf15a4 > --- /dev/null > +++ b/include/kunit/platform_device.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _KUNIT_PLATFORM_DRIVER_H > +#define _KUNIT_PLATFORM_DRIVER_H > + > +struct kunit; > +struct platform_device; > +struct platform_driver; > + > +struct platform_device * > +platform_device_alloc_kunit(struct kunit *test, const char *name, int id); > +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev); > + > +int platform_driver_register_kunit(struct kunit *test, struct platform_driver *drv); > + > +#endif > -- > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ > https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240422232404.213174-6-sboyd%40kernel.org.
Quoting David Gow (2024-05-01 00:55:46) > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote: > > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst > > new file mode 100644 > > index 000000000000..b228fb6558c2 > > --- /dev/null > > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst > > @@ -0,0 +1,10 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +=================== > > +Platform Device API > > +=================== > > + > > +The KUnit platform device API is used to test platform devices. > > + > > +.. kernel-doc:: drivers/base/test/platform_kunit.c > > + :export: > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile > > index e321dfc7e922..740aef267fbe 100644 > > --- a/drivers/base/test/Makefile > > +++ b/drivers/base/test/Makefile > > @@ -1,8 +1,11 @@ > > # SPDX-License-Identifier: GPL-2.0 > > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o > > > > +obj-$(CONFIG_KUNIT) += platform_kunit.o > > + > > Do we want this to be part of the kunit.ko module (and hence, > probably, under lib/kunit), or to keep this as a separate module. > I'm tempted, personally, to treat this as a part of KUnit, and have it > be part of the same module. There are a couple of reasons for this: > - It's nice to have CONFIG_KUNIT produce only one module. If we want > this to be separate, I'd be tempted to put it behind its own kconfig > entry. > - The name platform_kunit.ko suggests (to me, at least) that this is > the test for platform devices, not the implementation of the helper. I was following *_kunit as "helpers" and *_test as the test. Only loosely based on the documentation that mentions to use _test or _kunit for test files. Maybe it should have _kunit_helpers postfix? Following the single module design should I merge the tests for this code into kunit-test.c? And do the same sort of thing for clk helpers? That sounds like it won't scale very well if everything is in one module. Shouldn't the wrapper code for subsystems live in those subsystems like drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should be moved out to drivers/base/? lib/kunit can stay focused on providing pure kunit code then. > > I probably can be persuaded otherwise if you've got a strong > preference for it to stay as-is, though. > > > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c > > new file mode 100644 > > index 000000000000..54af6db2a6d8 > > --- /dev/null > > +++ b/drivers/base/test/platform_kunit.c > > @@ -0,0 +1,174 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Test managed platform driver > > + */ > > + > > +#include <linux/device/driver.h> > > +#include <linux/platform_device.h> > > + > > +#include <kunit/platform_device.h> > > +#include <kunit/resource.h> > > + > > +/** > > + * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device > > + * @test: test context > > + * @name: device name of platform device to alloc > > + * @id: identifier of platform device to alloc. > > + * > > + * Allocate a test managed platform device. The device is put when the test completes. > > + * > > + * Return: Allocated platform device on success, NULL on failure. > > + */ > > +struct platform_device * > > +platform_device_alloc_kunit(struct kunit *test, const char *name, int id) > > I'd prefer, personally, this be named something like > kunit_platform_device_alloc(), to match the existing > kunit_device_register() functions. > > > > +{ > > + struct platform_device *pdev; > > + > > + pdev = platform_device_alloc(name, id); > > + if (!pdev) > > + return NULL; > > + > > + if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev)) > > Alas, casting function pointers to kunit_action_t* breaks CFI. It's > worth using a wrapper, which can be created with the > KUNIT_DEFINE_ACTION_WRAPPER() macro, e.g. > > KUNIT_DEFINE_ACTION_WRAPPER(platform_device_put_wrapper, > platform_device_put, struct platform_device *); Thanks. I missed that. > > > + return NULL; > > + > > + return pdev; > > +} > > +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit); > > + > > +static void platform_device_add_kunit_exit(struct kunit_resource *res) > > +{ > > + struct platform_device *pdev = res->data; > > + > > + platform_device_unregister(pdev); > > +} > > + > > +static bool > > +platform_device_alloc_kunit_match(struct kunit *test, > > + struct kunit_resource *res, void *match_data) > > +{ > > + struct platform_device *pdev = match_data; > > + > > + return res->data == pdev; > > +} > > + > > +/** > > + * platform_device_add_kunit() - Register a KUnit test managed platform device > > + * @test: test context > > + * @pdev: platform device to add > > + * > > + * Register a test managed platform device. The device is unregistered when the > > + * test completes. > > + * > > + * Return: 0 on success, negative errno on failure. > > + */ > > +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev) > > As above, I'd lean towards naming this kunit_platform_device_add() for > consistency with the other KUnit device helpers. > > > +{ > > + struct kunit_resource *res; > > + int ret; > > + > > + ret = platform_device_add(pdev); > > + if (ret) > > + return ret; > > + > > + res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev); > > + if (res) { > > + /* > > + * Transfer the reference count of the platform device if it was > > + * allocated with platform_device_alloc_kunit(). In that case, > > + * calling platform_device_put() leads to reference count > > + * underflow because platform_device_unregister() does it for > > + * us and we call platform_device_unregister() from > > + * platform_device_add_kunit_exit(). > > + * > > + * Usually callers transfer the refcount from > > + * platform_device_alloc() to platform_device_add() and simply > > + * call platform_device_unregister() when done, but with kunit > > + * we have to keep this straight by redirecting the free > > + * routine for the resource. > > + */ > > + res->free = platform_device_add_kunit_exit; > > + kunit_put_resource(res); > > + } else if (kunit_add_action_or_reset(test, > > + (kunit_action_t *)&platform_device_unregister, > > + pdev)) { > > Nit: We don't want to cast directly to kunit_action_t *, as that > breaks CFI. Can we use KUNIT_DEFINE_ACTION_WRAPPER()? > > > + return -ENOMEM; > > Nit: This is fine, as kunit_add_action_or_reset() only returns 0 or > -ENOMEM at the moment, but it could cause problems down the line if we > ever want to return a different error. I don't think that's > particularly likely, but it might be nicer to properly propagate the > error. I will propagate the return value. > > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(platform_device_add_kunit); > > + > > +/** > > + * platform_driver_register_kunit() - Register a KUnit test managed platform driver > > + * @test: test context > > + * @drv: platform driver to register > > + * > > + * Register a test managed platform driver. This allows callers to embed the > > + * @drv in a container structure and use container_of() in the probe function > > + * to pass information to KUnit tests. It can be assumed that the driver has > > + * probed when this function returns. > > + * > > + * Example > > + * > > + * .. code-block:: c > > + * > > + * struct kunit_test_context { > > + * struct platform_driver pdrv; > > + * const char *data; > > + * }; > > + * > > + * static inline struct kunit_test_context * > > + * to_test_context(struct platform_device *pdev) > > + * { > > + * return container_of(to_platform_driver(pdev->dev.driver), > > + * struct kunit_test_context, > > + * pdrv); > > + * } > > + * > > + * static int kunit_platform_driver_probe(struct platform_device *pdev) > > + * { > > + * struct kunit_test_context *ctx; > > + * > > + * ctx = to_test_context(pdev); > > + * ctx->data = "test data"; > > + * > > + * return 0; > > + * } > > + * > > + * static void kunit_platform_driver_test(struct kunit *test) > > + * { > > + * struct kunit_test_context *ctx; > > + * > > + * ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); > > + * KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > > + * > > + * ctx->pdrv.probe = kunit_platform_driver_probe; > > + * ctx->pdrv.driver.name = "kunit-platform"; > > + * ctx->pdrv.driver.owner = THIS_MODULE; > > + * > > + * KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); > > + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > > + * } > > + * > > + * Return: 0 on success, negative errno on failure. > > + */ > > +int platform_driver_register_kunit(struct kunit *test, > > + struct platform_driver *drv) > > As above, I'd prefer kunit_platform_driver_register() > > > +{ > > + int ret; > > + > > + ret = platform_driver_register(drv); > > + if (ret) > > + return ret; > > + > > + /* > > + * Wait for the driver to probe (or at least flush out of the deferred > > + * workqueue) > > + */ > > + wait_for_device_probe(); > > Personally, I don't mind if this wrapper waits here (even if it makes > it less of a 'pure' wrapper), so long as we document it. Can you think > of any cases where we explicitly want _not_ to wait in a test? > I don't like it because it's not deterministic. The function doesn't take any struct device to wait for. I've already written the code to use a completion, and it works well enough so I'll just do that. Then we don't have to worry if this API goes away, or that it doesn't actually determine if the driver has probed the device.
On Fri, 3 May 2024 at 09:04, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting David Gow (2024-05-01 00:55:46) > > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote: > > > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst > > > new file mode 100644 > > > index 000000000000..b228fb6558c2 > > > --- /dev/null > > > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst > > > @@ -0,0 +1,10 @@ > > > +.. SPDX-License-Identifier: GPL-2.0 > > > + > > > +=================== > > > +Platform Device API > > > +=================== > > > + > > > +The KUnit platform device API is used to test platform devices. > > > + > > > +.. kernel-doc:: drivers/base/test/platform_kunit.c > > > + :export: > > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile > > > index e321dfc7e922..740aef267fbe 100644 > > > --- a/drivers/base/test/Makefile > > > +++ b/drivers/base/test/Makefile > > > @@ -1,8 +1,11 @@ > > > # SPDX-License-Identifier: GPL-2.0 > > > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o > > > > > > +obj-$(CONFIG_KUNIT) += platform_kunit.o > > > + > > > > Do we want this to be part of the kunit.ko module (and hence, > > probably, under lib/kunit), or to keep this as a separate module. > > I'm tempted, personally, to treat this as a part of KUnit, and have it > > be part of the same module. There are a couple of reasons for this: > > - It's nice to have CONFIG_KUNIT produce only one module. If we want > > this to be separate, I'd be tempted to put it behind its own kconfig > > entry. > > - The name platform_kunit.ko suggests (to me, at least) that this is > > the test for platform devices, not the implementation of the helper. > > I was following *_kunit as "helpers" and *_test as the test. Only > loosely based on the documentation that mentions to use _test or _kunit > for test files. Maybe it should have _kunit_helpers postfix? Yeah, the style guide currently suggests that *_test is the default for tests, but that _kunit may also be used for tests if _test is already used for non-KUnit tests: https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-names DRM has drm_kunit_helpers, so _kunit_helpers seems like a good suffix to settle on. > Following the single module design should I merge the tests for this > code into kunit-test.c? And do the same sort of thing for clk helpers? > That sounds like it won't scale very well if everything is in one module. I don't think it's as important that the tests live in the same module. It's nice from an ergonomic point-of-view to only have to modprobe the one thing, but we've already let that ship sail somewhat with string-stream-test. Either way, splitting up kunit-test.c is something we'll almost certainly want to do at some point, and we can always put them into the same module even if they're different source files if we have to. > > Shouldn't the wrapper code for subsystems live in those subsystems like > drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should > be moved out to drivers/base/? lib/kunit can stay focused on providing > pure kunit code then. I tend to agree that wrapper code for subsystems should live in those subsystems, especially if the subsystems are relatively self-contained (i.e., the helpers are used to test that subsystem itself, rather than exported for other parts of the kernel to use to test interactions with said subsystem). For 'core' parts of the kernel, I think it makes it easier to make these obviously part of KUnit (e.g. kunit_kzalloc() is easier to have within KUnit, rather than as a part of the allocators). The struct device wrappers have the problem that they rely on the kunit_bus being registered, which is currently done when the kunit module is loaded. So it hooks more deeply into KUnit than is comfortable to do from drivers/base. So we've treated it as a 'core' part of the kernel. Ultimately, it's a grey area, so I can live with this going either way, depending on the actual helpers, so long as we don't end up with lots of half-in/half-out helpers, which behave a bit like both. (For example, at the moment, helpers which live outside lib/kunit are documented and have headers in the respective subsystems' directories.) FWIW, my gut feeling for what's "most consistent" with what we've done so far is: 1. platform_device helpers should live alongside the current managed device stuff, which is currently in lib/kunit 2. clk helpers should probably live in clk 3. of/of_overlay sits a bit in the middle, but having thought more about it, it'd probably lean towards having it be part of 'of', not 'kunit. But all of this is, to some extent, just bikeshedding, so as long as we pick somewhere to put them, and don't mix things up too much, I don't think it matters exactly what side of this fuzzy line they end up on. > > > > I probably can be persuaded otherwise if you've got a strong > > preference for it to stay as-is, though. > > > > > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c > > > new file mode 100644 > > > index 000000000000..54af6db2a6d8 > > > --- /dev/null > > > +++ b/drivers/base/test/platform_kunit.c > > > @@ -0,0 +1,174 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Test managed platform driver > > > + */ > > > + > > > +#include <linux/device/driver.h> > > > +#include <linux/platform_device.h> > > > + > > > +#include <kunit/platform_device.h> > > > +#include <kunit/resource.h> > > > + > > > +/** > > > + * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device > > > + * @test: test context > > > + * @name: device name of platform device to alloc > > > + * @id: identifier of platform device to alloc. > > > + * > > > + * Allocate a test managed platform device. The device is put when the test completes. > > > + * > > > + * Return: Allocated platform device on success, NULL on failure. > > > + */ > > > +struct platform_device * > > > +platform_device_alloc_kunit(struct kunit *test, const char *name, int id) > > > > I'd prefer, personally, this be named something like > > kunit_platform_device_alloc(), to match the existing > > kunit_device_register() functions. > > > > > > > +{ > > > + struct platform_device *pdev; > > > + > > > + pdev = platform_device_alloc(name, id); > > > + if (!pdev) > > > + return NULL; > > > + > > > + if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev)) > > > > Alas, casting function pointers to kunit_action_t* breaks CFI. It's > > worth using a wrapper, which can be created with the > > KUNIT_DEFINE_ACTION_WRAPPER() macro, e.g. > > > > KUNIT_DEFINE_ACTION_WRAPPER(platform_device_put_wrapper, > > platform_device_put, struct platform_device *); > > Thanks. I missed that. > > > > > > + return NULL; > > > + > > > + return pdev; > > > +} > > > +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit); > > > + > > > +static void platform_device_add_kunit_exit(struct kunit_resource *res) > > > +{ > > > + struct platform_device *pdev = res->data; > > > + > > > + platform_device_unregister(pdev); > > > +} > > > + > > > +static bool > > > +platform_device_alloc_kunit_match(struct kunit *test, > > > + struct kunit_resource *res, void *match_data) > > > +{ > > > + struct platform_device *pdev = match_data; > > > + > > > + return res->data == pdev; > > > +} > > > + > > > +/** > > > + * platform_device_add_kunit() - Register a KUnit test managed platform device > > > + * @test: test context > > > + * @pdev: platform device to add > > > + * > > > + * Register a test managed platform device. The device is unregistered when the > > > + * test completes. > > > + * > > > + * Return: 0 on success, negative errno on failure. > > > + */ > > > +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev) > > > > As above, I'd lean towards naming this kunit_platform_device_add() for > > consistency with the other KUnit device helpers. > > > > > +{ > > > + struct kunit_resource *res; > > > + int ret; > > > + > > > + ret = platform_device_add(pdev); > > > + if (ret) > > > + return ret; > > > + > > > + res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev); > > > + if (res) { > > > + /* > > > + * Transfer the reference count of the platform device if it was > > > + * allocated with platform_device_alloc_kunit(). In that case, > > > + * calling platform_device_put() leads to reference count > > > + * underflow because platform_device_unregister() does it for > > > + * us and we call platform_device_unregister() from > > > + * platform_device_add_kunit_exit(). > > > + * > > > + * Usually callers transfer the refcount from > > > + * platform_device_alloc() to platform_device_add() and simply > > > + * call platform_device_unregister() when done, but with kunit > > > + * we have to keep this straight by redirecting the free > > > + * routine for the resource. > > > + */ > > > + res->free = platform_device_add_kunit_exit; > > > + kunit_put_resource(res); > > > + } else if (kunit_add_action_or_reset(test, > > > + (kunit_action_t *)&platform_device_unregister, > > > + pdev)) { > > > > Nit: We don't want to cast directly to kunit_action_t *, as that > > breaks CFI. Can we use KUNIT_DEFINE_ACTION_WRAPPER()? > > > > > + return -ENOMEM; > > > > Nit: This is fine, as kunit_add_action_or_reset() only returns 0 or > > -ENOMEM at the moment, but it could cause problems down the line if we > > ever want to return a different error. I don't think that's > > particularly likely, but it might be nicer to properly propagate the > > error. > > I will propagate the return value. > > > > > > + } > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(platform_device_add_kunit); > > > + > > > +/** > > > + * platform_driver_register_kunit() - Register a KUnit test managed platform driver > > > + * @test: test context > > > + * @drv: platform driver to register > > > + * > > > + * Register a test managed platform driver. This allows callers to embed the > > > + * @drv in a container structure and use container_of() in the probe function > > > + * to pass information to KUnit tests. It can be assumed that the driver has > > > + * probed when this function returns. > > > + * > > > + * Example > > > + * > > > + * .. code-block:: c > > > + * > > > + * struct kunit_test_context { > > > + * struct platform_driver pdrv; > > > + * const char *data; > > > + * }; > > > + * > > > + * static inline struct kunit_test_context * > > > + * to_test_context(struct platform_device *pdev) > > > + * { > > > + * return container_of(to_platform_driver(pdev->dev.driver), > > > + * struct kunit_test_context, > > > + * pdrv); > > > + * } > > > + * > > > + * static int kunit_platform_driver_probe(struct platform_device *pdev) > > > + * { > > > + * struct kunit_test_context *ctx; > > > + * > > > + * ctx = to_test_context(pdev); > > > + * ctx->data = "test data"; > > > + * > > > + * return 0; > > > + * } > > > + * > > > + * static void kunit_platform_driver_test(struct kunit *test) > > > + * { > > > + * struct kunit_test_context *ctx; > > > + * > > > + * ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); > > > + * KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > > > + * > > > + * ctx->pdrv.probe = kunit_platform_driver_probe; > > > + * ctx->pdrv.driver.name = "kunit-platform"; > > > + * ctx->pdrv.driver.owner = THIS_MODULE; > > > + * > > > + * KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); > > > + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > > > + * } > > > + * > > > + * Return: 0 on success, negative errno on failure. > > > + */ > > > +int platform_driver_register_kunit(struct kunit *test, > > > + struct platform_driver *drv) > > > > As above, I'd prefer kunit_platform_driver_register() > > > > > +{ > > > + int ret; > > > + > > > + ret = platform_driver_register(drv); > > > + if (ret) > > > + return ret; > > > + > > > + /* > > > + * Wait for the driver to probe (or at least flush out of the deferred > > > + * workqueue) > > > + */ > > > + wait_for_device_probe(); > > > > Personally, I don't mind if this wrapper waits here (even if it makes > > it less of a 'pure' wrapper), so long as we document it. Can you think > > of any cases where we explicitly want _not_ to wait in a test? > > > > I don't like it because it's not deterministic. The function doesn't > take any struct device to wait for. I've already written the code to use > a completion, and it works well enough so I'll just do that. Then we > don't have to worry if this API goes away, or that it doesn't actually > determine if the driver has probed the device. Sounds good! Cheers, -- David
Quoting David Gow (2024-05-04 01:30:34) > On Fri, 3 May 2024 at 09:04, Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting David Gow (2024-05-01 00:55:46) > > > On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@kernel.org> wrote: > > > > diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst > > > > new file mode 100644 > > > > index 000000000000..b228fb6558c2 > > > > --- /dev/null > > > > +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst > > > > @@ -0,0 +1,10 @@ > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +=================== > > > > +Platform Device API > > > > +=================== > > > > + > > > > +The KUnit platform device API is used to test platform devices. > > > > + > > > > +.. kernel-doc:: drivers/base/test/platform_kunit.c > > > > + :export: > > > > diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile > > > > index e321dfc7e922..740aef267fbe 100644 > > > > --- a/drivers/base/test/Makefile > > > > +++ b/drivers/base/test/Makefile > > > > @@ -1,8 +1,11 @@ > > > > # SPDX-License-Identifier: GPL-2.0 > > > > obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o > > > > > > > > +obj-$(CONFIG_KUNIT) += platform_kunit.o > > > > + > > > > > > Do we want this to be part of the kunit.ko module (and hence, > > > probably, under lib/kunit), or to keep this as a separate module. > > > I'm tempted, personally, to treat this as a part of KUnit, and have it > > > be part of the same module. There are a couple of reasons for this: > > > - It's nice to have CONFIG_KUNIT produce only one module. If we want > > > this to be separate, I'd be tempted to put it behind its own kconfig > > > entry. > > > - The name platform_kunit.ko suggests (to me, at least) that this is > > > the test for platform devices, not the implementation of the helper. > > > > I was following *_kunit as "helpers" and *_test as the test. Only > > loosely based on the documentation that mentions to use _test or _kunit > > for test files. Maybe it should have _kunit_helpers postfix? > > Yeah, the style guide currently suggests that *_test is the default > for tests, but that _kunit may also be used for tests if _test is > already used for non-KUnit tests: > https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-names > > DRM has drm_kunit_helpers, so _kunit_helpers seems like a good suffix > to settle on. Alright, I'll rename the files. > > > Following the single module design should I merge the tests for this > > code into kunit-test.c? And do the same sort of thing for clk helpers? > > That sounds like it won't scale very well if everything is in one module. > > I don't think it's as important that the tests live in the same > module. It's nice from an ergonomic point-of-view to only have to > modprobe the one thing, but we've already let that ship sail somewhat > with string-stream-test. > > Either way, splitting up kunit-test.c is something we'll almost > certainly want to do at some point, and we can always put them into > the same module even if they're different source files if we have to. Alright. > > > > > Shouldn't the wrapper code for subsystems live in those subsystems like > > drm_kunit_helpers.c does? Maybe the struct device kunit wrappers should > > be moved out to drivers/base/? lib/kunit can stay focused on providing > > pure kunit code then. > > I tend to agree that wrapper code for subsystems should live in those > subsystems, especially if the subsystems are relatively self-contained > (i.e., the helpers are used to test that subsystem itself, rather than > exported for other parts of the kernel to use to test interactions > with said subsystem). For 'core' parts of the kernel, I think it makes > it easier to make these obviously part of KUnit (e.g. kunit_kzalloc() > is easier to have within KUnit, rather than as a part of the > allocators). > > The struct device wrappers have the problem that they rely on the > kunit_bus being registered, which is currently done when the kunit > module is loaded. So it hooks more deeply into KUnit than is > comfortable to do from drivers/base. So we've treated it as a 'core' > part of the kernel. Ok, thanks. The kzalloc wrappers look like the best example here. They are so essential that they are in lib/kunit. The platform bus is built into the kernel all the time, similar to mm, so I can see it being essential and desired to have the wrappers in lib/kunit. > > Ultimately, it's a grey area, so I can live with this going either > way, depending on the actual helpers, so long as we don't end up with > lots of half-in/half-out helpers, which behave a bit like both. (For > example, at the moment, helpers which live outside lib/kunit are > documented and have headers in the respective subsystems' > directories.) > > FWIW, my gut feeling for what's "most consistent" with what we've done > so far is: > 1. platform_device helpers should live alongside the current managed > device stuff, which is currently in lib/kunit > 2. clk helpers should probably live in clk > 3. of/of_overlay sits a bit in the middle, but having thought more > about it, it'd probably lean towards having it be part of 'of', not > 'kunit. Sounds good. I'll follow this route. > > But all of this is, to some extent, just bikeshedding, so as long as > we pick somewhere to put them, and don't mix things up too much, I > don't think it matters exactly what side of this fuzzy line they end > up on. > Yeah. My final hesitation is that it will be "too easy" to make devices that live on the platform_bus when they should really be on the kunit_bus. I guess we'll have to watch out for folks making platform devices that don't use any other platform device APIs like IO resources, etc.
Quoting Stephen Boyd (2024-04-22 16:23:58) > diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c > new file mode 100644 > index 000000000000..54af6db2a6d8 > --- /dev/null > +++ b/drivers/base/test/platform_kunit.c > @@ -0,0 +1,174 @@ [...] > +struct platform_device * > +platform_device_alloc_kunit(struct kunit *test, const char *name, int id) > +{ > + struct platform_device *pdev; > + > + pdev = platform_device_alloc(name, id); > + if (!pdev) > + return NULL; > + > + if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev)) > + return NULL; > + > + return pdev; > +} > +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit); > + > +static void platform_device_add_kunit_exit(struct kunit_resource *res) > +{ > + struct platform_device *pdev = res->data; > + > + platform_device_unregister(pdev); > +} > + > +static bool > +platform_device_alloc_kunit_match(struct kunit *test, > + struct kunit_resource *res, void *match_data) > +{ > + struct platform_device *pdev = match_data; > + > + return res->data == pdev; > +} > + > +/** > + * platform_device_add_kunit() - Register a KUnit test managed platform device > + * @test: test context > + * @pdev: platform device to add > + * > + * Register a test managed platform device. The device is unregistered when the > + * test completes. > + * > + * Return: 0 on success, negative errno on failure. > + */ > +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev) > +{ > + struct kunit_resource *res; > + int ret; > + > + ret = platform_device_add(pdev); > + if (ret) > + return ret; > + > + res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev); This doesn't work because platform_device_alloc_kunit() used kunit_add_action_or_reset() which has a chained free routine and data pointer. I've added a test to make sure the platform device is removed from the bus. It's not super great though because when this code fails to find a match it will still remove the device by calling platform_device_unregister() when the test ends. It will follow that up with a call to platform_device_put(), which is the problem as that causes an underflow and operates on an already freed device. I couldn't come up with anything better than searching the platform bus. Maybe if there was a way to allocate the memory or redirect where platform_device_alloc_kunit() got memory from we could hold the device memory around after it should have been freed and make sure the kref for the device kobject is 0. That seems pretty invasive to do though so I'm just going to leave it for now and add this test to make sure it cleans up.
diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst index 282befa17edf..02b26f5e8750 100644 --- a/Documentation/dev-tools/kunit/api/index.rst +++ b/Documentation/dev-tools/kunit/api/index.rst @@ -10,6 +10,7 @@ API Reference resource functionredirection of + platformdevice This page documents the KUnit kernel testing API. It is divided into the @@ -36,3 +37,7 @@ Driver KUnit API Documentation/dev-tools/kunit/api/of.rst - Documents the KUnit device tree (OF) API + +Documentation/dev-tools/kunit/api/platformdevice.rst + + - Documents the KUnit platform device API diff --git a/Documentation/dev-tools/kunit/api/platformdevice.rst b/Documentation/dev-tools/kunit/api/platformdevice.rst new file mode 100644 index 000000000000..b228fb6558c2 --- /dev/null +++ b/Documentation/dev-tools/kunit/api/platformdevice.rst @@ -0,0 +1,10 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=================== +Platform Device API +=================== + +The KUnit platform device API is used to test platform devices. + +.. kernel-doc:: drivers/base/test/platform_kunit.c + :export: diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile index e321dfc7e922..740aef267fbe 100644 --- a/drivers/base/test/Makefile +++ b/drivers/base/test/Makefile @@ -1,8 +1,11 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE) += test_async_driver_probe.o +obj-$(CONFIG_KUNIT) += platform_kunit.o + obj-$(CONFIG_DM_KUNIT_TEST) += root-device-test.o obj-$(CONFIG_DM_KUNIT_TEST) += platform-device-test.o +obj-$(CONFIG_DM_KUNIT_TEST) += platform_kunit-test.o obj-$(CONFIG_DRIVER_PE_KUNIT_TEST) += property-entry-test.o CFLAGS_property-entry-test.o += $(DISABLE_STRUCTLEAK_PLUGIN) diff --git a/drivers/base/test/platform_kunit-test.c b/drivers/base/test/platform_kunit-test.c new file mode 100644 index 000000000000..ce545532d209 --- /dev/null +++ b/drivers/base/test/platform_kunit-test.c @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for platform driver infrastructure. + */ + +#include <linux/platform_device.h> + +#include <kunit/platform_device.h> +#include <kunit/test.h> + +static const char * const kunit_devname = "kunit-platform"; + +/* + * Test that platform_device_alloc_kunit() creates a platform device. + */ +static void platform_device_alloc_kunit_test(struct kunit *test) +{ + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, + platform_device_alloc_kunit(test, kunit_devname, 1)); +} + +/* + * Test that platform_device_add_kunit() registers a platform device on the + * platform bus with the proper name and id. + */ +static void platform_device_add_kunit_test(struct kunit *test) +{ + struct platform_device *pdev; + const char *name = kunit_devname; + const int id = -1; + + pdev = platform_device_alloc_kunit(test, name, id); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + + KUNIT_EXPECT_EQ(test, 0, platform_device_add_kunit(test, pdev)); + KUNIT_EXPECT_TRUE(test, dev_is_platform(&pdev->dev)); + KUNIT_EXPECT_STREQ(test, pdev->name, name); + KUNIT_EXPECT_EQ(test, pdev->id, id); +} + +/* + * Test that platform_device_add_kunit() called twice with the same device name + * and id fails the second time and properly cleans up. + */ +static void platform_device_add_kunit_twice_fails_test(struct kunit *test) +{ + struct platform_device *pdev; + const char *name = kunit_devname; + const int id = -1; + + pdev = platform_device_alloc_kunit(test, name, id); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev)); + + pdev = platform_device_alloc_kunit(test, name, id); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + + KUNIT_EXPECT_NE(test, 0, platform_device_add_kunit(test, pdev)); +} + +/* + * Test suite for struct platform_device kunit APIs + */ +static struct kunit_case platform_device_kunit_test_cases[] = { + KUNIT_CASE(platform_device_alloc_kunit_test), + KUNIT_CASE(platform_device_add_kunit_test), + KUNIT_CASE(platform_device_add_kunit_twice_fails_test), + {} +}; + +static struct kunit_suite platform_device_kunit_suite = { + .name = "platform_device_kunit", + .test_cases = platform_device_kunit_test_cases, +}; + +struct kunit_platform_driver_test_context { + struct platform_driver pdrv; + const char *data; +}; + +static const char * const test_data = "test data"; + +static inline struct kunit_platform_driver_test_context * +to_test_context(struct platform_device *pdev) +{ + return container_of(to_platform_driver(pdev->dev.driver), + struct kunit_platform_driver_test_context, + pdrv); +} + +static int kunit_platform_driver_probe(struct platform_device *pdev) +{ + struct kunit_platform_driver_test_context *ctx; + + ctx = to_test_context(pdev); + ctx->data = test_data; + + return 0; +} + +/* Test that platform_driver_register_kunit() registers a driver that probes. */ +static void platform_driver_register_kunit_test(struct kunit *test) +{ + struct platform_device *pdev; + struct kunit_platform_driver_test_context *ctx; + + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + + pdev = platform_device_alloc_kunit(test, kunit_devname, -1); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + KUNIT_ASSERT_EQ(test, 0, platform_device_add_kunit(test, pdev)); + + ctx->pdrv.probe = kunit_platform_driver_probe; + ctx->pdrv.driver.name = kunit_devname; + ctx->pdrv.driver.owner = THIS_MODULE; + + KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); + KUNIT_EXPECT_STREQ(test, ctx->data, test_data); +} + +static struct kunit_case platform_driver_kunit_test_cases[] = { + KUNIT_CASE(platform_driver_register_kunit_test), + {} +}; + +/* + * Test suite for struct platform_driver kunit APIs + */ +static struct kunit_suite platform_driver_kunit_suite = { + .name = "platform_driver_kunit", + .test_cases = platform_driver_kunit_test_cases, +}; + +kunit_test_suites( + &platform_device_kunit_suite, + &platform_driver_kunit_suite, +); + +MODULE_LICENSE("GPL"); diff --git a/drivers/base/test/platform_kunit.c b/drivers/base/test/platform_kunit.c new file mode 100644 index 000000000000..54af6db2a6d8 --- /dev/null +++ b/drivers/base/test/platform_kunit.c @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Test managed platform driver + */ + +#include <linux/device/driver.h> +#include <linux/platform_device.h> + +#include <kunit/platform_device.h> +#include <kunit/resource.h> + +/** + * platform_device_alloc_kunit() - Allocate a KUnit test managed platform device + * @test: test context + * @name: device name of platform device to alloc + * @id: identifier of platform device to alloc. + * + * Allocate a test managed platform device. The device is put when the test completes. + * + * Return: Allocated platform device on success, NULL on failure. + */ +struct platform_device * +platform_device_alloc_kunit(struct kunit *test, const char *name, int id) +{ + struct platform_device *pdev; + + pdev = platform_device_alloc(name, id); + if (!pdev) + return NULL; + + if (kunit_add_action_or_reset(test, (kunit_action_t *)&platform_device_put, pdev)) + return NULL; + + return pdev; +} +EXPORT_SYMBOL_GPL(platform_device_alloc_kunit); + +static void platform_device_add_kunit_exit(struct kunit_resource *res) +{ + struct platform_device *pdev = res->data; + + platform_device_unregister(pdev); +} + +static bool +platform_device_alloc_kunit_match(struct kunit *test, + struct kunit_resource *res, void *match_data) +{ + struct platform_device *pdev = match_data; + + return res->data == pdev; +} + +/** + * platform_device_add_kunit() - Register a KUnit test managed platform device + * @test: test context + * @pdev: platform device to add + * + * Register a test managed platform device. The device is unregistered when the + * test completes. + * + * Return: 0 on success, negative errno on failure. + */ +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev) +{ + struct kunit_resource *res; + int ret; + + ret = platform_device_add(pdev); + if (ret) + return ret; + + res = kunit_find_resource(test, platform_device_alloc_kunit_match, pdev); + if (res) { + /* + * Transfer the reference count of the platform device if it was + * allocated with platform_device_alloc_kunit(). In that case, + * calling platform_device_put() leads to reference count + * underflow because platform_device_unregister() does it for + * us and we call platform_device_unregister() from + * platform_device_add_kunit_exit(). + * + * Usually callers transfer the refcount from + * platform_device_alloc() to platform_device_add() and simply + * call platform_device_unregister() when done, but with kunit + * we have to keep this straight by redirecting the free + * routine for the resource. + */ + res->free = platform_device_add_kunit_exit; + kunit_put_resource(res); + } else if (kunit_add_action_or_reset(test, + (kunit_action_t *)&platform_device_unregister, + pdev)) { + return -ENOMEM; + } + + return 0; +} +EXPORT_SYMBOL_GPL(platform_device_add_kunit); + +/** + * platform_driver_register_kunit() - Register a KUnit test managed platform driver + * @test: test context + * @drv: platform driver to register + * + * Register a test managed platform driver. This allows callers to embed the + * @drv in a container structure and use container_of() in the probe function + * to pass information to KUnit tests. It can be assumed that the driver has + * probed when this function returns. + * + * Example + * + * .. code-block:: c + * + * struct kunit_test_context { + * struct platform_driver pdrv; + * const char *data; + * }; + * + * static inline struct kunit_test_context * + * to_test_context(struct platform_device *pdev) + * { + * return container_of(to_platform_driver(pdev->dev.driver), + * struct kunit_test_context, + * pdrv); + * } + * + * static int kunit_platform_driver_probe(struct platform_device *pdev) + * { + * struct kunit_test_context *ctx; + * + * ctx = to_test_context(pdev); + * ctx->data = "test data"; + * + * return 0; + * } + * + * static void kunit_platform_driver_test(struct kunit *test) + * { + * struct kunit_test_context *ctx; + * + * ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); + * KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + * + * ctx->pdrv.probe = kunit_platform_driver_probe; + * ctx->pdrv.driver.name = "kunit-platform"; + * ctx->pdrv.driver.owner = THIS_MODULE; + * + * KUNIT_EXPECT_EQ(test, 0, platform_driver_register_kunit(test, &ctx->pdrv)); + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); + * } + * + * Return: 0 on success, negative errno on failure. + */ +int platform_driver_register_kunit(struct kunit *test, + struct platform_driver *drv) +{ + int ret; + + ret = platform_driver_register(drv); + if (ret) + return ret; + + /* + * Wait for the driver to probe (or at least flush out of the deferred + * workqueue) + */ + wait_for_device_probe(); + + return kunit_add_action_or_reset(test, + (kunit_action_t *)&platform_driver_unregister, + drv); +} +EXPORT_SYMBOL_GPL(platform_driver_register_kunit); diff --git a/include/kunit/platform_device.h b/include/kunit/platform_device.h new file mode 100644 index 000000000000..28d28abf15a4 --- /dev/null +++ b/include/kunit/platform_device.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _KUNIT_PLATFORM_DRIVER_H +#define _KUNIT_PLATFORM_DRIVER_H + +struct kunit; +struct platform_device; +struct platform_driver; + +struct platform_device * +platform_device_alloc_kunit(struct kunit *test, const char *name, int id); +int platform_device_add_kunit(struct kunit *test, struct platform_device *pdev); + +int platform_driver_register_kunit(struct kunit *test, struct platform_driver *drv); + +#endif
Introduce KUnit resource wrappers around platform_driver_register(), platform_device_alloc(), and platform_device_add() so that test authors can register platform drivers/devices from their tests and have the drivers/devices automatically be unregistered when the test is done. This makes test setup code simpler when a platform driver or platform device is needed. Add a few test cases at the same time to make sure the APIs work as intended. Cc: Brendan Higgins <brendan.higgins@linux.dev> Cc: David Gow <davidgow@google.com> Cc: Rae Moar <rmoar@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- Documentation/dev-tools/kunit/api/index.rst | 5 + .../dev-tools/kunit/api/platformdevice.rst | 10 + drivers/base/test/Makefile | 3 + drivers/base/test/platform_kunit-test.c | 140 ++++++++++++++ drivers/base/test/platform_kunit.c | 174 ++++++++++++++++++ include/kunit/platform_device.h | 15 ++ 6 files changed, 347 insertions(+) create mode 100644 Documentation/dev-tools/kunit/api/platformdevice.rst create mode 100644 drivers/base/test/platform_kunit-test.c create mode 100644 drivers/base/test/platform_kunit.c create mode 100644 include/kunit/platform_device.h