[PATCH v2] clk: Avoid DT fetch in possible_parent_show if clk_hw is provided

[PATCH v2] clk: Avoid DT fetch in possible_parent_show if clk_hw is provided

[Yao Zi]

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
will fail as well, in which case possible_parent_show will fetch the
parent's clock-output-names property, treat the unintended, zeroed index
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@ziyao.cc>
Tested-by: Drew Fustini <fustini@kernel.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)";
-- 
2.52.0

Re: [PATCH v2] clk: Avoid DT fetch in possible_parent_show if clk_hw is provided

[Chen-Yu Tsai]

On Thu, Feb 5, 2026 at 11:42 PM Yao Zi <me@ziyao.cc> wrote:
>
> 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

                        ^ typo here.

> will fail as well, in which case possible_parent_show will fetch the
> parent's clock-output-names property, treat the unintended, zeroed index
> 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@ziyao.cc>
> Tested-by: Drew Fustini <fustini@kernel.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 think this is slightly wrong. It should never fall back to looking
to the other sources if parents[i].hw is a valid pointer, since that
takes precedence.

I think it should look more like:

parent = clk_core_get_parent_by_index(core, i);
if (parent) {
        seq_puts(s, parent->name);
} else if (!core->parents[i].hw) {
        if (core->parents[i].name) {
                seq_puts(s, core->parents[i].name);
        } else if (core->parents[i].fw_name) {
                seq_printf(s, "<%s>(fw)", core->parents[i].fw_name);
        } else {
                if (core->parents[i].index >= 0)
                        name = of_clk_get_parent_name(core->of_node,
core->parents[i].index);
                if (!name)
                        name = "(missing)";
                seq_puts(s, name);
        }
}

Anyway, thank you for fixing this. I wrote the original code and this was
one of the corner cases that we didn't have on the sunxi platform.


ChenYu

Re: [PATCH v2] clk: Avoid DT fetch in possible_parent_show if clk_hw is provided

[Stephen Boyd]

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@ziyao.cc>
> Tested-by: Drew Fustini <fustini@kernel.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/