Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752572AbbBKDIr (ORCPT ); Tue, 10 Feb 2015 22:08:47 -0500 Received: from mail-ig0-f174.google.com ([209.85.213.174]:40595 "EHLO mail-ig0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751694AbbBKDIp convert rfc822-to-8bit (ORCPT ); Tue, 10 Feb 2015 22:08:45 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Stephen Boyd , "Stephen Boyd" From: Mike Turquette In-Reply-To: <1423251765-2947-2-git-send-email-sboyd@codeaurora.org> Cc: linux-kernel@vger.kernel.org, "Sylwester Nawrocki" , "Alban Browaeys" , "Tomeu Vizoso" References: <1423251765-2947-1-git-send-email-sboyd@codeaurora.org> <1423251765-2947-2-git-send-email-sboyd@codeaurora.org> Message-ID: <20150211030837.421.29215@quantum> User-Agent: alot/0.3.5 Subject: Re: [PATCH v2 1/3] clkdev: Always allocate a struct clk and call __clk_get() w/ CCF Date: Tue, 10 Feb 2015 19:08:37 -0800 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12470 Lines: 294 Quoting Stephen Boyd (2015-02-06 11:42:43) > 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. Applied to clk-next. Regards, Mike > > 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 : [] lr : [] 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 > [] (__clk_put) from [] (of_clk_set_defaults+0xe0/0x2c0) > [] (of_clk_set_defaults) from [] (platform_drv_probe+0x18/0xa4) > [] (platform_drv_probe) from [] (driver_probe_device+0x10c/0x22c) > [] (driver_probe_device) from [] (__driver_attach+0x8c/0x90) > [] (__driver_attach) from [] (bus_for_each_dev+0x54/0x88) > [] (bus_for_each_dev) from [] (bus_add_driver+0xd4/0x1d0) > [] (bus_add_driver) from [] (driver_register+0x78/0xf4) > [] (driver_register) from [] (fimc_md_init+0x14/0x30) > [] (fimc_md_init) from [] (do_one_initcall+0x80/0x1d0) > [] (do_one_initcall) from [] (kernel_init_freeable+0x108/0x1d4) > [] (kernel_init_freeable) from [] (kernel_init+0x8/0xec) > [] (kernel_init) from [] (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 > use a union 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 > Reported-by: Alban Browaeys > Tested-by: Sylwester Nawrocki > Tested-by: Alban Browaeys > Reviewed-by: Tomeu Vizoso > Signed-off-by: Stephen Boyd > --- > drivers/clk/clk.c | 18 ++++++++++++---- > drivers/clk/clk.h | 19 ++++++++++++++++- > drivers/clk/clkdev.c | 59 ++++++++++++++++++++++++---------------------------- > 3 files changed, 59 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..043fd3633373 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,16 @@ 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 (IS_ERR(clk)) > + goto out; > + > + 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); > > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/