[PATCH/RFC] clk: gate: Add some kunit test suites
diff mbox series

Message ID 20200408035637.110858-1-sboyd@kernel.org
State New, archived
Headers show
Series
  • [PATCH/RFC] clk: gate: Add some kunit test suites
Related show

Commit Message

Stephen Boyd April 8, 2020, 3:56 a.m. UTC
Test various parts of the clk gate implementation with the kunit testing
framework.

Cc: Brendan Higgins <brendanhiggins@google.com>
Cc: <kunit-dev@googlegroups.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.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


base-commit: 7111951b8d4973bda27ff663f2cf18b663d15b48
prerequisite-patch-id: e066e74d3e3848a69239083647017b9e4b2a7b87
prerequisite-patch-id: ab1cc18da4a59b8973b4c8d85b6ea90eb6200df0
prerequisite-patch-id: 5608c2059c4ad2d76aae62cd9897862a1f447cfb
prerequisite-patch-id: 3a2300fc681af075232bfe62dbd1eb81290ca5a1
prerequisite-patch-id: 6268382cce3446e3eb3cec7fec86973ab3dc9e7c
prerequisite-patch-id: 57b540a6a65ffb9cc53bb4b9c3fd8c4f55a4ce05
prerequisite-patch-id: 2ec56bc3f971534850ebe7253d5ae023dfc87410
prerequisite-patch-id: 9500860f59b801c734ab22f18737ca0ceff8208c
prerequisite-patch-id: 09a4d90ae718ab61fc9423862b1c2044ea898e3e

Comments

Brendan Higgins April 9, 2020, 8:09 p.m. UTC | #1
On Tue, Apr 7, 2020 at 8:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Test various parts of the clk gate implementation with the kunit testing
> framework.
>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: <kunit-dev@googlegroups.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>

One very minor nit below, other than that this looks great! I couldn't
have done a better job myself.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
>
> 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

Minor nit: Elsewhere you use KUnit.

I wasn't going to say anything because so many people go with the
"Kunit" capitalization (and actually I kind of prefer it), but you
should at least be consistent within your patch.

> +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

Here is the other capitalization.

> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +
> +#include <kunit/test.h>

[...]

Cheers!
Stephen Boyd April 11, 2020, 2:10 a.m. UTC | #2
Quoting Brendan Higgins (2020-04-09 13:09:03)
> On Tue, Apr 7, 2020 at 8:56 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Test various parts of the clk gate implementation with the kunit testing
> > framework.
> >
> > Cc: Brendan Higgins <brendanhiggins@google.com>
> > Cc: <kunit-dev@googlegroups.com>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> 
> One very minor nit below, other than that this looks great! I couldn't
> have done a better job myself.
> 
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> 
> > ---
> >
> > 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
> 
> Minor nit: Elsewhere you use KUnit.
> 
> I wasn't going to say anything because so many people go with the
> "Kunit" capitalization (and actually I kind of prefer it), but you
> should at least be consistent within your patch.

Ok I will go with Kunit. Thanks!
Matti Vaittinen April 14, 2020, 11:46 a.m. UTC | #3
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 :]


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@google.com>
> Cc: <kunit-dev@googlegroups.com>
> Signed-off-by: Stephen Boyd <sboyd@kernel.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)

> +
> +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()?

> +
> +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?

> +
> +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?

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
> 

