Re: [PATCH 3/8] kunit: Add test managed platform_device/driver APIs

[David Gow <davidgow@google.com>]

[-- Attachment #1.1: Type: text/plain, Size: 15205 bytes --]

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, &params);
> +       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
>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[-- Attachment #2: Type: text/plain, Size: 152 bytes --]

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um