On 3/13/23 15:59, Matti Vaittinen wrote:
On 3/13/23 15:29, Andy Shevchenko wrote:
On Mon, Mar 13, 2023 at 03:11:52PM +0200, Matti Vaittinen wrote:
On 3/13/23 14:40, Andy Shevchenko wrote:
On Sun, Mar 12, 2023 at 05:08:48PM +0000, Jonathan Cameron wrote:
On Sun, 12 Mar 2023 17:06:38 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
...
Ah. I forgot the tests that don't have a device so can't use devm.
Why not? I have seen, IIRC, test cases inside the kernel that fakes
the device
for that.
I'd appreciated any pointer for such an example if you have one at
hand. (I
can do the digging if you don't though!)
I am not a fan of unit tests. They add huge amount of inertia to
development, and in worst case, they stop people from contributing where
improving a feature requires test code modification(s). And harder
the test
code is to understand, worse the unwanted side-effects. Also, harder the
test code is to read, more time and effort it requires to analyze a test
failure... Hence, I am _very_ conservative what comes to adding size
of test
code with anything that is not strictly required.
After that being said, unit tests are a great tool when carefully
used - and
I assume/hope stubbing a device for devm_ tests does not add much
extra...
But let me see if I can find an example :)
drivers/gpu/drm/tests/drm_managed_test.c ?
(somewhere underneath:
ret = platform_driver_register(&fake_platform_driver);
which suggests... what exactly? :-)
Thanks to pointer from Andy I found the
drm_kunit_helper_[alloc/free]_device() functions. I renamed them to
test_kunit_helper_[alloc/free]_device(), move them to drivers/base, add
declarations to include/kunit/test-helpers.h fixed KConfigs and existing
callers + added the tests for managed interfaces. I have this in place
in my personal playground where I am working towards the v4 of the series.
...
After that I asked from Maxime if he had a reason to not make those
generic and available to other subsystems besides drm in the first place...
And Maxime was kind enough to point me to the fact that something like
this was done in the CCF context:
https://lore.kernel.org/all/20230302013822.1808711-1-sboyd@xxxxxxxxxx/
I like the 'single function to get the dummy device which can be passed
to devm'-approach used in drm helpers. I do also like Stephen's idea of
having the prototypes in kunit/platform_device.h which matches the
linux/platform_device.h.
However, I don't know when Stephen's work will be finished and merged to
IIO-tree so that it could be used/extended for the needs of these tests.
Meanwhile, I don't think it makes sense to go forward with my changes
splitting the helpers out of drm until we see what Stephen's changes
will bring us. On the other hand, I don't like delaying the gts-helpers
or the sensor drivers.
So, any suggestions what I should do? I see following options:
1) Drop the tests for managed interfaces for now.
2) Add the tests with a yet-another duplicate implementation of the
dummy device for devm.
3) Add the tests using the helpers from drm as they are now.
option 1):
I like it as it would be an easy way (for now) - but I hate it as it may
be a hard way as well. In my experience, when a driver/helper lands
upstream it will get first few fixes quite fast - and not having a test
available upstream when this happens is bad. Bad because it means the
out-of-tree test may get broken, and bad because there is no easy way to
test the fixes.
option 2):
I hate it because it makes the test code more complex - and duplicates
the kernel code which is never nice. This could be reworked later when
Stephens work is done though.
option 3):
It's in general not nice to use functions exported for some other
subsystem's specific purposes. This would however keep the test code at
minimum, while leaving the same "I swear I'll fix this later when
dependencies have settled" - possibility as option 2) did.
Oh, in theory there is option 4) to just send out the changes I did(*)
which pull the drm_kunit_helper_[alloc/free]_device() out of the DRM -
but I guess that would lead some extra work to merge this later with
stuff Stephen's series does introduce.
Any suggestions which of the options to proceed with?
(*) For those interested in seeing the result of pulling the
drm_kunit_helper_[alloc/free]_device() out of DRM tests, below are links
to my personal playground with following remarks:
1) code one finds from there may be 100% untested
2) code one finds there may be written just for fun, or for a very
specific purpose
3) code one finds there is generally not maintained, may be rebased, may
vanish or turn into rabbits or turn you into a rabbit when you wear a
top hat.
commits to look at there are
https://github.com/M-Vaittinen/linux/commit/15d07e799f7c7fddc91030b16266d4a8bbaf1cc1
https://github.com/M-Vaittinen/linux/commit/6b4c4ba38b1f838fb0074befd2ca8734604464da
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
