Re: [PATCH 0/8] clk: Add kunit tests for fixed rate and parent data

[Frank Rowand <frowand.list@gmail.com>]

On 3/2/23 13:47, Geert Uytterhoeven wrote:
> Hi Stephen,
> 
> On Thu, Mar 2, 2023 at 8:28 PM Stephen Boyd <sboyd@kernel.org> wrote:
>> Quoting Rob Herring (2023-03-02 09:32:09)
>>> On Thu, Mar 2, 2023 at 2:14 AM David Gow <davidgow@google.com> wrote:
>>>> On Thu, 2 Mar 2023 at 09:38, Stephen Boyd <sboyd@kernel.org> wrote:
>>>>> This patch series adds unit tests for the clk fixed rate basic type and
>>>>> the clk registration functions that use struct clk_parent_data. To get
>>>>> there, we add support for loading a DTB into the UML kernel that's
>>>>> running the unit tests along with probing platform drivers to bind to
>>>>> device nodes specified in DT.
>>>>>
>>>>> With this series, we're able to exercise some of the code in the common
>>>>> clk framework that uses devicetree lookups to find parents and the fixed
>>>>> rate clk code that scans devicetree directly and creates clks. Please
>>>>> review.
>>>>>
>>>>
>>>> Thanks Stephen -- this is really neat!
>>>>
>>>> This works well here, and I love all of the tests for the
>>>> KUnit/device-tree integration as well.
>>>>
>>>> I'm still looking through the details of it (alas, I've mostly lived
>>>> in x86-land, so my device-tree knowledge is, uh, spotty to say the
>>>> least), but apart from possibly renaming some things or similarly
>>>> minor tweaks, I've not got any real suggestions thus far.
>>>>
>>>> I do wonder whether we'll want, on the KUnit side, to have some way of
>>>> supporting KUnit device trees on non-UML architecctures (e.g., if we
>>>> need to test something architecture-specific, or on a big-endian
>>>> platform, etc), but I think that's a question for the future, rather
>>>> than something that affects this series.
>>>
>>> I'll say that's a requirement. We should be able to structure the
>>> tests to not interfere with the running system's DT. The DT unittest
>>> does that.
>>
>> That could be another choice in the unit test choice menu.
>> CONFIG_OF_KUNIT_NOT_UML that injects some built-in DTB overlay on an
>> architecture that wants to run tests.
> 
> As long as you use compatible values that don't exist elsewhere,
> and don't overwrite anything, you can load your kunit test overlays
> on any running system that has DT support.
> 
>>> As a side topic, Is anyone looking at getting UML to work on arm64?
>>> It's surprising how much x86 stuff there is which is I guess one
>>> reason it hasn't happened.
>>
>> I've no idea but it would be nice indeed.
> 
> I believe that's non-trivial. At least for arm32 (I didn't have any arm64
> systems last time I asked the experts).
> 
>>>> Similarly, I wonder if there's something we could do with device tree
>>>> overlays, in order to make it possible for tests to swap nodes in and
>>>> out for testing.
>>>
>>> Yes, that's how the DT unittest works. But it is pretty much one big
>>> overlay (ignoring the overlay tests). It could probably be more
>>> modular where it is apply overlay, test, remove overlay, repeat.
>>
>> I didn't want to rely on the overlay code to inject DT nodes. Having
>> tests written for the fake KUnit machine is simple. It closely matches
>> how clk code probes the DTB and how nodes are created and populated on
>> the platform bus as devices. CLK_OF_DECLARE() would need the overlay to
>> be applied early too, which doesn't happen otherwise as far as I know.
> 
> Don't all generic clock drivers also create a platform driver?
> At least drivers/clk/clk-fixed-factor.c does.
> 
>> But perhaps this design is too much of an end-to-end test and not a unit
>> test? In the spirit of unit testing we shouldn't care about how the node
>> is added to the live devicetree, just that there is a devicetree at all.
>>
>> Supporting overlays to more easily test combinations sounds like a good
>> idea. Probably some kunit_*() prefixed functions could be used to
>> apply a test managed overlay and automatically remove it when the test
>> is over would work. The clk registration tests could use this API to
>> inject an overlay and then manually call the of_platform_populate()
>> function to create the platform device(s). The overlay could be built in
>> drivers/clk/ too and then probably some macroish function can find the
>> blob and apply it.
> 
> No need to manually call of_platform_populate() to create the
> platform devices. That is taken care of automatically when applying
> an overlay.
> 
>> Is there some way to delete the platform devices that we populate from
>> the overlay? I'd like the tests to be hermetic.
> 

> Removing the overlay will delete the platform devices.

I _think_ that is incorrect.  Do you have a pointer to the overlay code that
deletes the device?  (If I remember correctly, the overlay remove code does not
even check whether the device exists and whether a driver is bound to it -- but
this is on my todo list to look into.)

-Frank

> 
> All of that works if you have your own code to apply a DT overlay.
> The recent fw_devlinks patches did cause some regressions, cfr.
> https://lore.kernel.org/all/CAMuHMdXEnSD4rRJ-o90x4OprUacN_rJgyo8x6=9F9rZ+-KzjOg@mail.gmail.com
> 
> P.S. Shameless plug: for loading overlays from userspace, there are
>      my overlay branches, cfr. https://elinux.org/R-Car/DT-Overlays
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds