clk: fix clk-gpio.c with optional clock= DT property

When the clock DT property is not given, of_clk_get_parent_count() returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory, which of course fails, causing the whole driver to fail to create the clock.

This causes the SolidRun platforms to fail probing the SDHCI1 interface which is connected to the WiFi.

Fix this by detecting errno codes, skipping the allocation, and fixing of_clk_gpio_gate_delayed_register_get() to handle a NULL parent_names array.

Fixes 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer")

Signed-off-by Russell King <[email protected]>

---
This goes all the way back to 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer") introduced in June in v4.3-rc2 - which raises the question why _development_ work in clk is being merged outside of the merge window.

A rewrite of this patch will be necessary to apply to v4.3 kernels.

This applies on top of v4.4-rc6.

Hi Russell,

Quoting Russell King (2016-01-02 02:01:34)

Quoted 5 lines

When the clock DT property is not given, of_clk_get_parent_count() returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory, which of course fails, causing the whole driver to fail to create the clock.

Quoted 6 lines

This causes the SolidRun platforms to fail probing the SDHCI1 interface which is connected to the WiFi.

Fix this by detecting errno codes, skipping the allocation, and fixing of_clk_gpio_gate_delayed_register_get() to handle a NULL parent_names array.

Thanks for the fix. Applied to clk-next manually.

Quoted 4 lines

This goes all the way back to 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer") introduced in June in v4.3-rc2 - which raises the question why _development_ work in clk is being merged outside of the merge window.

That patch was merged into the clk tree in June 2015, based on 4.2-rc1.

Below is the modified version of your patch.

Quoted 1 line

That patch was merged into the clk tree in June 2015, based on 4.2-rc1.

Interesting... I guess git describe is buggy then:

$ git describe --contains 80eeb1f0f757

When the clock DT property is not given, of_clk_get_parent_count() returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory, which of course fails, causing the whole driver to fail to create the clock.

This causes the SolidRun platforms to fail probing the SDHCI1 interface which is connected to the WiFi.

Fix this by detecting errno codes, skipping the allocation, and fixing of_clk_gpio_gate_delayed_register_get() to handle a NULL parent_names array.

Fixes 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer")

Signed-off-by Russell King <[email protected]>

---
This goes all the way back to 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer") introduced in June in v4.3-rc2 - which raises the question why _development_ work in clk is being merged outside of the merge window.

A rewrite of this patch will be necessary to apply to v4.3 kernels.

This applies on top of v4.4-rc6.

 drivers/clk/clk-gpio.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/clk/clk-gpio.c +14 -9

