diff mbox series

[v2,1/3] drivers: base: Add basic devm tests for root devices

Message ID 20230329-kunit-devm-inconsistencies-test-v2-1-19feb71e864b@kernel.org
State Accepted
Commit 06188bc80ccbec0dc2f9e451b53b2e48321acbd3
Headers show
Series [v2,1/3] drivers: base: Add basic devm tests for root devices | expand

Commit Message

Maxime Ripard June 28, 2023, 9:49 a.m. UTC
The root devices show some odd behaviours compared to regular "bus" devices
that have been probed through the usual mechanism, so let's create kunit
tests to exercise those paths and odd cases.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/base/test/.kunitconfig       |   2 +
 drivers/base/test/Kconfig            |   4 ++
 drivers/base/test/Makefile           |   2 +
 drivers/base/test/root-device-test.c | 110 +++++++++++++++++++++++++++++++++++
 4 files changed, 118 insertions(+)

Comments

David Gow July 19, 2023, 9:13 a.m. UTC | #1
On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
>
> The root devices show some odd behaviours compared to regular "bus" devices
> that have been probed through the usual mechanism, so let's create kunit
> tests to exercise those paths and odd cases.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: David Gow <davidgow@google.com>

There's definitely an argument that root devices are not supposed to
be like regular devices, and so devm_ managed resources aren't
supposed to work with them. Either way:
- It needs to be documented somewhere (and this test makes for good
documentation, IMO).
- It should be consistent: if devm_ isn't to be used with root
devices, it should fail everywhere, and if it is, it should work in
all the cases below.

So I'm all in favour of including this test and making root devices work.

That being said, I am planning to send out a patchset adding a struct
kunit_device for use in tests, which will be attached to a "kunit"
bus. I think the combination of "fix devm_ with root devices" and
"don't recommend root devices as a 'fake' device for testing" is
probably the longer-term solution everyone can agree upon?

Cheers,
-- David


>  drivers/base/test/.kunitconfig       |   2 +
>  drivers/base/test/Kconfig            |   4 ++
>  drivers/base/test/Makefile           |   2 +
>  drivers/base/test/root-device-test.c | 110 +++++++++++++++++++++++++++++++++++
>  4 files changed, 118 insertions(+)
>
> diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
> new file mode 100644
> index 000000000000..473923f0998b
> --- /dev/null
> +++ b/drivers/base/test/.kunitconfig
> @@ -0,0 +1,2 @@
> +CONFIG_KUNIT=y
> +CONFIG_DM_KUNIT_TEST=y
> diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
> index 610a1ba7a467..9d42051f8f8e 100644
> --- a/drivers/base/test/Kconfig
> +++ b/drivers/base/test/Kconfig
> @@ -9,6 +9,10 @@ config TEST_ASYNC_DRIVER_PROBE
>
>           If unsure say N.
>
> +config DM_KUNIT_TEST
> +       tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
> +       depends on KUNIT

Could we add "default KUNIT_ALL_TESTS" here? Or if there's a good
reason to not make this run by default, remove the "if
!KUNIT_ALL_TESTS" condition above.


> +
>  config DRIVER_PE_KUNIT_TEST
>         bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
>         depends on KUNIT=y
> diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
> index 7f76fee6f989..d589ca3fa8fc 100644
> --- a/drivers/base/test/Makefile
> +++ b/drivers/base/test/Makefile
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)  += test_async_driver_probe.o
>
> +obj-$(CONFIG_DM_KUNIT_TEST)    += root-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/root-device-test.c b/drivers/base/test/root-device-test.c
> new file mode 100644
> index 000000000000..9a3e6cccae13
> --- /dev/null
> +++ b/drivers/base/test/root-device-test.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2023 Maxime Ripard <mripard@kernel.org>
> +
> +#include <kunit/resource.h>
> +
> +#include <linux/device.h>
> +
> +#define DEVICE_NAME "test"
> +
> +struct test_priv {
> +       bool probe_done;
> +       bool release_done;
> +       wait_queue_head_t release_wq;
> +       struct device *dev;
> +};
> +
> +static int root_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->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);
> +}
> +
> +#define RELEASE_TIMEOUT_MS     100
> +
> +/*
> + * Tests that a bus-less, non-probed device will run its device-managed
> + * actions when unregistered.
> + */
> +static void root_device_devm_register_unregister_test(struct kunit *test)
> +{
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       priv->dev = root_device_register(DEVICE_NAME);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       root_device_unregister(priv->dev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +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);
> +}
> +
> +/*
> + * Tests that a bus-less, non-probed device will run its device-managed
> + * actions when unregistered, even if someone still holds a reference to
> + * it.
> + */
> +static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
> +{
> +       struct test_priv *priv = test->priv;
> +       int ret;
> +
> +       kunit_skip(test, "This needs to be fixed in the core.");
> +
> +       priv->dev = root_device_register(DEVICE_NAME);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
> +
> +       get_device(priv->dev);
> +
> +       ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       root_device_unregister(priv->dev);
> +
> +       ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
> +                                              msecs_to_jiffies(RELEASE_TIMEOUT_MS));
> +       KUNIT_EXPECT_GT(test, ret, 0);
> +}
> +
> +static struct kunit_case root_device_devm_tests[] = {
> +       KUNIT_CASE(root_device_devm_register_unregister_test),
> +       KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
> +       {}
> +};
> +
> +static struct kunit_suite root_device_devm_test_suite = {
> +       .name = "root-device-devm",
> +       .init = root_device_devm_init,
> +       .test_cases = root_device_devm_tests,
> +};
> +
> +kunit_test_suite(root_device_devm_test_suite);
>
> --
> 2.40.0
>
Maxime Ripard July 20, 2023, 11:29 a.m. UTC | #2
Hi David,

