Message-ID: <CABVgOSnyudG-GYizLj8LThv5J54bWB=MX1e+arh6CvUd94On=A@mail.gmail.com>
Date:   Wed, 29 Apr 2020 12:15:37 +0800
From:   David Gow <davidgow@...gle.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     "sboyd@...nel.org" <sboyd@...nel.org>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "brendanhiggins@...gle.com" <brendanhiggins@...gle.com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kunit-dev@...glegroups.com" <kunit-dev@...glegroups.com>
Subject: Re: [PATCH/RFC] clk: gate: Add some kunit test suites

On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti
<Matti.Vaittinen@...rohmeurope.com> wrote:
>
> Hello Stephen & All,
>
> Prologue:
>
> I have been traumatized in the past - by unit tests :) Thus I am always
> a bit jumpy when I see people adding UTs. I always see the inertia UTs
> add to development - when people change anything they must also change
> impacted UTs - and every unnecessary UT case is actually burden. I was
> once buried under such burden.. Back at those times I quite often found
> myself wondering why I spend two days fixing UT cases after I did a
> necessary code change which took maybe 10 minutes. Please see my
> comments knowing this history and do your decisions based on less
> biased - brighter understanding :]
>
>

Hey Matti,

As someone who's definitely wasted a lot of time fixing
poorly-thought-through tests, but who is nevertheless working
enthusiastically on KUnit, I wanted to respond quickly here.

Certainly, the goal is not to reduce development velocity, though it
is redistributed a bit: hopefully the extra time spent updating tests
will be more than paid back in identifying and fixing bugs earlier, as
well as making testing one's own changes simpler. Only time will tell
if we're right or not, but if they turn out to cause more problems
than they solve we can always adjust or remove them. I remain
optimistic, though.

I do think that these tests are unlikely to increase the burden on
developers much: they seem mostly to test interfaces and behaviour
which is used quite a bit elsewhere in the kernel: changes that break
them seem likely to be reasonably disruptive anyway, so these tests
aren't going to add too much to that overall, and may ultimately make
things a bit simpler by helping to document and verify the changes.

A few other notes inline:

> On Tue, 2020-04-07 at 20:56 -0700, Stephen Boyd wrote:
> > Test various parts of the clk gate implementation with the kunit
> > testing
> > framework.
> >
> > Cc: Brendan Higgins <brendanhiggins@...gle.com>
> > Cc: <kunit-dev@...glegroups.com>
> > Signed-off-by: Stephen Boyd <sboyd@...nel.org>
> > ---
> >
> > This patch is on top of this series[1] that allows the clk
> > framework to be selected by Kconfig language.
> >
> > [1]
> > https://lore.kernel.org/r/20200405025123.154688-1-sboyd@kernel.org
> >
> >  drivers/clk/Kconfig         |   8 +
> >  drivers/clk/Makefile        |   1 +
> >  drivers/clk/clk-gate-test.c | 481
> > ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 490 insertions(+)
> >  create mode 100644 drivers/clk/clk-gate-test.c
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 6ea0631e3956..66193673bcdf 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -377,4 +377,12 @@ source "drivers/clk/ti/Kconfig"
> >  source "drivers/clk/uniphier/Kconfig"
> >  source "drivers/clk/zynqmp/Kconfig"
> >
> > +# Kunit test cases
> > +config CLK_GATE_TEST
> > +     tristate "Basic gate type Kunit test"
> > +     depends on KUNIT
> > +     default KUNIT
> > +     help
> > +       Kunit test for the basic clk gate type.
> > +
> >  endif
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index f4169cc2fd31..0785092880fd 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_COMMON_CLK)      += clk-divider.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-factor.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-fixed-rate.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-gate.o
> > +obj-$(CONFIG_CLK_GATE_TEST)  += clk-gate-test.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-multiplier.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-mux.o
> >  obj-$(CONFIG_COMMON_CLK)     += clk-composite.o
> > diff --git a/drivers/clk/clk-gate-test.c b/drivers/clk/clk-gate-
> > test.c
> > new file mode 100644
> > index 000000000000..b1d6c21e9698
> > --- /dev/null
> > +++ b/drivers/clk/clk-gate-test.c
> > @@ -0,0 +1,481 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * KUnit test for clk gate basic type
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <kunit/test.h>
> > +
> > +static void clk_gate_register_test_dev(struct kunit *test)
> > +{
> > +     struct clk_hw *ret;
> > +     struct platform_device *pdev;
> > +
> > +     pdev = platform_device_register_simple("test_gate_device", -1,
> > NULL, 0);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
> > +
> > +     ret = clk_hw_register_gate(&pdev->dev, "test_gate", NULL, 0,
> > NULL,
> > +                                0, 0, NULL);
> > +     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, ret);
> > +     KUNIT_EXPECT_STREQ(test, "test_gate", clk_hw_get_name(ret));
> > +     KUNIT_EXPECT_EQ(test, 0UL, clk_hw_get_flags(ret));
> > +
> > +     clk_hw_unregister_gate(ret);
> > +     platform_device_put(pdev);
> > +}
> > +
> > +static void clk_gate_register_test_parent_names(struct kunit *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *ret;
> > +
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         1000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +
> > +     ret = clk_hw_register_gate(NULL, "test_gate", "test_parent", 0,
> > NULL,
> > +                                0, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ret);
> > +     KUNIT_EXPECT_PTR_EQ(test, parent, clk_hw_get_parent(ret));
> > +
> > +     clk_hw_unregister_gate(ret);
> > +     clk_hw_unregister_fixed_rate(parent);
> > +}
> > +
> > +static void clk_gate_register_test_parent_data(struct kunit *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *ret;
> > +     struct clk_parent_data pdata = { };
> > +
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         1000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +     pdata.hw = parent;
> > +
> > +     ret = clk_hw_register_gate_parent_data(NULL, "test_gate",
> > &pdata, 0,
> > +                                            NULL, 0, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ret);
> > +     KUNIT_EXPECT_PTR_EQ(test, parent, clk_hw_get_parent(ret));
> > +
> > +     clk_hw_unregister_gate(ret);
> > +     clk_hw_unregister_fixed_rate(parent);
> > +}
> > +
> > +static void clk_gate_register_test_parent_data_legacy(struct kunit
> > *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *ret;
> > +     struct clk_parent_data pdata = { };
> > +
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         1000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +     pdata.name = "test_parent";
> > +
> > +     ret = clk_hw_register_gate_parent_data(NULL, "test_gate",
> > &pdata, 0,
> > +                                            NULL, 0, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ret);
> > +     KUNIT_EXPECT_PTR_EQ(test, parent, clk_hw_get_parent(ret));
> > +
> > +     clk_hw_unregister_gate(ret);
> > +     clk_hw_unregister_fixed_rate(parent);
> > +}
> > +
> > +static void clk_gate_register_test_parent_hw(struct kunit *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *ret;
> > +
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         1000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +
> > +     ret = clk_hw_register_gate_parent_hw(NULL, "test_gate", parent,
> > 0, NULL,
> > +                                          0, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ret);
> > +     KUNIT_EXPECT_PTR_EQ(test, parent, clk_hw_get_parent(ret));
> > +
> > +     clk_hw_unregister_gate(ret);
> > +     clk_hw_unregister_fixed_rate(parent);
> > +}
> > +
> > +static void clk_gate_register_test_hiword_invalid(struct kunit
> > *test)
> > +{
> > +     struct clk_hw *ret;
> > +
> > +     ret = clk_hw_register_gate(NULL, "test_gate", NULL, 0, NULL,
> > +                                20, CLK_GATE_HIWORD_MASK, NULL);
> > +
> > +     KUNIT_EXPECT_TRUE(test, IS_ERR(ret));
> > +}
>
> Do we really need all these registration tests? I guess most of the
> code path would be tested by just one of these - how much value we add
> by adding rest of the registration cases? (just wondering)
>

The advantage of having multiple tests here is that it's really
obvious if just one of these function breaks: that's probably not
enormously likely, but there's also not much of a cost to having them
— these test would run pretty quickly, and wouldn't be too expensive
to change in the unlikely event that was necessary.

