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

[Stephen Boyd <sboyd@kernel.org>]

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