| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6534e4c349253da8ee467ffeda8221ed.sboyd@kernel.org>
Date: Wed, 09 Aug 2023 18:37:30 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Maxime Ripard <mripard@...nel.org>,
Michael Turquette <mturquette@...libre.com>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
Maxime Ripard <mripard@...nel.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 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
Powered by blists - more mailing lists