public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/12] clk: Unit test clk unregistration paths
@ 2024-08-15  0:55 Stephen Boyd
  2024-08-15  0:55 ` [PATCH 01/12] clk: Fix clk not being unlinked from consumers list Stephen Boyd
                   ` (11 more replies)
  0 siblings, 12 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Brendan Higgins, David Gow,
	Rae Moar

The clk_hw_unregister() path is rarely used and almost entirely
untested. Case in point, the first patch in this series that fixes a bug
due to unregistering a clk_hw and calling clk_put(). Add unit tests for
this code and test the consumer APIs to make sure the behavior stays
consistent.

The last patch is a WIP because I haven't gotten around to fixing all
the problems.

Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>

Nuno Sá (1):
  clk: Fix clk not being unlinked from consumers list

Stephen Boyd (11):
  clk: test: Introduce clk_hw_unregister_kunit()
  clk: test: Introduce clk_put_kunit()
  clk: Add tests for unregistering clk_hw and using consumer APIs
  clk: Fail phase APIs after clk_hw is unregistered
  clk: Test clk_get_phase() behavior after clk_hw is unregistered
  clk: Fail duty cycle APIs after clk_hw is unregistered
  clk: Test clk_set_duty_cycle() behavior after clk_hw is unregistered
  clk: Prevent unregistered clk_hw from being reinserted into clk tree
  clk: Test clk_set_parent() behavior after clk_hw is unregistered
  clk: Test parent/clk flags combos while unregistering a clk_hw
  WIP: clk: Test behavior of children clks after a parent is
    unregistered

 drivers/clk/clk.c               |  26 +-
 drivers/clk/clk_kunit_helpers.c |  46 ++
 drivers/clk/clk_test.c          | 720 ++++++++++++++++++++++++++++++++
 include/kunit/clk.h             |   2 +
 4 files changed, 791 insertions(+), 3 deletions(-)


base-commit: 274aff8711b2e77c27bbda0ddc24caa39f154bfa
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 01/12] clk: Fix clk not being unlinked from consumers list
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 02/12] clk: test: Introduce clk_hw_unregister_kunit() Stephen Boyd
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Nuno Sá, linux-kernel, linux-clk, patches

From: Nuno Sá <nuno.sa@analog.com>

When a clk_hw is registered we add a struct clk handle to it's
consumers list. This handle is created in '__clk_register()' per the
'alloc_clk()' call.

As such, we need to remove this handle when unregistering the
clk_hw. This can actually lead to a use after free if a provider gets
removed before a consumer. When removing the consumer, '__clk_put()' is
called and that will do 'hlist_del(&clk->clks_node)' which will touch in
already freed memory.

Fixes: 1df4046a93e0 ("clk: Combine __clk_get() and __clk_create_clk()")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Link: https://lore.kernel.org/r/20240710-dev-clk-misc-v1-1-cd9d960099a2@analog.com
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 285ed1ad8a37..f5415aa70f81 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4606,6 +4606,8 @@ void clk_unregister(struct clk *clk)
 	if (clk->core->protect_count)
 		pr_warn("%s: unregistering protected clock: %s\n",
 					__func__, clk->core->name);
+
+	clk_core_unlink_consumer(clk);
 	clk_prepare_unlock();
 
 	kref_put(&clk->core->ref, __clk_release);
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 02/12] clk: test: Introduce clk_hw_unregister_kunit()
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
  2024-08-15  0:55 ` [PATCH 01/12] clk: Fix clk not being unlinked from consumers list Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 03/12] clk: test: Introduce clk_put_kunit() Stephen Boyd
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Brendan Higgins, David Gow,
	Rae Moar, Nuno Sá

Tests will need to unregister a clk_hw at a specific point during the
test. Add a clk_hw_unregister_kunit() function that releases the kunit
action created in a function like clk_hw_register_kunit() so that code
is kept tidy.

Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---

I need to introduce a kunit_release_action_or_fail() API.

 drivers/clk/clk_kunit_helpers.c | 25 +++++++++++++++++++++++++
 include/kunit/clk.h             |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/clk/clk_kunit_helpers.c b/drivers/clk/clk_kunit_helpers.c
index 52fd25594c96..87f97329ae0c 100644
--- a/drivers/clk/clk_kunit_helpers.c
+++ b/drivers/clk/clk_kunit_helpers.c
@@ -203,5 +203,30 @@ int of_clk_hw_register_kunit(struct kunit *test, struct device_node *node, struc
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_register_kunit);
 
+/**
+ * clk_hw_unregister_kunit() - Force a clk_hw to be unregistered that is
+ * managed by {of_}clk_hw_register_kunit()
+ * @test: The test context
+ * @hw: link to hardware-specific clock data passed to {of_}clk_hw_register_kunit()
+ *
+ * Just like clk_hw_unregister(), except that the clk_hw must have been registered by
+ * a KUnit helper like clk_hw_register_kunit(). This removes the deferred KUnit action
+ * made earlier in the test and unregisters the clk immediately.
+ *
+ * Usually a test will use clk_hw_register_kunit() and let the deferred action
+ * unregister the clk automatically after the test case concludes. This
+ * function can be used to unregister the clk_hw immediately.
+ *
+ * Return: 0 on success or a negative errno value on failure to find the deferred action.
+ */
+int clk_hw_unregister_kunit(struct kunit *test, struct clk_hw *hw)
+{
+	kunit_release_action(test, clk_hw_unregister_wrapper, hw);
+	/* TODO: Find the action and fail */
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_hw_unregister_kunit);
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("KUnit helpers for clk providers and consumers");
diff --git a/include/kunit/clk.h b/include/kunit/clk.h
index 73bc99cefe7b..c513fec8099b 100644
--- a/include/kunit/clk.h
+++ b/include/kunit/clk.h
@@ -24,5 +24,6 @@ int clk_prepare_enable_kunit(struct kunit *test, struct clk *clk);
 int clk_hw_register_kunit(struct kunit *test, struct device *dev, struct clk_hw *hw);
 int of_clk_hw_register_kunit(struct kunit *test, struct device_node *node,
 			     struct clk_hw *hw);
+int clk_hw_unregister_kunit(struct kunit *test, struct clk_hw *hw);
 
 #endif
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 03/12] clk: test: Introduce clk_put_kunit()
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
  2024-08-15  0:55 ` [PATCH 01/12] clk: Fix clk not being unlinked from consumers list Stephen Boyd
  2024-08-15  0:55 ` [PATCH 02/12] clk: test: Introduce clk_hw_unregister_kunit() Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 04/12] clk: Add tests for unregistering clk_hw and using consumer APIs Stephen Boyd
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Brendan Higgins, David Gow,
	Rae Moar, Nuno Sá

Tests will need to put a clk at a specific point during the test. Add a
clk_put_kunit() function that releases the kunit action created by a
function like clk_get_kunit() so that code is kept tidy.

Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_kunit_helpers.c | 21 +++++++++++++++++++++
 include/kunit/clk.h             |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/clk/clk_kunit_helpers.c b/drivers/clk/clk_kunit_helpers.c
index 87f97329ae0c..ddb9f2de1083 100644
--- a/drivers/clk/clk_kunit_helpers.c
+++ b/drivers/clk/clk_kunit_helpers.c
@@ -153,6 +153,27 @@ clk_hw_get_clk_prepared_enabled_kunit(struct kunit *test, struct clk_hw *hw,
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_clk_prepared_enabled_kunit);
 
+/**
+ * clk_put_kunit() - Force a clk_put() call on a clk managed by the @test
+ * @test: The test context
+ * @clk: Clk consumer to put with clk_put()
+ *
+ * Just like clk_put(), except the clk must be managed by the @test, i.e.
+ * returned from something like clk_get_kunit(). clk_put() will be called
+ * immediately and the deferred action will be removed.
+ *
+ * Return: 0 on success or a negative errno value on failure to find the
+ * deferred action.
+ */
+int clk_put_kunit(struct kunit *test, struct clk *clk)
+{
+	kunit_release_action(test, clk_put_wrapper, clk);
+	/* TODO: Find the action and fail */
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(clk_put_kunit);
+
 KUNIT_DEFINE_ACTION_WRAPPER(clk_hw_unregister_wrapper,
 			    clk_hw_unregister, struct clk_hw *);
 
diff --git a/include/kunit/clk.h b/include/kunit/clk.h
index c513fec8099b..d37fa6b87c2b 100644
--- a/include/kunit/clk.h
+++ b/include/kunit/clk.h
@@ -18,6 +18,7 @@ clk_hw_get_clk_kunit(struct kunit *test, struct clk_hw *hw, const char *con_id);
 struct clk *
 clk_hw_get_clk_prepared_enabled_kunit(struct kunit *test, struct clk_hw *hw,
 				      const char *con_id);
+int clk_put_kunit(struct kunit *test, struct clk *clk);
 
 int clk_prepare_enable_kunit(struct kunit *test, struct clk *clk);
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 04/12] clk: Add tests for unregistering clk_hw and using consumer APIs
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (2 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 03/12] clk: test: Introduce clk_put_kunit() Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 05/12] clk: Fail phase APIs after clk_hw is unregistered Stephen Boyd
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

Test the clk consumer APIs after unregistering the underlying clk_hw
that provides the clk_ops for the clk held by the consumer. The clk
framework replaces the clk_ops when the clk_hw has been unregistered
with functions that fail. Ensure that clk API behavior is consistent by
testing the APIs to make sure the original clk_ops aren't called and
that the clk APIs return failure as expected.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 298 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 298 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 41fc8eba3418..c06971fe9922 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3108,6 +3108,303 @@ static struct kunit_suite clk_register_clk_parent_data_device_suite = {
 	.test_cases = clk_register_clk_parent_data_device_test_cases,
 };
 
+struct clk_unregister_consumer_clk_ctx {
+	struct kunit *test;
+	struct clk_hw hw;
+	bool unregistered;
+	unsigned long rate;
+	unsigned long accuracy;
+	struct clk *clk;
+};
+
+/* Unregister the clk and mark it as unregistered for the tests. */
+static void clk_unregister_consumer_clk_unregister(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_kunit(test, &ctx->hw));
+	ctx->unregistered = true;
+}
+
+/* Test that clk_put() can be called after the clk_hw has been unregistered. */
+static void clk_unregister_consumer_clk_put(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_EQ(test, 0, clk_put_kunit(test, ctx->clk));
+}
+
+/* Test that clk_prepare() fails after the clk_hw has been unregistered. */
+static void clk_unregister_consumer_clk_prepare_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_prepare(ctx->clk));
+}
+
+/*
+ * Test that clk_unprepare() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_unprepare_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, 0, clk_prepare(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	clk_unprepare(ctx->clk);
+}
+
+/* Test that clk_enable() fails after the clk_hw has been unregistered. */
+static void clk_unregister_consumer_clk_enable_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, 0, clk_prepare(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_enable(ctx->clk));
+	clk_unprepare(ctx->clk);
+}
+
+/*
+ * Test that clk_disable() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_disable_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, 0, clk_prepare(ctx->clk));
+	KUNIT_ASSERT_EQ(test, 0, clk_enable(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	clk_disable(ctx->clk);
+	clk_unprepare(ctx->clk);
+}
+
+/*
+ * Test that clk_round_rate() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_round_rate_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const unsigned long test_rate = 10200;
+
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_round_rate(ctx->clk, test_rate));
+}
+
+/*
+ * Test that clk_set_rate() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_set_rate_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const unsigned long test_rate = 38000;
+
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_set_rate(ctx->clk, test_rate));
+	KUNIT_EXPECT_NE(test, ctx->rate, test_rate);
+}
+
+/*
+ * Test that clk_get_rate() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_get_rate_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, ctx->rate, clk_get_rate(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_EQ(test, ctx->rate, clk_get_rate(ctx->clk));
+}
+
+/*
+ * Test that clk_get_accuracy() doesn't call the clk_op after the clk_hw has been
+ * unregistered.
+ */
+static void clk_unregister_consumer_clk_get_accuracy_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
+}
+
+static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
+	KUNIT_CASE(clk_unregister_consumer_clk_prepare_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_unprepare_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_enable_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_disable_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_round_rate_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_set_rate_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_get_rate_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_get_accuracy_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_put),
+	{}
+};
+
+static void clk_unregister_consumer_clk_clk_op_called(struct clk_hw *hw,
+						       const char *clk_op_name)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+
+	/*
+	 * This code should never be reached because the clk_hw has been
+	 * unregistered.
+	 */
+	KUNIT_EXPECT_FALSE_MSG(ctx->test, ctx->unregistered,
+			       "clk_op %s was called for unregistered clk\n",
+			       clk_op_name);
+}
+
+static int clk_unregister_consumer_clk_op_prepare(struct clk_hw *hw)
+{
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return 0;
+}
+
+static void clk_unregister_consumer_clk_op_unprepare(struct clk_hw *hw)
+{
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+}
+
+static int clk_unregister_consumer_clk_op_enable(struct clk_hw *hw)
+{
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return 0;
+}
+
+static void clk_unregister_consumer_clk_op_disable(struct clk_hw *hw)
+{
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+}
+
+static unsigned long
+clk_unregister_consumer_clk_op_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return ctx->rate;
+}
+
+static int
+clk_unregister_consumer_clk_op_determine_rate(struct clk_hw *hw,
+					       struct clk_rate_request *req)
+{
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return 0;
+}
+
+static long
+clk_unregister_consumer_clk_op_round_rate(struct clk_hw *hw, unsigned long rate,
+					   unsigned long *parent_rate)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return ctx->rate;
+}
+
+static int
+clk_unregister_consumer_clk_op_set_rate(struct clk_hw *hw, unsigned long rate,
+					 unsigned long parent_rate)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	ctx->rate = rate;
+
+	return 0;
+}
+
+static unsigned long
+clk_unregister_consumer_clk_op_recalc_accuracy(struct clk_hw *hw,
+						unsigned long parent_accuracy)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return ctx->accuracy;
+}
+
+static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
+	.prepare = clk_unregister_consumer_clk_op_prepare,
+	.unprepare = clk_unregister_consumer_clk_op_unprepare,
+	.enable = clk_unregister_consumer_clk_op_enable,
+	.disable = clk_unregister_consumer_clk_op_disable,
+	.recalc_rate = clk_unregister_consumer_clk_op_recalc_rate,
+	.round_rate = clk_unregister_consumer_clk_op_round_rate,
+	.determine_rate = clk_unregister_consumer_clk_op_determine_rate,
+	.set_rate = clk_unregister_consumer_clk_op_set_rate,
+	.recalc_accuracy = clk_unregister_consumer_clk_op_recalc_accuracy,
+};
+
+static int clk_unregister_consumer_clk_init(struct kunit *test)
+{
+	struct clk *clk;
+	struct clk_init_data init = { };
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+	test->priv = ctx;
+	ctx->test = test;
+
+	init.name = "unregister_consumer_clk_test_clk";
+	init.ops = &clk_unregister_consumer_clk_clk_ops;
+	ctx->hw.init = &init;
+
+	ctx->rate = 42;
+	ctx->accuracy = 34;
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, &ctx->hw));
+
+	clk = clk_hw_get_clk_kunit(test, &ctx->hw, __func__);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk);
+	ctx->clk = clk;
+
+	return 0;
+}
+
+/*
+ * Test suite for clk consumer API behavior after the clk_hw has been
+ * unregistered. The clk consumer APIs should return failure and the clk_ops
+ * shouldn't be called.
+ */
+static struct kunit_suite clk_unregister_consumer_clk_suite = {
+	.name = "clk_unregister_consumer_clk",
+	.init = clk_unregister_consumer_clk_init,
+	.test_cases = clk_unregister_consumer_clk_test_cases,
+};
+
 kunit_test_suites(
 	&clk_leaf_mux_set_rate_parent_test_suite,
 	&clk_test_suite,
@@ -3124,6 +3421,7 @@ kunit_test_suites(
 	&clk_register_clk_parent_data_device_suite,
 	&clk_single_parent_mux_test_suite,
 	&clk_uncached_test_suite,
+	&clk_unregister_consumer_clk_suite,
 );
 MODULE_DESCRIPTION("Kunit tests for clk framework");
 MODULE_LICENSE("GPL v2");
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 05/12] clk: Fail phase APIs after clk_hw is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (3 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 04/12] clk: Add tests for unregistering clk_hw and using consumer APIs Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 06/12] clk: Test clk_get_phase() behavior " Stephen Boyd
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

The clk phase APIs don't fail when a clk_hw is unregistered. Add some
nodrv clk_ops that return failure so that clk_get_phase() starts failing
when a clk_hw is unregistered.

Cc: Nuno Sá <nuno.sa@analog.com>
Fixes: e59c5371fb9d ("clk: introduce clk_set_phase function & callback")
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f5415aa70f81..8909294cc52e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4486,7 +4486,7 @@ EXPORT_SYMBOL_GPL(of_clk_hw_register);
  * after clk_unregister() was called on a clock and until last clock
  * consumer calls clk_put() and the struct clk object is freed.
  */
-static int clk_nodrv_prepare_enable(struct clk_hw *hw)
+static int clk_nodrv_prepare_enable_get_phase(struct clk_hw *hw)
 {
 	return -ENXIO;
 }
@@ -4513,14 +4513,21 @@ static int clk_nodrv_determine_rate(struct clk_hw *hw,
 	return -ENXIO;
 }
 
+static int clk_nodrv_set_phase(struct clk_hw *hw, int degrees)
+{
+	return -ENXIO;
+}
+
 static const struct clk_ops clk_nodrv_ops = {
-	.enable		= clk_nodrv_prepare_enable,
+	.enable		= clk_nodrv_prepare_enable_get_phase,
 	.disable	= clk_nodrv_disable_unprepare,
-	.prepare	= clk_nodrv_prepare_enable,
+	.prepare	= clk_nodrv_prepare_enable_get_phase,
 	.unprepare	= clk_nodrv_disable_unprepare,
 	.determine_rate	= clk_nodrv_determine_rate,
 	.set_rate	= clk_nodrv_set_rate,
 	.set_parent	= clk_nodrv_set_parent,
+	.get_phase	= clk_nodrv_prepare_enable_get_phase,
+	.set_phase	= clk_nodrv_set_phase,
 };
 
 static void clk_core_evict_parent_cache_subtree(struct clk_core *root,
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 06/12] clk: Test clk_get_phase() behavior after clk_hw is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (4 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 05/12] clk: Fail phase APIs after clk_hw is unregistered Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 07/12] clk: Fail duty cycle APIs " Stephen Boyd
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

The previous commit fixed clk_get_phase() so that it returns failure
after a clk_hw is unregistered. Add a test for that so that stays true.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 62 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index c06971fe9922..f97d13bb0242 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3114,6 +3114,7 @@ struct clk_unregister_consumer_clk_ctx {
 	bool unregistered;
 	unsigned long rate;
 	unsigned long accuracy;
+	int phase;
 	struct clk *clk;
 };
 
@@ -3245,6 +3246,38 @@ static void clk_unregister_consumer_clk_get_accuracy_skips(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
 }
 
+/*
+ * Test that clk_set_phase() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns failure.
+ */
+static void clk_unregister_consumer_clk_set_phase_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const int degrees = 12;
+
+	KUNIT_ASSERT_NE(test, degrees, clk_get_phase(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_set_phase(ctx->clk, degrees));
+	/* Phase is unchanged */
+	KUNIT_EXPECT_NE(test, degrees, clk_get_phase(ctx->clk));
+}
+
+/*
+ * Test that clk_get_phase() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns 0.
+ */
+static void clk_unregister_consumer_clk_get_phase_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const int phase = ctx->phase;
+
+	KUNIT_ASSERT_EQ(test, phase, clk_get_phase(ctx->clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_get_phase(ctx->clk));
+}
+
 static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_prepare_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_unprepare_skips),
@@ -3254,6 +3287,8 @@ static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_set_rate_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_get_rate_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_get_accuracy_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_set_phase_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_get_phase_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_put),
 	{}
 };
@@ -3356,6 +3391,30 @@ clk_unregister_consumer_clk_op_recalc_accuracy(struct clk_hw *hw,
 	return ctx->accuracy;
 }
 
+static int clk_unregister_consumer_clk_op_get_phase(struct clk_hw *hw)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return ctx->phase;
+}
+
+static int
+clk_unregister_consumer_clk_op_set_phase(struct clk_hw *hw, int degrees)
+{
+
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	ctx->phase = degrees;
+
+	return 0;
+}
+
 static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
 	.prepare = clk_unregister_consumer_clk_op_prepare,
 	.unprepare = clk_unregister_consumer_clk_op_unprepare,
@@ -3366,6 +3425,8 @@ static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
 	.determine_rate = clk_unregister_consumer_clk_op_determine_rate,
 	.set_rate = clk_unregister_consumer_clk_op_set_rate,
 	.recalc_accuracy = clk_unregister_consumer_clk_op_recalc_accuracy,
+	.get_phase = clk_unregister_consumer_clk_op_get_phase,
+	.set_phase = clk_unregister_consumer_clk_op_set_phase,
 };
 
 static int clk_unregister_consumer_clk_init(struct kunit *test)
@@ -3385,6 +3446,7 @@ static int clk_unregister_consumer_clk_init(struct kunit *test)
 
 	ctx->rate = 42;
 	ctx->accuracy = 34;
+	ctx->phase = 90;
 	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, &ctx->hw));
 
 	clk = clk_hw_get_clk_kunit(test, &ctx->hw, __func__);
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 07/12] clk: Fail duty cycle APIs after clk_hw is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (5 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 06/12] clk: Test clk_get_phase() behavior " Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 08/12] clk: Test clk_set_duty_cycle() behavior " Stephen Boyd
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

The clk duty cycle APIs don't fail when a clk_hw is unregistered. Add a
nodrv clk_op that returns failure so that clk_set_duty_cycle() starts
failing after a clk_hw is unregistered.

Cc: Nuno Sá <nuno.sa@analog.com>
Fixes: 9fba738a53dd ("clk: add duty cycle support")
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8909294cc52e..a0c275e156ad 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4518,6 +4518,11 @@ static int clk_nodrv_set_phase(struct clk_hw *hw, int degrees)
 	return -ENXIO;
 }
 
+static int clk_nodrv_set_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
+{
+	return -ENXIO;
+}
+
 static const struct clk_ops clk_nodrv_ops = {
 	.enable		= clk_nodrv_prepare_enable_get_phase,
 	.disable	= clk_nodrv_disable_unprepare,
@@ -4528,6 +4533,7 @@ static const struct clk_ops clk_nodrv_ops = {
 	.set_parent	= clk_nodrv_set_parent,
 	.get_phase	= clk_nodrv_prepare_enable_get_phase,
 	.set_phase	= clk_nodrv_set_phase,
+	.set_duty_cycle	= clk_nodrv_set_duty_cycle,
 };
 
 static void clk_core_evict_parent_cache_subtree(struct clk_core *root,
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 08/12] clk: Test clk_set_duty_cycle() behavior after clk_hw is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (6 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 07/12] clk: Fail duty cycle APIs " Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 09/12] clk: Prevent unregistered clk_hw from being reinserted into clk tree Stephen Boyd
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

The previous commit fixed clk_set_duty_cycle() so that it returns
failure after a clk_hw is unregistered. Add a test for that so that
stays true.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index f97d13bb0242..1fea29f93b2a 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3115,6 +3115,7 @@ struct clk_unregister_consumer_clk_ctx {
 	unsigned long rate;
 	unsigned long accuracy;
 	int phase;
+	struct clk_duty duty;
 	struct clk *clk;
 };
 
@@ -3278,6 +3279,42 @@ static void clk_unregister_consumer_clk_get_phase_skips(struct kunit *test)
 	KUNIT_EXPECT_GT(test, 0, clk_get_phase(ctx->clk));
 }
 
+/*
+ * Test that clk_set_duty_cycle() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns failure.
+ */
+static void clk_unregister_consumer_clk_set_duty_cycle_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const int num = 20, den = 30;
+
+	KUNIT_ASSERT_NE(test, ctx->duty.num, num);
+	KUNIT_ASSERT_NE(test, ctx->duty.den, den);
+	KUNIT_ASSERT_EQ(test, ctx->duty.num, clk_get_scaled_duty_cycle(ctx->clk, ctx->duty.den));
+	clk_unregister_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_GT(test, 0, clk_set_duty_cycle(ctx->clk, num, den));
+	/* Duty cycle is unchanged */
+	KUNIT_EXPECT_EQ(test, ctx->duty.num, clk_get_scaled_duty_cycle(ctx->clk, ctx->duty.den));
+}
+
+/*
+ * Test that clk_get_scaled_duty_cycle() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns original duty cycle.
+ */
+static void clk_unregister_consumer_clk_get_duty_cycle_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const int num = ctx->duty.num;
+	const int den = ctx->duty.den;
+
+	KUNIT_ASSERT_EQ(test, num, clk_get_scaled_duty_cycle(ctx->clk, den));
+	clk_unregister_consumer_clk_unregister(test);
+
+	/* Duty cycle is unchanged */
+	KUNIT_EXPECT_EQ(test, num, clk_get_scaled_duty_cycle(ctx->clk, den));
+}
+
 static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_prepare_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_unprepare_skips),
@@ -3289,6 +3326,8 @@ static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_get_accuracy_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_set_phase_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_get_phase_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_set_duty_cycle_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_get_duty_cycle_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_put),
 	{}
 };
@@ -3415,6 +3454,33 @@ clk_unregister_consumer_clk_op_set_phase(struct clk_hw *hw, int degrees)
 	return 0;
 }
 
+static int clk_unregister_consumer_clk_op_get_duty_cycle(struct clk_hw *hw,
+							  struct clk_duty *duty)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	*duty = ctx->duty;
+
+	return 0;
+}
+
+static int clk_unregister_consumer_clk_op_set_duty_cycle(struct clk_hw *hw,
+							  struct clk_duty *duty)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	ctx->duty.num = duty->num;
+	ctx->duty.den = duty->den;
+
+	return 0;
+}
+
 static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
 	.prepare = clk_unregister_consumer_clk_op_prepare,
 	.unprepare = clk_unregister_consumer_clk_op_unprepare,
@@ -3427,6 +3493,8 @@ static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
 	.recalc_accuracy = clk_unregister_consumer_clk_op_recalc_accuracy,
 	.get_phase = clk_unregister_consumer_clk_op_get_phase,
 	.set_phase = clk_unregister_consumer_clk_op_set_phase,
+	.get_duty_cycle = clk_unregister_consumer_clk_op_get_duty_cycle,
+	.set_duty_cycle = clk_unregister_consumer_clk_op_set_duty_cycle,
 };
 
 static int clk_unregister_consumer_clk_init(struct kunit *test)
@@ -3447,6 +3515,8 @@ static int clk_unregister_consumer_clk_init(struct kunit *test)
 	ctx->rate = 42;
 	ctx->accuracy = 34;
 	ctx->phase = 90;
+	ctx->duty.num = 50;
+	ctx->duty.den = 100;
 	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, &ctx->hw));
 
 	clk = clk_hw_get_clk_kunit(test, &ctx->hw, __func__);
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 09/12] clk: Prevent unregistered clk_hw from being reinserted into clk tree
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (7 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 08/12] clk: Test clk_set_duty_cycle() behavior " Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 10/12] clk: Test clk_set_parent() behavior after clk_hw is unregistered Stephen Boyd
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá,
	Sylwester Nawrocki

The clk framework removes the clk_core from the clk tree by deleting the
'child_node' hlist for a clk_hw when it is unregistered in
clk_unregister(). This removes the clk from the clk tree, but a call to
clk_set_parent() will allow the clk to be added right back into the clk
tree, undoing all the work done in clk_unregister() to remove it from
the tree and orphan all descendants. This results in a double hlist
delete as well, because clk_reparent() deletes the 'child_node' hlist
before adding the clk to either the orphan list or the clk tree.

The 'clk_nodrv_ops' have a set_parent clk_op that returns failure, but
that still won't save us even if we fix the clk_reparent() code to
operate on an empty list. That's because we'll reparent the clk back to
the original parent when the clk_op returns an error, effectively
putting the clk back into the clk tree.

Force the number of parents to be zero in clk_unregister() so that
clk_set_parent() can't get past the part where it figures out which
index to use to call the clk_op with.

Cc: Nuno Sá <nuno.sa@analog.com>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Fixes: fcb0ee6a3d33 ("clk: Implement clk_unregister")
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a0c275e156ad..d99d2d4dd411 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4611,6 +4611,11 @@ void clk_unregister(struct clk *clk)
 	clk_core_evict_parent_cache(clk->core);
 
 	hlist_del_init(&clk->core->child_node);
+	/*
+	 * Prevent clk from being reinserted into the clk tree via
+	 * clk_set_parent()
+	 */
+	clk->core->num_parents = 0;
 
 	if (clk->core->prepare_count)
 		pr_warn("%s: unregistering prepared clock: %s\n",
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 10/12] clk: Test clk_set_parent() behavior after clk_hw is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (8 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 09/12] clk: Prevent unregistered clk_hw from being reinserted into clk tree Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 11/12] clk: Test parent/clk flags combos while unregistering a clk_hw Stephen Boyd
  2024-08-15  0:55 ` [PATCH 12/12] WIP: clk: Test behavior of children clks after a parent is unregistered Stephen Boyd
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

The previous commit fixed clk_set_parent() so that it returns failure
after a clk_hw is unregistered. Add a test for that so that stays true.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 101 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 1fea29f93b2a..90bd0e0b93d5 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3117,6 +3117,11 @@ struct clk_unregister_consumer_clk_ctx {
 	int phase;
 	struct clk_duty duty;
 	struct clk *clk;
+	struct {
+		struct clk_dummy_context ctx;
+		struct clk *clk;
+	} parents[2];
+	u8 parent;
 };
 
 /* Unregister the clk and mark it as unregistered for the tests. */
@@ -3315,6 +3320,40 @@ static void clk_unregister_consumer_clk_get_duty_cycle_skips(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, num, clk_get_scaled_duty_cycle(ctx->clk, den));
 }
 
+/*
+ * Test that clk_set_parent() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns failure.
+ */
+static void clk_unregister_consumer_clk_set_parent_fails(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	/* Setting current parent is a no-op */
+	KUNIT_EXPECT_EQ(test, 0, clk_set_parent(ctx->clk, ctx->parents[0].clk));
+	/* Setting a new parent should fail */
+	KUNIT_EXPECT_GT(test, 0, clk_set_parent(ctx->clk, ctx->parents[1].clk));
+	/* Parent is unchanged */
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
+}
+
+/*
+ * Test that clk_get_parent() doesn't call the clk_op after the clk_hw has been
+ * unregistered and returns original parent.
+ */
+static void clk_unregister_consumer_clk_get_parent_skips(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
+	clk_unregister_consumer_clk_unregister(test);
+
+	/* Parent is unchanged */
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
+}
+
 static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_prepare_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_unprepare_skips),
@@ -3328,6 +3367,8 @@ static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	KUNIT_CASE(clk_unregister_consumer_clk_get_phase_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_set_duty_cycle_fails),
 	KUNIT_CASE(clk_unregister_consumer_clk_get_duty_cycle_skips),
+	KUNIT_CASE(clk_unregister_consumer_clk_set_parent_fails),
+	KUNIT_CASE(clk_unregister_consumer_clk_get_parent_skips),
 	KUNIT_CASE(clk_unregister_consumer_clk_put),
 	{}
 };
@@ -3418,6 +3459,46 @@ clk_unregister_consumer_clk_op_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
+static int
+clk_unregister_consumer_clk_op_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	ctx->parent = index;
+
+	return 0;
+}
+
+static u8 clk_unregister_consumer_clk_op_get_parent(struct clk_hw *hw)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	return ctx->parent;
+}
+
+static int
+clk_unregister_consumer_clk_op_set_rate_and_parent(struct clk_hw *hw,
+						   unsigned long rate,
+						   unsigned long parent_rate,
+						   u8 index)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx;
+
+	ctx = container_of(hw, struct clk_unregister_consumer_clk_ctx, hw);
+	clk_unregister_consumer_clk_clk_op_called(hw, __func__);
+
+	ctx->parent = index;
+	ctx->rate = rate;
+
+	return 0;
+}
+
 static unsigned long
 clk_unregister_consumer_clk_op_recalc_accuracy(struct clk_hw *hw,
 						unsigned long parent_accuracy)
