* [PATCH 2/3] clk: test: Test clk_set_rate_range on orphan mux
2022-03-25 10:58 [PATCH 0/3] clk: Some Clock Range Fixes Maxime Ripard
2022-03-25 10:58 ` [PATCH 1/3] clk: Initialize orphan req_rate Maxime Ripard
@ 2022-03-25 10:58 ` Maxime Ripard
2022-03-25 10:58 ` [PATCH 3/3] clk: Drop the rate range on clk_put Maxime Ripard
2 siblings, 0 replies; 5+ messages in thread
From: Maxime Ripard @ 2022-03-25 10:58 UTC (permalink / raw)
To: linux-clk, Mike Turquette, Stephen Boyd; +Cc: Dmitry Osipenko, Maxime Ripard
A bug recently affected the Tegra30 where calling clk_set_rate_range()
on a clock would make it change its rate to the minimum.
This was due to the clock in question being a mux that was orphan at
registration, which lead to the clk_core req_rate being 0, and the
clk_set_rate_range() function then calling clk_set_rate() with req_rate,
effectively making that clock running at the minimum rate allowed, even
though the initial rate was within that range.
Make a test suite to create a mux initially orphan, and then make sure
that if our clock rate was initially within a given range, then
enforcing that range won't affect it.
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
drivers/clk/clk_test.c | 105 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 insertions(+)
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a92600311506..146b1759798e 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -72,6 +72,19 @@ static int clk_dummy_set_rate(struct clk_hw *hw,
return 0;
}
+static int clk_dummy_single_set_parent(struct clk_hw *hw, u8 index)
+{
+ if (index >= clk_hw_get_num_parents(hw))
+ return -EINVAL;
+
+ return 0;
+}
+
+static u8 clk_dummy_single_get_parent(struct clk_hw *hw)
+{
+ return 0;
+}
+
static const struct clk_ops clk_dummy_rate_ops = {
.recalc_rate = clk_dummy_recalc_rate,
.determine_rate = clk_dummy_determine_rate,
@@ -90,6 +103,11 @@ static const struct clk_ops clk_dummy_minimize_rate_ops = {
.set_rate = clk_dummy_set_rate,
};
+static const struct clk_ops clk_dummy_single_parent_ops = {
+ .set_parent = clk_dummy_single_set_parent,
+ .get_parent = clk_dummy_single_get_parent,
+};
+
static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops)
{
struct clk_dummy_context *ctx;
@@ -239,6 +257,92 @@ static struct kunit_suite clk_test_suite = {
.test_cases = clk_test_cases,
};
+struct clk_single_parent_ctx {
+ struct clk_dummy_context parent_ctx;
+ struct clk_hw hw;
+};
+
+static int clk_orphan_transparent_single_parent_mux_test_init(struct kunit *test)
+{
+ struct clk_single_parent_ctx *ctx;
+ struct clk_init_data init = { };
+ const char *parents[] = { "orphan_parent" };
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+ test->priv = ctx;
+
+ init.name = "test_orphan_dummy_parent";
+ init.ops = &clk_dummy_single_parent_ops;
+ init.parent_names = parents;
+ init.num_parents = ARRAY_SIZE(parents);
+ init.flags = CLK_SET_RATE_PARENT;
+ ctx->hw.init = &init;
+
+ ret = clk_hw_register(NULL, &ctx->hw);
+ if (ret)
+ return ret;
+
+ memset(&init, 0, sizeof(init));
+ init.name = "orphan_parent";
+ init.ops = &clk_dummy_rate_ops;
+ ctx->parent_ctx.hw.init = &init;
+ ctx->parent_ctx.rate = DUMMY_CLOCK_INIT_RATE;
+
+ ret = clk_hw_register(NULL, &ctx->parent_ctx.hw);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void clk_orphan_transparent_single_parent_mux_test_exit(struct kunit *test)
+{
+ struct clk_single_parent_ctx *ctx = test->priv;
+
+ clk_hw_unregister(&ctx->hw);
+ clk_hw_unregister(&ctx->parent_ctx.hw);
+}
+
+/*
+ * Test that a mux-only clock, with an initial rate within a range,
+ * will still have the same rate after the range has been enforced.
+ */
+static void clk_test_orphan_transparent_parent_mux_set_range(struct kunit *test)
+{
+ struct clk_single_parent_ctx *ctx = test->priv;
+ struct clk_hw *hw = &ctx->hw;
+ struct clk *clk = hw->clk;
+ unsigned long rate, new_rate;
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate_range(clk,
+ ctx->parent_ctx.rate - 1000,
+ ctx->parent_ctx.rate + 1000),
+ 0);
+
+ new_rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, new_rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, new_rate);
+}
+
+static struct kunit_case clk_orphan_transparent_single_parent_mux_test_cases[] = {
+ KUNIT_CASE(clk_test_orphan_transparent_parent_mux_set_range),
+ {}
+};
+
+static struct kunit_suite clk_orphan_transparent_single_parent_test_suite = {
+ .name = "clk-orphan-transparent-single-parent-test",
+ .init = clk_orphan_transparent_single_parent_mux_test_init,
+ .exit = clk_orphan_transparent_single_parent_mux_test_exit,
+ .test_cases = clk_orphan_transparent_single_parent_mux_test_cases,
+};
+
/*
* Test that clk_set_rate_range won't return an error for a valid range
* and that it will make sure the rate of the clock is within the
@@ -788,6 +892,7 @@ static struct kunit_suite clk_range_minimize_test_suite = {
kunit_test_suites(
&clk_test_suite,
+ &clk_orphan_transparent_single_parent_test_suite,
&clk_range_test_suite,
&clk_range_maximize_test_suite,
&clk_range_minimize_test_suite
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] clk: Drop the rate range on clk_put
2022-03-25 10:58 [PATCH 0/3] clk: Some Clock Range Fixes Maxime Ripard
2022-03-25 10:58 ` [PATCH 1/3] clk: Initialize orphan req_rate Maxime Ripard
2022-03-25 10:58 ` [PATCH 2/3] clk: test: Test clk_set_rate_range on orphan mux Maxime Ripard
@ 2022-03-25 10:58 ` Maxime Ripard
2022-03-25 15:13 ` kernel test robot
2 siblings, 1 reply; 5+ messages in thread
From: Maxime Ripard @ 2022-03-25 10:58 UTC (permalink / raw)
To: linux-clk, Mike Turquette, Stephen Boyd; +Cc: Dmitry Osipenko, Maxime Ripard
While the current code will trigger a new clk_set_rate call whenever the
rate boundaries are changed through clk_set_rate_range, this doesn't
occur when clk_put() is called.
However, this is essentially equivalent since, after clk_put()
completes, those boundaries won't be enforced anymore.
Let's add a call to clk_set_rate_range in clk_put to make sure those
rate boundaries are dropped and the clock drivers can react.
Let's also add a few tests to make sure this case is covered.
Fixes: c80ac50cbb37 ("clk: Always set the rate on clk_set_range_rate")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
drivers/clk/clk.c | 34 ++++++++++---
drivers/clk/clk_test.c | 108 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 136 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 915a2fa363b1..f21901b83a9b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2340,11 +2340,15 @@ EXPORT_SYMBOL_GPL(clk_set_rate_exclusive);
*
* Returns success (0) or negative errno.
*/
-int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
+static int clk_set_rate_range_nolock(struct clk *clk,
+ unsigned long min,
+ unsigned long max)
{
int ret = 0;
unsigned long old_min, old_max, rate;
+ lockdep_assert_held(&prepare_lock);
+
if (!clk)
return 0;
@@ -2357,8 +2361,6 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
return -EINVAL;
}
- clk_prepare_lock();
-
if (clk->exclusive_count)
clk_core_rate_unprotect(clk->core);
@@ -2402,6 +2404,28 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
if (clk->exclusive_count)
clk_core_rate_protect(clk->core);
+ return ret;
+}
+
+/**
+ * clk_set_rate_range - set a rate range for a clock source
+ * @clk: clock source
+ * @min: desired minimum clock rate in Hz, inclusive
+ * @max: desired maximum clock rate in Hz, inclusive
+ *
+ * Returns success (0) or negative errno.
+ */
+int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
+{
+ int ret;
+
+ if (!clk)
+ return 0;
+
+ clk_prepare_lock();
+
+ ret = clk_set_rate_range_nolock(clk, min, max);
+
clk_prepare_unlock();
return ret;
@@ -4403,9 +4427,7 @@ void __clk_put(struct clk *clk)
}
hlist_del(&clk->clks_node);
- if (clk->min_rate > clk->core->req_rate ||
- clk->max_rate < clk->core->req_rate)
- clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+ clk_set_rate_range_nolock(clk, 0, ULONG_MAX);
owner = clk->core->owner;
kref_put(&clk->core->ref, __clk_release);
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 146b1759798e..b205c329cf32 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -760,9 +760,65 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test)
clk_put(user1);
}
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), across multiple users, the core will reevaluate
+ * whether a new rate is needed, including when a user drop its clock.
+ *
+ * With clk_dummy_maximize_rate_ops, this means that the the rate will
+ * trail along the maximum as it evolves.
+ */
+static void clk_range_test_multiple_set_range_rate_put_maximized(struct kunit *test)
+{
+ struct clk_dummy_context *ctx = test->priv;
+ struct clk_hw *hw = &ctx->hw;
+ struct clk *clk = hw->clk;
+ struct clk *user1, *user2;
+ unsigned long rate;
+
+ user1 = clk_hw_get_clk(hw, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+ user2 = clk_hw_get_clk(hw, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate(clk, DUMMY_CLOCK_RATE_2 + 1000),
+ 0);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate_range(user1,
+ 0,
+ DUMMY_CLOCK_RATE_2),
+ 0);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate_range(user2,
+ 0,
+ DUMMY_CLOCK_RATE_1),
+ 0);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+ clk_put(user2);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+ clk_put(user1);
+}
+
static struct kunit_case clk_range_maximize_test_cases[] = {
KUNIT_CASE(clk_range_test_set_range_rate_maximized),
KUNIT_CASE(clk_range_test_multiple_set_range_rate_maximized),
+ KUNIT_CASE(clk_range_test_multiple_set_range_rate_put_maximized),
{}
};
@@ -877,9 +933,61 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test)
clk_put(user1);
}
+/*
+ * Test that if we have several subsequent calls to
+ * clk_set_rate_range(), across multiple users, the core will reevaluate
+ * whether a new rate is needed, including when a user drop its clock.
+ *
+ * With clk_dummy_minimize_rate_ops, this means that the the rate will
+ * trail along the minimum as it evolves.
+ */
+static void clk_range_test_multiple_set_range_rate_put_minimized(struct kunit *test)
+{
+ struct clk_dummy_context *ctx = test->priv;
+ struct clk_hw *hw = &ctx->hw;
+ struct clk *clk = hw->clk;
+ struct clk *user1, *user2;
+ unsigned long rate;
+
+ user1 = clk_hw_get_clk(hw, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user1);
+
+ user2 = clk_hw_get_clk(hw, NULL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, user2);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate_range(user1,
+ DUMMY_CLOCK_RATE_1,
+ ULONG_MAX),
+ 0);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+ KUNIT_ASSERT_EQ(test,
+ clk_set_rate_range(user2,
+ DUMMY_CLOCK_RATE_2,
+ ULONG_MAX),
+ 0);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2);
+
+ clk_put(user2);
+
+ rate = clk_get_rate(clk);
+ KUNIT_ASSERT_GT(test, rate, 0);
+ KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+ clk_put(user1);
+}
+
static struct kunit_case clk_range_minimize_test_cases[] = {
KUNIT_CASE(clk_range_test_set_range_rate_minimized),
KUNIT_CASE(clk_range_test_multiple_set_range_rate_minimized),
+ KUNIT_CASE(clk_range_test_multiple_set_range_rate_put_minimized),
{}
};
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread