Re: [PATCH v2] clkdev: Always allocate a struct clk and call __clk_get() w/ CCF
From: Sylwester Nawrocki
Date: Fri Feb 06 2015 - 05:57:30 EST
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.
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.
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/