linux-kernel - Re: [PATCH v2 03/10] clk: test: introduce a few specific rate constants for mock testing

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250610-hot-shapeless-skua-dcfc2a@houat>
Date: Tue, 10 Jun 2025 18:28:26 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Brian Masney <bmasney@...hat.com>
Cc: Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>, 
	Thomas Gleixner <tglx@...utronix.de>, Michael Turquette <mturquette@...libre.com>, 
	Alberto Ruiz <aruiz@...hat.com>
Subject: Re: [PATCH v2 03/10] clk: test: introduce a few specific rate
 constants for mock testing

On Fri, Jun 06, 2025 at 10:28:19AM -0400, Brian Masney wrote:
> On Fri, Jun 06, 2025 at 10:56:57AM +0200, Maxime Ripard wrote:
> > On Wed, May 28, 2025 at 07:16:49PM -0400, Brian Masney wrote:
> > > Some of the mock tests care about the relationship between two
> > > different rates, and the specific numbers are important, such as for
> > > mocking a divider.
> > > 
> > > Signed-off-by: Brian Masney <bmasney@...hat.com>
> > 
> > It's not obvious to me why they are important, actually. The relation
> > between the two is, but a divider (and our tests) should work with any
> > parent rate, so I guess we can expect it to be opaque.
> 
> I agree as well.
> 
> > Can you expand on why it's important?
> 
> I personally find that having specific numbers in some (but not) of the
> tests make the tests clearer that specific functionality within the clk
> core is exercised. For example, assume we have a parent that can do any
> rate, and two children that are dividers. We could have a test like the
> following:
> 
>     clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_1);
>     clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_2);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_1);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_2);
>     /*
>      * Make something to figure out what the ideal parent rate should be
>      * and test that as well?
>      */
> 
> So if we set child1 and child2 to 16 MHz and 32 MHz, then that exercises
> one path through the clk core. However, it will currently fail if we set
> the children to 32 MHz and 48 MHz. I have this working on a WIP branch
> and one of my new tests looks similar to:
> 
>     clk_set_rate(ctx->child1_clk, DUMMY_CLOCK_RATE_32_MHZ);
>     clk_set_rate(ctx->child2_clk, DUMMY_CLOCK_RATE_48_MHZ);
>     // This should test that it's a multiple of 96 MHz
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->parent_clk), DUMMY_CLOCK_RATE_96_MHZ);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child1_clk), DUMMY_CLOCK_RATE_32_MHZ);
>     KUNIT_EXPECT_EQ(test, clk_get_rate(ctx->child2_clk), DUMMY_CLOCK_RATE_48_MHZ);
> 
> Based on the work in my WIP branch, I think we need to make some of the
> divider only clk tests parameterized, and have a table with various
> specific frequencies so that various edge cases within the clk core are
> tested by the frequency combinations.
> 
> I think that instead of having a list of DUMMY_CLOCK_RATE_XXX_MHZ
> defines, a single define like this will suffice:
> 
> #define clk_dummy_rate_mhz(rate)      ((rate) * 1000 * 1000)

So, my main worry is that I'm concerned that some tests will only pass
because (or thanks to) the rates we've chosen, and not because they are
actually passing. Kind of like what happened with your earlier patch to
change the rate clk_recalc was called with.

I have the feeling our tests should have caught it, and maybe it's also
because we're missing some coverage, but didn't because we picked those
particular rates it worked by accident.

Maybe we should start using parent_rate / X in our assertions instead of
defines, or come up with better assertions?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)