> +
> +static struct kunit_suite clk_gate_test_invert_suite = {
> +	.name = "clk-gate-invert-test",
> +	.init = clk_gate_test_invert_init,
> +	.exit = clk_gate_test_exit,
> +	.test_cases = clk_gate_test_invert_cases,
> +};
> +
> +static void clk_gate_test_hiword_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(9) | BIT(9 + 16);
> +
> +	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));
> +}
> +
> +static void clk_gate_test_hiword_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(9) | BIT(9 + 16);
> +	u32 disable_val = BIT(9 + 16);
> +
> +	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_hiword_cases[] = {
> +	KUNIT_CASE(clk_gate_test_hiword_enable),
> +	KUNIT_CASE(clk_gate_test_hiword_disable),
> +	{}
> +};
> +
> +static int clk_gate_test_hiword_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, 9,
> +					    CLK_GATE_HIWORD_MASK,
> NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +
> +	ctx->hw = hw;
> +	ctx->parent = parent;
> +
> +	return 0;
> +}
> +
> +static struct kunit_suite clk_gate_test_hiword_suite = {
> +	.name = "clk-gate-hiword-test",
> +	.init = clk_gate_test_hiword_init,
> +	.exit = clk_gate_test_exit,
> +	.test_cases = clk_gate_test_hiword_cases,
> +};
> +
> +static void clk_gate_test_is_enabled(struct kunit *test)
> +{
> +	struct clk_hw *hw;
> +	struct clk_gate_test_context *ctx;
> +
> +	ctx = clk_gate_test_alloc_ctx(test);
> +	ctx->fake_reg = BIT(7);
> +	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx-
> >fake_mem, 7,
> +				  0, NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +	KUNIT_ASSERT_TRUE(test, clk_hw_is_enabled(hw));
> +
> +	clk_hw_unregister_gate(hw);
> +	kfree(ctx);
> +}
> +
> +static void clk_gate_test_is_disabled(struct kunit *test)
> +{
> +	struct clk_hw *hw;
> +	struct clk_gate_test_context *ctx;
> +
> +	ctx = clk_gate_test_alloc_ctx(test);
> +	ctx->fake_reg = BIT(4);
> +	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx-
> >fake_mem, 7,
> +				  0, NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +	KUNIT_ASSERT_FALSE(test, clk_hw_is_enabled(hw));
> +
> +	clk_hw_unregister_gate(hw);
> +	kfree(ctx);
> +}
> +
> +static void clk_gate_test_is_enabled_inverted(struct kunit *test)
> +{
> +	struct clk_hw *hw;
> +	struct clk_gate_test_context *ctx;
> +
> +	ctx = clk_gate_test_alloc_ctx(test);
> +	ctx->fake_reg = BIT(31);
> +	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx-
> >fake_mem, 2,
> +				  CLK_GATE_SET_TO_DISABLE, NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +	KUNIT_ASSERT_TRUE(test, clk_hw_is_enabled(hw));
> +
> +	clk_hw_unregister_gate(hw);
> +	kfree(ctx);
> +}
> +
> +static void clk_gate_test_is_disabled_inverted(struct kunit *test)
> +{
> +	struct clk_hw *hw;
> +	struct clk_gate_test_context *ctx;
> +
> +	ctx = clk_gate_test_alloc_ctx(test);
> +	ctx->fake_reg = BIT(29);
> +	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx-
> >fake_mem, 29,
> +				  CLK_GATE_SET_TO_DISABLE, NULL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +	KUNIT_ASSERT_FALSE(test, clk_hw_is_enabled(hw));
> +
> +	clk_hw_unregister_gate(hw);
> +	kfree(ctx);
> +}
> +
> +static struct kunit_case clk_gate_test_enabled_cases[] = {
> +	KUNIT_CASE(clk_gate_test_is_enabled),
> +	KUNIT_CASE(clk_gate_test_is_disabled),
> +	KUNIT_CASE(clk_gate_test_is_enabled_inverted),
> +	KUNIT_CASE(clk_gate_test_is_disabled_inverted),
> +	{}
> +};
> +
> +static struct kunit_suite clk_gate_test_enabled_suite = {
> +	.name = "clk-gate-is_enabled-test",
> +	.test_cases = clk_gate_test_enabled_cases,
> +};
> +
> +kunit_test_suites(
> +	&clk_gate_register_test_suite,
> +	&clk_gate_test_suite,
> +	&clk_gate_test_invert_suite,
> +	&clk_gate_test_hiword_suite,
> +	&clk_gate_test_enabled_suite
> +);
> +MODULE_LICENSE("GPL v2");
> 
> base-commit: 7111951b8d4973bda27ff663f2cf18b663d15b48
> prerequisite-patch-id: e066e74d3e3848a69239083647017b9e4b2a7b87
> prerequisite-patch-id: ab1cc18da4a59b8973b4c8d85b6ea90eb6200df0
> prerequisite-patch-id: 5608c2059c4ad2d76aae62cd9897862a1f447cfb
> prerequisite-patch-id: 3a2300fc681af075232bfe62dbd1eb81290ca5a1
> prerequisite-patch-id: 6268382cce3446e3eb3cec7fec86973ab3dc9e7c
> prerequisite-patch-id: 57b540a6a65ffb9cc53bb4b9c3fd8c4f55a4ce05
> prerequisite-patch-id: 2ec56bc3f971534850ebe7253d5ae023dfc87410
> prerequisite-patch-id: 9500860f59b801c734ab22f18737ca0ceff8208c
> prerequisite-patch-id: 09a4d90ae718ab61fc9423862b1c2044ea898e3e
> -- 
> Sent by a computer, using git, on the internet
>
David Gow April 29, 2020, 4:15 a.m. UTC | #4
On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti
<Matti.Vaittinen@fi.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@google.com>
> > Cc: <kunit-dev@googlegroups.com>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.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
Matti Vaittinen May 4, 2020, 5:54 a.m. UTC | #5
Hello David, & All

