Hi,
On 25.03.2022 17:11, Maxime Ripard wrote:
> 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@xxxxxxxxxx>
This patch landed recently in linux-next 20220328 as commit 7dabfa2bc480
("clk: Drop the rate range on clk_put()"). Sadly it breaks booting of
the few of my test systems: Samsung ARM 32bit Exynos3250 based Rinato
board and all Amlogic Meson G12B/SM1 based boards (Odroid C4, N2, Khadas
VIM3/VIM3l). Rinato hangs always with the following oops:
--->8---
Kernel panic - not syncing: MCT hangs after writing 4 (offset:0x420)
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.17.0-rc1-00014-g7dabfa2bc480
#11551
Hardware name: Samsung Exynos (Flattened Device Tree)
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from panic+0x10c/0x328
panic from exynos4_mct_tick_stop+0x0/0x2c
---[ end Kernel panic - not syncing: MCT hangs after writing 4
(offset:0x420) ]---
--->8---
Amlogic boards hang randomly during early userspace init, usually just
after loading the driver modules.
Reverting $subject on top of linux-next fixes all those problems.
I will try to analyze it a bit more and if possible provide some more
useful/meaning full logs later.
> ---
> drivers/clk/clk.c | 42 ++++++++++------
> drivers/clk/clk_test.c | 108 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 136 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 915a2fa363b1..91f863b7a824 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2332,19 +2332,15 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
> }
> EXPORT_SYMBOL_GPL(clk_set_rate_exclusive);
>
> -/**
> - * 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)
> +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 +2353,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 +2396,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 +4419,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),
> {}
> };
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
