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

On Wed, 28 Jun 2023 at 17:49, Maxime Ripard mripard@kernel.org 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@cerno.tech

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@google.com

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