index 335322dc403f..05cca9298f44 100644
@@ -264,8 +264,8 @@ static struct clk *of_clk_gpio_gate_delayed_register_get(const char *name,
 		const char * const *parent_names, u8 num_parents,
 		unsigned gpio, bool active_low)
 {
-	return clk_register_gpio_gate(NULL, name, parent_names[0],
-			gpio, active_low, 0);
+	return clk_register_gpio_gate(NULL, name, parent_names ?
+			parent_names[0] : NULL, gpio, active_low, 0);
 }
 
 static struct clk *of_clk_gpio_mux_delayed_register_get(const char *name,

@@ -292,13 +292,18 @@ static void __init of_gpio_clk_setup(struct device_node *node,
 		return;
 
 	num_parents = of_clk_get_parent_count(node);
-
-	parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
-	if (!parent_names)
-		return;
-
-	for (i = 0; i < num_parents; i++)
-		parent_names[i] = of_clk_get_parent_name(node, i);
+	if (num_parents > 0) {
+		parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
+		if (!parent_names) {
+			kfree(data);
+			return;
+		}
+
+		for (i = 0; i < num_parents; i++)
+			parent_names[i] = of_clk_get_parent_name(node, i);
+	} else {
+		parent_names = NULL;
+	}
 
 	data->num_parents = num_parents;
 	data->parent_names = parent_names;

Signature · 1 line

2.1.0

Quoted 66 lines

When the clock DT property is not given, of_clk_get_parent_count() returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory, which of course fails, causing the whole driver to fail to create the clock.

This causes the SolidRun platforms to fail probing the SDHCI1 interface which is connected to the WiFi.

Fix this by detecting errno codes, skipping the allocation, and fixing of_clk_gpio_gate_delayed_register_get() to handle a NULL parent_names array.

Fixes 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer")

Signed-off-by Russell King <[email protected]>

---
This goes all the way back to 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer") introduced in June in v4.3-rc2 - which raises the question why _development_ work in clk is being merged outside of the merge window.

A rewrite of this patch will be necessary to apply to v4.3 kernels.

This applies on top of v4.4-rc6.

 drivers/clk/clk-gpio.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/clk/clk-gpio.c +14 -9

index 335322dc403f..05cca9298f44 100644
@@ -264,8 +264,8 @@ static struct clk *of_clk_gpio_gate_delayed_register_get(const char *name,
              const char * const *parent_names, u8 num_parents,
              unsigned gpio, bool active_low)
 {
-     return clk_register_gpio_gate(NULL, name, parent_names[0],
-                     gpio, active_low, 0);
+     return clk_register_gpio_gate(NULL, name, parent_names ?
+                     parent_names[0] : NULL, gpio, active_low, 0);
 }
 
 static struct clk *of_clk_gpio_mux_delayed_register_get(const char *name,

@@ -292,13 +292,18 @@ static void __init of_gpio_clk_setup(struct device_node *node,
              return;
 
      num_parents = of_clk_get_parent_count(node);
-
-     parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
-     if (!parent_names)
-             return;
-
-     for (i = 0; i < num_parents; i++)
-             parent_names[i] = of_clk_get_parent_name(node, i);
+     if (num_parents > 0) {
+             parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
+             if (!parent_names) {
+                     kfree(data);
+                     return;
+             }
+
+             for (i = 0; i < num_parents; i++)
+                     parent_names[i] = of_clk_get_parent_name(node, i);
+     } else {
+             parent_names = NULL;
+     }
 

Quoted 4 lines

drivers/clk/clk-gpio.c +0 -0

      data->num_parents = num_parents;
      data->parent_names = parent_names;

Signature · 1 line

2.1.0

Yes, indeed, my patch above which fixed clk-gpio.c as can be seen above does not have the problem that I'm currently seeing...

Signature · 4 lines

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Quoting Russell King - ARM Linux (2016-02-17 15:05:29)

Quoted 67 lines · Russell King

Quoted 66 lines

When the clock DT property is not given, of_clk_get_parent_count() returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory, which of course fails, causing the whole driver to fail to create the clock.

This causes the SolidRun platforms to fail probing the SDHCI1 interface which is connected to the WiFi.

Fix this by detecting errno codes, skipping the allocation, and fixing of_clk_gpio_gate_delayed_register_get() to handle a NULL parent_names array.

Fixes 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer")

Signed-off-by Russell King <[email protected]>

---
This goes all the way back to 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer") introduced in June in v4.3-rc2 - which raises the question why _development_ work in clk is being merged outside of the merge window.

A rewrite of this patch will be necessary to apply to v4.3 kernels.

This applies on top of v4.4-rc6.

 drivers/clk/clk-gpio.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/clk/clk-gpio.c +14 -9

index 335322dc403f..05cca9298f44 100644
@@ -264,8 +264,8 @@ static struct clk *of_clk_gpio_gate_delayed_register_get(const char *name,
              const char * const *parent_names, u8 num_parents,
              unsigned gpio, bool active_low)
 {
-     return clk_register_gpio_gate(NULL, name, parent_names[0],
-                     gpio, active_low, 0);
+     return clk_register_gpio_gate(NULL, name, parent_names ?
+                     parent_names[0] : NULL, gpio, active_low, 0);
 }
 
 static struct clk *of_clk_gpio_mux_delayed_register_get(const char *name,

@@ -292,13 +292,18 @@ static void __init of_gpio_clk_setup(struct device_node *node,
              return;
 
      num_parents = of_clk_get_parent_count(node);
-
-     parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
-     if (!parent_names)
-             return;
-
-     for (i = 0; i < num_parents; i++)
-             parent_names[i] = of_clk_get_parent_name(node, i);
+     if (num_parents > 0) {
+             parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
+             if (!parent_names) {
+                     kfree(data);
+                     return;
+             }
+
+             for (i = 0; i < num_parents; i++)
+                     parent_names[i] = of_clk_get_parent_name(node, i);
+     } else {
+             parent_names = NULL;
+     }
 

Quoted 7 lines

Quoted 4 lines

drivers/clk/clk-gpio.c +0 -0

      data->num_parents = num_parents;
      data->parent_names = parent_names;

Signature · 1 line

2.1.0

Yes, indeed, my patch above which fixed clk-gpio.c as can be seen above does not have the problem that I'm currently seeing...

Hi Russell,

I must be missing something. After merging your patch on top of Brian's, the code looks like:

        ...
        int i, num_parents;

        num_parents = of_clk_get_parent_count(node);
        if (num_parents < 0)
                return;

        data = kzalloc(sizeof(*data), GFP_KERNEL);
        if (!data)
                return;

        if (num_parents) {
                parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
                if (!parent_names) {
                        kfree(data);
                        return;
                }

                for (i = 0; i < num_parents; i++)
                        parent_names[i] = of_clk_get_parent_name(node, i);
        } else {
                parent_names = NULL;
        }

Brian's if (num_parents < 0) check, followed by the if (num_parent) check appear equivalent to your original patch. Not sure why I merged it as if (num_parents) instead of if (num_parents > 0) as your original patch uses, but thanks to the extra check that predates your patch it should be equivalent.

Let me know if I've lost the plot.

This is even explained in the very first sentence of my commit log:

| When the clock DT property is not given, of_clk_get_parent_count()

                                           ^^^^^^^^^^^^^^^^^^^^^^^^^

| returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory,
^^^^^^^^^^^^^^^

-ENOENT is a negative number. Now look at the patch which totally rejects the clock if of_clk_get_parent_count() returns any negative number...

I assume, therefore, that you did not *even* read my commit log...

We mis-merged the original patch from Russell here and so the patch went almost all the way, except that we still failed to probe when there wasn't a clocks property in the DT node. Allow that case by making a negative value from of_clk_get_parent_count() into "no parents", like the original patch did.

Fixes 7ed88aa2efa5 ("clk: fix clk-gpio.c with optional clock= DT property")

Cc Russell King <[email protected]>

Cc Michael Turquette <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

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

drivers/clk/clk-gpio.c +1 -2

index 19fed65587e8..7b09a265d79f 100644
@@ -289,7 +289,7 @@ static void __init of_gpio_clk_setup(struct device_node *node,
 
        num_parents = of_clk_get_parent_count(node);
        if (num_parents < 0)
-               return;
+               num_parents = 0;
 
        data = kzalloc(sizeof(*data), GFP_KERNEL);
        if (!data)
--

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

We mis-merged the original patch from Russell here and so the patch went almost all the way, except that we still failed to probe when there wasn't a clocks property in the DT node. Allow that case by making a negative value from of_clk_get_parent_count() into "no parents", like the original patch did.

We mis-merged the original patch from Russell here and so the patch went almost all the way, except that we still failed to probe when there wasn't a clocks property in the DT node. Allow that case by making a negative value from of_clk_get_parent_count() into "no parents", like the original patch did.

Quoted 6 lines

We mis-merged the original patch from Russell here and so the patch went almost all the way, except that we still failed to probe when there wasn't a clocks property in the DT node. Allow that case by making a negative value from of_clk_get_parent_count() into "no parents", like the original patch did.

So you apply it anyway, and ignore all further discussion. I've no idea what kind of politics is going on here, but it has to stop.

I'm also still waiting for that apology from Mike - never apply a

modified patch without stating in the commit log that it was modified. It's basically fraud.

Signature · 4 lines

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Hi Russell,

Quoting Russell King - ARM Linux (2016-02-23 03:30:59)

Quoted 8 lines · Stephen Boyd

Quoted 6 lines

We mis-merged the original patch from Russell here and so the patch went almost all the way, except that we still failed to probe when there wasn't a clocks property in the DT node. Allow that case by making a negative value from of_clk_get_parent_count() into "no parents", like the original patch did.

Quoted 2 lines

So you apply it anyway, and ignore all further discussion. I've no idea what kind of politics is going on here, but it has to stop.

Stephen fixed the bug, and I didn't ask you to test because you made it clear that you did not have the time (understandable). Stephen reproduced the issue locally and insured that his version of the fix does actually fix it.

He is an equal co-maintainer so of course I support him to merge the fix, plus I find his version more readable.

Quoted 1 line

I'm also still waiting for that apology from Mike - never apply a

No thematics please. When you wrongly accused me of merging code outside of the merge window[0] did I ask for an apology?

Quoted 2 lines

modified patch without stating in the commit log that it was modified. It's basically fraud.

An oversight for which we are both equally guilty.

The "change" was the
resolution of a merge conflict, and I posted the resolved version of the patch to the thread[0]. You said, "Looks fine, thanks."

Quoted 12 lines · Russell King - ARM Linux

This is even explained in the very first sentence of my commit log:

| When the clock DT property is not given, of_clk_get_parent_count()

                                           ^^^^^^^^^^^^^^^^^^^^^^^^^

| returns -ENOENT, which then tries to allocate -2 x 4 bytes of memory,
^^^^^^^^^^^^^^^

-ENOENT is a negative number. Now look at the patch which totally rejects the clock if of_clk_get_parent_count() returns any negative number...

Quoted 2 lines

I assume, therefore, that you did not *even* read my commit log...

Ok. So the simplest fix is to just do this? I'll write up some commit text and get this into the rc.