Re: [PATCH v2 3/3] clk: Drop the rate range on clk_put

[Marek Szyprowski <m.szyprowski@samsung.com>]

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@cerno.tech>

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