Hi David, all,
On 3/23/23 09:30, David Gow wrote:
On Wed, 22 Mar 2023 at 17:06, Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
---
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@xxxxxxxxxx/
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@xxxxxxxxxx/
[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! ~~
