| Message ID | 20230721-clk-fix-kunit-lockdep-v1-0-32cdba4c8fc1@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: kunit: Fix the lockdep warnings | expand |
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
Quoting Maxime Ripard (2023-08-24 02:56:38) > 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, > > > > > > 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. Cool. Thanks for confirming. > > > 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. Ok. I see. > > > 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. > Makes sense. I've made a set of these tests that call some function, like __clk_determine_rate() or __clk_mux_determine_rate(), from the determine_rate clk_op of the leaf. The clk_hw pointer passed to the function under test is the parent of the clk (the intermediate empty clk_ops clk). The parent of that intermediate clk is the mux. I noticed that sometimes the parent_hw wasn't set to the mux_ctx if I didn't call clk_hw_forward_rate_request() in the clk_op. That's because functions like __clk_determine_rate() assume the best_parent_hw has been set already and simply skip setting it at all. It's possible that a driver may fail to call clk_hw_forward_rate_request() before calling some determine rate function on the parent. Looks like a pitfall but I'm not sure how to make it any better. I have this as two patches in my local tree. I'll send it out tomorrow. ----8<---- diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index a154ec9d0111..99b9f01ada71 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -2155,8 +2155,35 @@ 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 clk_hw parent; + struct clk_rate_request req; + int (*determine_rate_func)(struct clk_hw *, struct clk_rate_request *); }; +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); + int ret; + struct clk_rate_request *parent_req = &ctx->req; + + clk_hw_forward_rate_request(hw, req, req->best_parent_hw, parent_req, req->rate); + ret = ctx->determine_rate_func(req->best_parent_hw, parent_req); + if (ret) + return ret; + + req->rate = parent_req->rate; + + 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, +}; + +static const struct clk_ops empty_clk_ops = { }; + static int clk_leaf_mux_set_rate_parent_test_init(struct kunit *test) { @@ -2193,8 +2220,15 @@ clk_leaf_mux_set_rate_parent_test_init(struct kunit *test) if (ret) return ret; - ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->mux_ctx.hw, - &clk_dummy_single_parent_ops, + ctx->parent.init = CLK_HW_INIT_HW("test-parent", &ctx->mux_ctx.hw, + &empty_clk_ops, + CLK_SET_RATE_PARENT); + ret = clk_hw_register(NULL, &ctx->parent); + if (ret) + return ret; + + ctx->hw.init = CLK_HW_INIT_HW("test-clock", &ctx->parent, + &clk_leaf_mux_set_rate_parent_ops, CLK_SET_RATE_PARENT); ret = clk_hw_register(NULL, &ctx->hw); if (ret) @@ -2208,50 +2242,112 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test) struct clk_leaf_mux_ctx *ctx = test->priv; clk_hw_unregister(&ctx->hw); + clk_hw_unregister(&ctx->parent); clk_hw_unregister(&ctx->mux_ctx.hw); clk_hw_unregister(&ctx->mux_ctx.parents_ctx[0].hw); clk_hw_unregister(&ctx->mux_ctx.parents_ctx[1].hw); } +struct clk_leaf_mux_set_rate_parent_determine_rate_test_case { + const char *desc; + int (*determine_rate_func)(struct clk_hw *, struct clk_rate_request *); +}; + +static void +clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc( + const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *t, char *desc) +{ + strcpy(desc, t->desc); +} + +static const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case +clk_leaf_mux_set_rate_parent_determine_rate_test_cases[] = { + { + /* + * Test that __clk_determine_rate() on the parent that can't + * change rate doesn't return a clk_rate_request structure with + * the best_parent_hw pointer pointing to the parent. + */ + .desc = "clk_leaf_mux_set_rate_parent__clk_determine_rate_proper_parent", + .determine_rate_func = __clk_determine_rate, + }, + { + /* + * Test that __clk_mux_determine_rate() on the parent that + * can't change rate doesn't return a clk_rate_request + * structure with the best_parent_hw pointer pointing to + * the parent. + */ + .desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_proper_parent", + .determine_rate_func = __clk_mux_determine_rate, + }, + { + /* + * Test that __clk_mux_determine_rate_closest() on the parent + * that can't change rate doesn't return a clk_rate_request + * structure with the best_parent_hw pointer pointing to + * the parent. + */ + .desc = "clk_leaf_mux_set_rate_parent__clk_mux_determine_rate_closest_proper_parent", + .determine_rate_func = __clk_mux_determine_rate_closest, + }, + { + /* + * Test that clk_hw_determine_rate_no_reparent() on the parent + * that can't change rate doesn't return a clk_rate_request + * structure with the best_parent_hw pointer pointing to + * the parent. + */ + .desc = "clk_leaf_mux_set_rate_parent_clk_hw_determine_rate_no_reparent_proper_parent", + .determine_rate_func = clk_hw_determine_rate_no_reparent, + }, +}; + +KUNIT_ARRAY_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test, + clk_leaf_mux_set_rate_parent_determine_rate_test_cases, + clk_leaf_mux_set_rate_parent_determine_rate_test_case_to_desc) + /* - * 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. + * Test that when a clk that can't change rate itself calls a function like + * __clk_determine_rate() on its parent it doesn't get back a clk_rate_request + * structure that has the best_parent_hw pointer point to the clk_hw passed + * into the determine rate function. See commit 262ca38f4b6e ("clk: Stop + * forwarding clk_rate_requests to the parent") for more background. */ -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test) +static void clk_leaf_mux_set_rate_parent_determine_rate_test(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; + struct clk_rate_request *req = &ctx->req; unsigned long rate; - int ret; + const struct clk_leaf_mux_set_rate_parent_determine_rate_test_case *test_param; + + test_param = test->param_value; + ctx->determine_rate_func = test_param->determine_rate_func; rate = clk_get_rate(clk); KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1); + KUNIT_ASSERT_EQ(test, DUMMY_CLOCK_RATE_2, clk_round_rate(clk, DUMMY_CLOCK_RATE_2)); - 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); + 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), + KUNIT_CASE_PARAM(clk_leaf_mux_set_rate_parent_determine_rate_test, + clk_leaf_mux_set_rate_parent_determine_rate_test_gen_params), {} }; /* - * 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 - * requests to the mux, which will then select which parent is the best - * fit for a given rate. + * Test suite for a clock whose parent is a pass-through clk whose parent is a + * mux with multiple parents. The leaf and pass-through clocks have the + * CLK_SET_RATE_PARENT flag, and will forward rate requests to the mux, which + * will then select which parent is the best fit for a given rate. * * These tests exercise the behaviour of muxes, and the proper selection * of parents.
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,