Message-ID: <177065924524.54237.5857092620031619822@lazor>
Date: Mon, 09 Feb 2026 09:47:25 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Chen-Yu Tsai <wens@...e.org>, Michael Turquette <mturquette@...libre.com>, Yao Zi <me@...ao.cc>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, Yao Zi <me@...ao.cc>, Drew Fustini <fustini@...nel.org>, Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH v2] clk: Avoid DT fetch in possible_parent_show if clk_hw is provided

Quoting Yao Zi (2026-02-05 07:42:02)
> When showing a parent for which clk_core_get_parent_by_index fails, we
> may try using the parent's global name or the local name. If this fails
> either, the parent clock's clock-output-names is fetched through
> DT-index.
> 
> struct clk_hw pointer takes precedence with DT-index when registering
> clocks, thus most drivers only zero the index member of struct
> clk_parent_data when providing the parent through struct clk_hw pointer.
> If the pointer cannot resovle to a clock, clk_core_get_parent_by_index

Use clk_core_get_parent_by_index() to indicate this is a function.

> will fail as well, in which case possible_parent_show will fetch the

possible_parent_show()

> parent's clock-output-names property, treat the unintended, zeroed index

Use "clock-output-names" to indicate DT property.

> as valid, and yield a misleading name if the clock controller does come
> with a clocks property.
> 
> Let's add an extra check against the struct clk_hw pointer, and only
> perform the DT-index-based fetch if it isn't provided.
> 
> Fixes: 2d156b78ce8f ("clk: Fix debugfs clk_possible_parents for clks without parent string names")
> Signed-off-by: Yao Zi <me@...ao.cc>
> Tested-by: Drew Fustini <fustini@...nel.org>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> This was found when fixing the wrong parent description of
> clk-th1520-ap.c[1]. Without the patch,
> 
>         # cat /sys/kernel/debug/clk/c910/clk_possible_parents
>         osc_24m cpu-pll1
> 
> The first parent should be c910-i0, provided by an unresolvable struct
> clk_hw pointer. osc_24m is the first (and only) parent specified in
> devicetree for the clock controller. With the patch,
> 
>         # cat /sys/kernel/debug/clk/c910/clk_possible_parents
>         (missing) cpu-pll1
> 
> [1]: https://lore.kernel.org/linux-riscv/20250705052028.24611-1-ziyao@disroot.org/
> 
> Changed from v1:
> - Collect tags
> - Switch to my new mail address
> - Link to v1: https://lore.kernel.org/all/20250705095816.29480-2-ziyao@disroot.org/
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 85d2f2481acf..a9d1aea59689 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3588,7 +3588,7 @@ static void possible_parent_show(struct seq_file *s, struct clk_core *core,
>         } else if (core->parents[i].fw_name) {
>                 seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
>         } else {
> -               if (core->parents[i].index >= 0)
> +               if (!core->parents[i].hw && core->parents[i].index >= 0)
>                         name = of_clk_get_parent_name(core->of_node, core->parents[i].index);
>                 if (!name)
>                         name = "(missing)";

I had to read this and reference the code for about 10 minutes. It's
really confusing! Adding a well placed condition doesn't help me :(

My confusion comes from thinking that core->parents[i].index should be
negative if the 'hw' member is specified. See how the loop in
clk_core_populate_parent_map() sets the index to -1 at the start of the
loop? It seems that whatever the clk provider passes though will
overwrite that though, leading to the index becoming 0 again if the
clk_parent_data was static initialized. Oof.

How about we set index to -1 when 'hw' is present? This reminds me of a
thread[1] with Jerome years ago where we were discussing how confusing
this all is. It's my fault for missing that the index was getting
overwritten. Here's a totally untested patch. I'm tempted to print a
warning or really fail clk registration if we find that hw is NULL,
fw_name is NULL, and index is < 0. I don't see any case where we should
allow drivers to only put the legacy name in the clk_parent_data
structure.

----8<-----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 85d2f2481acf..b1a0da0991c4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4254,10 +4254,23 @@ static int clk_core_populate_parent_map(struct clk_core *core,
 			ret = clk_cpy_name(&parent->name, parent_names[i],
 					   true);
 		} else if (parent_data) {
+			/*
+			 * Most difficult case. Precedence order is 'hw',
+			 * 'fw_name', 'index', and finally 'name'. If 'hw' is
+			 * present it's easy, just use it. Otherwise 'fw_name'
+			 * or 'index' needs to be present, while 'name' can be
+			 * present in the case that a parent lookup should
+			 * fallback to the legacy path where global clk names
+			 * are used for clk topology.
+			 */
 			parent->hw = parent_data[i].hw;
-			parent->index = parent_data[i].index;
+			if (parent->hw)
+				continue;
+
 			ret = clk_cpy_name(&parent->fw_name,
 					   parent_data[i].fw_name, false);
+			if (!parent->fw_name)
+				parent->index = parent_data[i].index;
 			if (!ret)
 				ret = clk_cpy_name(&parent->name,
 						   parent_data[i].name,

Another thing we can probably do in debugfs is figure out that the clk
is known but not registered yet. That case can be detected if the 'hw'
member isn't NULL or if the fw_name/index field is set. In that case we
know without a doubt that either the clk hasn't been registered yet or
it never will appear in the case the DT node is disabled or the clocks
property has a <0> cell at that index. We signal this information from
clk_core_fill_parent_index() by setting 'parent' to -EPROBE_DEFER. We
could have a better function here that returns the parent clk_core
pointer and then in debugfs we can test that for -ENOENT vs.
-EPROBE_DEFER and distinguish "missing" from "waiting" or something like
that.

I'll also note that of_clk_get_parent_name() doesn't need to check for a
positive index. It will fail with a negative index like we intend
causing the end result to be "(missing)" anyway. We should remove that
condition to simplify.

[1] https://lore.kernel.org/all/6394b7e46a7967f4a5a6f77215e670e3e7a7ef10.camel@baylibre.com/

Powered by blists - more mailing lists