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

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
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(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 335322dc403f..05cca9298f44 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -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;
-- 
2.1.0

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

[Michael Turquette]

Hi Russell,

Quoting Russell King (2016-01-02 02:01:34)
> 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.

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

> 
> Fixes: 80eeb1f0f757 ("clk: add gpio controlled clock multiplexer")
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> 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.

v4.2 was release on Sunday, August 30, 2015. My pull request to Linus
containing this patch was sent on Monday, August 31, 2015. Thread
archived here:

http://lkml.kernel.org/r/<20150831192125.11508.92473@quantum>

Looks like it was merged in 4.3-rc1 as well:

$ git log v4.3-rc1..clk-for-linus-4.3
$

> 
> A rewrite of this patch will be necessary to apply to v4.3 kernels.
> 
> This applies on top of v4.4-rc6.

Alternatively we can send f66541ba02d5 "clk: gpio: Get parent clk names
in of_gpio_clk_setup()" along with your fix.

Below is the modified version of your patch.

Regards,
Mike



commit 60f7e5a537f367d3be7cff2bce13c91c10bdf451
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Sat Jan 2 10:01:34 2016 +0000

    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 <rmk+kernel@arm.linux.org.uk>
    Signed-off-by: Michael Turquette <mturquette@baylibre.com>

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 1767b9e..19fed65 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -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,
@@ -295,15 +295,19 @@ static void __init of_gpio_clk_setup(struct device_node *node,
 	if (!data)
 		return;
 
-	parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
-	if (!parent_names) {
-		kfree(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;
 	}
 
-	for (i = 0; i < num_parents; i++)
-		parent_names[i] = of_clk_get_parent_name(node, i);
-
 	data->num_parents = num_parents;
 	data->parent_names = parent_names;
 	data->node = node;

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

[Russell King - ARM Linux]

On Sat, Jan 02, 2016 at 10:47:49PM -0800, Michael Turquette wrote:
> Hi Russell,
> 
> Quoting Russell King (2016-01-02 02:01:34)
> > 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.
> 
> Thanks for the fix. Applied to clk-next manually.

Thanks.

> > 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.

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

$ git describe --contains 80eeb1f0f757
v4.3-rc2~4^2~124
$ git log v4.3-rc1..v4.3-rc2 | grep 80eeb1f0f757
$ git log v4.2..v4.3-rc1 | grep 80eeb1f0f757
commit 80eeb1f0f757c790b020d9f425bb0e824973d49c
$ git describe --help
...
       --contains
           Instead of finding the tag that predates the commit, find the tag
           that comes after the commit, and thus contains it. Automatically
           implies --tags.

> Below is the modified version of your patch.

Looks fine, thanks.

-- 
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.

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

[Fabio Estevam]

On Sun, Jan 3, 2016 at 8:53 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

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

'git tag --contains' shows the correct response.

$ git tag --contains 80eeb1f0f757
v4.3
v4.3-rc1
v4.3-rc2
v4.3-rc3
v4.3-rc4
v4.3-rc5
v4.3-rc6
v4.3-rc7
v4.4-rc1
v4.4-rc2
v4.4-rc3
v4.4-rc4
v4.4-rc5
v4.4-rc6
v4.4-rc7

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

[Russell King - ARM Linux]

On Sat, Jan 02, 2016 at 10:01:34AM +0000, Russell King wrote:
> 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 <rmk+kernel@arm.linux.org.uk>
> ---
> 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(-)
> 
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 335322dc403f..05cca9298f44 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -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;
> -- 
> 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...

-- 
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.

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

[Michael Turquette]

Quoting Russell King - ARM Linux (2016-02-17 15:05:29)
> On Sat, Jan 02, 2016 at 10:01:34AM +0000, Russell King wrote:
> > 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 <rmk+kernel@arm.linux.org.uk>
> > ---
> > 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(-)
> > 
> > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> > index 335322dc403f..05cca9298f44 100644
> > --- a/drivers/clk/clk-gpio.c
> > +++ b/drivers/clk/clk-gpio.c
> > @@ -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;
> > -- 
> > 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.

Regards,
Mike

> 
> -- 
> 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.

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

[Russell King - ARM Linux]

On Wed, Feb 17, 2016 at 04:07:47PM -0800, Michael Turquette wrote:
> Quoting Russell King - ARM Linux (2016-02-17 15:05:29)
> > On Sat, Jan 02, 2016 at 10:01:34AM +0000, Russell King wrote:
> > > 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 <rmk+kernel@arm.linux.org.uk>
> > > ---
> > > 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(-)
> > > 
> > > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> > > index 335322dc403f..05cca9298f44 100644
> > > --- a/drivers/clk/clk-gpio.c
> > > +++ b/drivers/clk/clk-gpio.c
> > > @@ -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;
> > > -- 
> > > 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.

You have.  Read the commit log on my patch.  Then look at this code:

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

compared to mine:

         num_parents = of_clk_get_parent_count(node);
         if (num_parents > 0) {
                 parent_names = kcalloc(num_parents, sizeof(char *), ...

It is very much _NOT_ equivalent.

The big pointer here is this:
* My patch works.
* The code you have in mainline does not work.

Therefore, they _can't_ be equivalent - because they have _visibly_
different behaviours.  So, they are different.

The former omits the _entire_ registration of the clock if
of_clk_get_parent_count() returns a negative number.  Thus, no
parent clocks, no clock gets registered.

The whole point behind my patch was to restore the regression
that occurred: the regression being that having no parent clocks
used to be explicitly allowed, and the patch I mentioned in my
commit message broke it.

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...

-- 
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.

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

[Stephen Boyd]

On 02/18, Russell King - ARM Linux wrote:
> 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...
> 

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

---8<---
diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 19fed65587e8..7b09a265d79f 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -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)

Maybe we should change of_clk_get_parent_count() to return an
unsigned int as well. I'm not sure why anyone would care whether
or not a node has "clocks" or not as a property. It seems that
all the callers of this function want to know how many parents
there are. In fact, some TI drivers assign the return value from
this function directly to clk_init_data::num_parents which is an
unsigned value. That's horribly broken if there isn't a proper DT
node.

So how about we apply this patch for the next release? We could
also change the return type to unsigned so that people don't get
the wrong idea. That type of change will be a bit larger though
because we'll have to remove impossible < 0 checks in drivers;
not a huge deal but slightly annoying. I can do that legwork
tomorrow.

----8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 67cd2f116c3b..cdf18f76acb0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3020,9 +3020,21 @@ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 	return __of_clk_get_from_provider(clkspec, NULL, __func__);
 }
 
+/**
+ * of_clk_get_parent_count() - Count the number of clocks a device node has
+ * @np: device node to count
+ *
+ * Returns: The number of clocks that are possible parents of this node
+ */
 int of_clk_get_parent_count(struct device_node *np)
 {
-	return of_count_phandle_with_args(np, "clocks", "#clock-cells");
+	int count;
+
+	count = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+	if (count < 0)
+		return 0;
+
+	return count;
 }
 EXPORT_SYMBOL_GPL(of_clk_get_parent_count);
 

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

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Stephen Boyd]

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 <rmk+kernel@arm.linux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 19fed65587e8..7b09a265d79f 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -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

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Michael Turquette]

On Thu, Feb 18, 2016 at 7:12 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> 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 <rmk+kernel@arm.linux.org.uk>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Ack.

Regards,
Mike

> ---
>  drivers/clk/clk-gpio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 19fed65587e8..7b09a265d79f 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -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
>



-- 
Michael Turquette
CEO
BayLibre - At the Heart of Embedded Linux
http://baylibre.com/

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Russell King - ARM Linux]

