Hi David,
On Thu, Mar 23, 2023 at 03:30:34PM +0800, David Gow wrote:
I think the questions we therefore need to answer are:
- Do we specifically need a platform_device (or, any other specific
device struct), or would any old struct device work? I can see why we would need a platform device for cases where we're testing things like device-tree properties (or, in the future, having e.g. USB-specific helpers which create usb_device). Most tests just use root_device_register() thus far, though.
- What helpers do we need to make creating, using, and cleaning up
these devices as simple as possible.
I think that in this particular case, we don't actually need a struct platform_device. Replacing these helpers with simple calls to root_device_register() and root_device_unregister() seems to work fine with this patch series for me. (It does break the drm_test_managed_run_action test, though.) So I don't think having these helpers actually help this series at the moment.
I'm not sure that a root_device is a good option, see below.
That being said, if they used the KUnit resource system to automatically clean up the device when the test is finished (or otherwise exits), that would seem like a real advantage. The clk and drm examples both do this, and I'm hoping to add an API to make it even simpler going forward. (With the work-in-progress API described here[1], the whole thing should boil down to "struct device *dev = root_device_register(name); kunit_defer(root_device_unregister, dev);".)
Oh, yes, please make it happen :)
The current API is a bit of a pain compared to other managed APIs like devm or drmm.
So, I guess we have three cases we need to look at:
- A test just needs any old struct device. Tests thus far have
hardcoded, or had their own wrappers around root_device_register() for this.
- A test needs a device attached to a bus (but doesn't care which
bus). Thus far, people have used struct platform_device for this (see the DRM helpers, which use a platform device for managed resource tests[2]). Maybe the right solution here is something like a struct kunit_device?
- A test needs a device attached to a specific bus. We'll probably
need some more detailed faking of that bus. This covers cases like having fake USB devices, devicetree integration, etc.
Yeah, from the current discussion on the IIO and clk patchsets, and what we've done in DRM, I guess there's two major use cases:
* You test an (isolated) function that takes a device as an argument. Here you probably don't really care about what the device is as long as you can provide one. This is what is being done for the DRM helpers at the moment, even though it's not really isolated. The device is essentially mocked. This could be your points 1 and 2.
* You want to probe the driver with a fake context. The device and drivers are very much real, but the device tree (or whatever) is mocked. This is what the clocks patches from Stephen are doing. This could be covered by your point 3.
I'd suggest that, for the majority of cases which only care about the first case, let's just use root_device_register() directly, or have a thin wrapper like the old root_device-based version of the DRM helpers[3]. This will probable serve us well enough while we work out how to handle the other two cases properly (which is already being looked at for the CLK/DeviceTree patches and the DRM stuff).
I disagree, and I think your cases 1 and 2 should be merged. We were initially using a root_device in DRM. We had to switch to platform_device (but actually any device attached to a bus) because a root device isn't attached to a bus and thus the devm resources are never freed.
When a function takes a device as an argument, there's a good chance that it will use devm nowadays, so we ended up exhausting resources pools (like IDs iirc) when running a lot of tests in sequence.
So yeah, I think you can't just assume that a root device will do even for a true unit test that would test an isolated function. It either needs to be tied to a bus, or we need to force the devm resource release when the root device is removed somehow.
Maxime