Hi Greg,
Thanks for looking at this.
On 3/22/23 14:07, Greg Kroah-Hartman wrote:
On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:
--- /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@xxxxxxxxxx>
+ */
+
+#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)
Please do not abuse platform devices and drivers for things that are not
actually platform devices and drivers.
+{
+ 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,
+ },
+};
Why do you need this fake platform driver at all?
Why not just use a virtual device?
I can only answer on my behalf. In my case the answer to why I used
platform_devices is practicality. I wanted to test devm_ APIs using
KUnit tests and I was pointed to an existing implementation in DRM (seen
in these patches). It didn't seem to make any sense to re-invent the
wheel by writing another implementation for the existing in-tree
functionality.
Maybe Maxime had a better reason to go with the platform devices.
+/**
+ * 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.
What specific "usual infrastructure" are you wanting to access here?
And again, if you want a fake device, make a virtual one, by just
calling device_create().
Or are you wanting to do "more" with that device pointer than
device_create() can give you?
Personally, I was (am) only interested in devm_ unwinding. I guess the
device_create(), device_add(), device_remove()... (didn't study this
sequence in details so sorry if there is errors) could've been
sufficient for me. I haven't looked how much of the code that there is
for 'platform devices' should be duplicated to support that sequence for
testability purposes.
The biggest thing for me is that I don't like the idea of creating own
'test device' in <add subsystem here> while we already have some in DRM
(or others). Thus, I do see value in adding generic helpers for
supporting running KUnit tests on devm_* APIs. Hence it'd be good to
have _some_ support for it. And having them in drivers/base/test seemed
like a correct place to me. What I really don't know is if there are
legitimate use-cases for using platform_devices in DRM tests. Perhaps
Maxime can shed light on that.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