On Thu, Feb 18, 2016 at 07:12:05PM -0800, Stephen Boyd wrote:
> 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.

NAK.  I'm not testing this patch, when I've already spent more than
enough time (a) developing the original patch and testing that, and
(b) re-diagnosing what's going wrong, and re-fixing the fuck up,
and testing that fix.

-- 
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.

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Russell King - ARM Linux]

On Thu, Feb 18, 2016 at 07:12:05PM -0800, Stephen Boyd wrote:
> 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.

-- 
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.

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Michael Turquette]

Hi Russell,

Quoting Russell King - ARM Linux (2016-02-23 03:30:59)
> On Thu, Feb 18, 2016 at 07:12:05PM -0800, Stephen Boyd wrote:
> > 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.

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.

> 
> 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?

> 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."

In summary: bugs happen. Let's please just move on and get back to work.

[0] http://lkml.kernel.org/r/<20160103105319.GB5779@n2100.arm.linux.org.uk>

Regards,
Mike

> 
> -- 
> 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.

[PATCH] clk: gpio: Really allow an optional clock= DT property

[Russell King - ARM Linux]

On Wed, Feb 24, 2016 at 01:47:06PM -0800, Michael Turquette wrote:
> Hi Russell,
> 
> Quoting Russell King - ARM Linux (2016-02-23 03:30:59)
> > On Thu, Feb 18, 2016 at 07:12:05PM -0800, Stephen Boyd wrote:
> > > 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.
> 
> 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.

