drivers/clk/clk.c | 11 +++++++ drivers/clk/clk.h | 4 +++ drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 1 deletion(-)
Clk lookup (by name) recently gained some performance improvements at
the expense of more complexity within the lookup code.
To make sure that this works as intended and doesn't break, add some
basic tests for this part of the CCF.
A new "clk_hw_lookup()" function is added purely for running kunit
tests.
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
drivers/clk/clk.c | 11 +++++++
drivers/clk/clk.h | 4 +++
drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++-
3 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 85d2f2481acf..a17d0070d11f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -778,6 +778,17 @@ struct clk *__clk_lookup(const char *name)
return !core ? NULL : core->hw->clk;
}
+#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
+/* This is only provided for kunit tests to test the core lookup functions. */
+struct clk_hw *clk_hw_lookup(const char *name)
+{
+ struct clk_core *core = clk_core_lookup(name);
+
+ return !core ? NULL : core->hw;
+}
+EXPORT_SYMBOL_GPL(clk_hw_lookup);
+#endif
+
static void clk_core_get_boundaries(struct clk_core *core,
unsigned long *min_rate,
unsigned long *max_rate)
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 2d801900cad5..a8ed54f5b572 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -8,6 +8,10 @@ struct clk_hw;
struct device;
struct of_phandle_args;
+#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
+struct clk_hw *clk_hw_lookup(const char *name);
+#endif
+
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
struct clk_hw *of_clk_get_hw(struct device_node *np,
int index, const char *con_id);
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a268d7b5d4cb..b3b5ce0ad897 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -175,6 +175,8 @@ static const struct clk_ops clk_multiple_parents_no_reparent_mux_ops = {
.set_parent = clk_multiple_parents_mux_set_parent,
};
+#define DUMMY_CLK_NAME "test_dummy_rate"
+
static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
{
struct clk_dummy_context *ctx;
@@ -187,7 +189,7 @@ static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
ctx->rate = DUMMY_CLOCK_INIT_RATE;
test->priv = ctx;
- init.name = "test_dummy_rate";
+ init.name = DUMMY_CLK_NAME;
init.ops = ops;
ctx->hw.init = &init;
@@ -3541,6 +3543,67 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
.test_cases = clk_hw_get_dev_of_node_test_cases,
};
+/*
+ * Test that clk lookup with a name that is not registered returns NULL.
+ */
+static void clk_lookup_not_registered_clk_returns_NULL(struct kunit *test)
+{
+ KUNIT_EXPECT_PTR_EQ(test, NULL, clk_hw_lookup(DUMMY_CLK_NAME));
+}
+
+/*
+ * Test that clk lookup with a name that is registered returns the clk.
+ */
+static void clk_lookup_registered_clk_returns_clk(struct kunit *test)
+{
+ struct clk_hw *hw;
+ struct clk_init_data init = {
+ .name = DUMMY_CLK_NAME,
+ .ops = &empty_clk_ops,
+ };
+
+ hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+
+ hw->init = &init;
+ KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, hw));
+
+ KUNIT_EXPECT_PTR_EQ(test, hw, clk_hw_lookup(DUMMY_CLK_NAME));
+}
+
+/*
+ * Test that clk lookup with a name that was unregistered returns NULL.
+ */
+static void clk_lookup_unregistered_clk_returns_NULL(struct kunit *test)
+{
+ struct clk_hw *hw;
+ struct clk_init_data init = {
+ .name = DUMMY_CLK_NAME,
+ .ops = &empty_clk_ops,
+ };
+
+ hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
+
+ hw->init = &init;
+ KUNIT_ASSERT_FALSE(test, clk_hw_register(NULL, hw));
+
+ clk_hw_unregister(hw);
+
+ KUNIT_EXPECT_PTR_EQ(test, NULL, clk_hw_lookup(DUMMY_CLK_NAME));
+}
+
+static struct kunit_case clk_lookup_test_cases[] = {
+ KUNIT_CASE(clk_lookup_not_registered_clk_returns_NULL),
+ KUNIT_CASE(clk_lookup_registered_clk_returns_clk),
+ KUNIT_CASE(clk_lookup_unregistered_clk_returns_NULL),
+ {}
+};
+
+static struct kunit_suite clk_lookup_test_suite = {
+ .name = "clk-lookup",
+ .test_cases = clk_lookup_test_cases,
+};
kunit_test_suites(
&clk_assigned_rates_suite,
@@ -3560,6 +3623,7 @@ kunit_test_suites(
&clk_register_clk_parent_data_device_suite,
&clk_single_parent_mux_test_suite,
&clk_uncached_test_suite,
+ &clk_lookup_test_suite,
);
MODULE_DESCRIPTION("Kunit tests for clk framework");
MODULE_LICENSE("GPL v2");
--
2.51.0.618.g983fd99d29-googQuoting Chen-Yu Tsai (2025-10-02 02:20:35)
> Clk lookup (by name) recently gained some performance improvements at
> the expense of more complexity within the lookup code.
>
> To make sure that this works as intended and doesn't break, add some
> basic tests for this part of the CCF.
>
> A new "clk_hw_lookup()" function is added purely for running kunit
> tests.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/clk/clk.c | 11 +++++++
> drivers/clk/clk.h | 4 +++
> drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..a17d0070d11f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -778,6 +778,17 @@ struct clk *__clk_lookup(const char *name)
> return !core ? NULL : core->hw->clk;
> }
>
> +#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
> +/* This is only provided for kunit tests to test the core lookup functions. */
> +struct clk_hw *clk_hw_lookup(const char *name)
> +{
> + struct clk_core *core = clk_core_lookup(name);
> +
> + return !core ? NULL : core->hw;
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_lookup);
> +#endif
I think we can't do this ifdef because we may have some sort of build
that builds the kunit module after the kernel is built with a different
configuration. Just do the module import thing instead. I hope that
avoids this problem.
> +
> static void clk_core_get_boundaries(struct clk_core *core,
> unsigned long *min_rate,
> unsigned long *max_rate)
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index a268d7b5d4cb..b3b5ce0ad897 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -175,6 +175,8 @@ static const struct clk_ops clk_multiple_parents_no_reparent_mux_ops = {
> .set_parent = clk_multiple_parents_mux_set_parent,
> };
>
> +#define DUMMY_CLK_NAME "test_dummy_rate"
> +
> static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
> {
> struct clk_dummy_context *ctx;
> @@ -187,7 +189,7 @@ static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
> ctx->rate = DUMMY_CLOCK_INIT_RATE;
> test->priv = ctx;
>
> - init.name = "test_dummy_rate";
> + init.name = DUMMY_CLK_NAME;
> init.ops = ops;
> ctx->hw.init = &init;
>
Please don't change existing tests in the same patch as introducing
other tests. A failure in the existing test is difficult to untangle
from the new test. Tests shouldn't be changed after they're written
either because we have to be careful that they're still correct when we
don't have tests for the tests.
> @@ -3541,6 +3543,67 @@ static struct kunit_suite clk_hw_get_dev_of_node_test_suite = {
> .test_cases = clk_hw_get_dev_of_node_test_cases,
> };
>
> +/*
> + * Test that clk lookup with a name that is not registered returns NULL.
> + */
> +static void clk_lookup_not_registered_clk_returns_NULL(struct kunit *test)
> +{
> + KUNIT_EXPECT_PTR_EQ(test, NULL, clk_hw_lookup(DUMMY_CLK_NAME));
Just write
KUNIT_EXPECT_PTR_EQ(test, NULL, clk_hw_lookup("not_registered"));
so we don't have to look at what the definition of DUMMY_CLK_NAME is.
> +}
> +
> +/*
> + * Test that clk lookup with a name that is registered returns the clk.
> + */
> +static void clk_lookup_registered_clk_returns_clk(struct kunit *test)
> +{
> + struct clk_hw *hw;
> + struct clk_init_data init = {
> + .name = DUMMY_CLK_NAME,
> + .ops = &empty_clk_ops,
> + };
Please do
const char *clk_name = "valid_clk";
struct clk_init_data init = {
.name = clk_name,
.ops = &empty_clk_ops,
};
> +
> + hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +
> + hw->init = &init;
> + KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, hw));
> +
> + KUNIT_EXPECT_PTR_EQ(test, hw, clk_hw_lookup(DUMMY_CLK_NAME));
And
KUNIT_EXPECT_PTR_EQ(test, hw, clk_hw_lookup(clk_name));
so that the name is fully self contained to this function.
> +}
> +
> +/*
> + * Test that clk lookup with a name that was unregistered returns NULL.
> + */
> +static void clk_lookup_unregistered_clk_returns_NULL(struct kunit *test)
> +{
> + struct clk_hw *hw;
> + struct clk_init_data init = {
> + .name = DUMMY_CLK_NAME,
> + .ops = &empty_clk_ops,
> + };
> +
> + hw = kunit_kzalloc(test, sizeof(*hw), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> +
> + hw->init = &init;
> + KUNIT_ASSERT_FALSE(test, clk_hw_register(NULL, hw));
Use KUNIT_ASSERT_EQ instead because clk_hw_register() doesn't return a
bool. Also, use the kunit registration function clk_hw_register_kunit()
to simplify the unregistration path which is missing here.
> +
> + clk_hw_unregister(hw);
Before we unregister here we should assert that clk_hw_lookup() returns
non-NULL.
> +
> + KUNIT_EXPECT_PTR_EQ(test, NULL, clk_hw_lookup(DUMMY_CLK_NAME));
Same name comment applies here as well.
> +}
> +
> +static struct kunit_case clk_lookup_test_cases[] = {
> + KUNIT_CASE(clk_lookup_not_registered_clk_returns_NULL),
> + KUNIT_CASE(clk_lookup_registered_clk_returns_clk),
> + KUNIT_CASE(clk_lookup_unregistered_clk_returns_NULL),
> + {}
> +};
> +
> +static struct kunit_suite clk_lookup_test_suite = {
> + .name = "clk-lookup",
> + .test_cases = clk_lookup_test_cases,
> +};
>
> kunit_test_suites(
> &clk_assigned_rates_suite,
> @@ -3560,6 +3623,7 @@ kunit_test_suites(
> &clk_register_clk_parent_data_device_suite,
> &clk_single_parent_mux_test_suite,
> &clk_uncached_test_suite,
> + &clk_lookup_test_suite,
Please keep this alphabetically sorted.
> );
> MODULE_DESCRIPTION("Kunit tests for clk framework");
> MODULE_LICENSE("GPL v2");
On Thu, Oct 02, 2025 at 05:20:35PM +0800, Chen-Yu Tsai wrote:
> Clk lookup (by name) recently gained some performance improvements at
> the expense of more complexity within the lookup code.
>
> To make sure that this works as intended and doesn't break, add some
> basic tests for this part of the CCF.
>
> A new "clk_hw_lookup()" function is added purely for running kunit
> tests.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> drivers/clk/clk.c | 11 +++++++
> drivers/clk/clk.h | 4 +++
> drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..a17d0070d11f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -778,6 +778,17 @@ struct clk *__clk_lookup(const char *name)
> return !core ? NULL : core->hw->clk;
> }
>
> +#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
> +/* This is only provided for kunit tests to test the core lookup functions. */
> +struct clk_hw *clk_hw_lookup(const char *name)
> +{
> + struct clk_core *core = clk_core_lookup(name);
> +
> + return !core ? NULL : core->hw;
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_lookup);
> +#endif
Use EXPORT_SYMBOL_IF_KUNIT instead for consistency with the rest of the
kernel. In clk_test.c, you'll also need to add:
MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
Since clk_hw_lookup() is only used by kunit, why not just put this new
function in clk-test.c, and use EXPORT_SYMBOL_IF_KUNIT on
clk_core_lookup?
Brian
On Thu, Oct 9, 2025 at 7:58 AM Brian Masney <bmasney@redhat.com> wrote:
>
> On Thu, Oct 02, 2025 at 05:20:35PM +0800, Chen-Yu Tsai wrote:
> > Clk lookup (by name) recently gained some performance improvements at
> > the expense of more complexity within the lookup code.
> >
> > To make sure that this works as intended and doesn't break, add some
> > basic tests for this part of the CCF.
> >
> > A new "clk_hw_lookup()" function is added purely for running kunit
> > tests.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > drivers/clk/clk.c | 11 +++++++
> > drivers/clk/clk.h | 4 +++
> > drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 85d2f2481acf..a17d0070d11f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -778,6 +778,17 @@ struct clk *__clk_lookup(const char *name)
> > return !core ? NULL : core->hw->clk;
> > }
> >
> > +#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
> > +/* This is only provided for kunit tests to test the core lookup functions. */
> > +struct clk_hw *clk_hw_lookup(const char *name)
> > +{
> > + struct clk_core *core = clk_core_lookup(name);
> > +
> > + return !core ? NULL : core->hw;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_hw_lookup);
> > +#endif
>
> Use EXPORT_SYMBOL_IF_KUNIT instead for consistency with the rest of the
> kernel. In clk_test.c, you'll also need to add:
>
> MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
Didn't know about this one. Thanks!
> Since clk_hw_lookup() is only used by kunit, why not just put this new
> function in clk-test.c, and use EXPORT_SYMBOL_IF_KUNIT on
> clk_core_lookup?
Then we end up sort of exposing clk_core_lookup as well?
I believe Stephen wants to keep things contained as much as possible.
ChenYu
On Thu, Oct 09, 2025 at 11:24:22AM +0800, Chen-Yu Tsai wrote:
> On Thu, Oct 9, 2025 at 7:58 AM Brian Masney <bmasney@redhat.com> wrote:
> >
> > On Thu, Oct 02, 2025 at 05:20:35PM +0800, Chen-Yu Tsai wrote:
> > > Clk lookup (by name) recently gained some performance improvements at
> > > the expense of more complexity within the lookup code.
> > >
> > > To make sure that this works as intended and doesn't break, add some
> > > basic tests for this part of the CCF.
> > >
> > > A new "clk_hw_lookup()" function is added purely for running kunit
> > > tests.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > drivers/clk/clk.c | 11 +++++++
> > > drivers/clk/clk.h | 4 +++
> > > drivers/clk/clk_test.c | 66 +++++++++++++++++++++++++++++++++++++++++-
> > > 3 files changed, 80 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 85d2f2481acf..a17d0070d11f 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -778,6 +778,17 @@ struct clk *__clk_lookup(const char *name)
> > > return !core ? NULL : core->hw->clk;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_CLK_KUNIT_TEST)
> > > +/* This is only provided for kunit tests to test the core lookup functions. */
> > > +struct clk_hw *clk_hw_lookup(const char *name)
> > > +{
> > > + struct clk_core *core = clk_core_lookup(name);
> > > +
> > > + return !core ? NULL : core->hw;
> > > +}
> > > +EXPORT_SYMBOL_GPL(clk_hw_lookup);
> > > +#endif
> >
> > Use EXPORT_SYMBOL_IF_KUNIT instead for consistency with the rest of the
> > kernel. In clk_test.c, you'll also need to add:
> >
> > MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
>
> Didn't know about this one. Thanks!
>
> > Since clk_hw_lookup() is only used by kunit, why not just put this new
> > function in clk-test.c, and use EXPORT_SYMBOL_IF_KUNIT on
> > clk_core_lookup?
>
> Then we end up sort of exposing clk_core_lookup as well?
>
> I believe Stephen wants to keep things contained as much as possible.
I agree about keeping things contained as much as possible as well.
clk_core_lookup() would only be exposed to the kunit tests if you used
EXPORT_SYMBOL_IF_KUNIT. Definitely go with whatever approach you think
Stephen will prefer.
Brian
© 2016 - 2025 Red Hat, Inc.