Messages in this thread | | | Date | Thu, 23 Mar 2023 10:35:21 +0200 | From | Matti Vaittinen <> | Subject | Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation |
| |
Hi David, all,
On 3/23/23 09:30, David Gow wrote: > On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@gmail.com> wrote: >> >> The creation of a dummy device in order to test managed interfaces is a >> generally useful test feature. The drm test helpers >> drm_kunit_helper_alloc_device() and drm_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 so stole these for more generic use. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> >> --- >> Changes: >> v4 => v5: >> - Add accidentally dropped header and email recipients >> - do not rename + move helpers from DRM but add temporary dublicates to >> simplify merging. (This patch does not touch DRM and can be merged >> separately. DRM patch and IIO test patch still depend on this one). >> >> Please note that there's something similar 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 >> --- > > Thanks for sending this my way. > > It's clear we need some way of creating "fake" devices for KUnit > tests. Given that there are now (at least) three different drivers > looking to use this, we'll ultimately need something which works for > everyone. > > 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.
Funny timing. I just found the root_device_register() while wondering the parent for auxiliary_devices.
I think the root_device_[un]register() meets my current needs.
> - 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 am afraid that further work with these helpers is out of the scope for me (at least for now). I'll drop the DRM and the helper patches from this series && go with the root_device_register(), root_device_unregister() in the IIO tests added in this series.
> That being said, if they used the KUnit resource system to > automatically clean up the device when the test is finished (or > otherwise exits),
My 10 cents to this is that yes, automatic unwinding at test exit would be simple - and also correct for most of the time. However, I think there might also be tests that would like to verify the unwinding process has really managed to do what it was intended to do. That, I think would require being able to manually drop the device in the middle of the test.
> 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.
As said above, my case fits this category.
> - 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?
This sounds like, how to put it, "architecturally correct". But...
> - 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.
...if we in any case need this, wouldn't the kunit_device just be unnecessary bloat because if the test does not care which bus it is sitting in, then it could probably use a bus-specific device as well?
> I'd suggest that, for the majority of cases which only care about the > first case, let's just use root_device_register() directly,
After finding the root_device_register() - I agree.
> 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). > > If the resulting helpers are generally useful enough, they can > probably sit in either drivers/base or lib/kunit. I'd rather not have > code that's really specific to certain busses sitting in lib/kunit > rather than alongside the device/bus code in drivers/base or some > other subsystem/driver path, but I can tolerate it for the very > generic struct device. > > Regardless, I've left a few notes on the patch itself below.
Thanks but I guess I'll just drop this one :)
> > Cheers, > -- David > > [1]: https://kunit-review.googlesource.com/c/linux/+/5434/3/include/kunit/resource.h > [2]: https://lore.kernel.org/all/20221123-rpi-kunit-tests-v3-8-4615a663a84a@cerno.tech/ > [3]: https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/tests/drm_kunit_helpers.c#L39
Thanks for the thorough analysis and these links! This was enlightening :)
Yours, -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |