[PATCH v13 3/6] clk: Make clk API return per-user struct clk instances

On 02/05/15 11:44, Sylwester Nawrocki wrote:

quoted

Hi Tomeu,

On 23/01/15 12:03, Tomeu Vizoso wrote:

quoted

 int __clk_get(struct clk *clk)
 {
-	if (clk) {
-		if (!try_module_get(clk->owner))
+	struct clk_core *core = !clk ? NULL : clk->core;
+
+	if (core) {
+		if (!try_module_get(core->owner))
 			return 0;
 
-		kref_get(&clk->ref);
+		kref_get(&core->ref);
 	}
 	return 1;
 }
 
-void __clk_put(struct clk *clk)
+static void clk_core_put(struct clk_core *core)
 {
 	struct module *owner;
 
-	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
-		return;
+	owner = core->owner;
 
 	clk_prepare_lock();
-	owner = clk->owner;
-	kref_put(&clk->ref, __clk_release);
+	kref_put(&core->ref, __clk_release);
 	clk_prepare_unlock();
 
 	module_put(owner);
 }
 
+void __clk_put(struct clk *clk)
+{
+	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+		return;
+
+	clk_core_put(clk->core);
+	kfree(clk);

Why do we have kfree() here? clk_get() doesn't allocate the data structure 
being freed here. What happens if we do clk_get(), clk_put(), clk_get() 
on same clock?

I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.

I tested linux-next on Odroid U3 and booting fails with oopses as below.
There is no problems when the above kfree() is commented out.

Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
return an allocated clk pointer. Let's fix that.

----8<----

From: Stephen Boyd <redacted>
Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions

of_clk_get_by_clkspec() returns a struct clk pointer but it
doesn't create a new handle for the consumers. Instead it just
returns whatever the OF clk provider hands out. Let's create a
per-user handle here so that clk_put() can properly unlink it and
free it when the consumer is done.

Fixes: 035a61c314eb "clk: Make clk API return per-user struct clk instances"
Reported-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Stephen Boyd <redacted>
---
 drivers/clk/clkdev.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..00d747d09b2a 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c

@@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
 
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_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)
+static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
+					 const char *dev_id, const char *con_id)
 {
 	struct clk *clk;
 

@@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
 	of_clk_lock();
 	clk = __of_clk_get_from_provider(clkspec);
 
+	if (!IS_ERR(clk))
+		clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
 	if (!IS_ERR(clk) && !__clk_get(clk))
 		clk = ERR_PTR(-ENOENT);