Right, so let's go back shall we.

The reason this bug happened is because you changed fully tested code
as a result of a merge conflict.  The result was not tested.

Now, rather than going back to the tested patch, you maintainers decide
to create yet another change, which is also untested and merge it
immediately into mainline.

That's really insane - you had a choice to do the sane thing: you could
have gone back and looked at my patch, realised that my patch resolves
the same problem that the conflicting patch did, and therefore the
correct resolution would be to make the code look the same after both
patches have been applied, as if it would have done if only my patch
had been applied.

And the resulting code would have been tested by implication of the
fact that the original code plus my patch was what I tested here.

But no, you guys think it's better to create some other solution to the
fuck up.

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

I really detest this maintainer shite - it seems anyone can walk up and
take 

> > 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?
> 
> > 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.

Sorry, I disagree.  I'm not guilty of doing that, because when I apply
patches, they come straight from the patch system and are applied to
my tree unmodified.  There's no intermediate modification step possible.
Any changes I want to make are done in a separate commit.

So that's another apology you now owe me.

> 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."

Yes, it _looked_ fine for a quick read, and I didn't test it.  I
trusted you to be competent at merge resolution, but I now see that
trust was misplaced.  I'll try to remember to pay more attention to
any changes you make in the future - or I'll merge them through my
tree.

Trust is something I initially give without earning - break that
trust and you've got to work really hard to regain it.

I'll also remind you that _I_ used to look after the clk code, until
you walked onto the scene and effectively took it away from me - I
didn't say a word about that (or all the other stuff that Linaro
"took" from me without asking.)  So you won't mind if I do a bit of
that myself by merging my own fixes through my tree.

-- 
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.

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

[Russell King - ARM Linux]

On Thu, Feb 18, 2016 at 07:07:31PM -0800, Stephen Boyd wrote:
> On 02/18, Russell King - ARM Linux wrote:
> > 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...
> > 
> 
> Ok. So the simplest fix is to just do this? I'll write up some
> commit text and get this into the rc.

I'm sorry, I've had enough of this crappy maintainer behaviour over
this issue.  There's a simple solution:

(a) making a patch which undoes the broken commit and gives the result
    from my _tested_ patch which is _known_ to work, rather than coming
    up with yet another potentially broken solution.

(b) someone apologising for modifying a patch without mentioning in the
    commit log that it was modified, where the modification makes the
    patch no longer match the commit log, and effectively making the
    patch totally useless.

(a) is easy:

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 19fed65587e8..05cca9298f44 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -287,15 +287,12 @@ static void __init of_gpio_clk_setup(struct device_node *node,
 	const char **parent_names;
 	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) {
+	num_parents = of_clk_get_parent_count(node);
+	if (num_parents > 0) {
 		parent_names = kcalloc(num_parents, sizeof(char *), GFP_KERNEL);
 		if (!parent_names) {
 			kfree(data);

Apply that with an apology and I'll be happy.  Do something else and
I'm not going to be happy, because it means more work for me.  And yes,
yet again, I've tested the above solution, and it works.  Of course it
works, it's my original solution, which was tested at the time.

The moral here is: do _not_ modify other peoples tested patches, or
if you do either _test_ them yourself or ask for them to be tested
before committing.  And if you modify a patch, _say so_ in the
commit text.

And yes, in this case, it's _easy_ for you to test.  You don't need
hardware other than a free gpio pin to come up with a DT fragment
to insert in a test platforms DT.

There is *no* excuse for this mess.

-- 
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.