[0/2] clk: kunit: Fix the lockdep warnings

  Hi,

Here's a small series to address the lockdep warning we have when
running the clk kunit tests with lockdep enabled.

For the record, it can be tested with:

$ ./tools/testing/kunit/kunit.py run \
    --kunitconfig=drivers/clk \
    --cross_compile aarch64-linux-gnu- --arch arm64 \
    --kconfig_add CONFIG_DEBUG_KERNEL=y \
    --kconfig_add CONFIG_PROVE_LOCKING=y

Let me know what you think,
Maxime

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
Maxime Ripard (2):
      clk: Introduce kunit wrapper around clk_hw_init_rate_request
      clk: Introduce kunit wrapper around __clk_determine_rate

 drivers/clk/clk.c            | 51 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk_test.c       |  4 ++--
 include/linux/clk-provider.h | 21 ++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)
---
base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252
change-id: 20230721-clk-fix-kunit-lockdep-c5e62b221118

Best regards,
  

On Fri, Jul 21, 2023 at 09:09:31AM +0200, Maxime Ripard wrote:
> Hi,
> 
> Here's a small series to address the lockdep warning we have when
> running the clk kunit tests with lockdep enabled.
> 
> For the record, it can be tested with:
> 
> $ ./tools/testing/kunit/kunit.py run \
>     --kunitconfig=drivers/clk \
>     --cross_compile aarch64-linux-gnu- --arch arm64 \
>     --kconfig_add CONFIG_DEBUG_KERNEL=y \
>     --kconfig_add CONFIG_PROVE_LOCKING=y
> 
> Let me know what you think,

The series fixes the problem for me. I sent Tested-by: tags for
both patches individually.

Thanks,
Guenter

> Maxime
> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
> Maxime Ripard (2):
>       clk: Introduce kunit wrapper around clk_hw_init_rate_request
>       clk: Introduce kunit wrapper around __clk_determine_rate
> 
>  drivers/clk/clk.c            | 51 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk_test.c       |  4 ++--
>  include/linux/clk-provider.h | 21 ++++++++++++++++++
>  3 files changed, 74 insertions(+), 2 deletions(-)
> ---
> base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252
> change-id: 20230721-clk-fix-kunit-lockdep-c5e62b221118
> 
> Best regards,
> -- 
> Maxime Ripard <mripard@kernel.org>
>
  

+kunit-dev

Quoting Maxime Ripard (2023-07-21 00:09:31)
> Hi,
> 
> Here's a small series to address the lockdep warning we have when
> running the clk kunit tests with lockdep enabled.
> 
> For the record, it can be tested with:
> 
> $ ./tools/testing/kunit/kunit.py run \
>     --kunitconfig=drivers/clk \
>     --cross_compile aarch64-linux-gnu- --arch arm64 \
>     --kconfig_add CONFIG_DEBUG_KERNEL=y \
>     --kconfig_add CONFIG_PROVE_LOCKING=y
> 
> Let me know what you think,

Thanks for doing this. I want to roll these helpers into the clk_kunit.c
file that I had created for some other clk tests[1]. That's mostly
because clk.c is already super long and adding kunit code there makes
that problem worse. I'll try to take that patch out of the rest of the
series and then add this series on top and resend.

I don't know what to do about the case where CONFIG_KUNIT=m though. We
have to export clk_prepare_lock/unlock()? I really don't want to do that
even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
was a GPL version of that, so proprietary modules can't get at kernel
internals on kunit enabled kernels.

But I also like the approach taken here of adding a small stub around
the call to make sure a test is running. Maybe I'll make a kunit
namespaced exported gpl symbol that bails if a test isn't running and
calls the clk_prepare_lock/unlock functions inside clk.c and then move
the rest of the code to clk_kunit.c to get something more strict.

[1] https://lore.kernel.org/all/20230327222159.3509818-9-sboyd@kernel.org/
  

On 8/9/23 16:21, Stephen Boyd wrote:
> +kunit-dev
> 
> Quoting Maxime Ripard (2023-07-21 00:09:31)
>> Hi,
>>
>> Here's a small series to address the lockdep warning we have when
>> running the clk kunit tests with lockdep enabled.
>>
>> For the record, it can be tested with:
>>
>> $ ./tools/testing/kunit/kunit.py run \
>>      --kunitconfig=drivers/clk \
>>      --cross_compile aarch64-linux-gnu- --arch arm64 \
>>      --kconfig_add CONFIG_DEBUG_KERNEL=y \
>>      --kconfig_add CONFIG_PROVE_LOCKING=y
>>
>> Let me know what you think,
> 
> Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> file that I had created for some other clk tests[1]. That's mostly
> because clk.c is already super long and adding kunit code there makes
> that problem worse. I'll try to take that patch out of the rest of the
> series and then add this series on top and resend.
> 
> I don't know what to do about the case where CONFIG_KUNIT=m though. We
> have to export clk_prepare_lock/unlock()? I really don't want to do that
> even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> was a GPL version of that, so proprietary modules can't get at kernel
> internals on kunit enabled kernels.
> 

EXPORT_SYMBOL_IF_KUNIT defines a module namespace. You could go a step
further and define a CLK_KUNIT module namespace or similar. That would
of course still permit abuse, but it would have to be _very_ intentional.
There is an EXPORT_SYMBOL_NS_GPL(), so you could further restrict it
to GPL only.

Guenter
  

Quoting Stephen Boyd (2023-08-09 16:21:50)
> +kunit-dev
> 
> Quoting Maxime Ripard (2023-07-21 00:09:31)
> > Hi,
> > 
> > Here's a small series to address the lockdep warning we have when
> > running the clk kunit tests with lockdep enabled.
> > 
> > For the record, it can be tested with:
> > 
> > $ ./tools/testing/kunit/kunit.py run \
> >     --kunitconfig=drivers/clk \
> >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > 
> > Let me know what you think,
> 
> Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> file that I had created for some other clk tests[1]. That's mostly
> because clk.c is already super long and adding kunit code there makes
> that problem worse. I'll try to take that patch out of the rest of the
> series and then add this series on top and resend.
> 
> I don't know what to do about the case where CONFIG_KUNIT=m though. We
> have to export clk_prepare_lock/unlock()? I really don't want to do that
> even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> was a GPL version of that, so proprietary modules can't get at kernel
> internals on kunit enabled kernels.
> 
> But I also like the approach taken here of adding a small stub around
> the call to make sure a test is running. Maybe I'll make a kunit
> namespaced exported gpl symbol that bails if a test isn't running and
> calls the clk_prepare_lock/unlock functions inside clk.c and then move
> the rest of the code to clk_kunit.c to get something more strict.
> 

What if we don't try to do any wrapper or export symbols and test
__clk_determine_rate() how it is called from the clk framework? The
downside is the code is not as simple because we have to check things
from within the clk_ops::determine_rate(), but the upside is that we can
avoid exporting internal clk APIs or wrap them so certain preconditions
are met like requiring them to be called from within a clk_op.

I also find it very odd to call clk_mux_determine_rate_closest() from a
clk that has one parent. Maybe the clk op should call
clk_hw_forward_rate_request() followed by __clk_determine_rate() on the
parent so we can test what the test comment says it wants to test.

-----8<-----
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index a154ec9d0111..b5b4f504b284 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -2155,6 +2155,53 @@ 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 kunit *test;
+	bool determine_rate_called;
+};
+
+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);
+	struct kunit *test = ctx->test;
+
+	KUNIT_ASSERT_EQ(test, req->rate, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, 0, __clk_mux_determine_rate_closest(hw, req));
+
+	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);
+
+	ctx->determine_rate_called = true;
+
+	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,
+};
+
+/*
+ * 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.
+ */
+static void clk_leaf_mux_set_rate_parent_determine_rate(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);
+
+	KUNIT_EXPECT_EQ(test, DUMMY_CLOCK_RATE_2, clk_round_rate(clk, DUMMY_CLOCK_RATE_2));
+	KUNIT_EXPECT_TRUE(test, ctx->determine_rate_called);
+
+	clk_put(clk);
+}
+
+static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
+	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
+	{}
 };
 
 static int
@@ -2168,6 +2215,7 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 	if (!ctx)
 		return -ENOMEM;
 	test->priv = ctx;
+	ctx->test = test;
 
 	ctx->mux_ctx.parents_ctx[0].hw.init = CLK_HW_INIT_NO_PARENT("parent-0",
 								    &clk_dummy_rate_ops,
@@ -2194,7 +2242,7 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test)
 		return ret;
 
 	ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw,
-				      &clk_dummy_single_parent_ops,
+				      &clk_leaf_mux_set_rate_parent_ops,
 				      CLK_SET_RATE_PARENT);
 	ret = clk_hw_register(NULL, &ctx->hw);
 	if (ret)
@@ -2213,40 +2261,6 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
 	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.
- */
-static void clk_leaf_mux_set_rate_parent_determine_rate(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);
-
-	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 struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
-	KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
-	{}
-};
-
 /*
  * 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
  

Hi Stephen,

On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> Quoting Stephen Boyd (2023-08-09 16:21:50)
> > +kunit-dev
> > 
> > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > Hi,
> > > 
> > > Here's a small series to address the lockdep warning we have when
> > > running the clk kunit tests with lockdep enabled.
> > > 
> > > For the record, it can be tested with:
> > > 
> > > $ ./tools/testing/kunit/kunit.py run \
> > >     --kunitconfig=drivers/clk \
> > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > 
> > > Let me know what you think,
> > 
> > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > file that I had created for some other clk tests[1]. That's mostly
> > because clk.c is already super long and adding kunit code there makes
> > that problem worse. I'll try to take that patch out of the rest of the
> > series and then add this series on top and resend.
> > 
> > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > was a GPL version of that, so proprietary modules can't get at kernel
> > internals on kunit enabled kernels.
> > 
> > But I also like the approach taken here of adding a small stub around
> > the call to make sure a test is running. Maybe I'll make a kunit
> > namespaced exported gpl symbol that bails if a test isn't running and
> > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > the rest of the code to clk_kunit.c to get something more strict.
> > 
> 
> What if we don't try to do any wrapper or export symbols and test
> __clk_determine_rate() how it is called from the clk framework? The
> downside is the code is not as simple because we have to check things
> from within the clk_ops::determine_rate(), but the upside is that we can
> avoid exporting internal clk APIs or wrap them so certain preconditions
> are met like requiring them to be called from within a clk_op.

The main reason for that test was linked to commit 262ca38f4b6e ("clk:
Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
hw struct and rate in best_parent_*.

So that test was mostly about making sure that __clk_determine_rate will
properly set the best_parent fields to match the clock's parent.

If we do a proper clock that uses __clk_determine_rate, we lose the
ability to check the content of the clk_rate_request returned by
__clk_determine_rate. It's up to you to tell whether it's a bad thing or
not :)

> I also find it very odd to call clk_mux_determine_rate_closest() from a
> clk that has one parent.

Yeah, the behaviour difference between determine_rate and
determine_rate_closest is weird to me too. We discussed it recently here:
https://lore.kernel.org/linux-clk/mlgxmfim3poke2j45vwh2htkn66hrrjd6ozjebtqhbf4wwljwo@hzi4dyplhdqg/

> Maybe the clk op should call clk_hw_forward_rate_request() followed by
> __clk_determine_rate() on the parent so we can test what the test
> comment says it wants to test.

I guess that would work too :)

Maxime
  

Quoting Maxime Ripard (2023-08-21 04:16:12)
> Hi Stephen,
> 
> On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> > Quoting Stephen Boyd (2023-08-09 16:21:50)
> > > +kunit-dev
> > > 
> > > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > > Hi,
> > > > 
> > > > Here's a small series to address the lockdep warning we have when
> > > > running the clk kunit tests with lockdep enabled.
> > > > 
> > > > For the record, it can be tested with:
> > > > 
> > > > $ ./tools/testing/kunit/kunit.py run \
> > > >     --kunitconfig=drivers/clk \
> > > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > > 
> > > > Let me know what you think,
> > > 
> > > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > > file that I had created for some other clk tests[1]. That's mostly
> > > because clk.c is already super long and adding kunit code there makes
> > > that problem worse. I'll try to take that patch out of the rest of the
> > > series and then add this series on top and resend.
> > > 
> > > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > > was a GPL version of that, so proprietary modules can't get at kernel
> > > internals on kunit enabled kernels.
> > > 
> > > But I also like the approach taken here of adding a small stub around
> > > the call to make sure a test is running. Maybe I'll make a kunit
> > > namespaced exported gpl symbol that bails if a test isn't running and
> > > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > > the rest of the code to clk_kunit.c to get something more strict.
> > > 
> > 
> > What if we don't try to do any wrapper or export symbols and test
> > __clk_determine_rate() how it is called from the clk framework? The
> > downside is the code is not as simple because we have to check things
> > from within the clk_ops::determine_rate(), but the upside is that we can
> > avoid exporting internal clk APIs or wrap them so certain preconditions
> > are met like requiring them to be called from within a clk_op.
> 
> The main reason for that test was linked to commit 262ca38f4b6e ("clk:
> Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
> clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
> hw struct and rate in best_parent_*.
> 
> So that test was mostly about making sure that __clk_determine_rate will
> properly set the best_parent fields to match the clock's parent.
> 
> If we do a proper clock that uses __clk_determine_rate, we lose the
> ability to check the content of the clk_rate_request returned by
> __clk_determine_rate. It's up to you to tell whether it's a bad thing or
> not :)

I'm a little confused here. Was the test trying to check the changed
lines in clk_core_round_rate_nolock() that were made in commit
262ca38f4b6e under the CLK_SET_RATE_PARENT condition? From what I can
tell that doesn't get tested here.

Instead, the test calls __clk_determine_rate() that calls
clk_core_round_rate_nolock() which falls into the clk_core_can_round()
condition because the hw has clk_dummy_single_parent_ops which has
.determine_rate op set to __clk_mux_determine_rate_closest(). Once we
find that the clk can round, we call __clk_mux_determine_rate_closest().
This patch still calls __clk_mux_determine_rate_closest() like
__clk_determine_rate() was doing in the test, and checks that the
request structure has the expected parent in the req->best_parent_hw.

If we wanted to test the changed lines in clk_core_round_rate_nolock()
we should have called __clk_determine_rate() on a clk_hw that didn't
have a .determine_rate or .round_rate clk_op. Then it would have fallen
into the if (core->flags & CLK_SET_RATE_PARENT) condition in
clk_core_round_rate_nolock() and made sure that the request structure
returned was properly forwarded to the parent.

We can still do that by making a child of the leaf, set clk_ops on that
to be this new determine_rate clk op that calls to the parent (the leaf
today), and make that leaf clk not have any determine_rate clk_op. Then
we will fall into the CLK_SET_RATE_PARENT condition and can make sure
the request structure returned points at the parent instead of the mux.

> 
> > I also find it very odd to call clk_mux_determine_rate_closest() from a
> > clk that has one parent.
> 
> Yeah, the behaviour difference between determine_rate and
> determine_rate_closest is weird to me too. We discussed it recently here:
> https://lore.kernel.org/linux-clk/mlgxmfim3poke2j45vwh2htkn66hrrjd6ozjebtqhbf4wwljwo@hzi4dyplhdqg/

Sure, but I'm saying that the clk has one parent, not more than one, so
by definition it isn't a mux. It can only choose one parent. It's odd
that "mux" is in the name.

> 
> > Maybe the clk op should call clk_hw_forward_rate_request() followed by
> > __clk_determine_rate() on the parent so we can test what the test
> > comment says it wants to test.
> 
> I guess that would work too :)
> 

Ok, but I think it doesn't test what was intended to be tested?
  

Hi Stephen,

On Wed, Aug 23, 2023 at 12:50:46PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2023-08-21 04:16:12)
> > Hi Stephen,
> > 
> > On Wed, Aug 09, 2023 at 06:37:30PM -0700, Stephen Boyd wrote:
> > > Quoting Stephen Boyd (2023-08-09 16:21:50)
> > > > +kunit-dev
> > > > 
> > > > Quoting Maxime Ripard (2023-07-21 00:09:31)
> > > > > Hi,
> > > > > 
> > > > > Here's a small series to address the lockdep warning we have when
> > > > > running the clk kunit tests with lockdep enabled.
> > > > > 
> > > > > For the record, it can be tested with:
> > > > > 
> > > > > $ ./tools/testing/kunit/kunit.py run \
> > > > >     --kunitconfig=drivers/clk \
> > > > >     --cross_compile aarch64-linux-gnu- --arch arm64 \
> > > > >     --kconfig_add CONFIG_DEBUG_KERNEL=y \
> > > > >     --kconfig_add CONFIG_PROVE_LOCKING=y
> > > > > 
> > > > > Let me know what you think,
> > > > 
> > > > Thanks for doing this. I want to roll these helpers into the clk_kunit.c
> > > > file that I had created for some other clk tests[1]. That's mostly
> > > > because clk.c is already super long and adding kunit code there makes
> > > > that problem worse. I'll try to take that patch out of the rest of the
> > > > series and then add this series on top and resend.
> > > > 
> > > > I don't know what to do about the case where CONFIG_KUNIT=m though. We
> > > > have to export clk_prepare_lock/unlock()? I really don't want to do that
> > > > even if kunit is enabled (see EXPORT_SYMBOL_IF_KUNIT). Maybe if there
> > > > was a GPL version of that, so proprietary modules can't get at kernel
> > > > internals on kunit enabled kernels.
> > > > 
> > > > But I also like the approach taken here of adding a small stub around
> > > > the call to make sure a test is running. Maybe I'll make a kunit
> > > > namespaced exported gpl symbol that bails if a test isn't running and
> > > > calls the clk_prepare_lock/unlock functions inside clk.c and then move
> > > > the rest of the code to clk_kunit.c to get something more strict.
> > > > 
> > > 
> > > What if we don't try to do any wrapper or export symbols and test
> > > __clk_determine_rate() how it is called from the clk framework? The
> > > downside is the code is not as simple because we have to check things
> > > from within the clk_ops::determine_rate(), but the upside is that we can
> > > avoid exporting internal clk APIs or wrap them so certain preconditions
> > > are met like requiring them to be called from within a clk_op.
> > 
> > The main reason for that test was linked to commit 262ca38f4b6e ("clk:
> > Stop forwarding clk_rate_requests to the parent"). Prior to it, if a
> > clock had CLK_SET_RATE_PARENT, we could end up with its parent's parent
> > hw struct and rate in best_parent_*.
> > 
> > So that test was mostly about making sure that __clk_determine_rate will
> > properly set the best_parent fields to match the clock's parent.
> > 
> > If we do a proper clock that uses __clk_determine_rate, we lose the
> > ability to check the content of the clk_rate_request returned by
> > __clk_determine_rate. It's up to you to tell whether it's a bad thing or
> > not :)
> 
> I'm a little confused here. Was the test trying to check the changed
> lines in clk_core_round_rate_nolock() that were made in commit
> 262ca38f4b6e under the CLK_SET_RATE_PARENT condition?

That's what I was trying to test, yeah. Not necessarily this function in
particular (there's several affected), but at least we would get
something sane in a common case.

> From what I can tell that doesn't get tested here.
> 
> Instead, the test calls __clk_determine_rate() that calls
> clk_core_round_rate_nolock() which falls into the clk_core_can_round()
> condition because the hw has clk_dummy_single_parent_ops which has
> .determine_rate op set to __clk_mux_determine_rate_closest(). Once we
> find that the clk can round, we call __clk_mux_determine_rate_closest().

clk_mux_determine_rate_flags was also affected by the same issue.

> This patch still calls __clk_mux_determine_rate_closest() like
> __clk_determine_rate() was doing in the test, and checks that the
> request structure has the expected parent in the req->best_parent_hw.
> 
> If we wanted to test the changed lines in clk_core_round_rate_nolock()
> we should have called __clk_determine_rate() on a clk_hw that didn't
> have a .determine_rate or .round_rate clk_op. Then it would have fallen
> into the if (core->flags & CLK_SET_RATE_PARENT) condition in
> clk_core_round_rate_nolock() and made sure that the request structure
> returned was properly forwarded to the parent.
>
> We can still do that by making a child of the leaf, set clk_ops on that
> to be this new determine_rate clk op that calls to the parent (the leaf
> today), and make that leaf clk not have any determine_rate clk_op. Then
> we will fall into the CLK_SET_RATE_PARENT condition and can make sure
> the request structure returned points at the parent instead of the mux.

But you're right clk_core_round_rate_nolock() wasn't properly tested. I
guess, if we find it worth it, we should add a test for that one too?

clk_mux_determine_rate_flags with multiple parents and
CLK_SET_RATE_PARENT was also affected and fixed in the commit mentioned
above.

Maxime