[PATCH 0/3] CCF fixes/cleanup for per-user series

[PATCH 0/3] CCF fixes/cleanup for per-user series

[Stephen Boyd]

Here are some fixes from today's dive into the fallout from the
per-user constraints patches. We should do a final sweep later
to rename local variables with type struct clk_core to core.

Stephen Boyd (3):
  clkdev: Always allocate a struct clk and call __clk_get() w/ CCF
  clk: Rename child_node to clks_node to avoid confusion
  clk: Replace of_clk_get_by_clkspec() with of_clk_get_from_provider()

 drivers/clk/clk-conf.c |  6 ++---
 drivers/clk/clk.c      | 59 ++++++++++++++++++++++++++++----------------------
 drivers/clk/clk.h      | 16 ++++++++++----
 drivers/clk/clkdev.c   | 55 ++++++++--------------------------------------
 include/linux/clkdev.h |  5 ++++-
 5 files changed, 61 insertions(+), 80 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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
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 <s.nawrocki@samsung.com>
Reported-by: Alban Browaeys <alban.browaeys@gmail.com>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c      | 18 ++++++++++++----
 drivers/clk/clk.h      | 13 +++++++++++-
 drivers/clk/clkdev.c   | 57 +++++++++++++++++++++-----------------------------
 include/linux/clkdev.h |  5 ++++-
 4 files changed, 54 insertions(+), 39 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..813c5b0754d3 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -13,10 +13,21 @@ 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
+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) { }
+#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..e5859a51a4e2 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -19,7 +19,6 @@
 #include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
-#include <linux/clk-provider.h>
 #include <linux/of.h>
 
 #include "clk.h"
@@ -29,6 +28,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 +52,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 +70,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 +78,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 +99,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 +204,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(cl->hw, 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);
 
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index 94bad77eeb4a..9551f84273b3 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -21,7 +21,10 @@ struct clk_lookup {
 	struct list_head	node;
 	const char		*dev_id;
 	const char		*con_id;
-	struct clk		*clk;
+	union {
+		struct clk		*clk;
+		struct clk_hw		*hw;
+	};
 };
 
 #define CLKDEV_INIT(d, n, c)	\
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

On 02/05/15 16:19, Stephen Boyd wrote:
>  
> diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
> index 94bad77eeb4a..9551f84273b3 100644
> --- a/include/linux/clkdev.h
> +++ b/include/linux/clkdev.h
> @@ -21,7 +21,10 @@ struct clk_lookup {
>  	struct list_head	node;
>  	const char		*dev_id;
>  	const char		*con_id;
> -	struct clk		*clk;
> +	union {
> +		struct clk		*clk;
> +		struct clk_hw		*hw;
> +	};

Oh right this won't work when CONFIG_COMMON_CLK=y and hw isn't assigned
by everyone... We'll have to scrap this part until later.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/3] clk: Rename child_node to clks_node to avoid confusion

[Stephen Boyd]

The child_node member of struct clk is named the same as the
child_node member of struct clk_core. Let's rename the struct
clk's member to clks_node to avoid getting confused with the
child_node member of struct clk_core and to match the name of the
list head, clks.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Alban Browaeys <alban.browaeys@gmail.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5469d7714f5d..315dc22b7356 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -83,7 +83,7 @@ struct clk {
 	const char *con_id;
 	unsigned long min_rate;
 	unsigned long max_rate;
-	struct hlist_node child_node;
+	struct hlist_node clks_node;
 };
 
 /***           locking             ***/
@@ -851,10 +851,10 @@ static void clk_core_get_boundaries(struct clk_core *clk,
 	*min_rate = 0;
 	*max_rate = ULONG_MAX;
 
-	hlist_for_each_entry(clk_user, &clk->clks, child_node)
+	hlist_for_each_entry(clk_user, &clk->clks, clks_node)
 		*min_rate = max(*min_rate, clk_user->min_rate);
 
-	hlist_for_each_entry(clk_user, &clk->clks, child_node)
+	hlist_for_each_entry(clk_user, &clk->clks, clks_node)
 		*max_rate = min(*max_rate, clk_user->max_rate);
 }
 
@@ -2376,7 +2376,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 	clk->max_rate = ULONG_MAX;
 
 	clk_prepare_lock();
-	hlist_add_head(&clk->child_node, &hw->core->clks);
+	hlist_add_head(&clk->clks_node, &hw->core->clks);
 	clk_prepare_unlock();
 
 	return clk;
@@ -2385,7 +2385,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
 void __clk_free_clk(struct clk *clk)
 {
 	clk_prepare_lock();
-	hlist_del(&clk->child_node);
+	hlist_del(&clk->clks_node);
 	clk_prepare_unlock();
 
 	kfree(clk);
@@ -2663,7 +2663,7 @@ void __clk_put(struct clk *clk)
 
 	clk_prepare_lock();
 
-	hlist_del(&clk->child_node);
+	hlist_del(&clk->clks_node);
 	clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/3] clk: Rename child_node to clks_node to avoid confusion

[Tomeu Vizoso]

On 02/06/2015 01:19 AM, Stephen Boyd wrote:
> The child_node member of struct clk is named the same as the
> child_node member of struct clk_core. Let's rename the struct
> clk's member to clks_node to avoid getting confused with the
> child_node member of struct clk_core and to match the name of the
> list head, clks.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Alban Browaeys <alban.browaeys@gmail.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/clk.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Yeah, I like this better.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks,

Tomeu

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5469d7714f5d..315dc22b7356 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -83,7 +83,7 @@ struct clk {
>  	const char *con_id;
>  	unsigned long min_rate;
>  	unsigned long max_rate;
> -	struct hlist_node child_node;
> +	struct hlist_node clks_node;
>  };
>  
>  /***           locking             ***/
> @@ -851,10 +851,10 @@ static void clk_core_get_boundaries(struct clk_core *clk,
>  	*min_rate = 0;
>  	*max_rate = ULONG_MAX;
>  
> -	hlist_for_each_entry(clk_user, &clk->clks, child_node)
> +	hlist_for_each_entry(clk_user, &clk->clks, clks_node)
>  		*min_rate = max(*min_rate, clk_user->min_rate);
>  
> -	hlist_for_each_entry(clk_user, &clk->clks, child_node)
> +	hlist_for_each_entry(clk_user, &clk->clks, clks_node)
>  		*max_rate = min(*max_rate, clk_user->max_rate);
>  }
>  
> @@ -2376,7 +2376,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
>  	clk->max_rate = ULONG_MAX;
>  
>  	clk_prepare_lock();
> -	hlist_add_head(&clk->child_node, &hw->core->clks);
> +	hlist_add_head(&clk->clks_node, &hw->core->clks);
>  	clk_prepare_unlock();
>  
>  	return clk;
> @@ -2385,7 +2385,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
>  void __clk_free_clk(struct clk *clk)
>  {
>  	clk_prepare_lock();
> -	hlist_del(&clk->child_node);
> +	hlist_del(&clk->clks_node);
>  	clk_prepare_unlock();
>  
>  	kfree(clk);
> @@ -2663,7 +2663,7 @@ void __clk_put(struct clk *clk)
>  
>  	clk_prepare_lock();
>  
> -	hlist_del(&clk->child_node);
> +	hlist_del(&clk->clks_node);
>  	clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>  	owner = clk->core->owner;
>  	kref_put(&clk->core->ref, __clk_release);
> 

[PATCH 3/3] clk: Replace of_clk_get_by_clkspec() with of_clk_get_from_provider()

[Stephen Boyd]

of_clk_get_by_clkspec() has the same function signature as
of_clk_get_from_provider()

 struct clk *of_clk_get_by_clkspec(struct of_phandle_args
 *clkspec)
 struct clk *of_clk_get_from_provider(struct of_phandle_args
 *clkspec)

except of_clk_get_by_clkspec() checks to make sure clkspec is not
NULL. Let's remove of_clk_get_by_clkspec() and replace the
callers of it (clkconf.c) with of_clk_get_from_provider().

Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk-conf.c |  6 +++---
 drivers/clk/clk.c      | 31 ++++++++++++++-----------------
 drivers/clk/clk.h      |  3 ---
 drivers/clk/clkdev.c   | 30 +-----------------------------
 4 files changed, 18 insertions(+), 52 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796aa3ed..6a681a2c4c89 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -39,7 +39,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 		}
 		if (clkspec.np == node && !clk_supplier)
 			return 0;
-		pclk = of_clk_get_by_clkspec(&clkspec);
+		pclk = of_clk_get_from_provider(&clkspec);
 		if (IS_ERR(pclk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
@@ -54,7 +54,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 			rc = 0;
 			goto err;
 		}
-		clk = of_clk_get_by_clkspec(&clkspec);
+		clk = of_clk_get_from_provider(&clkspec);
 		if (IS_ERR(clk)) {
 			pr_warn("clk: couldn't get parent clock %d for %s\n",
 				index, node->full_name);
@@ -98,7 +98,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
 			if (clkspec.np == node && !clk_supplier)
 				return 0;
 
-			clk = of_clk_get_by_clkspec(&clkspec);
+			clk = of_clk_get_from_provider(&clkspec);
 			if (IS_ERR(clk)) {
 				pr_warn("clk: couldn't get clock %d for %s\n",
 					index, node->full_name);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 315dc22b7356..5d804a43ab1a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2807,16 +2807,6 @@ static LIST_HEAD(of_clk_providers);
 static DEFINE_MUTEX(of_clk_mutex);
 
 /* of_clk_provider list locking helpers */
-void of_clk_lock(void)
-{
-	mutex_lock(&of_clk_mutex);
-}
-
-void of_clk_unlock(void)
-{
-	mutex_unlock(&of_clk_mutex);
-}
-
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				     void *data)
 {
@@ -2900,7 +2890,11 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
 
+	if (!clkspec)
+		return ERR_PTR(-EINVAL);
+
 	/* Check if we have such a provider in our array */
+	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(provider, &of_clk_providers, link) {
 		if (provider->node == clkspec->np)
 			clk = provider->get(clkspec, provider->data);
@@ -2916,19 +2910,22 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 			break;
 		}
 	}
+	mutex_unlock(&of_clk_mutex);
 
 	return clk;
 }
 
+/**
+ * of_clk_get_from_provider() - Lookup a clock from a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ */
 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, NULL, __func__);
-	mutex_unlock(&of_clk_mutex);
-
-	return clk;
+	return __of_clk_get_from_provider(clkspec, NULL, __func__);
 }
 
 int of_clk_get_parent_count(struct device_node *np)
diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h
index 813c5b0754d3..0f7f87d1fffc 100644
--- a/drivers/clk/clk.h
+++ b/drivers/clk/clk.h
@@ -12,11 +12,8 @@
 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,
 				       const char *dev_id, const char *con_id);
-void of_clk_lock(void);
-void of_clk_unlock(void);
 #endif
 
 #ifdef CONFIG_COMMON_CLK
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index e5859a51a4e2..9bef5ff0d3ab 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -27,34 +27,6 @@ static LIST_HEAD(clocks);
 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
- *
- * This function looks up a struct clk from the registered list of clock
- * providers, an input is a clock specifier data structure as returned
- * from the of_parse_phandle_with_args() function call.
- */
-struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
-{
-	return __of_clk_get_by_clkspec(clkspec, NULL, __func__);
-}
-
 static struct clk *__of_clk_get(struct device_node *np, int index,
 			       const char *dev_id, const char *con_id)
 {
@@ -70,7 +42,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, dev_id, con_id);
+	clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id);
 	of_node_put(clkspec.np);
 
 	return clk;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: Replace of_clk_get_by_clkspec() with of_clk_get_from_provider()

[Tomeu Vizoso]

On 02/06/2015 01:19 AM, Stephen Boyd wrote:
> of_clk_get_by_clkspec() has the same function signature as
> of_clk_get_from_provider()
> 
>  struct clk *of_clk_get_by_clkspec(struct of_phandle_args
>  *clkspec)
>  struct clk *of_clk_get_from_provider(struct of_phandle_args
>  *clkspec)
> 
> except of_clk_get_by_clkspec() checks to make sure clkspec is not
> NULL. Let's remove of_clk_get_by_clkspec() and replace the
> callers of it (clkconf.c) with of_clk_get_from_provider().
> 
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/clk-conf.c |  6 +++---
>  drivers/clk/clk.c      | 31 ++++++++++++++-----------------
>  drivers/clk/clk.h      |  3 ---
>  drivers/clk/clkdev.c   | 30 +-----------------------------
>  4 files changed, 18 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index aad4796aa3ed..6a681a2c4c89 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -39,7 +39,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>  		}
>  		if (clkspec.np == node && !clk_supplier)
>  			return 0;
> -		pclk = of_clk_get_by_clkspec(&clkspec);
> +		pclk = of_clk_get_from_provider(&clkspec);
>  		if (IS_ERR(pclk)) {
>  			pr_warn("clk: couldn't get parent clock %d for %s\n",
>  				index, node->full_name);
> @@ -54,7 +54,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>  			rc = 0;
>  			goto err;
>  		}
> -		clk = of_clk_get_by_clkspec(&clkspec);
> +		clk = of_clk_get_from_provider(&clkspec);
>  		if (IS_ERR(clk)) {
>  			pr_warn("clk: couldn't get parent clock %d for %s\n",
>  				index, node->full_name);
> @@ -98,7 +98,7 @@ static int __set_clk_rates(struct device_node *node, bool clk_supplier)
>  			if (clkspec.np == node && !clk_supplier)
>  				return 0;
>  
> -			clk = of_clk_get_by_clkspec(&clkspec);
> +			clk = of_clk_get_from_provider(&clkspec);
>  			if (IS_ERR(clk)) {
>  				pr_warn("clk: couldn't get clock %d for %s\n",
>  					index, node->full_name);
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 315dc22b7356..5d804a43ab1a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2807,16 +2807,6 @@ static LIST_HEAD(of_clk_providers);
>  static DEFINE_MUTEX(of_clk_mutex);
>  
>  /* of_clk_provider list locking helpers */

Patch looks good to me, but this comment can be removed now.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks,

Tomeu

Re: [PATCH 3/3] clk: Replace of_clk_get_by_clkspec() with of_clk_get_from_provider()

[Stephen Boyd]

On 02/06/15 03:43, Tomeu Vizoso wrote:
> On 02/06/2015 01:19 AM, Stephen Boyd wrote:
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 315dc22b7356..5d804a43ab1a 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2807,16 +2807,6 @@ static LIST_HEAD(of_clk_providers);
>>  static DEFINE_MUTEX(of_clk_mutex);
>>  
>>  /* of_clk_provider list locking helpers */
> Patch looks good to me, but this comment can be removed now.
>
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>

Done, plus I removed "clk.h" include in clk-conf.c.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Stephen Boyd]

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@samsung.com>
Reported-by: Alban Browaeys <alban.browaeys@gmail.com>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 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);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Sylwester Nawrocki]

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@samsung.com>
> Reported-by: Alban Browaeys <alban.browaeys@gmail.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

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@samsung.com>

> ---
>  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);
>  

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

[Tomeu Vizoso]

On 6 February 2015 at 11:57, Sylwester Nawrocki <s.nawrocki@samsung.com> 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@samsung.com>
>> Reported-by: Alban Browaeys <alban.browaeys@gmail.com>
>> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>
> 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@samsung.com>
>
>> ---
>>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Stephen Boyd]

On 02/06/15 02:57, Sylwester Nawrocki wrote:
>
> 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.

Yes it was to reduce diff. I plan to do the rename once the dust settles.

>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>
>

Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Tomeu Vizoso]

On 02/06/2015 02:40 AM, 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@samsung.com>
> Reported-by: Alban Browaeys <alban.browaeys@gmail.com>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  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

[snip]

> @@ -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);

We need to check for clk being an error either here or in __clk_get
because __clk_create_clk can return -ENOMEM.

> +	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);

Otherwise it looks good to me.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks,

Tomeu

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

[Stephen Boyd]

On 02/06/15 04:34, Tomeu Vizoso wrote:
> On 02/06/2015 02:40 AM, Stephen Boyd wrote:
>
>> @@ -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);
> We need to check for clk being an error either here or in __clk_get
> because __clk_create_clk can return -ENOMEM.

Good catch. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project