Re: [PATCH v3 01/10] clk: Add Kunit tests for rate

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2022-01-20 06:34:08)
> Let's test various parts of the rate-related clock API with the kunit
> testing framework.
> 
> Cc: kunit-dev@googlegroups.com
> Suggested-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

This is great! Thanks for doing this.

>  drivers/clk/Kconfig         |   7 +
>  drivers/clk/Makefile        |   1 +
>  drivers/clk/clk-rate-test.c | 278 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 286 insertions(+)
>  create mode 100644 drivers/clk/clk-rate-test.c

I was thinking this would be more generic so that one file tests clk.c
and all the code in there, but I guess there may be config dependencies
like CONFIG_OF that we may want to extract out and depend on
differently. I'm not sure how kunit will handle testing different paths
depending on build configuration so this approach may head off future
problems. If it doesn't then we can always slam the test together.

> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index f5807d190ba2..7ae48a91f738 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -436,4 +436,11 @@ config CLK_GATE_TEST
>         help
>           Kunit test for the basic clk gate type.
>  
> +config CLK_RATE_TEST

See v3 here[1] and have it follow that.

> +       tristate "Basic Core Rate Kunit Tests"
> +       depends on KUNIT
> +       default KUNIT
> +       help
> +         Kunit test for the basic clock rate management.
> +
>  endif
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index b940c6d35922..0238a595167a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -2,6 +2,7 @@
>  # common clock types
>  obj-$(CONFIG_HAVE_CLK)         += clk-devres.o clk-bulk.o clkdev.o
>  obj-$(CONFIG_COMMON_CLK)       += clk.o
> +obj-$(CONFIG_CLK_RATE_TEST)    += clk-rate-test.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-divider.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-factor.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-fixed-rate.o
> diff --git a/drivers/clk/clk-rate-test.c b/drivers/clk/clk-rate-test.c
> new file mode 100644
> index 000000000000..f2d3df791b5a
> --- /dev/null
> +++ b/drivers/clk/clk-rate-test.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for clk rate management
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +
> +#include <kunit/test.h>
> +
> +#define DUMMY_CLOCK_INIT_RATE  (42 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_1     (142 * 1000 * 1000)
> +#define DUMMY_CLOCK_RATE_2     (242 * 1000 * 1000)
> +
> +struct clk_dummy_rate_context {
> +       struct clk_hw hw;
> +       unsigned long rate;
> +};
> +
> +static unsigned long clk_dummy_rate_recalc_rate(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       return ctx->rate;
> +}
> +
> +static int clk_dummy_rate_determine_rate(struct clk_hw *hw,
> +                                        struct clk_rate_request *req)
> +{
> +       /* Just return the same rate without modifying it */
> +       return 0;
> +}
> +
> +static int clk_dummy_rate_set_rate(struct clk_hw *hw,
> +                                  unsigned long rate,
> +                                  unsigned long parent_rate)
> +{
> +       struct clk_dummy_rate_context *ctx =
> +               container_of(hw, struct clk_dummy_rate_context, hw);
> +
> +       ctx->rate = rate;
> +       return 0;
> +}
> +
> +const static struct clk_ops clk_dummy_rate_ops = {

static const?

> +       .recalc_rate = clk_dummy_rate_recalc_rate,
> +       .determine_rate = clk_dummy_rate_determine_rate,
> +       .set_rate = clk_dummy_rate_set_rate,
> +};
> +
> +static int clk_rate_test_init_with_ops(struct kunit *test,
> +                                      const struct clk_ops *ops)
> +{
> +       struct clk_dummy_rate_context *ctx;
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

Use kunit_kzalloc() here

> +       if (!ctx)
> +               return -ENOMEM;
> +       ctx->rate = DUMMY_CLOCK_INIT_RATE;
> +       test->priv = ctx;
> +
> +       init.name = "test_dummy_rate";
> +       init.ops = ops;
> +       ctx->hw.init = &init;
> +
> +       ret = clk_hw_register(NULL, &ctx->hw);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int clk_rate_test_init(struct kunit *test)
> +{
> +       return clk_rate_test_init_with_ops(test, &clk_dummy_rate_ops);
> +}
> +
> +static void clk_rate_test_exit(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +
> +       clk_hw_unregister(&ctx->hw);
> +       kfree(ctx);

And drop the kfree as it is now test managed.

> +}
> +
> +/*
> + * Test that the actual rate matches what is returned by clk_get_rate()
> + */
> +static void clk_rate_test_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, ctx->rate);

