This is a replacement for the series from Maxime[1]. I didn't want to
introduce more wrappers, so this series takes the approach of driving
the test from a clk_op instead. The second patch extends the test to
look at more determine_rate variants. We can probably extend the test
further to set or not set the CLK_SET_RATE_PARENT flag as well but I
didn't do that.
Stephen Boyd (2):
clk: Drive clk_leaf_mux_set_rate_parent test from clk_ops
clk: Parameterize clk_leaf_mux_set_rate_parent
drivers/clk/clk_test.c | 130 +++++++++++++++++++++++++++++++++++------
1 file changed, 113 insertions(+), 17 deletions(-)
Cc: Guenter Roeck <[email protected]>
Cc: Maxime Ripard <[email protected]>
[1] https://lore.kernel.org/r/[email protected]
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
Running this kunit test with lockdep enabled leads to warning splats
about calling clk provider APIs without the clk_prepare lock held. I
proposed adding a wrapper around these APIs to grab the prepare lock so
we can call them from anywhere, and Maxime implemented that approach[1],
but it didn't look great. That's because we had to make more kunit
testing APIs just to call code from a place that isn't a clk provider
when the prepare lock isn't held.
Instead of doing that, let's implement a determine_rate clk_op for a new
leaf clk that is the child of the existing leaf clk. We can call
__clk_determine_rate() on the existing leaf clk from there, and stash
away the clk_rate_request struct to check once the clk_op returns. Drive
that clk_op by calling clk_round_rate() to keep things similar to how it
was before (i.e. nothing actually changes rate, just the new rate is
determined). This silences the warning by driving the test from a
clk_op where we know the prepare lock is held.
While looking at this in more detail, it was determined that the code we
intended to test in commit 262ca38f4b6e ("clk: Stop forwarding
clk_rate_requests to the parent") wasn't actually tested. The call to
__clk_determine_rate() wasn't actually getting to the newly introduced
code under the CLK_SET_RATE_PARENT if condition in
clk_core_round_rate_nolock() because the parent clk (the mux) could
round rates. We introduce a new leaf and make sure the parent of that
clk has no clk_ops so that we can be certain that the
CLK_SET_RATE_PARENT condition in clk_core_round_rate_nolock() is
evaluated.
Reported-by: Guenter Roeck <[email protected]>
Closes: https://lore.kernel.org/linux-clk/[email protected]/
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-lkp/[email protected]
Cc: Maxime Ripard <[email protected]>
Link: https://lore.kernel.org/r/[email protected] [1]
Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk_test.c | 65 +++++++++++++++++++++++++++++++-----------
1 file changed, 48 insertions(+), 17 deletions(-)
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a154ec9d0111..43e85fc0b025 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -10,6 +10,8 @@
#include <kunit/test.h>
+static const struct clk_ops empty_clk_ops = { };
+
#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000)
#define DUMMY_CLOCK_RATE_1 (142 * 1000 * 1000)
#define DUMMY_CLOCK_RATE_2 (242 * 1000 * 1000)
@@ -2155,6 +2157,30 @@ static struct kunit_suite clk_range_minimize_test_suite = {
struct clk_leaf_mux_ctx {
struct clk_multiple_parent_ctx mux_ctx;
struct clk_hw hw;
+ struct clk_hw parent;
+ struct clk_rate_request *req;
+};
+
+static int clk_leaf_mux_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+ struct clk_leaf_mux_ctx *ctx = container_of(hw, struct clk_leaf_mux_ctx, hw);
+ int ret;
+ struct clk_rate_request *parent_req = ctx->req;
+
+ clk_hw_forward_rate_request(hw, req, req->best_parent_hw, parent_req, req->rate);
+ ret = __clk_determine_rate(req->best_parent_hw, parent_req);
+ if (ret)
+ return ret;
+
+ req->rate = parent_req->rate;
+
+ return 0;
+}
+
+static const struct clk_ops clk_leaf_mux_set_rate_parent_ops = {
+ .determine_rate = clk_leaf_mux_determine_rate,
+ .set_parent = clk_dummy_single_set_parent,
+ .get_parent = clk_dummy_single_get_parent,
};
static int
@@ -2193,8 +2219,14 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
if (ret)
return ret;
- ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw,
- &clk_dummy_single_parent_ops,
+ ctx->parent.init = CLK_HW_INIT_HW("test-parent", &ctx->mux_ctx.hw,
+ &empty_clk_ops, CLK_SET_RATE_PARENT);
+ ret = clk_hw_register(NULL, &ctx->parent);
+ if (ret)
+ return ret;
+
+ ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->parent,
+ &clk_leaf_mux_set_rate_parent_ops,
CLK_SET_RATE_PARENT);
ret = clk_hw_register(NULL, &ctx->hw);
if (ret)
@@ -2208,32 +2240,31 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
struct clk_leaf_mux_ctx *ctx = test->priv;
clk_hw_unregister(&ctx->hw);
+ clk_hw_unregister(&ctx->parent);
clk_hw_unregister(&ctx->mux_ctx.hw);
clk_hw_unregister(&ctx->mux_ctx.parents_ctx[0].hw);
clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw);
}
/*
- * Test that, for a clock that will forward any rate request to its
- * parent, the rate request structure returned by __clk_determine_rate
- * is sane and will be what we expect.
+ * Test that, for a clock that will forward any rate request to its parent, the
+ * rate request structure returned by __clk_determine_rate() is sane and
+ * doesn't have the clk_rate_request's best_parent_hw pointer point to the
+ * clk_hw passed into __clk_determine_rate(). See commit 262ca38f4b6e ("clk:
+ * Stop forwarding clk_rate_requests to the parent") for more background.
*/
-static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
+static void clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent(struct kunit *test)
{
struct clk_leaf_mux_ctx *ctx = test->priv;
struct clk_hw *hw = &ctx->hw;
struct clk *clk = clk_hw_get_clk(hw, NULL);
struct clk_rate_request req;
unsigned long rate;
- int ret;
+ ctx->req = &req;
rate = clk_get_rate(clk);
KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
-
- clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2);
-
- ret = __clk_determine_rate(hw, &req);
- KUNIT_ASSERT_EQ(test, ret, 0);
+ KUNIT_ASSERT_EQ(test, DUMMY_CLOCK_RATE_2, clk_round_rate(clk, DUMMY_CLOCK_RATE_2));
KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
@@ -2243,15 +2274,15 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
}
static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
- KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
+ KUNIT_CASE(clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent),
{}
};
/*
- * Test suite for a clock whose parent is a mux with multiple parents.
- * The leaf clock has CLK_SET_RATE_PARENT, and will forward rate
- * requests to the mux, which will then select which parent is the best
- * fit for a given rate.
+ * Test suite for a clock whose parent is a pass-through clk whose parent is a
+ * mux with multiple parents. The leaf and pass-through clocks have the
+ * CLK_SET_RATE_PARENT flag, and will forward rate requests to the mux, which
+ * will then select which parent is the best fit for a given rate.
*
* These tests exercise the behaviour of muxes, and the proper selection
* of parents.
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
Transform the existing clk_leaf_mux_set_rate_parent test into a
parameterized test that calls the various determine rate APIs that exist
for clk providers. This ensures that whatever determine rate API is used
by a clk provider will return the correct parent in the best_parent_hw
pointer of the clk_rate_request because clk_rate_requests are forwarded
properly.
Cc: Guenter Roeck <[email protected]>
Cc: Maxime Ripard <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clk/clk_test.c | 81 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 73 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 43e85fc0b025..39e2b5ff4f51 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2159,6 +2159,7 @@ struct clk_leaf_mux_ctx {
struct clk_hw hw;
struct clk_hw parent;
struct clk_rate_request *req;
+ int (*determine_rate_func)(struct clk_hw *hw, struct clk_rate_request *req);
};
static int clk_leaf_mux_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
@@ -2168,7 +2169,7 @@ static int clk_leaf_mux_determine_rate(struct clk_hw *hw, struct clk_rate_reques
struct clk_rate_request *parent_req = ctx->req;
clk_hw_forward_rate_request(hw, req, req->best_parent_hw, parent_req, req->rate);
- ret = __clk_determine_rate(req->best_parent_hw, parent_req);
+ ret = ctx->determine_rate_func(req->best_parent_hw, parent_req);
if (ret)
return ret;
@@ -2246,20 +2247,83 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw);
}
+struct clk_leaf_mux_set_rate_parent_determine_rate_test_case {
+ const char *desc;
+ int (*determine_rate_func)(struct clk_hw *hw, struct clk_rate_request *req);
+};
+
+static void
+clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc(
+ const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *t, char *desc)
+{
+ strcpy(desc, t->desc);
+}
+
+static const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case
+clk_leaf_mux_set_rate_parent_determine_rate_test_cases[] = {
+ {
+ /*
+ * Test that __clk_determine_rate() on the parent that can't
+ * change rate doesn't return a clk_rate_request structure with
+ * the best_parent_hw pointer pointing to the parent.
+ */
+ .desc = "clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent",
+ .determine_rate_func = __clk_determine_rate,
+ },
+ {
+ /*
+ * Test that __clk_mux_determine_rate() on the parent that
+ * can't change rate doesn't return a clk_rate_request
+ * structure with the best_parent_hw pointer pointing to
+ * the parent.
+ */
+ .desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_proper_parent",
+ .determine_rate_func = __clk_mux_determine_rate,
+ },
+ {
+ /*
+ * Test that __clk_mux_determine_rate_closest() on the parent
+ * that can't change rate doesn't return a clk_rate_request
+ * structure with the best_parent_hw pointer pointing to
+ * the parent.
+ */
+ .desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_closest_proper_parent",
+ .determine_rate_func = __clk_mux_determine_rate_closest,
+ },
+ {
+ /*
+ * Test that clk_hw_determine_rate_no_reparent() on the parent
+ * that can't change rate doesn't return a clk_rate_request
+ * structure with the best_parent_hw pointer pointing to
+ * the parent.
+ */
+ .desc = "clk_leaf_mux_set_rate_parent_clk_hw_determine_rate_no_reparent_proper_parent",
+ .determine_rate_func = clk_hw_determine_rate_no_reparent,
+ },
+};
+
+KUNIT_ARRAY_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test,
+ clk_leaf_mux_set_rate_parent_determine_rate_test_cases,
+ clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc)
+
/*
- * Test that, for a clock that will forward any rate request to its parent, the
- * rate request structure returned by __clk_determine_rate() is sane and
- * doesn't have the clk_rate_request's best_parent_hw pointer point to the
- * clk_hw passed into __clk_determine_rate(). See commit 262ca38f4b6e ("clk:
- * Stop forwarding clk_rate_requests to the parent") for more background.
+ * Test that when a clk that can't change rate itself calls a function like
+ * __clk_determine_rate() on its parent it doesn't get back a clk_rate_request
+ * structure that has the best_parent_hw pointer point to the clk_hw passed
+ * into the determine rate function. See commit 262ca38f4b6e ("clk: Stop
+ * forwarding clk_rate_requests to the parent") for more background.
*/
-static void clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent(struct kunit *test)
+static void clk_leaf_mux_set_rate_parent_determine_rate_test(struct kunit *test)
{
struct clk_leaf_mux_ctx *ctx = test->priv;
struct clk_hw *hw = &ctx->hw;
struct clk *clk = clk_hw_get_clk(hw, NULL);
struct clk_rate_request req;
unsigned long rate;
+ const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *test_param;
+
+ test_param = test->param_value;
+ ctx->determine_rate_func = test_param->determine_rate_func;
ctx->req = &req;
rate = clk_get_rate(clk);
@@ -2274,7 +2338,8 @@ static void clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent(struc
}
static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
- KUNIT_CASE(clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent),
+ KUNIT_CASE_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test,
+ clk_leaf_mux_set_rate_parent_determine_rate_test_gen_params),
{}
};
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
On Tue, 12 Sep 2023 10:55:29 -0700, Stephen Boyd wrote:
> This is a replacement for the series from Maxime[1]. I didn't want to
> introduce more wrappers, so this series takes the approach of driving
> the test from a clk_op instead. The second patch extends the test to
> look at more determine_rate variants. We can probably extend the test
> further to set or not set the CLK_SET_RATE_PARENT flag as well but I
>
> [ ... ]
Acked-by: Maxime Ripard <[email protected]>
Thanks!
Maxime
Quoting Stephen Boyd (2023-09-12 10:55:30)
> Running this kunit test with lockdep enabled leads to warning splats
> about calling clk provider APIs without the clk_prepare lock held. I
> proposed adding a wrapper around these APIs to grab the prepare lock so
> we can call them from anywhere, and Maxime implemented that approach[1],
> but it didn't look great. That's because we had to make more kunit
> testing APIs just to call code from a place that isn't a clk provider
> when the prepare lock isn't held.
>
> Instead of doing that, let's implement a determine_rate clk_op for a new
> leaf clk that is the child of the existing leaf clk. We can call
> __clk_determine_rate() on the existing leaf clk from there, and stash
> away the clk_rate_request struct to check once the clk_op returns. Drive
> that clk_op by calling clk_round_rate() to keep things similar to how it
> was before (i.e. nothing actually changes rate, just the new rate is
> determined). This silences the warning by driving the test from a
> clk_op where we know the prepare lock is held.
>
> While looking at this in more detail, it was determined that the code we
> intended to test in commit 262ca38f4b6e ("clk: Stop forwarding
> clk_rate_requests to the parent") wasn't actually tested. The call to
> __clk_determine_rate() wasn't actually getting to the newly introduced
> code under the CLK_SET_RATE_PARENT if condition in
> clk_core_round_rate_nolock() because the parent clk (the mux) could
> round rates. We introduce a new leaf and make sure the parent of that
> clk has no clk_ops so that we can be certain that the
> CLK_SET_RATE_PARENT condition in clk_core_round_rate_nolock() is
> evaluated.
>
> Reported-by: Guenter Roeck <[email protected]>
> Closes: https://lore.kernel.org/linux-clk/[email protected]/
> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]
> Cc: Maxime Ripard <[email protected]>
> Link: https://lore.kernel.org/r/[email protected] [1]
> Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Stephen Boyd (2023-09-12 10:55:31)
> Transform the existing clk_leaf_mux_set_rate_parent test into a
> parameterized test that calls the various determine rate APIs that exist
> for clk providers. This ensures that whatever determine rate API is used
> by a clk provider will return the correct parent in the best_parent_hw
> pointer of the clk_rate_request because clk_rate_requests are forwarded
> properly.
>
> Cc: Guenter Roeck <[email protected]>
> Cc: Maxime Ripard <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next