Re: [PATCH v2 2/3] drivers: base: Add basic devm tests for platform devices

[David Gow]

On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Platform devices show some inconsistencies with how devm resources are
> released when the device has been probed and when it hasn't. Let's add a
> few tests to exercise thos paths and odd cases.

Nit: "these".

Also, it'd be nice to call out the case that fails explicitly in the
commit message here, so it's obvious what the "inconsistency" is.

>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---

This looks good to me. I think this is, if anything, even more
obviously important than the root device issues, so we definitely need
to fix or document it.

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>


>  drivers/base/test/Makefile               |   1 +
>  drivers/base/test/platform-device-test.c | 222 +++++++++++++++++++++++++++++++
>  2 files changed, 223 insertions(+)
>
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index d589ca3fa8fc..e321dfc7e922 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
>
>  obj-$(CONFIG_DM_KUNIT_TEST)    += root-device-test.o
> +obj-$(CONFIG_DM_KUNIT_TEST)    += platform-device-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-device-test.c b/drivers/base/test/platform-device-test.c
> new file mode 100644
> index 000000000000..b6ebf1dcdffb
> --- /dev/null
> +++ b/drivers/base/test/platform-device-test.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/resource.h>
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#define DEVICE_NAME "test"
> +
> +struct test_priv {
> +       bool probe_done;
> +       bool release_done;
> +       wait_queue_head_t probe_wq;
> +       wait_queue_head_t release_wq;
> +       struct device *dev;
> +};
> +
> +static int platform_device_devm_init(struct kunit *test)
> +{
> +       struct test_priv *priv;
> +
> +       priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +       init_waitqueue_head(&priv->probe_wq);
> +       init_waitqueue_head(&priv->release_wq);
> +
> +       test->priv = priv;
> +
> +       return 0;
> +}
> +
> +static void devm_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +static void devm_put_device_action(void *ptr)
> +{
> +       struct test_priv *priv = ptr;
> +
> +       put_device(priv->dev);
> +       priv->release_done = true;
> +       wake_up_interruptible(&priv->release_wq);
> +}
> +
> +#define RELEASE_TIMEOUT_MS     100
> +
> +/*
> + * Tests that a platform bus, non-probed device will run its
> + * device-managed actions when unregistered.
> + */
> +static void platform_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       priv->dev = &pdev->dev;
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +/*
> + * Tests that a platform bus, non-probed device will run its
> + * device-managed actions when unregistered, even if someone still holds
> + * a reference to it.
> + */
> +static void platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       kunit_skip(test, "This needs to be fixed in the core.");
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       priv->dev = &pdev->dev;
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +static int fake_probe(struct platform_device *pdev)
> +{
> +       struct test_priv *priv = platform_get_drvdata(pdev);
> +
> +       priv->probe_done = true;
> +       wake_up_interruptible(&priv->probe_wq);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver fake_driver = {
> +       .probe  = fake_probe,
> +       .driver = {
> +               .name = DEVICE_NAME,
> +       },
> +};
> +
> +/*
> + * Tests that a platform bus, probed device will run its device-managed
> + * actions when unregistered.
> + */
> +static void probed_platform_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       ret = platform_driver_register(&fake_driver);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       priv->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_ASSERT_GT(test, ret, 0);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +
> +       platform_driver_unregister(&fake_driver);
> +}
> +
> +/*
> + * Tests that a platform bus, probed device will run its device-managed
> + * actions when unregistered, even if someone still holds a reference to
> + * it.
> + */
> +static void probed_platform_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct platform_device *pdev;
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       ret = platform_driver_register(&fake_driver);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       pdev = platform_device_alloc(DEVICE_NAME, PLATFORM_DEVID_NONE);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> +
> +       priv->dev = &pdev->dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       ret = platform_device_add(pdev);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = wait_event_interruptible_timeout(priv->probe_wq, priv->probe_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_ASSERT_GT(test, ret, 0);
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       platform_device_unregister(pdev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +
> +       platform_driver_unregister(&fake_driver);
> +}
> +
> +static struct kunit_case platform_device_devm_tests[] = {
> +       KUNIT_CASE(platform_device_devm_register_unregister_test),
> +       KUNIT_CASE(platform_device_devm_register_get_unregister_with_devm_test),
> +       KUNIT_CASE(probed_platform_device_devm_register_unregister_test),
> +       KUNIT_CASE(probed_platform_device_devm_register_get_unregister_with_devm_test),
> +       {}
> +};
> +
> +static struct kunit_suite platform_device_devm_test_suite = {
> +       .name = "platform-device-devm",
> +       .init = platform_device_devm_init,
> +       .test_cases = platform_device_devm_tests,
> +};
> +
> +kunit_test_suite(platform_device_devm_test_suite);
>
> --
> 2.40.0
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature