Re: [PATCH v2] clkdev: Always allocate a struct clk and call __clk_get() w/ CCF

From: Tomeu Vizoso
Date: Fri Feb 06 2015 - 06:24:03 EST

On 6 February 2015 at 11:57, Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> wrote:
> On 06/02/15 02:40, Stephen Boyd wrote:
>> of_clk_get_by_clkspec() returns a struct clk pointer but it
>> doesn't create a new handle for the consumers when we're using
>> the common clock framework. Instead it just returns whatever the
>> clk provider hands out. When the consumers go to call clk_put()
>> we get an Oops.
>>
>> Unable to handle kernel paging request at virtual address 00200200
>> pgd = c0004000
>> [00200200] *pgd=00000000
>> Internal error: Oops: 805 [#1] PREEMPT SMP ARM
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.19.0-rc1-00104-ga251361a-dirty #992
>> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
>> task: ee00b000 ti: ee088000 task.ti: ee088000
>> PC is at __clk_put+0x24/0xd0
>> LR is at clk_prepare_lock+0xc/0xec
>> pc : [<c03eef38>] lr : [<c03ec1f4>] psr: 20000153
>> sp : ee089de8 ip : 00000000 fp : 00000000
>> r10: ee02f480 r9 : 00000001 r8 : 00000000
>> r7 : ee031cc0 r6 : ee089e08 r5 : 00000000 r4 : ee02f480
>> r3 : 00100100 r2 : 00200200 r1 : 0000091e r0 : 00000001
>> Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel
>> Control: 10c5387d Table: 4000404a DAC: 00000015
>> Process swapper/0 (pid: 1, stack limit = 0xee088238)
>> Stack: (0xee089de8 to 0xee08a000)
>> 9de0: ee7c8f14 c03f0ec8 ee089e08 00000000 c0718dc8 00000001
>> 9e00: 00000000 c04ee0f0 ee7e0844 00000001 00000181 c04edb58 ee2bd320 00000000
>> 9e20: 00000000 c011dc5c ee16a1e0 00000000 00000000 c0718dc8 ee16a1e0 ee2bd1e0
>> 9e40: c0641740 ee16a1e0 00000000 ee2bd320 c0718dc8 ee1d3e10 ee1d3e10 00000000
>> 9e60: c0769a88 00000000 c0718dc8 00000000 00000000 c02c3124 c02c310c ee1d3e10
>> 9e80: c07b4eec 00000000 c0769a88 c02c1d0c ee1d3e10 c0769a88 ee1d3e44 00000000
>> 9ea0: c07091dc c02c1eb8 00000000 c0769a88 c02c1e2c c02c0544 ee005478 ee1676c0
>> 9ec0: c0769a88 ee3a4e80 c0760ce8 c02c150c c0669b90 c0769a88 c0746cd8 c0769a88
>> 9ee0: c0746cd8 ee2bc4c0 c0778c00 c02c24e0 00000000 c0746cd8 c0746cd8 c07091f0
>> 9f00: 00000000 c0008944 c04f405c 00000025 ee00b000 60000153 c074ab00 00000000
>> 9f20: 00000000 c074ab90 60000153 00000000 ef7fca5d c050860c 000000b6 c0036b88
>> 9f40: c065ecc4 c06bc728 00000006 00000006 c074ab30 ef7fca40 c0739bdc 00000006
>> 9f60: c0718dbc c0778c00 000000b6 c0718dc8 c06ed598 c06edd64 00000006 00000006
>> 9f80: c06ed598 c003b438 00000000 c04e64f4 00000000 00000000 00000000 00000000
>> 9fa0: 00000000 c04e64fc 00000000 c000e838 00000000 00000000 00000000 00000000
>> 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 c0c0c0c0 c0c0c0c0
>> [<c03eef38>] (__clk_put) from [<c03f0ec8>] (of_clk_set_defaults+0xe0/0x2c0)
>> [<c03f0ec8>] (of_clk_set_defaults) from [<c02c3124>] (platform_drv_probe+0x18/0xa4)
>> [<c02c3124>] (platform_drv_probe) from [<c02c1d0c>] (driver_probe_device+0x10c/0x22c)
>> [<c02c1d0c>] (driver_probe_device) from [<c02c1eb8>] (__driver_attach+0x8c/0x90)
>> [<c02c1eb8>] (__driver_attach) from [<c02c0544>] (bus_for_each_dev+0x54/0x88)
>> [<c02c0544>] (bus_for_each_dev) from [<c02c150c>] (bus_add_driver+0xd4/0x1d0)
>> [<c02c150c>] (bus_add_driver) from [<c02c24e0>] (driver_register+0x78/0xf4)
>> [<c02c24e0>] (driver_register) from [<c07091f0>] (fimc_md_init+0x14/0x30)
>> [<c07091f0>] (fimc_md_init) from [<c0008944>] (do_one_initcall+0x80/0x1d0)
>> [<c0008944>] (do_one_initcall) from [<c06edd64>] (kernel_init_freeable+0x108/0x1d4)
>> [<c06edd64>] (kernel_init_freeable) from [<c04e64fc>] (kernel_init+0x8/0xec)
>> [<c04e64fc>] (kernel_init) from [<c000e838>] (ret_from_fork+0x14/0x3c)
>> Code: ebfff4ae e5943014 e5942018 e3530000 (e5823000)
>>
>> Let's create a per-user handle here so that clk_put() can
>> properly unlink it and free the handle. Now that we allocate a
>> clk structure here we need to free it if __clk_get() fails so
>> bury the __clk_get() call in __of_clk_get_from_provider(). We
>> need to handle the same problem in clk_get_sys() so export
>> __clk_free_clk() to clkdev.c and do the same thing, except let's
>> do some more inline functions to make this code #ifdef free.
>>
>> This fixes the above crash, properly calls __clk_get() when
>> of_clk_get_from_provider() is called, and cleans up the clk
>> structure on the error path of clk_get_sys().
>>
>> Fixes: 035a61c314eb "clk: Make clk API return per-user struct clk instances"
>> Reported-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> Reported-by: Alban Browaeys <alban.browaeys@xxxxxxxxx>
>> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
>> Signed-off-by: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
>
> Thanks Stephen, with this patch everything seems to be back to normal!
>
> What are the main reasons to introduce per-user struct clk though?
> It's not clear from the changelog which introduces it.

People have long complained about consumers of the same clock stepping
on each other's toes (when preparing/enabling, or when changing rates,
etc). This patch allows us to know which consumer is causing trouble
and the patch that adds clock constraints allows more than one
consumer to influence the effective rate of a clock in a more ordered
fashion.

