[PATCH 0/5] clk: forbit multiple adding of clock providers and fix false positive EPROBE_DEFER

[PATCH 0/5] clk: forbit multiple adding of clock providers and fix false positive EPROBE_DEFER

[Masahiro Yamada]

I dropped a question mail around 10 days ago,
https://lkml.org/lkml/2016/7/8/627

but I did not get any reply.

At least, nobody stopped me from going with this fix,
so here is the series to fix it.



Masahiro Yamada (5):
  clk: add a helper function __of_clk_find_provider()
  clk: avoid adding clock provider multiple times for one node
  clk: refactor of_clk_del_provider()
  clk: fix false positive EPROBE_DEFER of
    __of_clk_get_hw_from_provider()
  clk: return -EINVAL if clock provider has no .get callback

 drivers/clk/clk.c | 77 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 26 deletions(-)

-- 
1.9.1

[PATCH 1/5] clk: add a helper function __of_clk_find_provider()

[Masahiro Yamada]

This helper is useful to find an OF clock provider registered for a
device node.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 484acc2..e8c79a7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3010,6 +3010,21 @@ of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data)
 EXPORT_SYMBOL_GPL(of_clk_hw_onecell_get);
 
 /**
+ * __of_clk_find_provider - Find a clock provider associated with a device node
+ * @np: device node to obtain the clock provider for
+ */
+static struct of_clk_provider *__of_clk_find_provider(struct device_node *np)
+{
+	struct of_clk_provider *cp;
+
+	list_for_each_entry(cp, &of_clk_providers, link)
+		if (cp->node == np)
+			return cp;
+
+	return NULL;
+}
+
+/**
  * of_clk_add_provider() - Register a clock provider for a node
  * @np: Device node pointer associated with clock provider
  * @clk_src_get: callback for decoding clock
-- 
1.9.1

[PATCH 2/5] clk: avoid adding clock provider multiple times for one node

[Masahiro Yamada]

It is insane to add multiple OF clock providers against one device
node.

Let of_clk_add(_hw)_provider() fail with -EEXIST if the given node
is already associated with an OF clock provider.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e8c79a7..7832343 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3038,15 +3038,24 @@ int of_clk_add_provider(struct device_node *np,
 	struct of_clk_provider *cp;
 	int ret;
 
+	mutex_lock(&of_clk_mutex);
+
+	cp = __of_clk_find_provider(np);
+	if (cp) {
+		mutex_unlock(&of_clk_mutex);
+		return -EEXIST;
+	}
+
 	cp = kzalloc(sizeof(struct of_clk_provider), GFP_KERNEL);
-	if (!cp)
+	if (!cp) {
+		mutex_unlock(&of_clk_mutex);
 		return -ENOMEM;
+	}
 
 	cp->node = of_node_get(np);
 	cp->data = data;
 	cp->get = clk_src_get;
 
-	mutex_lock(&of_clk_mutex);
 	list_add(&cp->link, &of_clk_providers);
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clock from %s\n", np->full_name);
@@ -3073,15 +3082,24 @@ int of_clk_add_hw_provider(struct device_node *np,
 	struct of_clk_provider *cp;
 	int ret;
 
+	mutex_lock(&of_clk_mutex);
+
+	cp = __of_clk_find_provider(np);
+	if (cp) {
+		mutex_unlock(&of_clk_mutex);
+		return -EEXIST;
+	}
+
 	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
-	if (!cp)
+	if (!cp) {
+		mutex_unlock(&of_clk_mutex);
 		return -ENOMEM;
+	}
 
 	cp->node = of_node_get(np);
 	cp->data = data;
 	cp->get_hw = get;
 
-	mutex_lock(&of_clk_mutex);
 	list_add(&cp->link, &of_clk_providers);
 	mutex_unlock(&of_clk_mutex);
 	pr_debug("Added clk_hw provider from %s\n", np->full_name);
-- 
1.9.1

[PATCH 3/5] clk: refactor of_clk_del_provider()

[Masahiro Yamada]

Now, it is guaranteed that a single node does not appear twice in
the list of OF clock providers.  So, of_clk_del_provider() only
needs to free the first occurrence.  This can be implemented more
simply with __of_clk_find_provider().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7832343..60daf60 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3121,13 +3121,11 @@ void of_clk_del_provider(struct device_node *np)
 	struct of_clk_provider *cp;
 
 	mutex_lock(&of_clk_mutex);
-	list_for_each_entry(cp, &of_clk_providers, link) {
-		if (cp->node == np) {
-			list_del(&cp->link);
-			of_node_put(cp->node);
-			kfree(cp);
-			break;
-		}
+	cp = __of_clk_find_provider(np);
+	if (cp) {
+		list_del(&cp->link);
+		of_node_put(cp->node);
+		kfree(cp);
 	}
 	mutex_unlock(&of_clk_mutex);
 }
-- 
1.9.1

[PATCH 4/5] clk: fix false positive EPROBE_DEFER of __of_clk_get_hw_from_provider()

[Masahiro Yamada]

Currently, this function iterates over the whole list of OF clock
providers.  If the given node is found and __of_clk_get_hw_provider()
fails, it continues to search for the next node.  In the end, it will
reach the end of the list and return -EPROBE_DEFER.  This is odd.
An OF clock provider is already registered for the given node, so it
is not deferred probe at all.

Actually, I was hit by this case when a clock is queried with a too
large index against an already registered OF clock provider.  In this
case, of_clk_src_onecell_get() returns -EINVAL (this is correct), but
clk_get() returns -EPROBE_DEFER, so the driver is visited later,
where it never succeeds in getting the clock.

Another good thing is this function is much more readable now.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 60daf60..2207098 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3156,25 +3156,19 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec,
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-EPROBE_DEFER);
-	struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
+	struct clk_hw *hw;
 
 	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)
-			hw = __of_clk_get_hw_from_provider(provider, clkspec);
-		if (!IS_ERR(hw)) {
-			clk = __clk_create_clk(hw, dev_id, con_id);
-
-			if (!IS_ERR(clk) && !__clk_get(clk)) {
-				__clk_free_clk(clk);
-				clk = ERR_PTR(-ENOENT);
-			}
-
-			break;
+	provider = __of_clk_find_provider(clkspec->np);
+	if (provider) {
+		hw = __of_clk_get_hw_from_provider(provider, clkspec);
+		clk = __clk_create_clk(hw, dev_id, con_id);
+		if (!IS_ERR(clk) && !__clk_get(clk)) {
+			__clk_free_clk(clk);
+			clk = ERR_PTR(-ENOENT);
 		}
 	}
 	mutex_unlock(&of_clk_mutex);
-- 
1.9.1

[PATCH 5/5] clk: return -EINVAL if clock provider has no .get callback

[Masahiro Yamada]

Currently, __of_clk_get_hw_from_provider() returns -EPROBE_DEFER
if neither .get_hw nor .get is registered for the given provider.
This means of_clk_add(_hw)_provider() has been already called for
the node in a wrong way; it is not deferred probing at all.  EINVAL
is a better fit for the case.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2207098..3004388 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3136,7 +3136,7 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider,
 			      struct of_phandle_args *clkspec)
 {
 	struct clk *clk;
-	struct clk_hw *hw = ERR_PTR(-EPROBE_DEFER);
+	struct clk_hw *hw = ERR_PTR(-EINVAL);
 
 	if (provider->get_hw) {
 		hw = provider->get_hw(clkspec, provider->data);
-- 
1.9.1