> > +
> > +static struct kunit_case clk_gate_register_test_cases[] = {
> > +     KUNIT_CASE(clk_gate_register_test_dev),
> > +     KUNIT_CASE(clk_gate_register_test_parent_names),
> > +     KUNIT_CASE(clk_gate_register_test_parent_data),
> > +     KUNIT_CASE(clk_gate_register_test_parent_data_legacy),
> > +     KUNIT_CASE(clk_gate_register_test_parent_hw),
> > +     KUNIT_CASE(clk_gate_register_test_hiword_invalid),
> > +     {}
> > +};
> > +
> > +static struct kunit_suite clk_gate_register_test_suite = {
> > +     .name = "clk-gate-register-test",
> > +     .test_cases = clk_gate_register_test_cases,
> > +};
> > +
> > +struct clk_gate_test_context {
> > +     void __iomem *fake_mem;
> > +     struct clk_hw *hw;
> > +     struct clk_hw *parent;
> > +     u32 fake_reg; /* Keep at end, KASAN can detect out of bounds */
> > +};
> > +
> > +static struct clk_gate_test_context *clk_gate_test_alloc_ctx(struct
> > kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx;
> > +
> > +     test->priv = ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> > +     ctx->fake_mem = (void __force __iomem *)&ctx->fake_reg;
> > +
> > +     return ctx;
> > +}
> > +
> > +static void clk_gate_test_parent_rate(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +     struct clk_hw *parent = ctx->parent;
> > +     struct clk_hw *hw = ctx->hw;
> > +     unsigned long prate = clk_hw_get_rate(parent);
> > +     unsigned long rate = clk_hw_get_rate(hw);
> > +
> > +     KUNIT_EXPECT_EQ(test, prate, rate);
> > +}
> > +
> > +static void clk_gate_test_enable(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +     struct clk_hw *parent = ctx->parent;
> > +     struct clk_hw *hw = ctx->hw;
> > +     struct clk *clk = hw->clk;
> > +     int ret;
> > +     u32 enable_val = BIT(5);
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > +     KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(parent));
> > +}
>
> Is this really necessary? Wouldn't most of this be tested by
> clk_gate_test_disable()?
>

While _enable and _disable are likely pretty similar, having both
tests here would catch things like copy-paste bugs from one to the
other, which is probably worth the extra test. Particularly given that
basically all of the functions tested are actually different
functions.


> > +
> > +static void clk_gate_test_disable(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +     struct clk_hw *parent = ctx->parent;
> > +     struct clk_hw *hw = ctx->hw;
> > +     struct clk *clk = hw->clk;
> > +     int ret;
> > +     u32 enable_val = BIT(5);
> > +     u32 disable_val = 0;
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
> > +     KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
> > +
> > +     clk_disable_unprepare(clk);
> > +     KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(parent));
> > +}
> > +
> > +static struct kunit_case clk_gate_test_cases[] = {
> > +     KUNIT_CASE(clk_gate_test_parent_rate),
> > +     KUNIT_CASE(clk_gate_test_enable),
> > +     KUNIT_CASE(clk_gate_test_disable),
> > +     {}
> > +};
> > +
> > +static int clk_gate_test_init(struct kunit *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *hw;
> > +     struct clk_gate_test_context *ctx;
> > +
> > +     ctx = clk_gate_test_alloc_ctx(test);
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         2000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +
> > +     hw = clk_hw_register_gate_parent_hw(NULL, "test_gate", parent,
> > 0,
> > +                                         ctx->fake_mem, 5, 0, NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> > +
> > +     ctx->hw = hw;
> > +     ctx->parent = parent;
> > +
> > +     return 0;
> > +}
> > +
> > +static void clk_gate_test_exit(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +
> > +     clk_hw_unregister_gate(ctx->hw);
> > +     clk_hw_unregister_fixed_rate(ctx->parent);
> > +     kfree(ctx);
> > +}
>
> Aren't these init and exit actually covering some of the things tested
> in clk_gate_register_test_cases? Perhaps we could reduce some tests or
> use some test functions as init/exit?
>

Logically, these init/exit functions are not supposed to be testing
anything in and of themselves, just setting up the environment for
other tests.
I do like the idea of reducing code duplication by, say, moving some
of the code into a separate helper function. I'm a little wary of
actually using the same function as an init function as a test: that
seems more confusing than the duplication to me, but that's just
personal preference, I think.

> > +
> > +static struct kunit_suite clk_gate_test_suite = {
> > +     .name = "clk-gate-test",
> > +     .init = clk_gate_test_init,
> > +     .exit = clk_gate_test_exit,
> > +     .test_cases = clk_gate_test_cases,
> > +};
> > +
> > +static void clk_gate_test_invert_enable(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +     struct clk_hw *parent = ctx->parent;
> > +     struct clk_hw *hw = ctx->hw;
> > +     struct clk *clk = hw->clk;
> > +     int ret;
> > +     u32 enable_val = 0;
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
> > +
> > +     KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
> > +     KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(parent));
> > +}
>
> Is this really adding value after clk_gate_test_invert_disable() has
> passed?
>
> +
> > +static void clk_gate_test_invert_disable(struct kunit *test)
> > +{
> > +     struct clk_gate_test_context *ctx = test->priv;
> > +     struct clk_hw *parent = ctx->parent;
> > +     struct clk_hw *hw = ctx->hw;
> > +     struct clk *clk = hw->clk;
> > +     int ret;
> > +     u32 enable_val = 0;
> > +     u32 disable_val = BIT(15);
> > +
> > +     ret = clk_prepare_enable(clk);
> > +     KUNIT_ASSERT_EQ(test, ret, 0);
> > +     KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
> > +
> > +     clk_disable_unprepare(clk);
> > +     KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
> > +     KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(parent));
> > +}
> > +
> > +static struct kunit_case clk_gate_test_invert_cases[] = {
> > +     KUNIT_CASE(clk_gate_test_invert_enable),
> > +     KUNIT_CASE(clk_gate_test_invert_disable),
> > +     {}
> > +};
> > +
> > +static int clk_gate_test_invert_init(struct kunit *test)
> > +{
> > +     struct clk_hw *parent;
> > +     struct clk_hw *hw;
> > +     struct clk_gate_test_context *ctx;
> > +
> > +     ctx = clk_gate_test_alloc_ctx(test);
> > +     parent = clk_hw_register_fixed_rate(NULL, "test_parent", NULL,
> > 0,
> > +                                         2000000);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
> > +
> > +     ctx->fake_reg = BIT(15); /* Default to off */
> > +     hw = clk_hw_register_gate_parent_hw(NULL, "test_gate", parent,
> > 0,
> > +                                         ctx->fake_mem, 15,
> > +                                         CLK_GATE_SET_TO_DISABLE,
> > NULL);
> > +     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> > +
> > +     ctx->hw = hw;
> > +     ctx->parent = parent;
> > +
> > +     return 0;
> > +}
>
> Aren't these init and exit actually covering some of the things tested
> in clk_gate_register_test_cases? Perhaps we could reduce some tests or
> use some test functions as init/exit?

As above, I personally like having the test versions as well, as it
ensures that there's at least one test showing the failure in its
actual test function. But reusing the actual code wouldn't be a
problem, and is probably just a matter of taste.

> I stopped reviewing here - I think you know what I was up-to and you
> are the best experts to evaluate whether there is something to
> squash/drop also in the rest of the tests :)
>
>
> Epilogue:
>
> In general, I would be very careful when adding UT-code and I would try
> to minimize the required test changes when for example one of the clk
> APIs require a change. (I know, the test changes are least thing to
> worry if these APIs need change - but effort is still cumulative and if
> we can avoid some - we should).
>
> I don't mean to be disrespectful. I know my place in the food-chain :p
> Besides, I have seen the great work Stephen has been doing with clk -
> and I believe he knows what he is doing. But sometimes little poking
> can invoke new thoughts :grin: You can ignore my comments if they make
> no sense to you.
>
> Best Regards
>   --Matti
> >
>

While I obviously sit more on the "yes" side of the unit-testing
fence, you've definitely raised some interesting points. Personally,
I'd really like to see this test go in (whether as-is, or with minor
adjustments), but writing up a KUnit "testing style guide" or similar
is on my to-do list, and I'm definitely going to want to think more
about how best to handle some of these situations (and better
articulate why).

Cheers,
-- David

Powered by blists - more mailing lists