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

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

[Stephen Boyd]

Here are some fixes from yesterdays'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.

Changes from v1:
 * Picked up tags
 * Dropped clk.h include in last patch
 * Handled IS_ERR() retrurn from __clk_create_clk() in clkdev in first patch
 * Removed dead comment in last patch

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 |  7 +++---
 drivers/clk/clk.c      | 60 +++++++++++++++++++++++++++-----------------------
 drivers/clk/clk.h      | 22 ++++++++++++++----
 drivers/clk/clkdev.c   | 57 ++++++++++-------------------------------------
 4 files changed, 66 insertions(+), 80 deletions(-)

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

[PATCH v2 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>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Alban Browaeys <prahal@yahoo.com>
Reviewed-by: 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 | 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

[PATCH v2 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.

Reviewed-by: 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

[PATCH v2 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>
Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk-conf.c |  7 +++----
 drivers/clk/clk.c      | 32 ++++++++++++++------------------
 drivers/clk/clk.h      |  3 ---
 drivers/clk/clkdev.c   | 30 +-----------------------------
 4 files changed, 18 insertions(+), 54 deletions(-)

diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
index aad4796aa3ed..48a65b2b4027 100644
--- a/drivers/clk/clk-conf.c
+++ b/drivers/clk/clk-conf.c
@@ -13,7 +13,6 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/printk.h>
-#include "clk.h"
 
 static int __set_clk_parents(struct device_node *node, bool clk_supplier)
 {
@@ -39,7 +38,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 +53,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 +97,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..80e961984cda 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2806,17 +2806,6 @@ static const struct of_device_id __clk_of_table_sentinel
 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 +2889,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 +2909,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 ba845408cc3e..00b35a13cdf3 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 043fd3633373..1fcb6ef2cdac 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -28,34 +28,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)
 {
@@ -71,7 +43,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 v2 1/3] clkdev: Always allocate a struct clk and call __clk_get() w/ CCF

[Mike Turquette]

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 : [<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>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Tested-by: Alban Browaeys <prahal@yahoo.com>
> Reviewed-by: 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 | 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
> 

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

[Stephen Boyd]

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

Applied to clk-next

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

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

[Stephen Boyd]

On 02/06/15 11:42, 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>
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---

Applied to clk-next


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