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

From: Stephen Boyd
Date: Mon Dec 02 2019 - 13:19:25 EST


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?

> 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".

> +{
> + 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.

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