Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

On Fri, Feb 25, 2022 at 02:44:04PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-02-25 06:26:06)
> > Hi Stephen,
> > 
> > On Thu, Feb 24, 2022 at 02:54:20PM -0800, Stephen Boyd wrote:
> > > Quoting Daniel Latypov (2022-02-23 14:50:59)
> > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard <maxime@cerno.tech> wrote:
> > > > Incremental coverage for 3/9 files in --diff_file
> > > > Total incremental: 99.29% coverage (281/283 lines)
> > > >   drivers/clk/clk.c: 84.62% coverage (11/13 lines)
> > > >   drivers/clk/clk_test.c: 100.00% coverage (269/269 lines)
> > > >   include/linux/clk.h: 100.00% coverage (1/1 lines)
> > > > 
> > > > Missing lines are drivers/clk/clk.c:2397-8, i.e. this part of the diff:
> > > > +       if (ret) {
> > > > +               /* rollback the changes */
> > > > +               clk->min_rate = old_min; <- 2397
> > > > +               clk->max_rate = old_max; <- 2398
> > > > 
> > > > These are from before and were just moved around.
> > > 
> > > We could trigger a failure in the provider when the rate is set, and
> > > then we could call round_rate() again and make sure the boundaries from
> > > before are maintained.
> > 
> > I tried to do that, and it turns out we can't, since we ignore the
> > set_rate return code:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2107
> > 
> > We could make determine_rate fail, but then clk_round_rate would fail as
> > well and wouldn't allow us to test whether the boundaries are still in
> > place.
> > 
> 
> The test could still do it at a high level right? And when/if we decide
> to bubble up the set_rate failure then we would be testing these lines.
> Seems like a good idea to implement it with a TODO note that clk.c is
> ignoring the set_rate clk_op returning failure.

I'm sorry, but I don't get what I need to implement here

The trivial test for this would be to have a driver with set_rate that
returns some error, and then:

KUNIT_ASSERT_LT(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);
KUNIT_EXPECT_LT(test,
		clk_set_rate_range(clk,
				   DUMMY_CLOCK_RATE_1,
				   DUMMY_CLOCK_RATE_2),
		0);

rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
KUNIT_ASSERT_GT(test, rate, 0);
KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 - 1000);

clk_set_rate_range will never return an error code though, and the
round_rate tests won't work either because the set_rate error was not
propagated, and therefore the boundaries won't be reverted either. So
not only the test will fail, but it will also not increase the coverage.

It's really not clear to me what you expect here. Or we should just
create it but skip it all the time with a FIXME? But then again, it
doesn't increase the coverage, so I'm not sure why it's holding off that
series.

Honestly, I'm getting a bit frustrated by all this. This started as a
small fix, and now we keep moving the goalposts, with more and more
expectations and fixes for things that have nothing related to the
original series. And we're now arguing about gaining a few percent of
code coverage on some code that without this series has a 0% percent
coverage.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]