@@ -3490,6 +3571,9 @@ static const struct clk_ops clk_unregister_consumer_clk_clk_ops = {
 	.round_rate = clk_unregister_consumer_clk_op_round_rate,
 	.determine_rate = clk_unregister_consumer_clk_op_determine_rate,
 	.set_rate = clk_unregister_consumer_clk_op_set_rate,
+	.set_parent = clk_unregister_consumer_clk_op_set_parent,
+	.get_parent = clk_unregister_consumer_clk_op_get_parent,
+	.set_rate_and_parent = clk_unregister_consumer_clk_op_set_rate_and_parent,
 	.recalc_accuracy = clk_unregister_consumer_clk_op_recalc_accuracy,
 	.get_phase = clk_unregister_consumer_clk_op_get_phase,
 	.set_phase = clk_unregister_consumer_clk_op_set_phase,
@@ -3502,14 +3586,31 @@ static int clk_unregister_consumer_clk_init(struct kunit *test)
 	struct clk *clk;
 	struct clk_init_data init = { };
 	struct clk_unregister_consumer_clk_ctx *ctx;
+	struct clk_hw *parent0_hw, *parent1_hw;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 	test->priv = ctx;
 	ctx->test = test;
 
+	parent0_hw = &ctx->parents[0].ctx.hw;
+	parent0_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk0",
+						&clk_dummy_rate_ops, 0);
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent0_hw));
+	ctx->parents[0].clk = clk_hw_get_clk_kunit(test, parent0_hw, "p0");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[0].clk);
+
+	parent1_hw = &ctx->parents[1].ctx.hw;
+	parent1_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk1",
+						&clk_dummy_rate_ops, 0);
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent1_hw));
+	ctx->parents[1].clk = clk_hw_get_clk_kunit(test, parent1_hw, "p1");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[1].clk);
+
 	init.name = "unregister_consumer_clk_test_clk";
 	init.ops = &clk_unregister_consumer_clk_clk_ops;
+	init.parent_hws = (const struct clk_hw *[]){ parent0_hw, parent1_hw };
+	init.num_parents = ARRAY_SIZE(ctx->parents);
 	ctx->hw.init = &init;
 
 	ctx->rate = 42;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 11/12] clk: Test parent/clk flags combos while unregistering a clk_hw
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (9 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 10/12] clk: Test clk_set_parent() behavior after clk_hw is unregistered Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  2024-08-15  0:55 ` [PATCH 12/12] WIP: clk: Test behavior of children clks after a parent is unregistered Stephen Boyd
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

Extend the clk_unregister_consumer_clk test suite to test different
combinations of number of parents and clk flags. The behavior should
stay consistent regardless of how many parents there are and what clk
flags were used during registration.

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 203 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 169 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 90bd0e0b93d5..591897162056 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3148,6 +3148,9 @@ static void clk_unregister_consumer_clk_prepare_fails(struct kunit *test)
 {
 	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
 
+	if (clk_hw_get_flags(&ctx->hw) & CLK_IS_CRITICAL)
+		kunit_skip(test, "Critical clks are already prepared");
+
 	clk_unregister_consumer_clk_unregister(test);
 
 	KUNIT_EXPECT_GT(test, 0, clk_prepare(ctx->clk));
@@ -3172,6 +3175,9 @@ static void clk_unregister_consumer_clk_enable_fails(struct kunit *test)
 {
 	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
 
+	if (clk_hw_get_flags(&ctx->hw) & CLK_IS_CRITICAL)
+		kunit_skip(test, "Critical clks are already enabled");
+
 	KUNIT_ASSERT_EQ(test, 0, clk_prepare(ctx->clk));
 	clk_unregister_consumer_clk_unregister(test);
 
@@ -3231,11 +3237,17 @@ static void clk_unregister_consumer_clk_set_rate_fails(struct kunit *test)
 static void clk_unregister_consumer_clk_get_rate_skips(struct kunit *test)
 {
 	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const unsigned long flags = clk_hw_get_flags(&ctx->hw);
 
 	KUNIT_ASSERT_EQ(test, ctx->rate, clk_get_rate(ctx->clk));
 	clk_unregister_consumer_clk_unregister(test);
 
-	KUNIT_EXPECT_EQ(test, ctx->rate, clk_get_rate(ctx->clk));
+	if (flags & CLK_GET_RATE_NOCACHE) {
+		/* No cache clks start returning 0 and also skip */
+		KUNIT_EXPECT_EQ(test, 0, clk_get_rate(ctx->clk));
+	} else {
+		KUNIT_EXPECT_EQ(test, ctx->rate, clk_get_rate(ctx->clk));
+	}
 }
 
 /*
@@ -3245,11 +3257,17 @@ static void clk_unregister_consumer_clk_get_rate_skips(struct kunit *test)
 static void clk_unregister_consumer_clk_get_accuracy_skips(struct kunit *test)
 {
 	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	const unsigned long flags = clk_hw_get_flags(&ctx->hw);
 
 	KUNIT_ASSERT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
 	clk_unregister_consumer_clk_unregister(test);
 
-	KUNIT_EXPECT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
+	if (flags & CLK_GET_ACCURACY_NOCACHE) {
+		/* No cache clks start returning 0 and also skip */
+		KUNIT_EXPECT_EQ(test, 0, clk_get_accuracy(ctx->clk));
+	} else {
+		KUNIT_EXPECT_EQ(test, ctx->accuracy, clk_get_accuracy(ctx->clk));
+	}
 }
 
 /*
@@ -3334,7 +3352,8 @@ static void clk_unregister_consumer_clk_set_parent_fails(struct kunit *test)
 	/* Setting current parent is a no-op */
 	KUNIT_EXPECT_EQ(test, 0, clk_set_parent(ctx->clk, ctx->parents[0].clk));
 	/* Setting a new parent should fail */
-	KUNIT_EXPECT_GT(test, 0, clk_set_parent(ctx->clk, ctx->parents[1].clk));
+	if (ctx->parents[1].clk)
+		KUNIT_EXPECT_GT(test, 0, clk_set_parent(ctx->clk, ctx->parents[1].clk));
 	/* Parent is unchanged */
 	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
 }
@@ -3354,22 +3373,108 @@ static void clk_unregister_consumer_clk_get_parent_skips(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
 }
 
+/**
+ * struct clk_register_params - Test parameters for different combinations of clk_init_data
+ * @clk_flags: Flags to set in struct clk_init_data::flags
+ * @num_parents: Number of parents to register (0, 1, or 2)
+ */
+struct clk_register_params {
+	unsigned long clk_flags;
+	unsigned int num_parents;
+};
+
+static void clk_register_params_to_desc(const char *test_name,
+					const struct clk_register_params *p,
+					char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s %d parents %#lx flags",
+		 test_name, p->num_parents, p->clk_flags);
+}
+
+/**
+ * clk_register_gen_params - Generate parameters for struct clk_init_data
+ * @test_name: Test name
+ * @_prev: Previous return value from this function
+ * @desc: Test description (to be filled in)
+ *
+ * Use this function in KUNIT_CASE_PARAM to generate struct clk_init_data
+ * parameters for a test that registers clks. It will return combinations of
+ * clk flags (exclusive of each other) and numbers of parents.
+ *
+ * Return: Test parameters in a struct clk_register_params.
+ */
+static const void *clk_register_gen_params(const char *test_name,
+					   const void *_prev, char *desc)
+{
+	const struct clk_register_params *prev = _prev;
+	struct clk_register_params *next;
+
+	next = krealloc(prev, sizeof(*next), GFP_KERNEL);
+	if (!next)
+		return NULL;
+	if (!prev)
+		memset(next, 0, sizeof(*next));
+
+	if (prev) {
+		if (next->clk_flags == 0)
+			next->clk_flags = 1;
+		else
+			next->clk_flags <<= 1;
+	}
+
+	if (next->clk_flags > CLK_DUTY_CYCLE_PARENT) {
+		next->clk_flags = 0;
+		next->num_parents++;
+		if (next->num_parents > 2)
+			return NULL;
+	}
+
+	clk_register_params_to_desc(test_name, next, desc);
+	return next;
+}
+
+#define CLK_REGISTER_GEN_PARAMS(name)					\
+	static const void *name##_gen_params(const void *prev,		\
+					    char *desc)			\
+	{								\
+		return clk_register_gen_params(#name, prev, desc);	\
+	}
+
+#define CLK_REGISTER_KUNIT_CASE_PARAM(name)				\
+	KUNIT_CASE_PARAM(name, name##_gen_params)
+
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_prepare_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_unprepare_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_enable_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_disable_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_round_rate_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_set_rate_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_get_rate_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_get_accuracy_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_set_phase_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_get_phase_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_set_duty_cycle_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_get_duty_cycle_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_set_parent_fails)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_get_parent_skips)
+CLK_REGISTER_GEN_PARAMS(clk_unregister_consumer_clk_put)
+
 static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
-	KUNIT_CASE(clk_unregister_consumer_clk_prepare_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_unprepare_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_enable_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_disable_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_round_rate_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_set_rate_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_get_rate_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_get_accuracy_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_set_phase_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_get_phase_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_set_duty_cycle_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_get_duty_cycle_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_set_parent_fails),
-	KUNIT_CASE(clk_unregister_consumer_clk_get_parent_skips),
-	KUNIT_CASE(clk_unregister_consumer_clk_put),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_prepare_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_unprepare_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_enable_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_disable_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_round_rate_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_set_rate_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_rate_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_accuracy_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_set_phase_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_phase_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_set_duty_cycle_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_duty_cycle_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_set_parent_fails),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_parent_skips),
+	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_put),
 	{}
 };
 
@@ -3586,33 +3691,63 @@ static int clk_unregister_consumer_clk_init(struct kunit *test)
 	struct clk *clk;
 	struct clk_init_data init = { };
 	struct clk_unregister_consumer_clk_ctx *ctx;
-	struct clk_hw *parent0_hw, *parent1_hw;
+	struct clk_hw *parent0_hw = NULL;
+	struct clk_hw *parent1_hw = NULL;
+	const struct clk_register_params *test_param;
+	unsigned int num_parents;
+	unsigned long clk_flags;
+	struct clk_ops *clk_ops;
+
+	test_param = test->param_value;
+	if (test_param) {
+		num_parents = test_param->num_parents;
+		clk_flags = test_param->clk_flags;
+	} else {
+		num_parents = 0;
+		clk_flags = 0;
+	}
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
 	test->priv = ctx;
 	ctx->test = test;
 
-	parent0_hw = &ctx->parents[0].ctx.hw;
-	parent0_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk0",
-						&clk_dummy_rate_ops, 0);
-	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent0_hw));
-	ctx->parents[0].clk = clk_hw_get_clk_kunit(test, parent0_hw, "p0");
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[0].clk);
+	if (num_parents >= 1) {
+		parent0_hw = &ctx->parents[0].ctx.hw;
+		parent0_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk0",
+							&clk_dummy_rate_ops, 0);
+		KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent0_hw));
+		ctx->parents[0].clk = clk_hw_get_clk_kunit(test, parent0_hw, "p0");
+		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[0].clk);
+	}
 
-	parent1_hw = &ctx->parents[1].ctx.hw;
-	parent1_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk1",
-						&clk_dummy_rate_ops, 0);
-	KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent1_hw));
-	ctx->parents[1].clk = clk_hw_get_clk_kunit(test, parent1_hw, "p1");
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[1].clk);
+	if (num_parents >= 2) {
+		parent1_hw = &ctx->parents[1].ctx.hw;
+		parent1_hw->init = CLK_HW_INIT_NO_PARENT("parent-clk1",
+							&clk_dummy_rate_ops, 0);
+		KUNIT_ASSERT_EQ(test, 0, clk_hw_register_kunit(test, NULL, parent1_hw));
+		ctx->parents[1].clk = clk_hw_get_clk_kunit(test, parent1_hw, "p1");
+		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[1].clk);
+	}
 
 	init.name = "unregister_consumer_clk_test_clk";
-	init.ops = &clk_unregister_consumer_clk_clk_ops;
-	init.parent_hws = (const struct clk_hw *[]){ parent0_hw, parent1_hw };
-	init.num_parents = ARRAY_SIZE(ctx->parents);
 	ctx->hw.init = &init;
 
+	clk_ops = kunit_kzalloc(test, sizeof(*clk_ops), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, clk_ops);
+	/* Copy clk_ops so we can modify them for different scenarios */
+	memcpy(clk_ops, &clk_unregister_consumer_clk_clk_ops, sizeof(*clk_ops));
+	init.ops = clk_ops;
+
+	init.flags = clk_flags;
+	init.num_parents = num_parents;
+	init.parent_hws = (const struct clk_hw *[]){ parent0_hw, parent1_hw };
+	if (!num_parents) {
+		clk_ops->get_parent = NULL;
+		clk_ops->set_parent = NULL;
+		clk_ops->set_rate_and_parent = NULL;
+	}
+
 	ctx->rate = 42;
 	ctx->accuracy = 34;
 	ctx->phase = 90;
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 12/12] WIP: clk: Test behavior of children clks after a parent is unregistered
  2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
                   ` (10 preceding siblings ...)
  2024-08-15  0:55 ` [PATCH 11/12] clk: Test parent/clk flags combos while unregistering a clk_hw Stephen Boyd
@ 2024-08-15  0:55 ` Stephen Boyd
  11 siblings, 0 replies; 13+ messages in thread