> As a side note, IMHO recent changes made the code much harder to follow,
> e.g. by using 'clk' local variable name interchangeably for 'struct clk'
> and 'struct clk_core' types and by using same field name in different data
> structures, like 'child_node'. I guess the changes were limited to make
> the diff smaller, nevertheless it seems we ended up with something rather
> illegible.

I would like to see clk_core eventually removed and have all provider
state in clk_hw, with struct clk containing the per-user state.

Regards,

Tomeu

> Tested-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>
>> ---
>> drivers/clk/clk.c | 18 +++++++++++++----
>> drivers/clk/clk.h | 19 +++++++++++++++++-
>> drivers/clk/clkdev.c | 56 ++++++++++++++++++++++------------------------------
>> 3 files changed, 56 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 113456030d66..5469d7714f5d 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2382,7 +2382,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
>> return clk;
>> }
>>
>> -static void __clk_free_clk(struct clk *clk)
>> +void __clk_free_clk(struct clk *clk)
>> {
>> clk_prepare_lock();
>> hlist_del(&clk->child_node);
>> @@ -2894,7 +2894,8 @@ void of_clk_del_provider(struct device_node *np)
>> }
>> EXPORT_SYMBOL_GPL(of_clk_del_provider);
>>
>> -struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
>> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
>> + const char *dev_id, const char *con_id)
>> {
>> struct of_clk_provider *provider;
>> struct clk *clk = ERR_PTR(-EPROBE_DEFER);
>> @@ -2903,8 +2904,17 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
>> list_for_each_entry(provider, &of_clk_providers, link) {
>> if (provider->node == clkspec->np)
>> clk = provider->get(clkspec, provider->data);
>> - if (!IS_ERR(clk))
>> + if (!IS_ERR(clk)) {
>> + clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
>> + con_id);
>> +
>> + if (!IS_ERR(clk) && !__clk_get(clk)) {
>> + __clk_free_clk(clk);
>> + clk = ERR_PTR(-ENOENT);
>> + }
>> +
>> break;
>> + }
>> }
>>
>> return clk;
>> @@ -2915,7 +2925,7 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
>> struct clk *clk;
>>
>> mutex_lock(&of_clk_mutex);
>> - clk = __of_clk_get_from_provider(clkspec);
>> + clk = __of_clk_get_from_provider(clkspec, NULL, __func__);
>> mutex_unlock(&of_clk_mutex);
>>
>> return clk;
>> diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
>> index 23c44e51df69..ba845408cc3e 100644
>> --- a/drivers/clk/clk.h
>> +++ b/drivers/clk/clk.h
>> @@ -13,10 +13,27 @@ struct clk_hw;
>>
>> #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>> struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
>> -struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
>> + const char *dev_id, const char *con_id);
>> void of_clk_lock(void);
>> void of_clk_unlock(void);
>> #endif
>>
>> +#ifdef CONFIG_COMMON_CLK
>> struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
>> const char *con_id);
>> +void __clk_free_clk(struct clk *clk);
>> +#else
>> +/* All these casts to avoid ifdefs in clkdev... */
>> +static inline struct clk *
>> +__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id)
>> +{
>> + return (struct clk *)hw;
>> +}
>> +static inline void __clk_free_clk(struct clk *clk) { }
>> +static struct clk_hw *__clk_get_hw(struct clk *clk)
>> +{
>> + return (struct clk_hw *)clk;
>> +}
>> +
>> +#endif
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> index 29a1ab7af4b8..ec6b3dc1c2ce 100644
>> --- a/drivers/clk/clkdev.c
>> +++ b/drivers/clk/clkdev.c
>> @@ -29,6 +29,20 @@ static DEFINE_MUTEX(clocks_mutex);
>>
>> #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>>
>> +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
>> + const char *dev_id, const char *con_id)
>> +{
>> + struct clk *clk;
>> +
>> + if (!clkspec)
>> + return ERR_PTR(-EINVAL);
>> +
>> + of_clk_lock();
>> + clk = __of_clk_get_from_provider(clkspec, dev_id, con_id);
>> + of_clk_unlock();
>> + return clk;
>> +}
>> +
>> /**
>> * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
>> * @clkspec: pointer to a clock specifier data structure
>> @@ -39,22 +53,11 @@ static DEFINE_MUTEX(clocks_mutex);
>> */
>> struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
>> {
>> - struct clk *clk;
>> -
>> - if (!clkspec)
>> - return ERR_PTR(-EINVAL);
>> -
>> - of_clk_lock();
>> - clk = __of_clk_get_from_provider(clkspec);
>> -
>> - if (!IS_ERR(clk) && !__clk_get(clk))
>> - clk = ERR_PTR(-ENOENT);
>> -
>> - of_clk_unlock();
>> - return clk;
>> + return __of_clk_get_by_clkspec(clkspec, NULL, __func__);
>> }
>>
>> -static struct clk *__of_clk_get(struct device_node *np, int index)
>> +static struct clk *__of_clk_get(struct device_node *np, int index,
>> + const char *dev_id, const char *con_id)
>> {
>> struct of_phandle_args clkspec;
>> struct clk *clk;
>> @@ -68,7 +71,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index)
>> if (rc)
>> return ERR_PTR(rc);
>>
>> - clk = of_clk_get_by_clkspec(&clkspec);
>> + clk = __of_clk_get_by_clkspec(&clkspec, dev_id, con_id);
>> of_node_put(clkspec.np);
>>
>> return clk;
>> @@ -76,12 +79,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index)
>>
>> struct clk *of_clk_get(struct device_node *np, int index)
>> {
>> - struct clk *clk = __of_clk_get(np, index);
>> -
>> - if (!IS_ERR(clk))
>> - clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, NULL);
>> -
>> - return clk;
>> + return __of_clk_get(np, index, np->full_name, NULL);
>> }
>> EXPORT_SYMBOL(of_clk_get);
>>
>> @@ -102,12 +100,10 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
>> */
>> if (name)
>> index = of_property_match_string(np, "clock-names", name);
>> - clk = __of_clk_get(np, index);
>> + clk = __of_clk_get(np, index, dev_id, name);
>> if (!IS_ERR(clk)) {
>> - clk = __clk_create_clk(__clk_get_hw(clk), dev_id, name);
>> break;
>> - }
>> - else if (name && index >= 0) {
>> + } else if (name && index >= 0) {
>> if (PTR_ERR(clk) != -EPROBE_DEFER)
>> pr_err("ERROR: could not get clock %s:%s(%i)\n",
>> np->full_name, name ? name : "", index);
>> @@ -209,17 +205,13 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
>> if (!cl)
>> goto out;
>>
>> - if (!__clk_get(cl->clk)) {
>> + clk = __clk_create_clk(__clk_get_hw(cl->clk), dev_id, con_id);
>> + if (!__clk_get(clk)) {
>> + __clk_free_clk(clk);
>> cl = NULL;
>> goto out;
>> }
>>
>> -#if defined(CONFIG_COMMON_CLK)
>> - clk = __clk_create_clk(__clk_get_hw(cl->clk), dev_id, con_id);
>> -#else
>> - clk = cl->clk;
>> -#endif
>> -
>> out:
>> mutex_unlock(&clocks_mutex);
>>
>
> --
> 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/
--
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/