Messages in this thread | | | Subject | Re: [PATCH v2 1/2] clk: Warn and add workaround on misuse of .parent_data with .name only | From | Stephen Boyd <> | Date | Wed, 15 Feb 2023 10:54:56 -0800 |
| |
Quoting Christian Marangi (2023-02-10 10:34:11) > On Fri, Feb 10, 2023 at 04:40:29PM -0800, Stephen Boyd wrote: > > Quoting Christian Marangi (2023-01-31 08:08:28) > > > By a simple mistake in a .parent_names to .parent_data conversion it was > > > found that clk core assume fw_name is always provided with a parent_data > > > struct for each parent and never fallback to .name to get parent name even > > > if declared. > > > > It sounds like you have clk_parent_data and the .index member is 0? Can > > you show an example structure? I'm guessing it is like this: > > > > struct clk_parent_data pdata = { .name = "global_name" }; > > > > An example of this problem and the relative fix is here > 35dc8e101a8e08f69f4725839b98ec0f11a8e2d3 > > You example is also ok and this patch wants to handle just a case like > that.
Ok, so you have a firmware .index of 0. The .name is a fallback. I suppose you want the .name to be a fallback if there isn't a clocks property in the registering device node? I thought that should already work but maybe there is a bug somewhere. Presumably you have a gcc node that doesn't have a clocks property
gcc: gcc@1800000 { compatible = "qcom,gcc-ipq8074"; reg = <0x01800000 0x80000>; #clock-cells = <0x1>; #power-domain-cells = <1>; #reset-cells = <0x1>; };
Looking at clk_core_get() we'll call of_parse_clkspec() and that should fail
struct clk_hw *hw = ERR_PTR(-ENOENT);
...
if (np && (name || index >= 0) && !of_parse_clkspec(np, index, name, &clkspec)) { ... } else if (name) { ... }
if (IS_ERR(hw)) return ERR_CAST(hw);
so we should have a -ENOENT clk_hw pointer in clk_core_fill_parent_index(). That should land in this if condition in clk_core_fill_parent_index()
parent = clk_core_get(core, index); if (PTR_ERR(parent) == -ENOENT && entry->name) parent = clk_core_lookup(entry->name);
and then entry->name should be used.
> > > > > > > This is caused by clk_core_get that only checks for parent .fw_name and > > > doesn't handle .name. > > > > clk_core_get() is not supposed to operate on the .name member. It is a > > firmware based lookup with clkdev as a fallback because clkdev is a > > psudeo-firmware interface to assign a name to a clk when some device > > pointer is used in conjunction with it. > > > > And the problem is just that. We currently permit to have a > configuration with .name but no .fw_name. In a case like that a dev may > think that this configuration is valid but in reality the clk is > silently ignored/not found and cause clk problem with selecting a > parent.
It is valid though.
> > Took some good hours to discover this and to me it seems an error that > everybody can do since nowhere is specificed that the following > parent_data configuration is illegal. >
I'll look at adding a test. Seems to be the best way to solve this.
| |