| Message ID | 20230302013822.1808711-4-sboyd@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | clk: Add kunit tests for fixed rate and parent data | expand |
On Thu, 2 Mar 2023 at 09:38, 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > --- > > Should this be moved to drivers/base/ and called platform_kunit.c? > The include/kunit/platform_driver.h could also be > kunit/platform_device.h to match linux/platform_device.h if that is more > familiar. DRM has a similar thing already (albeit with a root_device, which is more common with KUnit tests generally): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h But that's reasonably drm-specific, so it makes sense that it lives with DRM stuff. platform_device is a bit more generic. I'd probably personally err on the side of having these in drivers/base/, as I think we'll ultimately need similar things for a lot of different devices, and I'd rather not end up with things like USB device helpers living in the lib/kunit directory alongside the "core" KUnit code. But I could be persuaded otherwise. > > And I'm not super certain about allocating a driver structure and > embedding it in a wrapper struct. Maybe the code should just use > kunit_get_current_test() instead? I think there are enough cases througout the kernel where device/driver structs are needed that having this makes sense. Combined with the fact that, while kunit_get_current_test() can be used even when KUnit is not loaded, actually doing anything with the resulting struct kunit pointer will probably require (at least for the moment) KUnit functions to be reachable, so would break if CONFIG_KUNIT=m. So, unless you actually find kunit_get_current_test() and friends to be easier to work with, I'd probably stick with this. > > include/kunit/platform_driver.h | 15 +++ > lib/kunit/Makefile | 6 + > lib/kunit/platform_driver-test.c | 107 ++++++++++++++++ > lib/kunit/platform_driver.c | 207 +++++++++++++++++++++++++++++++ > 4 files changed, 335 insertions(+) > create mode 100644 include/kunit/platform_driver.h > create mode 100644 lib/kunit/platform_driver-test.c > create mode 100644 lib/kunit/platform_driver.c > > diff --git a/include/kunit/platform_driver.h b/include/kunit/platform_driver.h > new file mode 100644 > index 000000000000..dc211ff8f893 > --- /dev/null > +++ b/include/kunit/platform_driver.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 * > +kunit_platform_device_alloc(struct kunit *test, const char *name, int id); > +int kunit_platform_device_add(struct kunit *test, struct platform_device *pdev); > + > +int kunit_platform_driver_register(struct kunit *test, struct platform_driver *drv); > + > +#endif > diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile > index 29aff6562b42..5964d8231ff5 100644 > --- a/lib/kunit/Makefile > +++ b/lib/kunit/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_KUNIT) += kunit.o > > +# Core KUnit code > kunit-objs += test.o \ > resource.o \ > string-stream.o \ > @@ -11,7 +12,12 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y) > kunit-objs += debugfs.o > endif > > +# KUnit helpers > +kunit-objs += platform_driver.o > + > +# KUnit tests > obj-$(CONFIG_KUNIT_TEST) += kunit-test.o > +obj-$(CONFIG_KUNIT_TEST) += platform_driver-test.o > > # string-stream-test compiles built-in only. > ifeq ($(CONFIG_KUNIT_TEST),y) > diff --git a/lib/kunit/platform_driver-test.c b/lib/kunit/platform_driver-test.c > new file mode 100644 > index 000000000000..c926fe01b40a > --- /dev/null > +++ b/lib/kunit/platform_driver-test.c > @@ -0,0 +1,107 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KUnit test for platform driver infrastructure. > + */ > + > +#include <linux/platform_device.h> > + > +#include <kunit/platform_driver.h> > +#include <kunit/test.h> > + > +/* > + * Test that kunit_platform_device_alloc() creates a platform device. > + */ > +static void kunit_platform_device_alloc_test(struct kunit *test) > +{ > + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > + kunit_platform_device_alloc(test, "kunit-platform", 1)); > +} > + > +/* > + * Test that kunit_platform_device_add() registers a platform device on the > + * platform bus with the proper name and id. > + */ > +static void kunit_platform_device_add_test(struct kunit *test) > +{ > + struct platform_device *pdev; > + const char *name = "kunit-platform"; > + const int id = -1; > + > + pdev = kunit_platform_device_alloc(test, name, id); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + > + KUNIT_EXPECT_EQ(test, 0, kunit_platform_device_add(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); > +} > + > +static struct kunit_case kunit_platform_device_test_cases[] = { > + KUNIT_CASE(kunit_platform_device_alloc_test), > + KUNIT_CASE(kunit_platform_device_add_test), > + {} > +}; > + > +static struct kunit_suite kunit_platform_device_suite = { > + .name = "kunit_platform_device", > + .test_cases = kunit_platform_device_test_cases, > +}; > + > +struct kunit_platform_driver_test_context { > + struct platform_driver pdrv; > + const char *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 kunit_platform_driver_register() registers a driver that probes. */ > +static void kunit_platform_driver_register_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 = kunit_platform_device_alloc(test, "kunit-platform", -1); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); > + KUNIT_ASSERT_EQ(test, 0, kunit_platform_device_add(test, pdev)); > + > + ctx->pdrv.probe = kunit_platform_driver_probe; > + ctx->pdrv.driver.name = "kunit-platform"; > + ctx->pdrv.driver.owner = THIS_MODULE; > + > + KUNIT_EXPECT_EQ(test, 0, kunit_platform_driver_register(test, &ctx->pdrv)); > + KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > +} > + > +static struct kunit_case kunit_platform_driver_test_cases[] = { > + KUNIT_CASE(kunit_platform_driver_register_test), > + {} > +}; > + > +static struct kunit_suite kunit_platform_driver_suite = { > + .name = "kunit_platform_driver", > + .test_cases = kunit_platform_driver_test_cases, > +}; > + > +kunit_test_suites(&kunit_platform_device_suite, > + &kunit_platform_driver_suite); > + > +MODULE_LICENSE("GPL"); > diff --git a/lib/kunit/platform_driver.c b/lib/kunit/platform_driver.c > new file mode 100644 > index 000000000000..11d155114936 > --- /dev/null > +++ b/lib/kunit/platform_driver.c > @@ -0,0 +1,207 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Test managed platform driver > + */ > + > +#include <linux/device/driver.h> > +#include <linux/platform_device.h> > + > +#include <kunit/resource.h> > + > +struct kunit_platform_device_alloc_params { > + const char *name; > + int id; > +}; FYI: It's my plan to eventually get rid of (or at least de-emphasize) the whole 'init' function aspect of KUnit resources so we don't need all of these extra structs and the like. It probably won't make it in for 6.4, but we'll see... > + > +static int kunit_platform_device_alloc_init(struct kunit_resource *res, void *context) > +{ > + struct kunit_platform_device_alloc_params *params = context; > + struct platform_device *pdev; > + > + pdev = platform_device_alloc(params->name, params->id); > + if (!pdev) > + return -ENOMEM; > + > + res->data = pdev; > + > + return 0; > +} > + > +static void kunit_platform_device_alloc_exit(struct kunit_resource *res) > +{ > + struct platform_device *pdev = res->data; > + > + platform_device_put(pdev); > +} > + > +/** > + * kunit_platform_device_alloc() - Allocate a KUnit test managed platform device > + * @test: test context > + * @dev: platform device to alloc > + * > + * Register a test managed platform device. The device is put when the test completes. > + * > + * Returns: 0 on success, negative errno on failure. > + */ > +struct platform_device * > +kunit_platform_device_alloc(struct kunit *test, const char *name, int id) > +{ > + struct platform_device *pdev; > + struct kunit_platform_device_alloc_params params = { > + .name = name, > + .id = id, > + }; > + > + pdev = kunit_alloc_resource(test, > + kunit_platform_device_alloc_init, > + kunit_platform_device_alloc_exit, > + GFP_KERNEL, ¶ms); > + if (!pdev) > + return ERR_PTR(-ENOMEM); > + > + return pdev; > +} > +EXPORT_SYMBOL_GPL(kunit_platform_device_alloc); > + > +static int kunit_platform_device_add_init(struct kunit_resource *res, void *context) > +{ > + struct platform_device *pdev = context; > + int ret; > + > + ret = platform_device_add(pdev); > + if (ret) { > + platform_device_put(pdev); > + return ret; > + } > + res->data = pdev; > + > + return 0; > +} > + > +static void kunit_platform_device_add_exit(struct kunit_resource *res) > +{ > + struct platform_device *pdev = res->data; > + > + platform_device_unregister(pdev); > +} > + > +/** > + * kunit_platform_device_add() - Register a KUnit test managed platform device > + * @test: test context > + * @dev: platform device to add > + * > + * Register a test managed platform device. The device is unregistered when the > + * test completes. > + * > + * Returns: 0 on success, negative errno on failure. > + */ > +int kunit_platform_device_add(struct kunit *test, struct platform_device *pdev) > +{ > + struct platform_device *res; > + > + res = kunit_alloc_resource(test, > + kunit_platform_device_add_init, > + kunit_platform_device_add_exit, > + GFP_KERNEL, pdev); > + if (!res) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(kunit_platform_device_add); > + > +static int kunit_platform_driver_register_init(struct kunit_resource *res, void *context) > +{ > + struct platform_driver *drv = context; > + int ret; > + > + ret = platform_driver_register(drv); > + if (ret) > + return ret; > + res->data = drv; > + > + /* > + * Wait for the driver to probe (or at least flush out of the deferred > + * workqueue) > + */ > + wait_for_device_probe(); > + > + return 0; > +} > + > +static void kunit_platform_driver_register_exit(struct kunit_resource *res) > +{ > + struct platform_driver *drv = res->data; > + > + platform_driver_unregister(drv); > +} > + > +/** > + * kunit_platform_driver_register() - 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, kunit_platform_driver_register(test, &ctx->pdrv)); > + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); > + * } > + * > + * Returns: 0 on success, negative errno on failure. > + */ > +int kunit_platform_driver_register(struct kunit *test, > + struct platform_driver *drv) > +{ > + struct platform_driver *res; > + > + res = kunit_alloc_resource(test, > + kunit_platform_driver_register_init, > + kunit_platform_driver_register_exit, > + GFP_KERNEL, drv); > + if (!res) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(kunit_platform_driver_register); > -- > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/ > https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git >
On Fri, Mar 03, 2023 at 03:15:31PM +0800, David Gow wrote: > On Thu, 2 Mar 2023 at 09:38, 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > --- > > > > Should this be moved to drivers/base/ and called platform_kunit.c? > > The include/kunit/platform_driver.h could also be > > kunit/platform_device.h to match linux/platform_device.h if that is more > > familiar. > > DRM has a similar thing already (albeit with a root_device, which is > more common with KUnit tests generally): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h > > But that's reasonably drm-specific, so it makes sense that it lives > with DRM stuff. platform_device is a bit more generic. I'd be very happy to get something from the core to address the same thing. I think the main thing we needed that isn't covered by this patch is we wanted the device to be bound to its driver, so with probe being called before calling the test (see 57a84a97bbda). Maxime
Quoting David Gow (2023-03-02 23:15:31) > On Thu, 2 Mar 2023 at 09:38, 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > --- > > > > Should this be moved to drivers/base/ and called platform_kunit.c? > > The include/kunit/platform_driver.h could also be > > kunit/platform_device.h to match linux/platform_device.h if that is more > > familiar. > > DRM has a similar thing already (albeit with a root_device, which is > more common with KUnit tests generally): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h > > But that's reasonably drm-specific, so it makes sense that it lives > with DRM stuff. platform_device is a bit more generic. > > I'd probably personally err on the side of having these in > drivers/base/, as I think we'll ultimately need similar things for a > lot of different devices, and I'd rather not end up with things like > USB device helpers living in the lib/kunit directory alongside the > "core" KUnit code. But I could be persuaded otherwise. Ok no problem. I'll move it. > > > > > And I'm not super certain about allocating a driver structure and > > embedding it in a wrapper struct. Maybe the code should just use > > kunit_get_current_test() instead? > > I think there are enough cases througout the kernel where > device/driver structs are needed that having this makes sense. > Combined with the fact that, while kunit_get_current_test() can be > used even when KUnit is not loaded, actually doing anything with the > resulting struct kunit pointer will probably require (at least for the > moment) KUnit functions to be reachable, so would break if > CONFIG_KUNIT=m. Wouldn't it still work in that case? The unit tests would be modular as well because they depend on CONFIG_KUNIT. > > So, unless you actually find kunit_get_current_test() and friends to > be easier to work with, I'd probably stick with this. > Alright thanks. > > diff --git a/lib/kunit/platform_driver.c b/lib/kunit/platform_driver.c > > new file mode 100644 > > index 000000000000..11d155114936 > > --- /dev/null > > +++ b/lib/kunit/platform_driver.c > > @@ -0,0 +1,207 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Test managed platform driver > > + */ > > + > > +#include <linux/device/driver.h> > > +#include <linux/platform_device.h> > > + > > +#include <kunit/resource.h> > > + > > +struct kunit_platform_device_alloc_params { > > + const char *name; > > + int id; > > +}; > > FYI: It's my plan to eventually get rid of (or at least de-emphasize) > the whole 'init' function aspect of KUnit resources so we don't need > all of these extra structs and the like. It probably won't make it in > for 6.4, but we'll see... Will we be able to get the error values out of the init function? It's annoying that the error values can't be returned as error pointers to kunit_alloc_resource(). I end up skipping init, and doing it directly before or after calling the kunit_alloc_resource() function. I'll try to avoid init functions in the allocations.
Quoting Maxime Ripard (2023-03-03 06:35:28) > On Fri, Mar 03, 2023 at 03:15:31PM +0800, David Gow wrote: > > > > DRM has a similar thing already (albeit with a root_device, which is > > more common with KUnit tests generally): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h > > > > But that's reasonably drm-specific, so it makes sense that it lives > > with DRM stuff. platform_device is a bit more generic. > > I'd be very happy to get something from the core to address the same > thing. > > I think the main thing we needed that isn't covered by this patch is we > wanted the device to be bound to its driver, so with probe being called > before calling the test (see 57a84a97bbda). > Can you clarify? This patch makes a poor attempt at waiting for the platform driver to bind, but in reality it may not be bound by the time the driver register function returns.
On Fri, 10 Mar 2023 at 07:25, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting David Gow (2023-03-02 23:15:31) > > On Thu, 2 Mar 2023 at 09:38, 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > > Signed-off-by: Stephen Boyd <sboyd@kernel.org> > > > --- > > > > > > Should this be moved to drivers/base/ and called platform_kunit.c? > > > The include/kunit/platform_driver.h could also be > > > kunit/platform_device.h to match linux/platform_device.h if that is more > > > familiar. > > > > DRM has a similar thing already (albeit with a root_device, which is > > more common with KUnit tests generally): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h > > > > But that's reasonably drm-specific, so it makes sense that it lives > > with DRM stuff. platform_device is a bit more generic. > > > > I'd probably personally err on the side of having these in > > drivers/base/, as I think we'll ultimately need similar things for a > > lot of different devices, and I'd rather not end up with things like > > USB device helpers living in the lib/kunit directory alongside the > > "core" KUnit code. But I could be persuaded otherwise. > > Ok no problem. I'll move it. > > > > > > > > > And I'm not super certain about allocating a driver structure and > > > embedding it in a wrapper struct. Maybe the code should just use > > > kunit_get_current_test() instead? > > > > I think there are enough cases througout the kernel where > > device/driver structs are needed that having this makes sense. > > Combined with the fact that, while kunit_get_current_test() can be > > used even when KUnit is not loaded, actually doing anything with the > > resulting struct kunit pointer will probably require (at least for the > > moment) KUnit functions to be reachable, so would break if > > CONFIG_KUNIT=m. > > Wouldn't it still work in that case? The unit tests would be modular as > well because they depend on CONFIG_KUNIT. > Yeah, the only case where this starts to get hairy is if the tests end up in the same module as the thing being tested (which sometimes happens to avoid having to export a bunch of symbols: see, e.g. thunderbolt and amdgpu), and then someone wants to build production kernels with CONFIG_KUNIT=m (alas, Red Hat and Android). So that's the only real place where you might need to avoid the non-'hook' KUnit functions, but those drivers are pretty few and far between, and most of the really useful functionality should be moving to 'hooks' which will be patched out cleanly at runtime. > > > > So, unless you actually find kunit_get_current_test() and friends to > > be easier to work with, I'd probably stick with this. > > > > Alright thanks. > > > > diff --git a/lib/kunit/platform_driver.c b/lib/kunit/platform_driver.c > > > new file mode 100644 > > > index 000000000000..11d155114936 > > > --- /dev/null > > > +++ b/lib/kunit/platform_driver.c > > > @@ -0,0 +1,207 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Test managed platform driver > > > + */ > > > + > > > +#include <linux/device/driver.h> > > > +#include <linux/platform_device.h> > > > + > > > +#include <kunit/resource.h> > > > + > > > +struct kunit_platform_device_alloc_params { > > > + const char *name; > > > + int id; > > > +}; > > > > FYI: It's my plan to eventually get rid of (or at least de-emphasize) > > the whole 'init' function aspect of KUnit resources so we don't need > > all of these extra structs and the like. It probably won't make it in > > for 6.4, but we'll see... > > Will we be able to get the error values out of the init function? It's > annoying that the error values can't be returned as error pointers to > kunit_alloc_resource(). I end up skipping init, and doing it directly > before or after calling the kunit_alloc_resource() function. I'll try to > avoid init functions in the allocations. Yeah, that's largely why the plan is to get rid of them: it just made passing things around an enormous pain. Just doing your own initialisation before adding it as a resource is usually the right thing to do. There's also going to be a simpler kunit_defer() wrapper around it, which would just allow you to schedule a cleanup function to be called (without the need to keep kunit_resource pointers around, etc), for the cases where you don't need to look up resources elsewhere. But just doing your own thing and calling kunit_alloc_resource() is probably best for now, and should map well onto whatever this ends up evolving into. Cheers, -- David
Hi Stephen, On Thu, Mar 09, 2023 at 03:31:15PM -0800, Stephen Boyd wrote: > Quoting Maxime Ripard (2023-03-03 06:35:28) > > On Fri, Mar 03, 2023 at 03:15:31PM +0800, David Gow wrote: > > > > > > DRM has a similar thing already (albeit with a root_device, which is > > > more common with KUnit tests generally): > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_kunit_helpers.h > > > > > > But that's reasonably drm-specific, so it makes sense that it lives > > > with DRM stuff. platform_device is a bit more generic. > > > > I'd be very happy to get something from the core to address the same > > thing. > > > > I think the main thing we needed that isn't covered by this patch is we > > wanted the device to be bound to its driver, so with probe being called > > before calling the test (see 57a84a97bbda). > > Can you clarify? This patch makes a poor attempt at waiting for the > platform driver to bind, but in reality it may not be bound by the time > the driver register function returns. The issue was that devm will only clean up the resources if the device was bound to a driver so we were exhausting resources when running dozens of test in a sequence. The way I solved it for vc4 was to create a dumb platform driver with a waitqueue, and wait for probe to be called. I think we could make it more generic by allowing a pointer to a probe function and calling it into our own probe implementation. What do you think? Maxime
diff --git a/include/kunit/platform_driver.h b/include/kunit/platform_driver.h new file mode 100644 index 000000000000..dc211ff8f893 --- /dev/null +++ b/include/kunit/platform_driver.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 * +kunit_platform_device_alloc(struct kunit *test, const char *name, int id); +int kunit_platform_device_add(struct kunit *test, struct platform_device *pdev); + +int kunit_platform_driver_register(struct kunit *test, struct platform_driver *drv); + +#endif diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 29aff6562b42..5964d8231ff5 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_KUNIT) += kunit.o +# Core KUnit code kunit-objs += test.o \ resource.o \ string-stream.o \ @@ -11,7 +12,12 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o endif +# KUnit helpers +kunit-objs += platform_driver.o + +# KUnit tests obj-$(CONFIG_KUNIT_TEST) += kunit-test.o +obj-$(CONFIG_KUNIT_TEST) += platform_driver-test.o # string-stream-test compiles built-in only. ifeq ($(CONFIG_KUNIT_TEST),y) diff --git a/lib/kunit/platform_driver-test.c b/lib/kunit/platform_driver-test.c new file mode 100644 index 000000000000..c926fe01b40a --- /dev/null +++ b/lib/kunit/platform_driver-test.c @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for platform driver infrastructure. + */ + +#include <linux/platform_device.h> + +#include <kunit/platform_driver.h> +#include <kunit/test.h> + +/* + * Test that kunit_platform_device_alloc() creates a platform device. + */ +static void kunit_platform_device_alloc_test(struct kunit *test) +{ + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, + kunit_platform_device_alloc(test, "kunit-platform", 1)); +} + +/* + * Test that kunit_platform_device_add() registers a platform device on the + * platform bus with the proper name and id. + */ +static void kunit_platform_device_add_test(struct kunit *test) +{ + struct platform_device *pdev; + const char *name = "kunit-platform"; + const int id = -1; + + pdev = kunit_platform_device_alloc(test, name, id); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + + KUNIT_EXPECT_EQ(test, 0, kunit_platform_device_add(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); +} + +static struct kunit_case kunit_platform_device_test_cases[] = { + KUNIT_CASE(kunit_platform_device_alloc_test), + KUNIT_CASE(kunit_platform_device_add_test), + {} +}; + +static struct kunit_suite kunit_platform_device_suite = { + .name = "kunit_platform_device", + .test_cases = kunit_platform_device_test_cases, +}; + +struct kunit_platform_driver_test_context { + struct platform_driver pdrv; + const char *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 kunit_platform_driver_register() registers a driver that probes. */ +static void kunit_platform_driver_register_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 = kunit_platform_device_alloc(test, "kunit-platform", -1); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev); + KUNIT_ASSERT_EQ(test, 0, kunit_platform_device_add(test, pdev)); + + ctx->pdrv.probe = kunit_platform_driver_probe; + ctx->pdrv.driver.name = "kunit-platform"; + ctx->pdrv.driver.owner = THIS_MODULE; + + KUNIT_EXPECT_EQ(test, 0, kunit_platform_driver_register(test, &ctx->pdrv)); + KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); +} + +static struct kunit_case kunit_platform_driver_test_cases[] = { + KUNIT_CASE(kunit_platform_driver_register_test), + {} +}; + +static struct kunit_suite kunit_platform_driver_suite = { + .name = "kunit_platform_driver", + .test_cases = kunit_platform_driver_test_cases, +}; + +kunit_test_suites(&kunit_platform_device_suite, + &kunit_platform_driver_suite); + +MODULE_LICENSE("GPL"); diff --git a/lib/kunit/platform_driver.c b/lib/kunit/platform_driver.c new file mode 100644 index 000000000000..11d155114936 --- /dev/null +++ b/lib/kunit/platform_driver.c @@ -0,0 +1,207 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Test managed platform driver + */ + +#include <linux/device/driver.h> +#include <linux/platform_device.h> + +#include <kunit/resource.h> + +struct kunit_platform_device_alloc_params { + const char *name; + int id; +}; + +static int kunit_platform_device_alloc_init(struct kunit_resource *res, void *context) +{ + struct kunit_platform_device_alloc_params *params = context; + struct platform_device *pdev; + + pdev = platform_device_alloc(params->name, params->id); + if (!pdev) + return -ENOMEM; + + res->data = pdev; + + return 0; +} + +static void kunit_platform_device_alloc_exit(struct kunit_resource *res) +{ + struct platform_device *pdev = res->data; + + platform_device_put(pdev); +} + +/** + * kunit_platform_device_alloc() - Allocate a KUnit test managed platform device + * @test: test context + * @dev: platform device to alloc + * + * Register a test managed platform device. The device is put when the test completes. + * + * Returns: 0 on success, negative errno on failure. + */ +struct platform_device * +kunit_platform_device_alloc(struct kunit *test, const char *name, int id) +{ + struct platform_device *pdev; + struct kunit_platform_device_alloc_params params = { + .name = name, + .id = id, + }; + + pdev = kunit_alloc_resource(test, + kunit_platform_device_alloc_init, + kunit_platform_device_alloc_exit, + GFP_KERNEL, ¶ms); + if (!pdev) + return ERR_PTR(-ENOMEM); + + return pdev; +} +EXPORT_SYMBOL_GPL(kunit_platform_device_alloc); + +static int kunit_platform_device_add_init(struct kunit_resource *res, void *context) +{ + struct platform_device *pdev = context; + int ret; + + ret = platform_device_add(pdev); + if (ret) { + platform_device_put(pdev); + return ret; + } + res->data = pdev; + + return 0; +} + +static void kunit_platform_device_add_exit(struct kunit_resource *res) +{ + struct platform_device *pdev = res->data; + + platform_device_unregister(pdev); +} + +/** + * kunit_platform_device_add() - Register a KUnit test managed platform device + * @test: test context + * @dev: platform device to add + * + * Register a test managed platform device. The device is unregistered when the + * test completes. + * + * Returns: 0 on success, negative errno on failure. + */ +int kunit_platform_device_add(struct kunit *test, struct platform_device *pdev) +{ + struct platform_device *res; + + res = kunit_alloc_resource(test, + kunit_platform_device_add_init, + kunit_platform_device_add_exit, + GFP_KERNEL, pdev); + if (!res) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_GPL(kunit_platform_device_add); + +static int kunit_platform_driver_register_init(struct kunit_resource *res, void *context) +{ + struct platform_driver *drv = context; + int ret; + + ret = platform_driver_register(drv); + if (ret) + return ret; + res->data = drv; + + /* + * Wait for the driver to probe (or at least flush out of the deferred + * workqueue) + */ + wait_for_device_probe(); + + return 0; +} + +static void kunit_platform_driver_register_exit(struct kunit_resource *res) +{ + struct platform_driver *drv = res->data; + + platform_driver_unregister(drv); +} + +/** + * kunit_platform_driver_register() - 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, kunit_platform_driver_register(test, &ctx->pdrv)); + * KUNIT_EXPECT_STREQ(test, ctx->data, "test data"); + * } + * + * Returns: 0 on success, negative errno on failure. + */ +int kunit_platform_driver_register(struct kunit *test, + struct platform_driver *drv) +{ + struct platform_driver *res; + + res = kunit_alloc_resource(test, + kunit_platform_driver_register_init, + kunit_platform_driver_register_exit, + GFP_KERNEL, drv); + if (!res) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_GPL(kunit_platform_driver_register);
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: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- Should this be moved to drivers/base/ and called platform_kunit.c? The include/kunit/platform_driver.h could also be kunit/platform_device.h to match linux/platform_device.h if that is more familiar. And I'm not super certain about allocating a driver structure and embedding it in a wrapper struct. Maybe the code should just use kunit_get_current_test() instead? include/kunit/platform_driver.h | 15 +++ lib/kunit/Makefile | 6 + lib/kunit/platform_driver-test.c | 107 ++++++++++++++++ lib/kunit/platform_driver.c | 207 +++++++++++++++++++++++++++++++ 4 files changed, 335 insertions(+) create mode 100644 include/kunit/platform_driver.h create mode 100644 lib/kunit/platform_driver-test.c create mode 100644 lib/kunit/platform_driver.c