Re: [PATCH v3 7/8] clk: Make clk API return per-user struct clk instances

[Tomeu Vizoso]

On 10 October 2014 01:22, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 10/09, Tomeu Vizoso wrote:
>>  arch/arm/mach-omap2/cclock3xxx_data.c   | 108 ++++--
>>  arch/arm/mach-omap2/clock.h             |  11 +-
>>  arch/arm/mach-omap2/clock_common_data.c |   5 +-
>>  drivers/clk/clk.c                       | 630 ++++++++++++++++++++------------
>>  drivers/clk/clk.h                       |   5 +
>>  drivers/clk/clkdev.c                    |  24 +-
>>  include/linux/clk-private.h             |  35 +-
>>  include/linux/clk-provider.h            |  22 +-
>>  8 files changed, 549 insertions(+), 291 deletions(-)
>
> The difstat looks good.
>
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index fb820bf..4db918a 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -695,6 +731,13 @@ struct clk *__clk_lookup(const char *name)
>>       return NULL;
>>  }
>>
>> +struct clk *__clk_lookup(const char *name)
>> +{
>> +     struct clk_core *clk = clk_core_lookup(name);
>> +
>> +     return !clk ? NULL : clk->hw->clk;
>
> This just looks weird with clk->hw->clk. I know we're trying to
> keep the diff small by not renaming clk to core when it's used
> extensively throughout the code, but for small little additions
> like this I would prefer we use core for clk_core pointers and
> clk for clk pointers. Then a patch at the end can rename
> everything to be consistent. This thing also threw me off because
> I searched for kfree(core) but couldn't find it so I thought we
> leaked the clk_core structure.

Makes sense, I have tried to reduce confusion in this regard.

>> +}
>> +
>>  /*
>>   * Helper for finding best parent to provide a given frequency. This can be used
>>   * directly as a determine_rate callback (e.g. for a mux), or from a more
>> @@ -2175,24 +2298,24 @@ void clk_unregister(struct clk *clk)
>>        * a reference to this clock.
>>        */
>>       flags = clk_enable_lock();
>> -     clk->ops = &clk_nodrv_ops;
>> +     clk->core->ops = &clk_nodrv_ops;
>>       clk_enable_unlock(flags);
>>
>> -     if (!hlist_empty(&clk->children)) {
>> -             struct clk *child;
>> +     if (!hlist_empty(&clk->core->children)) {
>> +             struct clk_core *child;
>>               struct hlist_node *t;
>>
>>               /* Reparent all children to the orphan list. */
>> -             hlist_for_each_entry_safe(child, t, &clk->children, child_node)
>> -                     clk_set_parent(child, NULL);
>> +             hlist_for_each_entry_safe(child, t, &clk->core->children, child_node)
>> +                     clk_core_set_parent(child, NULL);
>>       }
>>
>> -     hlist_del_init(&clk->child_node);
>> +     hlist_del_init(&clk->core->child_node);
>>
>> -     if (clk->prepare_count)
>> +     if (clk->core->prepare_count)
>>               pr_warn("%s: unregistering prepared clock: %s\n",
>> -                                     __func__, clk->name);
>> -     kref_put(&clk->ref, __clk_release);
>> +                                     __func__, clk->core->name);
>> +     kref_put(&clk->core->ref, __clk_release);
>>
>>       clk_prepare_unlock();
>
> It might be worth it to make a "core" local variable in this
> function.
>
>>  }
>> @@ -2255,32 +2378,38 @@ void devm_clk_unregister(struct device *dev, struct clk *clk)
>>  }
>>  EXPORT_SYMBOL_GPL(devm_clk_unregister);
>>
>> +static void clk_core_put(struct clk_core *clk)
>> +{
>> +     struct module *owner;
>> +
>> +     if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> +             return;
>> +
>> +     clk_prepare_lock();
>> +     owner = clk->owner;
>
> Same here too, we don't need to protect the access to owner so it
> can move outside the lock.
>
>> +     kref_put(&clk->ref, __clk_release);
>> +     module_put(owner);
>> +     clk_prepare_unlock();
>> +}
>> +
>>  /*
>>   * clkdev helpers
>>   */
>>  int __clk_get(struct clk *clk)
>>  {
>>       if (clk) {
>> -             if (!try_module_get(clk->owner))
>> +             if (!try_module_get(clk->core->owner))
>>                       return 0;
>>
>> -             kref_get(&clk->ref);
>> +             kref_get(&clk->core->ref);
>>       }
>>       return 1;
>
> Grow a core pointer?
>
>>  }
>> @@ -2391,6 +2520,31 @@ int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_notifier_unregister);
>>
>> +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
>
> Curious, why the underscore?

