Messages in this thread Patch in this message | | | Subject | Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings | From | Stephen Boyd <> | Date | Wed, 09 Aug 2023 18:37:30 -0700 |
| |
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
| |