This review and mail is more for pointing out the downsides of UTs. I
am not demanding any changes, these comments can be seen as 'nit's.

On Wed, 2020-04-29 at 12:15 +0800, David Gow wrote:
> On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.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.

I appreciate your reply. And I appreciate your (and others) work on
KUinit. I don't think UTs are bad. I believe UTs are great and
powerfull tool - when used on specific occasions. But UTs definitely
are "double-edged sword" - you can hit your own leg.

> 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.

Fixing and identifying bugs is definitely "the thing" in UTs. But what
comes to weighing the benefits of UTs Vs. downsides - that's hard.
First thing on that front is to understand the cost of UTs. And in my
experience - many people underestimate that. It's easy too see things
black & white and think UTs are only good - which is not true.

UT cases are code which must be maintained as any code. And we must
never add code which is not beneficial as maintaining it is work. We do
constantly work to decrease amount of code to be maintaned - UTs are no
exception - all code is a burden. Unnecessary code is burden with no
purpose. And UTs have no other purpose but to point out mistakes. Only
good test is a test that is failing and pointing out a bug which would
have otherwise gone unnoticed. Passing test is a waste.

So key thing when considering if adding a test is beneficial is whether
the test is likely to point out a bug (in early phase). A bug that
would otherwise have gone through unnoticed.

> I do think that these tests are unlikely to increase the burden on
> developers much:

All code which uses kernel APIs is increasing burden for someone who
needs to change the API. Much can be discussed ;)

>  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,

This statement is valid for almost all exported kernel APIs - yet
adding tests for all APIs is definitely a bad idea. The effort is
cumulative. We should never add new code just because maintenance
effort is relatively small. We must only add code if it adds value.

>  and may ultimately make
> things a bit simpler by helping to document and verify the changes.

Yes. Definitely. But we must never deny that all added (test) code adds
work. And do weighing pros an cons only after that. Danger of UTs is
that we don't admit the cons.

> 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@google.com>
> > > Cc: <kunit-dev@googlegroups.com>
> > > Signed-off-by: Stephen Boyd <sboyd@kernel.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,

My point exactly.

>  but there's also not much of a cost to having them

And this is the other side of the equation. And I leave this estimation
to others - I just want to point out that imbalance in this equation is
what usually leads to writing excessive amounts of tests - which
eventually just hinders development with little or no benefits. But as
I said, I will leave this to smarter guys to evaluate. Just wanted to
point out that this should be considered :)

> — 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.

The disable test already does the enable. Just adding another set of
state checks before disable would verify whole thing, right? So, if I
am not mistaken then - by minimum - these could be tested at one go.
(If being beneficial at all).

> > > +
> > > +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).

I think this was more of my point than trying to demand actual changes
here. Thank you for considering this! :) It is important to admit that
_all_ code, whether test or implementation, require work and
maintenance and slow down development. We should _always_ consider if
adding a test is actually needed or if we could do it so that code
change require less test changes.

If I had all the power in the world I would never add a test for code
which is clean and simple. It is likely to not be beneficial. I would
mostly utilize UTs at spots where the logic is somewhat complex and
making a bug is likely. UTs do great job for example at verifying data
parsing functions (I've voluntarily tested bunch of netlink message
parsing code with UTs in some projects. I also added tests for
linear_ranges helpers in series I recently submitted.) But UTs are more
of a waste when added carelessly. I hope the UT documentation would
also cover that front.

Thanks for open discussion!

Yours,
--Matti
Brendan Higgins May 4, 2020, 8:19 p.m. UTC | #6
On Sun, May 3, 2020 at 10:54 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
> Hello David, & All
>
> This review and mail is more for pointing out the downsides of UTs. I
> am not demanding any changes, these comments can be seen as 'nit's.

Understood.