To denote it's internal to the CCF implementation.

>> +                          const char *con_id)
>> +{
>> +     struct clk *clk;
>> +
>> +     /* This is to allow this function to be chained to others */
>> +     if (!hw || IS_ERR(hw))
>> +             return (struct clk *) hw;
>> +
>> +     clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>> +     if (!clk)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     clk->core = hw->core;
>> +     clk->dev_id = dev_id;
>> +     clk->con_id = con_id;
>> +
>> +     return clk;
>> +}
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index da4bda8..4411db6 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -69,6 +70,10 @@ struct clk *of_clk_get(struct device_node *np, int index)
>>
>>       clk = of_clk_get_by_clkspec(&clkspec);
>>       of_node_put(clkspec.np);
>> +
>> +     if (!IS_ERR(clk))
>> +             clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, NULL);
>
> We lost the debugging information here about the device
> requesting this clock and the name they called it. We get the
> device node name but that might not match the device name. We
> probably need to make private functions in here that allow such
> information to be passed without changing the API for
> of_clk_get(), of_clk_get_by_name(), etc.

Ok, I have made sure that we don't lose any information in that regard.

>> +
>>       return clk;
>>  }
>>  EXPORT_SYMBOL(of_clk_get);
>> @@ -168,14 +173,29 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id)
>>  struct clk *clk_get_sys(const char *dev_id, const char *con_id)
>>  {
>>       struct clk_lookup *cl;
>> +     struct clk *clk = NULL;
>>
>>       mutex_lock(&clocks_mutex);
>> +
>>       cl = clk_find(dev_id, con_id);
>> -     if (cl && !__clk_get(cl->clk))
>> +     if (!cl)
>> +             goto out;
>> +
>> +     if (!__clk_get(cl->clk)) {
>>               cl = NULL;
>> +             goto out;
>> +     }
>> +
>> +#if defined(CONFIG_COMMON_CLK)
>> +     clk = __clk_create_clk(__clk_get_hw(cl->clk), dev_id, con_id);
>
> I was hoping we could put the clk_hw pointer next to the clk
> pointer in the lookup structure. Then providers don't have to
> deal with clk pointers at all and can just assign their clk_hw
> pointer in the lookup. This would make most of the omap usage of
> struct clk unnecessary. It doesn't seem necessary though so I
> guess we can do that in another series?

Yes, I think this is the kind of refinements that we want to do in
further series, moving the drivers one by one.

>> +#else
>> +     clk = cl->clk;
>> +#endif
>> +
>> +out:
>>       mutex_unlock(&clocks_mutex);
>>
>> -     return cl ? cl->clk : ERR_PTR(-ENOENT);
>> +     return cl ? clk : ERR_PTR(-ENOENT);
>>  }
>>  EXPORT_SYMBOL(clk_get_sys);
>>
>> @@ -554,6 +559,19 @@ long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate,
>>                             unsigned long *best_parent_rate,
>>                             struct clk_hw **best_parent_p);
>>
>> +/**
>> + * __clk_core_to_clk - return per-user clk
>> + * @clk: struct clk_core for which we want a per-user clk
>> + *
>> + * Returns a per-user clock that is owned by its provider. The caller shall not
>> + * call clk_get() on it.
>> + *
>> + * This function should be only needed by implementors of
>> + * clk_ops.determine_rate() and should be dropped once all have moved to a
>> + * variant that returns **clk_core instead.
>> + */
>> +struct clk *__clk_core_to_clk(struct clk_core *clk);
>> +
>
> We can drop this now right?

Indeed.

I will be sending soon a v4 with your comments addressed. Thanks for
the thorough reviews!

Cheers,

Tomeu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/