[PATCH v3 0/2] clk: fix corner case of clk_mux_determine_rate_flags()

[PATCH v3 0/2] clk: fix corner case of clk_mux_determine_rate_flags()

[Yang Xiwen via B4 Relay]

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v3:
- Add many unittest functions for mux determine_rate
- Add more checks for ULONG_MAX
- Link to v2: https://lore.kernel.org/r/20230421-clk-v2-0-74c6cc2214d1@outlook.com

Changes in v2:
- Add a unit test to clk_test.c
- Link to v1: https://lore.kernel.org/r/20230421-clk-v1-1-bb503f2f2cf3@outlook.com

---
Yang Xiwen (2):
      clk: use ULONG_MAX as the initial value for the iteration in clk_mux_determine_rate_flags()
      clk: tests: Add missing test cases for mux determine_rate

 drivers/clk/clk.c      |  13 ++++-
 drivers/clk/clk_test.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 141 insertions(+), 3 deletions(-)
---
base-commit: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8
change-id: 20230421-clk-64c6b6e21cdb

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

[PATCH v3 1/2] clk: use ULONG_MAX as the initial value for the iteration in clk_mux_determine_rate_flags()

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Currently, clk_mux_determine_rate_flags() use 0 as the initial value for
selecting the best matching parent. However, this will choose a
non-existant rate(0) if the requested rate is closer to 0 than the
minimum rate the parents have.

Fix that by initializing the initial value to ULONG_MAX and treat it as a
magic number.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/clk/clk.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ae07685c7588b..6184d4aa88193 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -541,6 +541,17 @@ EXPORT_SYMBOL_GPL(__clk_is_enabled);
 static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 			   unsigned long best, unsigned long flags)
 {
+	if (rate == ULONG_MAX) {
+		// select the highest clock rate
+		if (best == ULONG_MAX)
+			return true;
+		else
+			return now > best;
+	}
+
+	if (best == ULONG_MAX && now <= rate)
+		return true;
+
 	if (flags & CLK_MUX_ROUND_CLOSEST)
 		return abs(now - rate) < abs(best - rate);
 
@@ -600,7 +611,7 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
 {
 	struct clk_core *core = hw->core, *parent, *best_parent = NULL;
 	int i, num_parents, ret;
-	unsigned long best = 0;
+	unsigned long best = ULONG_MAX;
 
 	/* if NO_REPARENT flag set, pass through to current parent */
 	if (core->flags & CLK_SET_RATE_NO_REPARENT) {

-- 
2.39.2

[PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Add lots of test cases for mux determine_rate to cover more possible
cases, the original test case is renamed for the change.

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/clk/clk_test.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index f9a5c2964c65d..4f7f9a964637a 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
  * parent, the rate request structure returned by __clk_determine_rate
  * is sane and will be what we expect.
  */
-static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
+static void clk_leaf_mux_set_rate_parent_determine_rate_case1(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;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_hw_init_rate_request(hw, &req, 0);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+
+	clk_put(clk);
+}
+
+static void clk_leaf_mux_set_rate_parent_determine_rate_case2(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;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+
+	clk_put(clk);
+}
+
+static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test)
 {
 	struct clk_leaf_mux_ctx *ctx = test->priv;
 	struct clk_hw *hw = &ctx->hw;
@@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
 	clk_put(clk);
 }
 
+static void clk_leaf_mux_set_rate_parent_determine_rate_case4(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;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
+
+	clk_put(clk);
+}
+
+static void clk_leaf_mux_set_rate_parent_determine_rate_case5(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;
+
+	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 + 100000);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
+
+	clk_put(clk);
+}
+
+static void clk_leaf_mux_set_rate_parent_determine_rate_case6(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;
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
+
+	clk_hw_init_rate_request(hw, &req, ULONG_MAX);
+
+	ret = __clk_determine_rate(hw, &req);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
+
+	clk_put(clk);
+}
+
+/* We test 6 cases here:
+ * 1. The requested rate is 0;
+ * 2. The requested rate is not 0 but lower than any rate that parents could offer;
+ * 3. The requested rate is exactly one of the parents' clock rate;
+ * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer;
+ * 5. The requested rate is larger than all rates that parents could offer;
+ * 6. The requested rate is ULONG_MAX.
+ *
+ * Hopefully they covered all cases.
+ */
 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_determine_rate_case1),
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2),
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3),
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5),
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6),
 	{}
 };
 