> On Wed, 2020-04-29 at 12:15 +0800, David Gow wrote:
> > On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti
> > <Matti.Vaittinen@fi.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.
>
> I appreciate your reply. And I appreciate your (and others) work on
> KUinit. I don't think UTs are bad. I believe UTs are great and
> powerfull tool - when used on specific occasions. But UTs definitely
> are "double-edged sword" - you can hit your own leg.

Sure, I saw you just added a unit test, so I would assume that you
don't think that UTs are inherently bad :-)

> > 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.
>
> Fixing and identifying bugs is definitely "the thing" in UTs. But what
> comes to weighing the benefits of UTs Vs. downsides - that's hard.
> First thing on that front is to understand the cost of UTs. And in my
> experience - many people underestimate that. It's easy too see things
> black & white and think UTs are only good - which is not true.
>
> UT cases are code which must be maintained as any code. And we must
> never add code which is not beneficial as maintaining it is work. We do
> constantly work to decrease amount of code to be maintaned - UTs are no
> exception - all code is a burden. Unnecessary code is burden with no
> purpose. And UTs have no other purpose but to point out mistakes. Only

I am in agreement with everything you said so far.

> good test is a test that is failing and pointing out a bug which would
> have otherwise gone unnoticed. Passing test is a waste.

Not to be uncharitable in how I read this, but I don't think this is
quite right. I think we would all agree that a test that *cannot* fail
is a waste, but on the other hand, you should never submit code that
doesn't pass *good tests*, right? Like you wrote tests for your linear
range stuff to verify that it worked as expected; I don't know if some
of the tests didn't pass on the first try, but you did ultimately fix
all the issues, and submitted only tests that pass, right? I am pretty
sure no one would accept code accompanied with tests that don't pass.

So would a better way to phrase this be: "tests that can only pass are
a waste"? Or maybe, "tests that don't provide useful information on a
failure are a waste"?

> So key thing when considering if adding a test is beneficial is whether
> the test is likely to point out a bug (in early phase). A bug that
> would otherwise have gone through unnoticed.

Why not a bug later on? On another project, I noticed a piece of code
that kept breaking, so I added a test for that piece of code, and
people stopped breaking it: mission accomplished, right?

>
> > I do think that these tests are unlikely to increase the burden on
> > developers much:
>
> All code which uses kernel APIs is increasing burden for someone who
> needs to change the API. Much can be discussed ;)
>
> >  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,
>
> This statement is valid for almost all exported kernel APIs - yet
> adding tests for all APIs is definitely a bad idea. The effort is

I don't understand, can you elaborate? From my experience, APIs are
frequently one of the most valuable places to test.

> cumulative. We should never add new code just because maintenance
> effort is relatively small. We must only add code if it adds value.

I agree with this statement, but I don't see how it shows that API
testing is inherently bad. I know you are countering David's statement
that it doesn't add much cost; however, I think that API testing not
only doesn't add much cost, if done properly, I think it actually
reduces future effort in changing those APIs.

> >  and may ultimately make
> > things a bit simpler by helping to document and verify the changes.
>
> Yes. Definitely. But we must never deny that all added (test) code adds
> work. And do weighing pros an cons only after that. Danger of UTs is
> that we don't admit the cons.

True. There is no magic bullet here. UTs are but one tool in a
toolbox; there are other tools better suited to solve some problems,
and no problem is likely to be solved well if the person using the
tool uses the tool improperly. I don't think anyone here thinks that
UTs are inherently good and all UTs make all code better.

> > 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@google.com>
> > > > Cc: <kunit-dev@googlegroups.com>
> > > > Signed-off-by: Stephen Boyd <sboyd@kernel.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,
>
> My point exactly.

I honestly don't know. TBH, I think I have a good idea what a good
test case should look like, as for measuring how useful a test case
is, that's harder and I am not sure anyone really has a great answer
for this. I just read a literature review on measuring code quality
and they concluded that coverage provided only a weak signal at best
and the best signal came from "engineers' perception of code quality".

The way I usually write tests is: I write my code first. I then write
some unit tests to exercise some basic features of my code. Then I
write some integration or end-to-end tests to make sure my code "fits"
in the job it's supposed to do. If everything works, I am done. If
not, then I start writing tests to narrow down what doesn't work the
way I expect. Eventually I write a test that clearly exposes the
issue. I fix the issue, and I rerun the tests to make sure they flip
from fail to pass. I then look through all the new tests I wrote and
try to make a judgement call about how useful I think the test will be
in the future. (I am curious what other people's thoughts are on this
approach.)

When it comes down to it, I don't know this code base well enough to
know whether all these registration tests are useful, but I think, as
they are written, they are easy to understand, and that is what I was
reviewing.

This is all a very long way of saying: "I think this is a question for
Stephen" :-)

> >  but there's also not much of a cost to having them
>
> And this is the other side of the equation. And I leave this estimation
> to others - I just want to point out that imbalance in this equation is
> what usually leads to writing excessive amounts of tests - which
> eventually just hinders development with little or no benefits. But as
> I said, I will leave this to smarter guys to evaluate. Just wanted to
> point out that this should be considered :)

Fair enough. And I think that's great! Unit testing is "new" to the
Linux kernel and I want to make sure we do it the right way. From my
experience, I think the best way to bring about a "new" thing like
this is to get someone who is overly liberal in adopting the new
thing, someone who is more conservative, and maybe some invested, but
less opinionated bystanders and make them all try to agree.

Once we have debated over a number of tests, it should start getting
easier. Hopefully, a good pattern will emerge and people will start
following that pattern.

> > — 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.
>
> The disable test already does the enable. Just adding another set of
> state checks before disable would verify whole thing, right? So, if I
> am not mistaken then - by minimum - these could be tested at one go.
> (If being beneficial at all).

True, but if enable is *super prone to breakage* then having a test
that tells you whether you broke it might still be more useful than
not. Stephen, thoughts?

> > > > +
> > > > +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).
>
> I think this was more of my point than trying to demand actual changes
> here. Thank you for considering this! :) It is important to admit that
> _all_ code, whether test or implementation, require work and
> maintenance and slow down development. We should _always_ consider if
> adding a test is actually needed or if we could do it so that code
> change require less test changes.

Agreed.

> If I had all the power in the world I would never add a test for code
> which is clean and simple. It is likely to not be beneficial. I would

Probably semantics, but I would argue what is clean and simple to one
person is often not to someone else. A test at least gives you an
isolated context to test your assumptions.

> mostly utilize UTs at spots where the logic is somewhat complex and
> making a bug is likely. UTs do great job for example at verifying data
> parsing functions (I've voluntarily tested bunch of netlink message
> parsing code with UTs in some projects. I also added tests for
> linear_ranges helpers in series I recently submitted.) But UTs are more
> of a waste when added carelessly. I hope the UT documentation would
> also cover that front.
>
> Thanks for open discussion!

Thank *you*! It is good to have someone challenging our ideas. I get
worried when I don't have anyone questioning what I am doing: being
questioned is the only way that I can feel confident that what I am
doing is right. And in this case, I think you have leveled a number of
good points.

Cheers!
Matti Vaittinen May 5, 2020, 6:15 a.m. UTC | #7
On Mon, 2020-05-04 at 13:19 -0700, Brendan Higgins wrote:
> On Sun, May 3, 2020 at 10:54 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Wed, 2020-04-29 at 12:15 +0800, David Gow wrote:
> > > On Tue, Apr 14, 2020 at 7:46 PM Vaittinen, Matti
> > > <Matti.Vaittinen@fi.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.
> > 
> > I appreciate your reply. And I appreciate your (and others) work on
> > KUinit. I don't think UTs are bad. I believe UTs are great and
> > powerfull tool - when used on specific occasions. But UTs
> > definitely
> > are "double-edged sword" - you can hit your own leg.
> 
> Sure, I saw you just added a unit test, so I would assume that you
> don't think that UTs are inherently bad :-)
> 
> > > 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.
> > 
> > Fixing and identifying bugs is definitely "the thing" in UTs. But
> > what
> > comes to weighing the benefits of UTs Vs. downsides - that's hard.
> > First thing on that front is to understand the cost of UTs. And in
> > my
> > experience - many people underestimate that. It's easy too see
> > things
> > black & white and think UTs are only good - which is not true.
> > 
> > UT cases are code which must be maintained as any code. And we must
> > never add code which is not beneficial as maintaining it is work.
> > We do
> > constantly work to decrease amount of code to be maintaned - UTs
> > are no
> > exception - all code is a burden. Unnecessary code is burden with
> > no
> > purpose. And UTs have no other purpose but to point out mistakes.
> > Only
> 
> I am in agreement with everything you said so far.
> 
> > good test is a test that is failing and pointing out a bug which
> > would
> > have otherwise gone unnoticed. Passing test is a waste.
> 
> Not to be uncharitable in how I read this, but I don't think this is
> quite right. I think we would all agree that a test that *cannot*
> fail
> is a waste, but on the other hand, you should never submit code that
> doesn't pass *good tests*, right? Like you wrote tests for your
> linear
> range stuff to verify that it worked as expected; I don't know if
> some
> of the tests didn't pass on the first try, but you did ultimately fix
> all the issues, and submitted only tests that pass, right? I am
> pretty
> sure no one would accept code accompanied with tests that don't pass.
> 
> So would a better way to phrase this be: "tests that can only pass
> are
> a waste"? Or maybe, "tests that don't provide useful information on a
> failure are a waste"?

What I mean is really that a test that is not failing is a waste. To
make it more accurate though - a test that is not failing at some point
during development is a waste.

> > So key thing when considering if adding a test is beneficial is
> > whether
> > the test is likely to point out a bug (in early phase). A bug that
> > would otherwise have gone through unnoticed.
> 
> Why not a bug later on? On another project, I noticed a piece of code
> that kept breaking, so I added a test for that piece of code, and
> people stopped breaking it: mission accomplished, right?

You are right. "Later on" is fine :) As is "on another project".

> > > I do think that these tests are unlikely to increase the burden
> > > on
> > > developers much:
> > 
> > All code which uses kernel APIs is increasing burden for someone
> > who
> > needs to change the API. Much can be discussed ;)
> > 
> > >  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,
> > 
> > This statement is valid for almost all exported kernel APIs - yet
> > adding tests for all APIs is definitely a bad idea. The effort is
> 
> I don't understand, can you elaborate? From my experience, APIs are
> frequently one of the most valuable places to test.

Yes. APIs are what we should focus. But not _all_ APIs. APIs to test
should be carefully selected. 


1. I would not add tests to APIs which have simple implementation. I
know you said "simple" is subjective. I'll do some pseudo code to
elaborate:

I think we can agree that for example APIs like

bool is_foo(sometype maybefoo) {
    return !!maybefoo.foo;
}

are simple and not likely to break. Adding test

sometype yesfoo = { .foo = 1 };
sometype nofoo = { .foo = 0 };
footest()
{
	TEST_ASSERT(is_foo(yesfoo));
	TEST_ASSERT(!is_foo(nofoo));
}

is unlikely to bring any value - but is likely to cause extra work when
sometype is replaced with new cooler construct - or foo is renamed or
anything else changes this.

This is just an extreme example - from my perspective the amount of
work required to write and maintain a test will easily exceed the
benefits even for _much_ more complex constructs. But as you said,
"simplicity" is subjective and I need to trust the maintainers to
understand where to draw the line :)

Another example of APIs for which I would omit tests are APIs whose
functionality is tested by some other tests. If we stick to clk domain
we can look following example:

Clk has few 'bulk' APIs that repeat some operation for many clk
entities.

https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/clk/clk-bulk.c

Let's assume someone wrote test(s) for:
clk_bulk_put():

void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
{
	while (--num_clks >= 0) {
		clk_put(clks[num_clks].clk);
		clks[num_clks].clk = NULL;
	}
}

Then I'd argue that writing tests for clk_put() were not bringing much
of additional value.

So - I would straight away drop at least the tests for simple APIs and
APIs that are tested by other tests.

> 
> > cumulative. We should never add new code just because maintenance
> > effort is relatively small. We must only add code if it adds value.
> 
> I agree with this statement, but I don't see how it shows that API
> testing is inherently bad.

My intention was not to say API testing is inherently bad. My purpose
was to say that not _all_ APIs should be tested - even though all
exported APIs usually fulfill the David's criteria of being also used
in many other places but tests. So, I was just saying that we should
definitely drop the "this is used by other code so adding test doesn't
matter" - argument. That argument leads to the dark side :]

>  I know you are countering David's statement
> that it doesn't add much cost; however, I think that API testing not
> only doesn't add much cost, if done properly, I think it actually
> reduces future effort in changing those APIs.

Yes. I agree. A good test can reduce effort when it is written for a
complex, error prone implementation. But for many APIs the existing
test load would (in my opinion) just hinder the changes.

> 
> > >  and may ultimately make
> > > things a bit simpler by helping to document and verify the
> > > changes.
> > 
> > Yes. Definitely. But we must never deny that all added (test) code
> > adds
> > work. And do weighing pros an cons only after that. Danger of UTs
> > is
> > that we don't admit the cons.
> 
> True. There is no magic bullet here. UTs are but one tool in a
> toolbox; there are other tools better suited to solve some problems,
> and no problem is likely to be solved well if the person using the
> tool uses the tool improperly. I don't think anyone here thinks that
> UTs are inherently good and all UTs make all code better.

You optimist :p XD

> > > 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@google.com>
> > > > > Cc: <kunit-dev@googlegroups.com>
> > > > > Signed-off-by: Stephen Boyd <sboyd@kernel.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,
> > 
> > My point exactly.
> 
> I honestly don't know. TBH, I think I have a good idea what a good
> test case should look like, as for measuring how useful a test case
> is, that's harder and I am not sure anyone really has a great answer
> for this. I just read a literature review on measuring code quality
> and they concluded that coverage provided only a weak signal at best
> and the best signal came from "engineers' perception of code
> quality".
> 
> The way I usually write tests is: I write my code first. I then write
> some unit tests to exercise some basic features of my code. Then I
> write some integration or end-to-end tests to make sure my code
> "fits"
> in the job it's supposed to do. If everything works, I am done. If
> not, then I start writing tests to narrow down what doesn't work the
> way I expect. Eventually I write a test that clearly exposes the
> issue. I fix the issue, and I rerun the tests to make sure they flip
> from fail to pass. I then look through all the new tests I wrote and
> try to make a judgement call about how useful I think the test will
> be
> in the future. (I am curious what other people's thoughts are on this
> approach.)

I like this approach. I know colleagues who claim that writing the test
first is the only way to go. I also know colleagues who rely only on
UTs at some simulated environment. And I know colleagues who hardly
test at all.

And I usually have an opinion on the colleagues as engineers. I know
what to think when I ask from myself whether I would hire this or that
colleague for my own company or take him/her on my project. (And for
all of the jealous persons or persons who started to think how to
utilize my money and position: No - I don't have a company, I am not
rich and I am not in a position where I pick co-workers. So just forget
it. XD ). And I must say that their preferred way of testing does not
correlate with this. So I'd say there is no single good approach to
make you a better engineer.

I do also write the code first. Then I ran it in it's real environment
and do some tests which verify the intended functionality. At that
phase I usually fix the code. Then I evaluate the complexity and add
UTs for portions which I think are error prone. And after the
release/submission I try to address the questions/error reports
quickly.

> When it comes down to it, I don't know this code base well enough to
> know whether all these registration tests are useful, but I think, as
> they are written, they are easy to understand, and that is what I was
> reviewing.

I think we can't demand more from you. I hope someone will be there
questioning if tests are usefull or could be shrinked - but I don't
think we can demand that from you. Problem is that many people see test
code as a secondary, less interesting thing and tend to skip reviewing
it - but as I said in many places - test code has potentially huge
impact to code maintenance. Hence we do need experts of that particular
area to pay attention on test code too. And I think I have now done my
share on trying to ensure it happens. Raised awareness is the term. And
no doubt many people think I've now done much more than I should :p

> 
> This is all a very long way of saying: "I think this is a question
> for
> Stephen" :-)

Yes. I think Stephen is perfectly capable of deciding if tests are good
as they were or if he thinks some of my comments were relevant.

> 
> > >  but there's also not much of a cost to having them
> > 
> > And this is the other side of the equation. And I leave this
> > estimation
> > to others - I just want to point out that imbalance in this
> > equation is
> > what usually leads to writing excessive amounts of tests - which
> > eventually just hinders development with little or no benefits. But
> > as
> > I said, I will leave this to smarter guys to evaluate. Just wanted
> > to
> > point out that this should be considered :)
> 
> Fair enough. And I think that's great! Unit testing is "new" to the
> Linux kernel and I want to make sure we do it the right way. From my
> experience, I think the best way to bring about a "new" thing like
> this is to get someone who is overly liberal in adopting the new
> thing, someone who is more conservative, and maybe some invested, but
> less opinionated bystanders and make them all try to agree.

You didn't write overly conservative, thanks for that ;) (I'm using vim
not vi after all. And that's released at 90's which makes me pretty
modern, right?)

> Once we have debated over a number of tests, it should start getting
> easier. Hopefully, a good pattern will emerge and people will start
> following that pattern.