These should be KUNIT_EXPECT_*() as we don't want to stop the test if
the rate is wrong, we want to check that the rate is what we expected it
to be. Assertions are about making sure things are sane and if not we
should stop testing, whereas expectations are about testing the code. A
test must have an EXPECT while it can have an ASSERT.

Maybe kunit should check that there was an EXPECT on return from the
test. Daniel?

> +}
> +
> +/*
> + * Test that, after a call to clk_set_rate(), the rate returned by
> + * clk_get_rate() matches.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

I'd like to throw the ret check directly into KUNIT_ASSERT_EQ() here.

	KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1), 0);

so we can easily see if something goes wrong that the set rate failed.
Good use of assert here, we don't want to continue with the test unless
the set rate actually worked.

> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +}
> +
> +/*
> + * Test that, after several calls to clk_set_rate(), the rate returned
> + * by clk_get_rate() matches the last one.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_test_set_set_get_rate(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
> +}
> +
> +static struct kunit_case clk_rate_test_cases[] = {
> +       KUNIT_CASE(clk_rate_test_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_get_rate),
> +       KUNIT_CASE(clk_rate_test_set_set_get_rate),
> +       {}
> +};
> +
> +static struct kunit_suite clk_rate_test_suite = {
> +       .name = "clk-rate-test",
> +       .init = clk_rate_test_init,
> +       .exit = clk_rate_test_exit,
> +       .test_cases = clk_rate_test_cases,
> +};
> +
> +/*
> + * Test that clk_set_rate_range won't return an error for a valid range.
> + */
> +static void clk_rate_range_test_set_range(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1,
> +                                DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);

Also make sure that the rate is within the bounds of rate_1 and rate_2?

> +}
> +
> +/*
> + * Test that calling clk_set_rate_range with a minimum rate higher than
> + * the maximum rate returns an error.
> + */
> +static void clk_rate_range_test_set_range_invalid(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       int ret;
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1 + 1000,
> +                                DUMMY_CLOCK_RATE_1);
> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);

Let's not check for a specific error, but a negative value instead. I'd
rather not encode particular error values unless we need them to be
particular.

> +}
> +
> +/*
> + * Test that if our clock has a rate lower than the minimum set by a
> + * call to clk_set_rate_range(), the rate will be raised to match the
> + * new minimum.
> + *
> + * This assumes that clk_ops.determine_rate or clk_ops.round_rate won't
> + * modify the requested rate, which is our case in clk_dummy_rate_ops.
> + */
> +static void clk_rate_range_test_set_range_get_rate_raised(struct kunit *test)
> +{
> +       struct clk_dummy_rate_context *ctx = test->priv;
> +       struct clk_hw *hw = &ctx->hw;
> +       struct clk *clk = hw->clk;
> +       unsigned long rate;
> +       int ret;
> +
> +       ret = clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       ret = clk_set_rate_range(clk,
> +                                DUMMY_CLOCK_RATE_1,
> +                                DUMMY_CLOCK_RATE_2);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_TRUE(test, rate > 0);

KUNIT_EXPECT_LE(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_2);

Or just drop it entirely as DUMMY_CLOCK_RATE_1 is greater than 0 and
it's tested next.

> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);

KUNIT_EXPECT_EQ(test, clk_get_rate(clk), DUMMY_CLOCK_RATE_1);

> +}
> +