On 3/17/23 12:19, Maxime Ripard wrote: > On Wed, Mar 15, 2023 at 12:51:26PM +0200, Matti Vaittinen wrote: >> 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? > > I think the best course of action would be to synchronize with Stephen, > and make sure that whatever patch you're doing can be used for his work. > > Once it works for both of you, then I guess it can go through the kunit > tree and you will use it both. Thanks Andy and Maxime! I appreciate your help. I guess I'll ping Stephen with a separate mail (although he's in CC here) and CC the relevant patches to him as well. I assume that gives him a chance to take a look what I am doing and suggest changes if needed. My contribution is anyways small - it's mostly just renaming and moving those two SRm helper APIs + changing the callers. Most of that is probably outside the scope of his work - it would just fit the same files he will be adding =) Yours, -- Matti -- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland ~~ When things go utterly wrong vim users can always type :help! ~~