I like your view :) Well said.

Best Regards
    --Matti

Patch
diff mbox series

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));
+}
+
+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));
+}
+
+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);
+}
+
+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));
+}
+
+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;
+}
+
+static struct kunit_suite clk_gate_test_invert_suite = {
+	.name = "clk-gate-invert-test",
+	.init = clk_gate_test_invert_init,
+	.exit = clk_gate_test_exit,
+	.test_cases = clk_gate_test_invert_cases,
+};
+
+static void clk_gate_test_hiword_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(9) | BIT(9 + 16);
+
+	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));
+}
+
+static void clk_gate_test_hiword_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(9) | BIT(9 + 16);
+	u32 disable_val = BIT(9 + 16);
+
+	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_hiword_cases[] = {
+	KUNIT_CASE(clk_gate_test_hiword_enable),
+	KUNIT_CASE(clk_gate_test_hiword_disable),
+	{}
+};
+
+static int clk_gate_test_hiword_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, 9,
+					    CLK_GATE_HIWORD_MASK, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+
+	ctx->hw = hw;
+	ctx->parent = parent;
+
+	return 0;
+}
+
+static struct kunit_suite clk_gate_test_hiword_suite = {
+	.name = "clk-gate-hiword-test",
+	.init = clk_gate_test_hiword_init,
+	.exit = clk_gate_test_exit,
+	.test_cases = clk_gate_test_hiword_cases,
+};
+
+static void clk_gate_test_is_enabled(struct kunit *test)
+{
+	struct clk_hw *hw;
+	struct clk_gate_test_context *ctx;
+
+	ctx = clk_gate_test_alloc_ctx(test);
+	ctx->fake_reg = BIT(7);
+	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
+				  0, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+	KUNIT_ASSERT_TRUE(test, clk_hw_is_enabled(hw));
+
+	clk_hw_unregister_gate(hw);
+	kfree(ctx);
+}
+
+static void clk_gate_test_is_disabled(struct kunit *test)
+{
+	struct clk_hw *hw;
+	struct clk_gate_test_context *ctx;
+
+	ctx = clk_gate_test_alloc_ctx(test);
+	ctx->fake_reg = BIT(4);
+	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
+				  0, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+	KUNIT_ASSERT_FALSE(test, clk_hw_is_enabled(hw));
+
+	clk_hw_unregister_gate(hw);
+	kfree(ctx);
+}
+
+static void clk_gate_test_is_enabled_inverted(struct kunit *test)
+{
+	struct clk_hw *hw;
+	struct clk_gate_test_context *ctx;
+
+	ctx = clk_gate_test_alloc_ctx(test);
+	ctx->fake_reg = BIT(31);
+	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 2,
+				  CLK_GATE_SET_TO_DISABLE, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+	KUNIT_ASSERT_TRUE(test, clk_hw_is_enabled(hw));
+
+	clk_hw_unregister_gate(hw);
+	kfree(ctx);
+}
+
+static void clk_gate_test_is_disabled_inverted(struct kunit *test)
+{
+	struct clk_hw *hw;
+	struct clk_gate_test_context *ctx;
+
+	ctx = clk_gate_test_alloc_ctx(test);
+	ctx->fake_reg = BIT(29);
+	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 29,
+				  CLK_GATE_SET_TO_DISABLE, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+	KUNIT_ASSERT_FALSE(test, clk_hw_is_enabled(hw));
+
+	clk_hw_unregister_gate(hw);
+	kfree(ctx);
+}
+
+static struct kunit_case clk_gate_test_enabled_cases[] = {
+	KUNIT_CASE(clk_gate_test_is_enabled),
+	KUNIT_CASE(clk_gate_test_is_disabled),
+	KUNIT_CASE(clk_gate_test_is_enabled_inverted),
+	KUNIT_CASE(clk_gate_test_is_disabled_inverted),
+	{}
+};
+
+static struct kunit_suite clk_gate_test_enabled_suite = {
+	.name = "clk-gate-is_enabled-test",
+	.test_cases = clk_gate_test_enabled_cases,
+};
+
+kunit_test_suites(
+	&clk_gate_register_test_suite,
+	&clk_gate_test_suite,
+	&clk_gate_test_invert_suite,
+	&clk_gate_test_hiword_suite,
+	&clk_gate_test_enabled_suite
+);
+MODULE_LICENSE("GPL v2");