-- 
2.39.2

Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Stephen Boyd]

Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> index f9a5c2964c65d..4f7f9a964637a 100644
> --- a/drivers/clk/clk_test.c
> +++ b/drivers/clk/clk_test.c
> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
>   * parent, the rate request structure returned by __clk_determine_rate
>   * is sane and will be what we expect.
>   */
> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)

Just leave this one alone and put the other test cases right after it.
Don't rename it and also move it lower down. It makes the diff hard to
read.

> +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(struct kunit *test)

Please add a comment above each test case like there is for
clk_leaf_mux_set_rate_parent_determine_rate() that describes what is
being tested.

> +{
> +       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;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +
> +       clk_hw_init_rate_request(hw, &req, 0);
> +
> +       ret = __clk_determine_rate(hw, &req);
> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> +
> +       clk_put(clk);
> +}
> +
> +static void clk_leaf_mux_set_rate_parent_determine_rate_case2(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;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +
> +       clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE);
> +
> +       ret = __clk_determine_rate(hw, &req);
> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);

There should be some KUNIT_EXPECT statement in each test.

> +
> +       clk_put(clk);
> +}
> +
> +static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test)
>  {
>         struct clk_leaf_mux_ctx *ctx = test->priv;
>         struct clk_hw *hw = &ctx->hw;
> @@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>         clk_put(clk);
>  }
>  
> +static void clk_leaf_mux_set_rate_parent_determine_rate_case4(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;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +
> +       clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2);
> +
> +       ret = __clk_determine_rate(hw, &req);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1);
> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1);
> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
> +
> +       clk_put(clk);
> +}
> +
> +static void clk_leaf_mux_set_rate_parent_determine_rate_case5(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;
> +
> +       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 + 100000);
> +
> +       ret = __clk_determine_rate(hw, &req);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
> +
> +       clk_put(clk);
> +}
> +
> +static void clk_leaf_mux_set_rate_parent_determine_rate_case6(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;
> +
> +       rate = clk_get_rate(clk);
> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
> +
> +       clk_hw_init_rate_request(hw, &req, ULONG_MAX);
> +
> +       ret = __clk_determine_rate(hw, &req);
> +       KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
> +
> +       clk_put(clk);
> +}
> +
> +/* We test 6 cases here:
> + * 1. The requested rate is 0;
> + * 2. The requested rate is not 0 but lower than any rate that parents could offer;
> + * 3. The requested rate is exactly one of the parents' clock rate;
> + * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer;
> + * 5. The requested rate is larger than all rates that parents could offer;
> + * 6. The requested rate is ULONG_MAX.
> + *
> + * Hopefully they covered all cases.
> + */

Please remove this comment and name the cases better.

>  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_determine_rate_case1),

Maybe call it clk_leaf_mux_determine_rate_request_zero?

> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2),

clk_leaf_mux_determine_rate_lower_than_parents_fails

> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3),

clk_leaf_mux_determine_rate_exactly_parent1

> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),

I'm not sure I understand what is being tested in this case. Are we
testing that __clk_determine_rate() with a rate between parent0 and
parent1 picks parent1?

> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5),

clk_leaf_mux_determine_rate_larger_than_parents

> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6),

clk_leaf_mux_determine_rate_ULONG_MAX_picks_parent1

Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Yang Xiwen]

On 5/3/2023 11:08 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index f9a5c2964c65d..4f7f9a964637a 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
>>   * parent, the rate request structure returned by __clk_determine_rate
>>   * is sane and will be what we expect.
>>   */
>> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
> 
> Just leave this one alone and put the other test cases right after it.
> Don't rename it and also move it lower down. It makes the diff hard to
> read.
> 
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(struct kunit *test)
> 
> Please add a comment above each test case like there is for
> clk_leaf_mux_set_rate_parent_determine_rate() that describes what is
> being tested.
> 
>> +{
>> +       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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, 0);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case2(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> 
> There should be some KUNIT_EXPECT statement in each test.>
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test)
>>  {
>>         struct clk_leaf_mux_ctx *ctx = test->priv;
>>         struct clk_hw *hw = &ctx->hw;
>> @@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>>         clk_put(clk);
>>  }
>>  
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case4(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case5(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;
>> +
>> +       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 + 100000);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case6(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, ULONG_MAX);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +/* We test 6 cases here:
>> + * 1. The requested rate is 0;
>> + * 2. The requested rate is not 0 but lower than any rate that parents could offer;
>> + * 3. The requested rate is exactly one of the parents' clock rate;
>> + * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer;
>> + * 5. The requested rate is larger than all rates that parents could offer;
>> + * 6. The requested rate is ULONG_MAX.
>> + *
>> + * Hopefully they covered all cases.
>> + */
> 
> Please remove this comment and name the cases better.
Thanks, I will follow your suggestions and rename them in next version.
> 
>>  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_determine_rate_case1),
> 
> Maybe call it clk_leaf_mux_determine_rate_request_zero?
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2),
> 
> clk_leaf_mux_determine_rate_lower_than_parents_fails
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3),
> 
> clk_leaf_mux_determine_rate_exactly_parent1
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
> 
> I'm not sure I understand what is being tested in this case. Are we
> testing that __clk_determine_rate() with a rate between parent0 and
> parent1 picks parent1?Yes, that's it. For example, 2 parents offer 100MHz and 200MHz, we
request 150MHz, and 100MHz should be determined(the highest possible
rate lower than or equal to the requested rate). Actually the flag
CLK_MUX_ROUND_CLOSEST is still missing in the test cases. I suppose it
should be in another patchset.
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5),
> 
> clk_leaf_mux_determine_rate_larger_than_parents
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6),
> 
> clk_leaf_mux_determine_rate_ULONG_MAX_picks_parent1
-- 
Best regards,
Yang Xiwen

Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Yang Xiwen]

On 5/3/2023 11:08 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index f9a5c2964c65d..4f7f9a964637a 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
>>   * parent, the rate request structure returned by __clk_determine_rate
>>   * is sane and will be what we expect.
>>   */
>> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
> 
> Just leave this one alone and put the other test cases right after it.
> Don't rename it and also move it lower down. It makes the diff hard to
> read.
I don't quite understand. In this patch, I renamed it to case3. Here I
think you'd like it to remain as-is. But I think the comments below said
I should rename it to clk_leaf_mux_determine_rate_exactly_parent1()?
Actually what this test has done should be included in this series of
test cases as one of them.
> 
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(struct kunit *test)
> 
> Please add a comment above each test case like there is for
> clk_leaf_mux_set_rate_parent_determine_rate() that describes what is
> being tested.
> 
>> +{
>> +       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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, 0);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case2(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> 
> There should be some KUNIT_EXPECT statement in each test.
> 
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test)
>>  {
>>         struct clk_leaf_mux_ctx *ctx = test->priv;
>>         struct clk_hw *hw = &ctx->hw;
>> @@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>>         clk_put(clk);
>>  }
>>  
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case4(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case5(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;
>> +
>> +       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 + 100000);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case6(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;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, ULONG_MAX);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +/* We test 6 cases here:
>> + * 1. The requested rate is 0;
>> + * 2. The requested rate is not 0 but lower than any rate that parents could offer;
>> + * 3. The requested rate is exactly one of the parents' clock rate;
>> + * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer;
>> + * 5. The requested rate is larger than all rates that parents could offer;
>> + * 6. The requested rate is ULONG_MAX.
>> + *
>> + * Hopefully they covered all cases.
>> + */
> 
> Please remove this comment and name the cases better.
> 
>>  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_determine_rate_case1),
> 
> Maybe call it clk_leaf_mux_determine_rate_request_zero?
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2),
> 
> clk_leaf_mux_determine_rate_lower_than_parents_fails
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3),
> 
> clk_leaf_mux_determine_rate_exactly_parent1
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
> 
> I'm not sure I understand what is being tested in this case. Are we
> testing that __clk_determine_rate() with a rate between parent0 and
> parent1 picks parent1?
I think I have to speak more about how these tests are arranged. Imagine
that there is a segment starting at 0, ending at ULONG_MAX. Now add two
points on it, assuming DUMMY_CLOCK_RATE_1(142MHz) and
DUMMY_CLOCK_RATE_2(242MHz). We split the segment into 3 segments, and we
have 4 endpoints in total. They are: 0, 0-142MHz, 142MHz, 142MHz-242MHz,
242MHz-ULONG_MAX, ULONG_MAX. Ideally, we need to test all these 7 cases.
For those 4 points, the tests should be straightforward. But for the 3
segments, we can only extract a random point on it to test. Besides, I
think requesting for 142MHz or 242MHz has no big difference, so I
omitted one of them. The original
clk_leaf_mux_set_rate_parent_determine_rate() is considered to be one of
the cases here, for which I think should be absorbed and renamed.

And as said in the previous email, the situation here would become even
more complicated if CLK_MUX_ROUND_CLOSEST is true. I guess it should be
in another patchset.
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5),
> 
> clk_leaf_mux_determine_rate_larger_than_parents
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6),
> 
> clk_leaf_mux_determine_rate_ULONG_MAX_picks_parent1

-- 
Best regards,
Yang Xiwen

Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux determine_rate

[Stephen Boyd]

Quoting Yang Xiwen (2023-05-03 11:44:45)
> On 5/3/2023 11:08 AM, Stephen Boyd wrote:
> > Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
> >> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
> >> index f9a5c2964c65d..4f7f9a964637a 100644
> >> --- a/drivers/clk/clk_test.c
> >> +++ b/drivers/clk/clk_test.c
> >> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
> >>   * parent, the rate request structure returned by __clk_determine_rate
> >>   * is sane and will be what we expect.
> >>   */
> >> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
> > 
> > Just leave this one alone and put the other test cases right after it.
> > Don't rename it and also move it lower down. It makes the diff hard to
> > read.
> I don't quite understand. In this patch, I renamed it to case3. Here I
> think you'd like it to remain as-is. But I think the comments below said
> I should rename it to clk_leaf_mux_determine_rate_exactly_parent1()?
> Actually what this test has done should be included in this series of
> test cases as one of them.

You can rename it, but don't move it lower in the file.

[...]
> >> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
> > 
> > I'm not sure I understand what is being tested in this case. Are we
> > testing that __clk_determine_rate() with a rate between parent0 and
> > parent1 picks parent1?
> I think I have to speak more about how these tests are arranged.

The order that tests run in should not be relied upon. The tests should
be hermetic.

> Imagine
> that there is a segment starting at 0, ending at ULONG_MAX. Now add two
> points on it, assuming DUMMY_CLOCK_RATE_1(142MHz) and
> DUMMY_CLOCK_RATE_2(242MHz). We split the segment into 3 segments, and we
> have 4 endpoints in total. They are: 0, 0-142MHz, 142MHz, 142MHz-242MHz,
> 242MHz-ULONG_MAX, ULONG_MAX. Ideally, we need to test all these 7 cases.
> For those 4 points, the tests should be straightforward. But for the 3
> segments, we can only extract a random point on it to test. Besides, I
> think requesting for 142MHz or 242MHz has no big difference, so I
> omitted one of them. The original
> clk_leaf_mux_set_rate_parent_determine_rate() is considered to be one of
> the cases here, for which I think should be absorbed and renamed.

Got it. Adding various tests is OK. Hard-coding a rate, instead of doing
math makes it easier to read. Dividing by 2 threw me off because I was
trying to figure out why it was important.

I'll note that you're basically testing the rounding implementation of
the clk's determine_rate clk_op though, which may be different from what
this test suite is testing. The test suite talks about making sure the
struct clk_rate_request is correct. Maybe the suite should be renamed to
"clk-leaf-mux-determine-rate-request" because it doesn't call set_rate
at all.

If you want to add tests that check the rounding behavior then maybe add
a test for clk-mux.c called clk-mux_test.c and follow the
clk-gate_test.c approach to fake the register. Then also add tests for
the exported functions in there like clk_mux_val_to_index() and
clk_mux_determine_rate_flags(), etc. The mux determine rate API is in
clk.c, but that's mostly because it is so generic it can be used by any
mux type of clk hardware.

Please Cc Maxime on these patches too, because the lines you're
modifying were authored by Maxime.

> 
> And as said in the previous email, the situation here would become even
> more complicated if CLK_MUX_ROUND_CLOSEST is true. I guess it should be
> in another patchset.

Yes, that's different, and probably should be implemented as a direct
call to __clk_mux_determine_rate_closest() instead.