linux-kernel - Re: [PATCH v3 1/4] clk: add a clk_hw helpers to get the clock device or device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1jpli223d4.fsf@starbuckisacylon.baylibre.com>
Date: Thu, 27 Mar 2025 11:07:51 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Kevin Hilman <khilman@...libre.com>,  Martin Blumenstingl
 <martin.blumenstingl@...glemail.com>,  Michael Turquette
 <mturquette@...libre.com>,  Neil Armstrong <neil.armstrong@...aro.org>,
  linux-clk@...r.kernel.org,  linux-kernel@...r.kernel.org,
  linux-amlogic@...ts.infradead.org,  linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/4] clk: add a clk_hw helpers to get the clock
 device or device_node

On Tue 25 Mar 2025 at 14:57, Stephen Boyd <sboyd@...nel.org> wrote:

> Quoting Jerome Brunet (2025-03-21 10:53:49)
>> On Wed 26 Feb 2025 at 17:01, Stephen Boyd <sboyd@...nel.org> wrote:
>> 
>> 
>> >> +static void clk_hw_get_of_node_test(struct kunit *test)
>> >> +{
>> >> +       struct device_node *np;
>> >> +       struct clk_hw *hw;
>> >> +
>> >> +       hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
>> >> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
>> >> +
>> >> +       np = of_find_compatible_node(NULL, NULL, "test,clk-dummy-device");
>> >> +       hw->init = CLK_HW_INIT_NO_PARENT("test_get_of_node",
>> >> +                                        &clk_dummy_rate_ops, 0);
>> >> +       of_node_put_kunit(test, np);
>> >> +
>> >> +       KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, np, hw));
>> >
>> > The stuff before the expectation should likely go to the init function.
>> > Or it can use the genparams stuff so we can set some struct members to
>> > indicate if the pointer should be NULL or not and then twist through the
>> > code a couple times.
>> >
>> 
>> I'm trying to address all your comments but I'm starting to wonder if
>> this isn't going a bit too far ? The functions tested are one line
>> returns. Is it really worth all this ?
>> 
>> I do understand the idea for things that actually do something, such as
>> reparenting, setting rates or what not ... But this ? It feels like a
>> lot of test code for very little added value, don't you think ?
>> 
>
> Just so I understand, you're saying that this is always going to be a
> simple "getter" API that doesn't do much else? We're not _only_ testing
> the getter API, we're also testing the registration path that actually
> sets the device or of_node pointers for a clk. I'm not really thinking
> about the one line return functions here.

Oh, that was not clear to me. I assumed the registration path was
already tested to an appropriate level, so I did not consider this.
Makes sense.

>
> Writing tests is definitely a balancing act.

That's where my question came from actually. We are aligned on this :)

> I'd say we want to test the
> behavior of the API in relation to how a clk is registered and writing
> tests to show the intended usage is helpful to understand if we've
> thought of corner cases like the clk was registered with a device
> pointer that also has an of_node associated with it. (Did we remember to
> stash that of_node pointer too?) We have a bunch of clk registration
> APIs, and we want to make sure this getter API works with all of them.
> Note that we don't care about the clk flags or parent relation chains
> here, just that the device or of_node passed in to registration comes
> out the other side with the getter API.
>
> A little code duplication is OK, as long as the test is easy to
> understand. Maybe genparams stuff is going too far, I don't know, but at
> the least we want to make sure the clk registration APIs behave as
> expected when the getter API is used to get the device or of_node
> later.

Now that the goal is more clear (to me), I'll try to find a good balance.
I'll also split the helper from the tests, so I can progress on the driver
front while we refine the tests, if that's OK with you ? It is not overly
critical for both to land at the same time, is it ?

>
> I've found this google chapter[1] useful for unit testing best
> practices. I recommend reading it if you haven't already.
>
> [1] https://abseil.io/resources/swe-book/html/ch12.html

I will, thanks

-- 
Jerome