[PATCH v4 2/8] kunit: drm/tests: move generic helpers

Matti Vaittinen posted 8 patches 2 years, 11 months ago
There is a newer version of this series
[PATCH v4 2/8] kunit: drm/tests: move generic helpers
Posted by Matti Vaittinen 2 years, 11 months ago
The creation of a dummy device in order to test managed interfaces is a
generally useful test feature. The drm test helpers
test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
are doing exactly this. It makes no sense that each and every component
which intends to be testing managed interfaces will create similar
helpers.

Move these functions to place where it is more obvious they can be used
also by other subsystems but drm.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---

Please note that there's something similat ongoing in the CCF:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/

I do like the simplicity of these DRM-originated helpers so I think
they're worth. I do equally like the Stephen's idea of having the
"dummy platform device" related helpers under drivers/base and the
header being in include/kunit/platform_device.h which is similar to real
platform device under include/linux/platform_device.h - so, in the end
of the day I hope Stephen's changes as well as the changes this patch
introduces to end up in those files. This, however, will require some
co-operation to avoid conflicts.
---
 drivers/base/test/Kconfig                     |  5 ++
 drivers/base/test/Makefile                    |  2 +
 drivers/base/test/test_kunit_device.c         | 83 +++++++++++++++++++
 drivers/gpu/drm/Kconfig                       |  2 +
 .../gpu/drm/tests/drm_client_modeset_test.c   |  1 +
 drivers/gpu/drm/tests/drm_kunit_helpers.c     | 69 ---------------
 drivers/gpu/drm/tests/drm_managed_test.c      |  1 +
 drivers/gpu/drm/tests/drm_modes_test.c        |  1 +
 drivers/gpu/drm/tests/drm_probe_helper_test.c |  1 +
 drivers/gpu/drm/vc4/Kconfig                   |  1 +
 drivers/gpu/drm/vc4/tests/vc4_mock.c          |  1 +
 .../gpu/drm/vc4/tests/vc4_test_pv_muxing.c    |  1 +
 include/drm/drm_kunit_helpers.h               |  3 -
 13 files changed, 99 insertions(+), 72 deletions(-)
 create mode 100644 drivers/base/test/test_kunit_device.c

diff --git a/drivers/base/test/Kconfig b/drivers/base/test/Kconfig
index 610a1ba7a467..642b5b183c10 100644
--- a/drivers/base/test/Kconfig
+++ b/drivers/base/test/Kconfig
@@ -1,4 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
+
+config TEST_KUNIT_DEVICE_HELPERS
+	depends on KUNIT
+	tristate
+
 config TEST_ASYNC_DRIVER_PROBE
 	tristate "Build kernel module to test asynchronous driver probing"
 	depends on m
diff --git a/drivers/base/test/Makefile b/drivers/base/test/Makefile
index 7f76fee6f989..49926524ec6f 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_TEST_KUNIT_DEVICE_HELPERS) += test_kunit_device.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/test_kunit_device.c b/drivers/base/test/test_kunit_device.c
new file mode 100644
index 000000000000..75790e15b85c
--- /dev/null
+++ b/drivers/base/test/test_kunit_device.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * These helpers have been extracted from drm test code at
+ * drm_kunit_helpers.c which was authored by
+ * Maxime Ripard <maxime@cerno.tech>
+ */
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include <kunit/platform_device.h>
+
+#define KUNIT_DEVICE_NAME	"test-kunit-mock-device"
+
+static int fake_probe(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static int fake_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver fake_platform_driver = {
+	.probe	= fake_probe,
+	.remove	= fake_remove,
+	.driver = {
+		.name	= KUNIT_DEVICE_NAME,
+	},
+};
+
+/**
+ * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
+ * @test: The test context object
+ *
+ * This allocates a fake struct &device to create a mock for a KUnit
+ * test. The device will also be bound to a fake driver. It will thus be
+ * able to leverage the usual infrastructure and most notably the
+ * device-managed resources just like a "real" device.
+ *
+ * Callers need to make sure test_kunit_helper_free_device() on the
+ * device when done.
+ *
+ * Returns:
+ * A pointer to the new device, or an ERR_PTR() otherwise.
+ */
+struct device *test_kunit_helper_alloc_device(struct kunit *test)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	ret = platform_driver_register(&fake_platform_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+
+	ret = platform_device_add(pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	return &pdev->dev;
+}
+EXPORT_SYMBOL_GPL(test_kunit_helper_alloc_device);
+
+/**
+ * test_kunit_helper_free_device - Frees a mock device
+ * @test: The test context object
+ * @dev: The device to free
+ *
+ * Frees a device allocated with test_kunit_helper_alloc_device().
+ */
+void test_kunit_helper_free_device(struct kunit *test, struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	platform_device_unregister(pdev);
+	platform_driver_unregister(&fake_platform_driver);
+}
+EXPORT_SYMBOL_GPL(test_kunit_helper_free_device);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index dc0f94f02a82..0ee8ebe64f57 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -66,6 +66,7 @@ config DRM_USE_DYNAMIC_DEBUG
 config DRM_KUNIT_TEST_HELPERS
 	tristate
 	depends on DRM && KUNIT
+	select TEST_KUNIT_DEVICE_HELPERS
 	help
 	  KUnit Helpers for KMS drivers.
 
@@ -80,6 +81,7 @@ config DRM_KUNIT_TEST
 	select DRM_BUDDY
 	select DRM_EXPORT_FOR_TESTS if m
 	select DRM_KUNIT_TEST_HELPERS
+	select TEST_KUNIT_DEVICE_HELPERS
 	default KUNIT_ALL_TESTS
 	help
 	  This builds unit tests for DRM. This option is not useful for
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index 27ab03d1c518..d7eaa0938eb4 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2022 Maxime Ripard <mripard@kernel.org>
  */
 
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
 #include <drm/drm_connector.h>
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index ec93b0300f03..ae84d0ed8744 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -9,78 +9,9 @@
 #include <linux/device.h>
 #include <linux/platform_device.h>
 
-#define KUNIT_DEVICE_NAME	"drm-kunit-mock-device"
-
 static const struct drm_mode_config_funcs drm_mode_config_funcs = {
 };
 
-static int fake_probe(struct platform_device *pdev)
-{
-	return 0;
-}
-
-static int fake_remove(struct platform_device *pdev)
-{
-	return 0;
-}
-
-static struct platform_driver fake_platform_driver = {
-	.probe	= fake_probe,
-	.remove	= fake_remove,
-	.driver = {
-		.name	= KUNIT_DEVICE_NAME,
-	},
-};
-
-/**
- * test_kunit_helper_alloc_device - Allocate a mock device for a KUnit test
- * @test: The test context object
- *
- * This allocates a fake struct &device to create a mock for a KUnit
- * test. The device will also be bound to a fake driver. It will thus be
- * able to leverage the usual infrastructure and most notably the
- * device-managed resources just like a "real" device.
- *
- * Callers need to make sure test_kunit_helper_free_device() on the
- * device when done.
- *
- * Returns:
- * A pointer to the new device, or an ERR_PTR() otherwise.
- */
-struct device *test_kunit_helper_alloc_device(struct kunit *test)
-{
-	struct platform_device *pdev;
-	int ret;
-
-	ret = platform_driver_register(&fake_platform_driver);
-	KUNIT_ASSERT_EQ(test, ret, 0);
-
-	pdev = platform_device_alloc(KUNIT_DEVICE_NAME, PLATFORM_DEVID_NONE);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
-
-	ret = platform_device_add(pdev);
-	KUNIT_ASSERT_EQ(test, ret, 0);
-
-	return &pdev->dev;
-}
-EXPORT_SYMBOL_GPL(test_kunit_helper_alloc_device);
-
-/**
- * test_kunit_helper_free_device - Frees a mock device
- * @test: The test context object
- * @dev: The device to free
- *
- * Frees a device allocated with test_kunit_helper_alloc_device().
- */
-void test_kunit_helper_free_device(struct kunit *test, struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-
-	platform_device_unregister(pdev);
-	platform_driver_unregister(&fake_platform_driver);
-}
-EXPORT_SYMBOL_GPL(test_kunit_helper_free_device);
-
 struct drm_device *
 __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
 						struct device *dev,
diff --git a/drivers/gpu/drm/tests/drm_managed_test.c b/drivers/gpu/drm/tests/drm_managed_test.c
index 53f870493577..6b39d2cde164 100644
--- a/drivers/gpu/drm/tests/drm_managed_test.c
+++ b/drivers/gpu/drm/tests/drm_managed_test.c
@@ -4,6 +4,7 @@
 #include <drm/drm_kunit_helpers.h>
 #include <drm/drm_managed.h>
 
+#include <kunit/platform_device.h>
 #include <kunit/resource.h>
 
 #include <linux/device.h>
diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
index 1bd8540086e9..addc4d923a26 100644
--- a/drivers/gpu/drm/tests/drm_modes_test.c
+++ b/drivers/gpu/drm/tests/drm_modes_test.c
@@ -7,6 +7,7 @@
 #include <drm/drm_kunit_helpers.h>
 #include <drm/drm_modes.h>
 
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
 #include <linux/units.h>
diff --git a/drivers/gpu/drm/tests/drm_probe_helper_test.c b/drivers/gpu/drm/tests/drm_probe_helper_test.c
index bc4b271bec09..f23213464d34 100644
--- a/drivers/gpu/drm/tests/drm_probe_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_probe_helper_test.c
@@ -13,6 +13,7 @@
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_probe_helper.h>
 
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
 struct drm_probe_helper_test_priv {
diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
index 91dcf8d174d6..a4bd96445315 100644
--- a/drivers/gpu/drm/vc4/Kconfig
+++ b/drivers/gpu/drm/vc4/Kconfig
@@ -39,6 +39,7 @@ config DRM_VC4_KUNIT_TEST
 	tristate "KUnit tests for VC4" if !KUNIT_ALL_TESTS
 	depends on DRM_VC4 && KUNIT
 	select DRM_KUNIT_TEST_HELPERS
+	select TEST_KUNIT_DEVICE_HELPERS
 	default KUNIT_ALL_TESTS
 	help
 	  This builds unit tests for the VC4 DRM/KMS driver. This option is
diff --git a/drivers/gpu/drm/vc4/tests/vc4_mock.c b/drivers/gpu/drm/vc4/tests/vc4_mock.c
index 5bb1fa828a3f..29eb045b0db1 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_mock.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_mock.c
@@ -3,6 +3,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_kunit_helpers.h>
 
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
 #include "vc4_mock.h"
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 8b373fa76d6f..64b90e2e3706 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -12,6 +12,7 @@
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_plane.h>
 
+#include <kunit/platform_device.h>
 #include <kunit/test.h>
 
 #include "../vc4_drv.h"
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index 8e3aae6a5ed5..ab438d97aed3 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -8,9 +8,6 @@
 struct drm_device;
 struct kunit;
 
-struct device *test_kunit_helper_alloc_device(struct kunit *test);
-void test_kunit_helper_free_device(struct kunit *test, struct device *dev);
-
 struct drm_device *
 __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
 						struct device *dev,
-- 
2.39.2


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 
Re: [PATCH v4 2/8] kunit: drm/tests: move generic helpers
Posted by Maxime Ripard 2 years, 11 months ago
Hi Matti,

On Fri, Mar 17, 2023 at 04:42:25PM +0200, Matti Vaittinen wrote:
> The creation of a dummy device in order to test managed interfaces is a
> generally useful test feature. The drm test helpers
> test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
> are doing exactly this. It makes no sense that each and every component
> which intends to be testing managed interfaces will create similar
> helpers.
> 
> Move these functions to place where it is more obvious they can be used
> also by other subsystems but drm.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> 
> Please note that there's something similat ongoing in the CCF:
> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
> 
> I do like the simplicity of these DRM-originated helpers so I think
> they're worth. I do equally like the Stephen's idea of having the
> "dummy platform device" related helpers under drivers/base and the
> header being in include/kunit/platform_device.h which is similar to real
> platform device under include/linux/platform_device.h - so, in the end
> of the day I hope Stephen's changes as well as the changes this patch
> introduces to end up in those files. This, however, will require some
> co-operation to avoid conflicts.

I think you would have an easier time if you just copied and renamed
them into the kunit folder as an preparation series.

That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just
create new helpers that can be reused/converted to by everyone eventually.

Maxime
Re: [PATCH v4 2/8] kunit: drm/tests: move generic helpers
Posted by Matti Vaittinen 2 years, 10 months ago
Hi Maxime & All

First of all - I am sorry. During the last minute rebase I accidentally 
dropped the header file from this series. Will fix that for v5. (Also 
the build bot pointed this mistake).

On 3/17/23 17:09, Maxime Ripard wrote:
> Hi Matti,
> 
> On Fri, Mar 17, 2023 at 04:42:25PM +0200, Matti Vaittinen wrote:
>> The creation of a dummy device in order to test managed interfaces is a
>> generally useful test feature. The drm test helpers
>> test_kunit_helper_alloc_device() and test_kunit_helper_free_device()
>> are doing exactly this. It makes no sense that each and every component
>> which intends to be testing managed interfaces will create similar
>> helpers.
>>
>> Move these functions to place where it is more obvious they can be used
>> also by other subsystems but drm.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>>
>> Please note that there's something similat ongoing in the CCF:
>> https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@kernel.org/
>>
>> I do like the simplicity of these DRM-originated helpers so I think
>> they're worth. I do equally like the Stephen's idea of having the
>> "dummy platform device" related helpers under drivers/base and the
>> header being in include/kunit/platform_device.h which is similar to real
>> platform device under include/linux/platform_device.h - so, in the end
>> of the day I hope Stephen's changes as well as the changes this patch
>> introduces to end up in those files. This, however, will require some
>> co-operation to avoid conflicts.
> 
> I think you would have an easier time if you just copied and renamed
> them into the kunit folder as an preparation series.

Yes. That would simplify the syncing between the trees. It slightly bugs 
me to add dublicate code in kernel-but the clean-up series for DRM users 
could be prepared at the same time. It would be even possible to just 
change the drm-helper to be a wrapper for the generic one - and leave 
the callers intact - although it leaves some seemingly unnecessary 
"onion code" there.

> That way, you wouldn't have to coordinate DRM, CCF and IIO, you'd just
> create new helpers that can be reused/converted to by everyone eventually

Yes. Thanks - I think I may go with this approach for the v5 :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~