Message-ID: <431171223433496db0a85072be5c83ba.sboyd@kernel.org>
Date: Thu, 02 May 2024 18:03:57 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: David Gow <davidgow@...gle.com>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, patches@...ts.linux.dev, kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org, devicetree@...r.kernel.org, Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rafael J . Wysocki <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, Daniel Latypov <dlatypov@...gle.com>, Christian Marangi <ansuelsmth@...il.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Maxime Ripard <maxime@...no.tech>
Subject: Re: [PATCH v4 05/10] platform: Add test managed platform_device/driver APIs

Quoting David Gow (2024-05-01 00:55:46)
> On Tue, 23 Apr 2024 at 07:24, Stephen Boyd <sboyd@...nel.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.

Powered by blists - more mailing lists