Re: [PATCH v5 13/28] clk: Take into account uncached clocks in clk_set_rate_range()

[Maxime Ripard <maxime@cerno.tech>]

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

On Wed, Jun 29, 2022 at 02:01:44AM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-05-16 06:25:12)
> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index 8de6339f4f8d..9aa5b946f324 100644
> > --- a/drivers/clk/clk_test.c
> > +++ b/drivers/clk/clk_test.c
> > @@ -362,9 +362,37 @@ static void clk_test_uncached_set_range(struct kunit *test)
> >         KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
> >  }
> >  
> > +/*
> > + * Test that for an uncached clock, clk_set_rate_range() will work
> > + * properly if the rate has changed in hardware.
> > + *
> > + * In this case, it means that if the rate wasn't initially in the range
> > + * we're trying to set, but got changed at some point into the range
> > + * without the kernel knowing about it, its rate shouldn't be affected.
> > + */
> > +static void clk_test_uncached_updated_rate_set_range(struct kunit *test)
> > +{
> > +       struct clk_dummy_context *ctx = test->priv;
> > +       struct clk_hw *hw = &ctx->hw;
> > +       struct clk *clk = hw->clk;
> > +       unsigned long rate;
> > +
> > +       ctx->rate = DUMMY_CLOCK_RATE_1 + 1000;
> 
> Is this where we set the rate behind clk framework's back? Maybe add a
> comment here to state that.

Yes, I'll add a comment

> > +       KUNIT_ASSERT_EQ(test,
> > +                       clk_set_rate_range(clk,
> > +                                          DUMMY_CLOCK_RATE_1,
> > +                                          DUMMY_CLOCK_RATE_2),
> > +                       0);
> > +
> > +       rate = clk_get_rate(clk);
> > +       KUNIT_ASSERT_GT(test, rate, 0);
> 
> This will almost always be true because rate is unsigned. Should it be
> KUNIT_ASSERT_NE() instead?
> 
> Is there any benefit at all to this check?  We're going to check the
> rate with an expectation in the next line for what we're actually
> testing for, so it's not like we need to assert that the rate is
> non-zero before checking that it is exactly DUMMY_CLOCK_RATE_1 + 1000.
> 
> I thought assertions were about checking sanity of the parts of the test
> that aren't under test. If the assertion fails then our test is so
> busted the expectation can't be trusted and we shouldn't even try to
> continue. It's similar to BUG_ON() and WARN_ON().

I'm not entirely up to speed on what is our expectations when it comes
to unit tests.

clk_get_rate() mentions that it can return 0, and it definitely
shouldn't with the setup we are testing here. So it was my impression
that we were in "totally busted" territory, but if you feel it's
overkill I'll remove it.

Maxime

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