On Wed, Jul 19, 2023 at 05:13:45PM +0800, David Gow wrote:
> On Wed, 28 Jun 2023 at 17:49, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > The root devices show some odd behaviours compared to regular "bus" devices
> > that have been probed through the usual mechanism, so let's create kunit
> > tests to exercise those paths and odd cases.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> 
> Reviewed-by: David Gow <davidgow@google.com>

Thanks!

> There's definitely an argument that root devices are not supposed to
> be like regular devices, and so devm_ managed resources aren't
> supposed to work with them. Either way:
> - It needs to be documented somewhere (and this test makes for good
> documentation, IMO).
> - It should be consistent: if devm_ isn't to be used with root
> devices, it should fail everywhere, and if it is, it should work in
> all the cases below.
> 
> So I'm all in favour of including this test and making root devices work.

I agree 100%. I've reworded the commit log a bit to make it clearer
that's what we should strive for, and that this is what this patch is
doing.

> That being said, I am planning to send out a patchset adding a struct
> kunit_device for use in tests, which will be attached to a "kunit"
> bus. I think the combination of "fix devm_ with root devices" and
> "don't recommend root devices as a 'fake' device for testing" is
> probably the longer-term solution everyone can agree upon?

Yeah, that sounds reasonable to me

Maxime
diff mbox series

Patch

diff --git a/drivers/base/test/.kunitconfig b/drivers/base/test/.kunitconfig
new file mode 100644
index 000000000000..473923f0998b
--- /dev/null
+++ b/drivers/base/test/.kunitconfig
@@ -0,0 +1,2 @@ 
+CONFIG_KUNIT=y
+CONFIG_DM_KUNIT_TEST=y
diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
index 610a1ba7a467..9d42051f8f8e 100644
--- a/drivers/base/test/Kconfig
+++ b/drivers/base/test/Kconfig
@@ -9,6 +9,10 @@  config TEST_ASYNC_DRIVER_PROBE
 
 	  If unsure say N.
 
+config DM_KUNIT_TEST
+	tristate "KUnit Tests for the device model" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+
 config DRIVER_PE_KUNIT_TEST
 	bool "KUnit Tests for property entry API" if !KUNIT_ALL_TESTS
 	depends on KUNIT=y
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 7f76fee6f989..d589ca3fa8fc 100644
--- a/drivers/base/test/Makefile
+++ b/drivers/base/test/Makefile
@@ -1,5 +1,7 @@ 
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_TEST_ASYNC_DRIVER_PROBE)	+= test_async_driver_probe.o
 
+obj-$(CONFIG_DM_KUNIT_TEST)	+= root-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/root-device-test.c b/drivers/base/test/root-device-test.c
new file mode 100644
index 000000000000..9a3e6cccae13
--- /dev/null
+++ b/drivers/base/test/root-device-test.c
@@ -0,0 +1,110 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright 2023 Maxime Ripard <mripard@kernel.org>
+
+#include <kunit/resource.h>
+
+#include <linux/device.h>
+
+#define DEVICE_NAME "test"
+
+struct test_priv {
+	bool probe_done;
+	bool release_done;
+	wait_queue_head_t release_wq;
+	struct device *dev;
+};
+
+static int root_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->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);
+}
+
+#define RELEASE_TIMEOUT_MS	100
+
+/*
+ * Tests that a bus-less, non-probed device will run its device-managed
+ * actions when unregistered.
+ */
+static void root_device_devm_register_unregister_test(struct kunit *test)
+{
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+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);
+}
+
+/*
+ * Tests that a bus-less, non-probed device will run its device-managed
+ * actions when unregistered, even if someone still holds a reference to
+ * it.
+ */
+static void root_device_devm_register_get_unregister_with_devm_test(struct kunit *test)
+{
+	struct test_priv *priv = test->priv;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	priv->dev = root_device_register(DEVICE_NAME);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+	get_device(priv->dev);
+
+	ret = devm_add_action_or_reset(priv->dev, devm_put_device_action, priv);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	root_device_unregister(priv->dev);
+
+	ret = wait_event_interruptible_timeout(priv->release_wq, priv->release_done,
+					       msecs_to_jiffies(RELEASE_TIMEOUT_MS));
+	KUNIT_EXPECT_GT(test, ret, 0);
+}
+
+static struct kunit_case root_device_devm_tests[] = {
+	KUNIT_CASE(root_device_devm_register_unregister_test),
+	KUNIT_CASE(root_device_devm_register_get_unregister_with_devm_test),
+	{}
+};
+
+static struct kunit_suite root_device_devm_test_suite = {
+	.name = "root-device-devm",
+	.init = root_device_devm_init,
+	.test_cases = root_device_devm_tests,
+};
+
+kunit_test_suite(root_device_devm_test_suite);