Re: [PATCH 11/22] clk: Skip set_rate_range if our clock is orphan

Re: [PATCH 11/22] clk: Skip set_rate_range if our clock is orphan

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5086 bytes --]

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220408091037.2041955-12-maxime@cerno.tech>
References: <20220408091037.2041955-12-maxime@cerno.tech>
TO: Maxime Ripard <maxime@cerno.tech>
TO: Mike Turquette <mturquette@baylibre.com>
TO: Stephen Boyd <sboyd@kernel.org>
TO: linux-clk(a)vger.kernel.org
CC: Naresh Kamboju <naresh.kamboju@linaro.org>
CC: Alexander Stein <alexander.stein@ew.tq-group.com>
CC: Marek Szyprowski <m.szyprowski@samsung.com>
CC: Tony Lindgren <tony@atomide.com>
CC: Jerome Brunet <jbrunet@baylibre.com>
CC: Yassine Oudjana <y.oudjana@protonmail.com>
CC: Neil Armstrong <narmstrong@baylibre.com>
CC: Maxime Ripard <maxime@cerno.tech>

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on linus/master v5.18-rc1 next-20220408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/clk-More-clock-rate-fixes-and-tests/20220408-171635
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
:::::: branch date: 16 hours ago
:::::: commit date: 16 hours ago
config: riscv-randconfig-m031-20220408 (https://download.01.org/0day-ci/archive/20220409/202204090942.TPcGLjZX-lkp(a)intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/clk/clk_test.c:801 clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate() warn: ignoring unreachable code.

Old smatch warnings:
drivers/clk/clk_test.c:929 clk_test_single_parent_mux_set_range_disjoint_child_last() warn: ignoring unreachable code.
drivers/clk/clk_test.c:959 clk_test_single_parent_mux_set_range_disjoint_parent_last() warn: ignoring unreachable code.

vim +801 drivers/clk/clk_test.c

d61deb9e36bc91 Maxime Ripard 2022-04-08  780  
2f0b8e802c7321 Maxime Ripard 2022-04-08  781  /*
2f0b8e802c7321 Maxime Ripard 2022-04-08  782   * Test that, for a mux that started orphan, was assigned and rate and
2f0b8e802c7321 Maxime Ripard 2022-04-08  783   * then got switched to a valid parent, its rate is eventually within
2f0b8e802c7321 Maxime Ripard 2022-04-08  784   * range.
2f0b8e802c7321 Maxime Ripard 2022-04-08  785   *
2f0b8e802c7321 Maxime Ripard 2022-04-08  786   * FIXME: Even though we update the rate as part of clk_set_parent(), we
2f0b8e802c7321 Maxime Ripard 2022-04-08  787   * don't evaluate whether that new rate is within range and needs to be
2f0b8e802c7321 Maxime Ripard 2022-04-08  788   * adjusted.
2f0b8e802c7321 Maxime Ripard 2022-04-08  789   */
2f0b8e802c7321 Maxime Ripard 2022-04-08  790  static void
2f0b8e802c7321 Maxime Ripard 2022-04-08  791  clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(struct kunit *test)
2f0b8e802c7321 Maxime Ripard 2022-04-08  792  {
2f0b8e802c7321 Maxime Ripard 2022-04-08  793  	struct clk_multiple_parent_ctx *ctx = test->priv;
2f0b8e802c7321 Maxime Ripard 2022-04-08  794  	struct clk_hw *hw = &ctx->hw;
2f0b8e802c7321 Maxime Ripard 2022-04-08  795  	struct clk *clk = hw->clk, *parent;
2f0b8e802c7321 Maxime Ripard 2022-04-08  796  	unsigned long rate;
2f0b8e802c7321 Maxime Ripard 2022-04-08  797  	int ret;
2f0b8e802c7321 Maxime Ripard 2022-04-08  798  
2f0b8e802c7321 Maxime Ripard 2022-04-08  799  	kunit_skip(test, "This needs to be fixed in the core.");
2f0b8e802c7321 Maxime Ripard 2022-04-08  800  
2f0b8e802c7321 Maxime Ripard 2022-04-08 @801  	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
2f0b8e802c7321 Maxime Ripard 2022-04-08  802  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
2f0b8e802c7321 Maxime Ripard 2022-04-08  803  
2f0b8e802c7321 Maxime Ripard 2022-04-08  804  	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
2f0b8e802c7321 Maxime Ripard 2022-04-08  805  	KUNIT_ASSERT_EQ(test, ret, 0);
2f0b8e802c7321 Maxime Ripard 2022-04-08  806  
2f0b8e802c7321 Maxime Ripard 2022-04-08  807  	ret = clk_set_parent(clk, parent);
2f0b8e802c7321 Maxime Ripard 2022-04-08  808  	KUNIT_ASSERT_EQ(test, ret, 0);
2f0b8e802c7321 Maxime Ripard 2022-04-08  809  
2f0b8e802c7321 Maxime Ripard 2022-04-08  810  	rate = clk_get_rate(clk);
2f0b8e802c7321 Maxime Ripard 2022-04-08  811  	KUNIT_ASSERT_GT(test, rate, 0);
2f0b8e802c7321 Maxime Ripard 2022-04-08  812  	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
2f0b8e802c7321 Maxime Ripard 2022-04-08  813  	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
2f0b8e802c7321 Maxime Ripard 2022-04-08  814  
2f0b8e802c7321 Maxime Ripard 2022-04-08  815  	clk_put(parent);
2f0b8e802c7321 Maxime Ripard 2022-04-08  816  }
2f0b8e802c7321 Maxime Ripard 2022-04-08  817  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH 00/22] clk: More clock rate fixes and tests

[Maxime Ripard]

Hi,

Thanks to the feedback I got on the previous series, I found and fixed a
number of bugs in the clock framework and how it deals with rates,
especially when it comes to orphan clocks.

In order to make sure this doesn't pop up again as a regression, I've
extended the number of tests.

The first patch reintroduces the clk_set_rate_range call on clk_put, but
this time will only do so if there was a range set on that clock to
begin with. It should be less intrusive, and reduce the number of
potential side effects considerably.

All the other patches should be probably be flagged as fixes, but
they've never seem to have shown any real-world issues until now, and
they aren't all really trivial to backport either, so I'm not sure it's
worth it.

The last patch will probably prove to be controversial, but it can be
left out without affecting the rest of the series. It will affect the
meson clock drivers for the g12b SoC at least. It stems from the
realisation that on the g12b, 4 PLLs (and thus all their children) have
a rate of 0, pretty much forever which feels like a bug considering the
rate 0 is used as an error in most places.

Those 4 PLLs have a rate of 0 because meson_clk_pll_recalc_rate will
return 0 if the diviser of the PLL is set to 0 in the register, but:

  - pcie_pll_dco has a few registers to initialize set in
    g12a_pcie_pll_dco, but meson_clk_pcie_pll_ops don't set the init
    hook and will instead call it in the enable hook. This looks like a
    bug and could be easily fixed by adding that init hook.

  - gp0_pll_dco and hifi_pll_dco both don't set any of there n field in
    the initialisation of their register (g12a_gp0_init_regs and
    g12a_hifi_init_regs). So if the bootloader doesn't set it, and as
    long as set_rate isn't called, that field is going to remain at 0. And
    since all but one users don't have CLK_SET_RATE_PARENT, this is
    still fairly unlikely.

  - hdmi_pll_dco doesn't set the n field in the initialisation either,
    but also doesn't have a set_rate implementation. Thus, if the
    bootloader doesn't set it, this clock and all its children will
    always report a rate of 0, even if the clock is functional.

During the discussion with amlogic clock maintainers, we kind of ended
up on a disagreement of whether returning 0 was ok or not, hence the
expected controversy :)

Let me know what you think,
Maxime

Maxime Ripard (22):
  clk: Drop the rate range on clk_put()
  clk: tests: Add test suites description
  clk: tests: Add reference to the orphan mux bug report
  clk: tests: Add tests for uncached clock
  clk: tests: Add tests for single parent mux
  clk: tests: Add tests for mux with multiple parents
  clk: tests: Add some tests for orphan with multiple parents
  clk: Take into account uncached clocks in clk_set_rate_range()
  clk: Fix clk_get_parent() documentation
  clk: Set req_rate on reparenting
  clk: Skip set_rate_range if our clock is orphan
  clk: Add our request boundaries in clk_core_init_rate_req
  clk: Change clk_core_init_rate_req prototype
  clk: Introduce clk_hw_init_rate_request()
  clk: Add missing clk_core_init_rate_req calls
  clk: Remove redundant clk_core_init_rate_req() call
  clk: Switch from __clk_determine_rate to clk_core_round_rate_nolock
  clk: Introduce clk_core_has_parent()
  clk: Stop forwarding clk_rate_requests to the parent
  clk: Zero the clk_rate_request structure
  clk: Test the clock pointer in clk_hw_get_name()
  clk: Prevent a clock without a rate to register

 drivers/clk/clk.c            |  239 +++++--
 drivers/clk/clk_test.c       | 1256 +++++++++++++++++++++++++++++++++-
 include/linux/clk-provider.h |    6 +
 include/linux/clk.h          |    5 +-
 4 files changed, 1439 insertions(+), 67 deletions(-)

-- 
2.35.1

[PATCH 11/22] clk: Skip set_rate_range if our clock is orphan

[Maxime Ripard]

clk_set_rate_range will now force a clk_set_rate() call to
core->req_rate. However, if our clock is orphan, req_rate will be 0 and
we will thus end up calling a set_rate to 0 on potentially all that
clock subtree.

This can be fairly invasive and result in poor decisions. In such a
case, let's just store the new range but bail out and skip the set_rate.

When that clock is no longer orphan though, we should be enforcing the
new range but we don't. Let's add a skipped test to make sure we don't
forget about that corner case.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c      |  6 +++++
 drivers/clk/clk_test.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5b6ec11fcfcd..80fbec87309e 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2389,6 +2389,12 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 	if (clk->core->flags & CLK_GET_RATE_NOCACHE)
 		rate = clk_core_get_rate_recalc(clk->core);
 
+	if (clk->core->orphan && !rate) {
+		pr_warn("%s: clk %s: Clock is orphan and doesn't have a rate!\n",
+			__func__, clk->core->name);
+		goto out;
+	}
+
 	/*
 	 * Since the boundaries have been changed, let's give the
 	 * opportunity to the provider to adjust the clock rate based on
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index fa6e32c460ac..0fffc70f9e2c 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -735,6 +735,26 @@ clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouched(s
 	clk_put(parent);
 }
 
+/*
+ * Test that, for a mux whose current parent hasn't been registered yet,
+ * calling clk_set_rate_range() will succeed but won't affect its rate.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_range_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk;
+	unsigned long rate;
+	int ret;
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_EXPECT_EQ(test, rate, 0);
+}
+
 /*
  * Test that, for a mux whose current parent hasn't been registered yet,
  * calling clk_set_rate_range() will succeed, and will be taken into
@@ -758,6 +778,43 @@ clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate(struct kuni
 	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
 }
 
+/*
+ * Test that, for a mux that started orphan, was assigned and rate and
+ * then got switched to a valid parent, its rate is eventually within
+ * range.
+ *
+ * FIXME: Even though we update the rate as part of clk_set_parent(), we
+ * don't evaluate whether that new rate is within range and needs to be
+ * adjusted.
+ */
+static void
+clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate(struct kunit *test)
+{
+	struct clk_multiple_parent_ctx *ctx = test->priv;
+	struct clk_hw *hw = &ctx->hw;
+	struct clk *clk = hw->clk, *parent;
+	unsigned long rate;
+	int ret;
+
+	kunit_skip(test, "This needs to be fixed in the core.");
+
+	parent = clk_hw_get_clk(&ctx->parents_ctx[1].hw, NULL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+
+	ret = clk_set_rate_range(clk, DUMMY_CLOCK_RATE_1, DUMMY_CLOCK_RATE_2);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = clk_set_parent(clk, parent);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	rate = clk_get_rate(clk);
+	KUNIT_ASSERT_GT(test, rate, 0);
+	KUNIT_EXPECT_GE(test, rate, DUMMY_CLOCK_RATE_1);
+	KUNIT_EXPECT_LE(test, rate, DUMMY_CLOCK_RATE_2);
+
+	clk_put(parent);
+}
+
 static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[] = {
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_get_parent),
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent),
@@ -766,7 +823,9 @@ static struct kunit_case clk_orphan_transparent_multiple_parent_mux_test_cases[]
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_put),
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_modified),
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_parent_set_range_untouched),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_get_rate),
 	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_round_rate),
+	KUNIT_CASE(clk_test_orphan_transparent_multiple_parent_mux_set_range_set_parent_get_rate),
 	{}
 };
 
-- 
2.35.1