Message-ID: <b8a6b1157012a6171c7d11a9bcc4a57d.sboyd@kernel.org>
Date:   Mon, 11 Sep 2023 17:53:44 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Maxime Ripard <mripard@...nel.org>
Cc:     Michael Turquette <mturquette@...libre.com>,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        Guenter Roeck <linux@...ck-us.net>,
        kernel test robot <yujie.liu@...el.com>,
        kunit-dev@...glegroups.com
Subject: Re: [PATCH 0/2] clk: kunit: Fix the lockdep warnings

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.

Powered by blists - more mailing lists