From: Stephen Boyd @ 2024-08-15  0:55 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, patches, Nuno Sá

When a parent clk is unregistered, descendant clks are orphaned and
removed from the clk tree. Test this scenario to make sure clk consumer
APIs with a child clk don't cause problems.

TODO: Fix the crashes

Cc: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk_test.c | 60 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index 591897162056..e6479c002023 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -3133,6 +3133,17 @@ static void clk_unregister_consumer_clk_unregister(struct kunit *test)
 	ctx->unregistered = true;
 }
 
+static void clk_unregister_parent_consumer_clk_unregister(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+	struct clk_hw *parent_hw = &ctx->parents[0].ctx.hw;
+	struct clk *parent_clk = ctx->parents[0].clk;
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_clk);
+	KUNIT_ASSERT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), parent_clk));
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_kunit(test, parent_hw));
+}
+
 /* Test that clk_put() can be called after the clk_hw has been unregistered. */
 static void clk_unregister_consumer_clk_put(struct kunit *test)
 {
@@ -3143,6 +3154,15 @@ static void clk_unregister_consumer_clk_put(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 0, clk_put_kunit(test, ctx->clk));
 }
 
+static void clk_unregister_parent_consumer_clk_put(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	clk_unregister_parent_consumer_clk_unregister(test);
+
+	KUNIT_EXPECT_EQ(test, 0, clk_put_kunit(test, ctx->clk));
+}
+
 /* Test that clk_prepare() fails after the clk_hw has been unregistered. */
 static void clk_unregister_consumer_clk_prepare_fails(struct kunit *test)
 {
@@ -3358,6 +3378,24 @@ static void clk_unregister_consumer_clk_set_parent_fails(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
 }
 
+/*
+ * Test that clk_set_parent() doesn't re-parent the clk back to the clk that
+ * was unregistered.
+ */
+static void clk_unregister_parent_consumer_clk_set_parent(struct kunit *test)
+{
+	struct clk_unregister_consumer_clk_ctx *ctx = test->priv;
+
+	kunit_skip(test, "Fix in the core. This blows up spectacularly!");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->parents[0].clk);
+	KUNIT_ASSERT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), ctx->parents[0].clk));
+	KUNIT_ASSERT_EQ(test, 0, clk_hw_unregister_kunit(test, &ctx->parents[0].ctx.hw));
+
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), NULL));
+	KUNIT_EXPECT_GT(test, 0, clk_set_parent(ctx->clk, ctx->parents[0].clk));
+	KUNIT_EXPECT_TRUE(test, clk_is_match(clk_get_parent(ctx->clk), NULL));
+}
+
 /*
  * Test that clk_get_parent() doesn't call the clk_op after the clk_hw has been
  * unregistered and returns original parent.
@@ -3396,6 +3434,8 @@ static void clk_register_params_to_desc(const char *test_name,
  * @test_name: Test name
  * @_prev: Previous return value from this function
  * @desc: Test description (to be filled in)
+ * @must_have_parents: True if struct clk_register_params::num_parents must be
+ * 1 or greater
  *
  * Use this function in KUNIT_CASE_PARAM to generate struct clk_init_data
  * parameters for a test that registers clks. It will return combinations of
@@ -3404,7 +3444,8 @@ static void clk_register_params_to_desc(const char *test_name,
  * Return: Test parameters in a struct clk_register_params.
  */
 static const void *clk_register_gen_params(const char *test_name,
-					   const void *_prev, char *desc)
+					   const void *_prev, char *desc,
+					   bool must_have_parents)
 {
 	const struct clk_register_params *prev = _prev;
 	struct clk_register_params *next;
@@ -3412,8 +3453,11 @@ static const void *clk_register_gen_params(const char *test_name,
 	next = krealloc(prev, sizeof(*next), GFP_KERNEL);
 	if (!next)
 		return NULL;
-	if (!prev)
+	if (!prev) {
 		memset(next, 0, sizeof(*next));
+		if (must_have_parents)
+			next->num_parents = 1;
+	}
 
 	if (prev) {
 		if (next->clk_flags == 0)
@@ -3433,11 +3477,19 @@ static const void *clk_register_gen_params(const char *test_name,
 	return next;
 }
 
+static const void *
+clk_unregister_parent_consumer_clk_gen_params(const void *prev,
+					      char *desc)
+{
+	return clk_register_gen_params("parent", prev, desc, true);
+}
+
 #define CLK_REGISTER_GEN_PARAMS(name)					\
 	static const void *name##_gen_params(const void *prev,		\
 					    char *desc)			\
 	{								\
-		return clk_register_gen_params(#name, prev, desc);	\
+		return clk_register_gen_params(#name, prev, desc,	\
+					       false);			\
 	}
 
 #define CLK_REGISTER_KUNIT_CASE_PARAM(name)				\
@@ -3475,6 +3527,8 @@ static struct kunit_case clk_unregister_consumer_clk_test_cases[] = {
 	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_set_parent_fails),
 	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_get_parent_skips),
 	CLK_REGISTER_KUNIT_CASE_PARAM(clk_unregister_consumer_clk_put),
+	KUNIT_CASE_PARAM(clk_unregister_parent_consumer_clk_put, clk_unregister_parent_consumer_clk_gen_params),
+	KUNIT_CASE_PARAM(clk_unregister_parent_consumer_clk_set_parent, clk_unregister_parent_consumer_clk_gen_params),
 	{}
 };
 
-- 
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-08-15  0:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-15  0:55 [PATCH 00/12] clk: Unit test clk unregistration paths Stephen Boyd
2024-08-15  0:55 ` [PATCH 01/12] clk: Fix clk not being unlinked from consumers list Stephen Boyd
2024-08-15  0:55 ` [PATCH 02/12] clk: test: Introduce clk_hw_unregister_kunit() Stephen Boyd
2024-08-15  0:55 ` [PATCH 03/12] clk: test: Introduce clk_put_kunit() Stephen Boyd
2024-08-15  0:55 ` [PATCH 04/12] clk: Add tests for unregistering clk_hw and using consumer APIs Stephen Boyd
2024-08-15  0:55 ` [PATCH 05/12] clk: Fail phase APIs after clk_hw is unregistered Stephen Boyd
2024-08-15  0:55 ` [PATCH 06/12] clk: Test clk_get_phase() behavior " Stephen Boyd
2024-08-15  0:55 ` [PATCH 07/12] clk: Fail duty cycle APIs " Stephen Boyd
2024-08-15  0:55 ` [PATCH 08/12] clk: Test clk_set_duty_cycle() behavior " Stephen Boyd
2024-08-15  0:55 ` [PATCH 09/12] clk: Prevent unregistered clk_hw from being reinserted into clk tree Stephen Boyd
2024-08-15  0:55 ` [PATCH 10/12] clk: Test clk_set_parent() behavior after clk_hw is unregistered Stephen Boyd
2024-08-15  0:55 ` [PATCH 11/12] clk: Test parent/clk flags combos while unregistering a clk_hw Stephen Boyd
2024-08-15  0:55 ` [PATCH 12/12] WIP: clk: Test behavior of children clks after a parent is unregistered Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox