On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
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@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)
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.
That's fine, but please, let's do this right if it's going to be in the driver core, that way we can actually test the driver core code as well.
+/**
- 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.
Any device can access devm_ code, there's no need for it to be a platform device at all.
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.
I agree, let's use a virtual device and a virtual bus (you can use the auxbus code for this as that's all there for this type of thing) and then you can test the devm_* code, _AND_ you can test the driver core at the same time.
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.
I agree that this could be in drivers/base/test/ but then let's test the driver core, not just provide a dummy platform device.
If you want to test the platform driver/device api, that would be great too, that can be plaform device/driver specific, but don't use one for some other random driver core functionality please.
thanks,
greg k-h