Re: [PATCH] clk: walk orphan list on clock provider registration

From: Jerome Brunet
Date: Mon Dec 02 2019 - 13:59:33 EST



On Mon 02 Dec 2019 at 19:19, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:

> Quoting Jerome Brunet (2019-11-29 08:16:58)
>> So far, we walked the orphan list every time a new clock was registered
>> in CCF. This was fine since the clocks were only referenced by name.
>>
>> Now that the clock can be referenced through DT, it is not enough:
>> * Controller A register first a reference clocks from controller B
>> through DT.
>> * Controller B register all its clocks then register the provider.
>>
>> Each time controller B registers a new clock, the orphan list is walked
>> but it can't match since the provider is registered yet. When the
>> provider is finally registered, the orphan list is not walked unless
>> another clock is registered afterward.
>>
>> This can lead to situation where some clocks remain orphaned even if
>> the parent is available.
>>
>> Walking the orphan list on provider registration solves the problem.
>>
>> Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names")
>> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>> ---
>
> Sounds right. Thanks for making the fix!
>
> I suspect there should be a reported-by tag though?

laboriously, yes

>
>> drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++------------------
>> 1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index ef4416721777..917ba37c3b9d 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -3249,6 +3249,34 @@ static inline void clk_debug_unregister(struct clk_core *core)
>> }
>> #endif
>>
>> +static void __clk_core_reparent_orphan(void)
>
> Maybe drop the double underscore. clk_core prefix already means "private
> to this file".

I've done it this way just to keep coherent with __clk_core_init(),
which the code was extracted from

In the end, I don't mind so I'll drop it in the v2

>
>> +{
>> + struct clk_core *orphan;
>> + struct hlist_node *tmp2;
>> +
>> + /*
>> + * walk the list of orphan clocks and reparent any that newly finds a
>> + * parent.
>> + */
>> + hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> + struct clk_core *parent = __clk_init_parent(orphan);
>> +
>> + /*
>> + * We need to use __clk_set_parent_before() and _after() to
>> + * to properly migrate any prepare/enable count of the orphan
>> + * clock. This is important for CLK_IS_CRITICAL clocks, which
>> + * are enabled during init but might not have a parent yet.
>> + */
>> + if (parent) {
>> + /* update the clk tree topology */
>> + __clk_set_parent_before(orphan, parent);
>> + __clk_set_parent_after(orphan, parent, NULL);
>> + __clk_recalc_accuracies(orphan);
>> + __clk_recalc_rates(orphan, 0);
>> + }
>> + }
>> +}
>> +
>> /**
>> * __clk_core_init - initialize the data structures in a struct clk_core
>> * @core: clk_core being initialized
>> @@ -3259,8 +3287,6 @@ static inline void clk_debug_unregister(struct clk_core *core)
>> static int __clk_core_init(struct clk_core *core)
>> {
>> int ret;
>> - struct clk_core *orphan;
>> - struct hlist_node *tmp2;
>> unsigned long rate;
>>
>> if (!core)
>> @@ -3416,27 +3442,8 @@ static int __clk_core_init(struct clk_core *core)
>> clk_enable_unlock(flags);
>> }
>>
>> - /*
>> - * walk the list of orphan clocks and reparent any that newly finds a
>> - * parent.
>> - */
>> - hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node) {
>> - struct clk_core *parent = __clk_init_parent(orphan);
>> + __clk_core_reparent_orphan();
>>
>> - /*
>> - * We need to use __clk_set_parent_before() and _after() to
>> - * to properly migrate any prepare/enable count of the orphan
>> - * clock. This is important for CLK_IS_CRITICAL clocks, which
>> - * are enabled during init but might not have a parent yet.
>> - */
>> - if (parent) {
>> - /* update the clk tree topology */
>> - __clk_set_parent_before(orphan, parent);
>> - __clk_set_parent_after(orphan, parent, NULL);
>> - __clk_recalc_accuracies(orphan);
>> - __clk_recalc_rates(orphan, 0);
>> - }
>> - }
>>
>> kref_init(&core->ref);
>> out:
>> @@ -4288,6 +4295,10 @@ int of_clk_add_provider(struct device_node *np,
>> mutex_unlock(&of_clk_mutex);
>> pr_debug("Added clock from %pOF\n", np);
>>
>> + clk_prepare_lock();
>> + __clk_core_reparent_orphan();
>> + clk_prepare_unlock();
>> +
>
> Maybe make a locked version of this function and an unlocked version?
>
>> ret = of_clk_set_defaults(np, true);
>> if (ret < 0)
>> of_clk_del_provider(np);
>> @@ -4323,6 +4334,10 @@ int of_clk_add_hw_provider(struct device_node *np,
>> mutex_unlock(&of_clk_mutex);
>> pr_debug("Added clk_hw provider from %pOF\n", np);
>>
>> + clk_prepare_lock();
>> + __clk_core_reparent_orphan();
>> + clk_prepare_unlock();
>> +
>
> So we don't duplicate this twice.
>

Sure.

>> ret = of_clk_set_defaults(np, true);
>> if (ret < 0)