Re: [PATCH v5 11/11] clk: Add KUnit tests for clks registered with struct clk_parent_data

[Stephen Boyd <sboyd@kernel.org>]

Quoting David Gow (2024-06-13 00:56:08)
> On Tue, 4 Jun 2024 at 06:38, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Test that clks registered with 'struct clk_parent_data' work as
> > intended and can find their parents.
> >
> > Cc: Christian Marangi <ansuelsmth@gmail.com>
> > Cc: Brendan Higgins <brendan.higgins@linux.dev>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Rae Moar <rmoar@google.com>
> > Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> 
> This seems good to me overall, but will break if we can't compile the
> dtbo.o file. Maybe these need to live behind a  #if
> IS_ENABLED(CONFIG_OF) or equivalent.
> 
> Also, there's a cast to kunit_action_t* which needs to use a wrapper.
> 
> Otherwise,
> Reviewed-by: David Gow <davidgow@google.com>
> 
> Cheers,
> -- David
> 
> >  drivers/clk/Kconfig                         |   1 +
> >  drivers/clk/Makefile                        |   3 +-
> >  drivers/clk/clk_parent_data_test.h          |  10 +
> >  drivers/clk/clk_test.c                      | 451 +++++++++++++++++++-
> >  drivers/clk/kunit_clk_parent_data_test.dtso |  28 ++
> >  5 files changed, 491 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/clk/clk_parent_data_test.h
> >  create mode 100644 drivers/clk/kunit_clk_parent_data_test.dtso
> >
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index f649f2a0279c..c33fdf9fdcd6 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -508,6 +508,7 @@ config CLK_KUNIT_TEST
> >         tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS
> >         depends on KUNIT
> >         default KUNIT_ALL_TESTS
> > +       select OF_OVERLAY if OF
> >         help
> >           Kunit tests for the common clock framework.
> >
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 7b57e3d22cee..ed4e1a0e6943 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -2,7 +2,8 @@
> >  # common clock types
> >  obj-$(CONFIG_HAVE_CLK)         += clk-devres.o clk-bulk.o clkdev.o
> >  obj-$(CONFIG_COMMON_CLK)       += clk.o
> > -obj-$(CONFIG_CLK_KUNIT_TEST)   += clk_test.o
> > +obj-$(CONFIG_CLK_KUNIT_TEST)   += clk_test.o \
> > +                                  kunit_clk_parent_data_test.dtbo.o
> 
> This breaks if CONFIG_OF isn't enabled, as there's no rule to compile it:
> make[5]: *** No rule to make target
> 'drivers/clk/kunit_clk_parent_data_test.dtbo.o', needed by
> 'drivers/clk/modules.order'.  Stop.
> 

Ah, I see that I need to set CONFIG_DTC or the DT compiler (dtc) won't
be built. Maybe I should just select OF_OVERLAY instead of being nice
and letting OF be disabled? The problem is that I can't test the
CONFIG_OF=n case easily then.

For now I'll go with 'select DTC', but it really feels like we should
just get rid of that Kconfig and build 'dtc' if it is needed.

> > diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> > index 39e2b5ff4f51..bdf3c4bb2243 100644
> > --- a/drivers/clk/clk_test.c
> > +++ b/drivers/clk/clk_test.c
> > @@ -4,12 +4,19 @@
> >   */
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> >
> >  /* Needed for clk_hw_get_clk() */
> >  #include "clk.h"
[...]
> > +
> > +/**
> > + * struct clk_register_clk_parent_data_of_ctx - Context for clk_parent_data OF tests
> > + * @np: device node of clk under test
> > + * @hw: clk_hw for clk under test
> > + */
> > +struct clk_register_clk_parent_data_of_ctx {
> > +       struct device_node *np;
> > +       struct clk_hw hw;
> > +};
> > +
> > +static int clk_register_clk_parent_data_of_test_init(struct kunit *test)
> > +{
> > +       struct clk_register_clk_parent_data_of_ctx *ctx;
> > +
> > +       KUNIT_ASSERT_EQ(test, 0,
> > +                       of_overlay_apply_kunit(test, kunit_clk_parent_data_test));
> > +
> > +       ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > +       if (!ctx)
> > +               return -ENOMEM;
> > +       test->priv = ctx;
> > +
> > +       ctx->np = of_find_compatible_node(NULL, NULL, "test,clk-parent-data");
> > +       if (!ctx->np)
> > +               return -ENODEV;
> > +
> > +       return kunit_add_action_or_reset(test, (kunit_action_t *)&of_node_put, ctx->np);
> 
> We should use an action wrapper here (KUNIT_DEFINE_ACTION_WRAPPER()),
> as casting function pointers to kunit_action_t* breaks control-flow
> integrity.

Got it, thanks. Maybe there should be an of_node_put_kunit_exit() helper that
does that and can be used anywhere.