Quoting David Gow (2024-06-13 00:56:08)
> On Tue, 4 Jun 2024 at 06:38, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> >
> > Test that clks registered with 'struct clk_parent_data' work as
> > intended and can find their parents.
> >
> > Cc: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > Cc: Brendan Higgins <brendan.higgins@xxxxxxxxx>
> > Cc: David Gow <davidgow@xxxxxxxxxx>
> > Cc: Rae Moar <rmoar@xxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxx>
>
> 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.
