[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[Vladimir Zapolskiy]

The checks for clk argument are doubled in __clk_disable() and
__clk_enable() functions and thus may be skipped in clk_disable() and
clk_enable() bodies.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 arch/arm/plat-mxc/clock.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
index 323ff8c..2daec3c 100644
--- a/arch/arm/plat-mxc/clock.c
+++ b/arch/arm/plat-mxc/clock.c
@@ -82,9 +82,6 @@ int clk_enable(struct clk *clk)
 {
 	int ret = 0;
 
-	if (clk == NULL || IS_ERR(clk))
-		return -EINVAL;
-
 	mutex_lock(&clocks_mutex);
 	ret = __clk_enable(clk);
 	mutex_unlock(&clocks_mutex);
@@ -99,9 +96,6 @@ EXPORT_SYMBOL(clk_enable);
  */
 void clk_disable(struct clk *clk)
 {
-	if (clk == NULL || IS_ERR(clk))
-		return;
-
 	mutex_lock(&clocks_mutex);
 	__clk_disable(clk);
 	mutex_unlock(&clocks_mutex);
-- 
1.6.6.1

[PATCH 2/2] [ARM] [IMX]: Fix clock usecount counter from underflow.

[Vladimir Zapolskiy]

If clk_disable() is called for already disabled clock clk->usecount
value is decremented anyway. This leads to a problem that sequent
clk_enable() call doesn't enable the clock as expected.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 arch/arm/plat-mxc/clock.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
index 2daec3c..fd7596c 100644
--- a/arch/arm/plat-mxc/clock.c
+++ b/arch/arm/plat-mxc/clock.c
@@ -53,10 +53,15 @@ static void __clk_disable(struct clk *clk)
 	if (clk == NULL || IS_ERR(clk))
 		return;
 
+        if (!clk->usecount) {
+                printk(KERN_ERR "Trying to disable a clock with 0 usecount\n");
+                WARN_ON(1);
+                return;
+        }
+
 	__clk_disable(clk->parent);
 	__clk_disable(clk->secondary);
 
-	WARN_ON(!clk->usecount);
 	if (!(--clk->usecount) && clk->disable)
 		clk->disable(clk);
 }
-- 
1.6.6.1

[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[Uwe Kleine-König]

Hello Vladimir,

On Wed, Mar 17, 2010 at 10:11:55AM +0300, Vladimir Zapolskiy wrote:
> The checks for clk argument are doubled in __clk_disable() and
> __clk_enable() functions and thus may be skipped in clk_disable() and
> clk_enable() bodies.
Maybe better get rid of the test in __clk_{en,dis}able, as these are
called more often?  Actually I think even a WARN_ON(clk == NULL ||
IS_ERR(clk)) would be OK.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] [ARM] [IMX]: Fix clock usecount counter from underflow.

[Uwe Kleine-König]

On Wed, Mar 17, 2010 at 10:11:56AM +0300, Vladimir Zapolskiy wrote:
> If clk_disable() is called for already disabled clock clk->usecount
> value is decremented anyway. This leads to a problem that sequent
> clk_enable() call doesn't enable the clock as expected.
> 
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/plat-mxc/clock.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
> index 2daec3c..fd7596c 100644
> --- a/arch/arm/plat-mxc/clock.c
> +++ b/arch/arm/plat-mxc/clock.c
> @@ -53,10 +53,15 @@ static void __clk_disable(struct clk *clk)
>  	if (clk == NULL || IS_ERR(clk))
>  		return;
>  
> +        if (!clk->usecount) {
> +                printk(KERN_ERR "Trying to disable a clock with 0 usecount\n");
> +                WARN_ON(1);
> +                return;
> +        }
> +
>  	__clk_disable(clk->parent);
>  	__clk_disable(clk->secondary);
>  
> -	WARN_ON(!clk->usecount);
>  	if (!(--clk->usecount) && clk->disable)
>  		clk->disable(clk);
>  }
I'm not sure this is worth it.  IMHO an unbalanced clk_disable is a
severe bug that doesn't need to be handled smoothly.

But maybe move the WARN_ON before the __clk_disable(clk->parent)?  This
way the disabled parent clock cannot stop the message to appear.

Other than that, please use WARN instead of printk + WARN_ON.  Then the
message is printed only after the oops begin marker.

And your indention is broken.  Please use tabs.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] [ARM] [IMX]: Fix clock usecount counter from underflow.

[javier Martin]

2010/3/17 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> I'm not sure this is worth it. ?IMHO an unbalanced clk_disable is a
> severe bug that doesn't need to be handled smoothly.
>
> But maybe move the WARN_ON before the __clk_disable(clk->parent)? ?This
> way the disabled parent clock cannot stop the message to appear.
>
> Other than that, please use WARN instead of printk + WARN_ON. ?Then the
> message is printed only after the oops begin marker.

I agree with Uwe, I myself tried to do something similar in the past and people
made me realize that we don't have to fix here what is really a problem in some
driver which calls unbalanced disable/enable.

However, I think using WARN would be a good idea since it allows the
kernel hacker
to track an error which according to my own experience is very
difficult to detect.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[Vladimir Zapolskiy]

Hello Uwe,

__clk_enable() and __clk_disable() are recursive with another arguments,
that means it is hardly possible to remove the checks from them.

2010/3/17 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

> Hello Vladimir,
>
> On Wed, Mar 17, 2010 at 10:11:55AM +0300, Vladimir Zapolskiy wrote:
> > The checks for clk argument are doubled in __clk_disable() and
> > __clk_enable() functions and thus may be skipped in clk_disable() and
> > clk_enable() bodies.
> Maybe better get rid of the test in __clk_{en,dis}able, as these are
> called more often?  Actually I think even a WARN_ON(clk == NULL ||
> IS_ERR(clk)) would be OK.
>
> Best regards
> Uwe
>
> With best wishes,
Vladimir
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100317/08a7f91b/attachment-0001.htm>

[PATCH 2/2] [ARM] [IMX]: Fix clock usecount counter from underflow.

[Vladimir Zapolskiy]

Hello all,

and thank you for helpful comments.

2010/3/17 javier Martin <javier.martin@vista-silicon.com>

> 2010/3/17 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > I'm not sure this is worth it.  IMHO an unbalanced clk_disable is a
> > severe bug that doesn't need to be handled smoothly.
> >
> > But maybe move the WARN_ON before the __clk_disable(clk->parent)?  This
> > way the disabled parent clock cannot stop the message to appear.
> >
> > Other than that, please use WARN instead of printk + WARN_ON.  Then the
> > message is printed only after the oops begin marker.
>
> I agree with Uwe, I myself tried to do something similar in the past and
> people
> made me realize that we don't have to fix here what is really a problem in
> some
> driver which calls unbalanced disable/enable.
>
> However, I think using WARN would be a good idea since it allows the
> kernel hacker
> to track an error which according to my own experience is very
> difficult to detect.
>
>
I agree with all of you that this patch is not a bug fix, but definitely a
misfeature elimination.

WARN must be present anyway to inform about an incident of multiple
clk_disable() calls, no doubt. But I assume that if a board can be easily
left in working condition after all, this could help the kernel hacker too.

Let me send a patch with respect to Uwe's comments firstly.

With best wishes,
Vladimir
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100317/96ceefa3/attachment-0001.htm>

[PATCH 2/2 v2] [ARM] [IMX]: Fix clock use counter from underflow on multiple clk_disable().

[Vladimir Zapolskiy]

If clk_disable() is called for already disabled clock clk->usecount
value is decremented anyway. This leads to a problem that sequent
clk_enable() call doesn't enable the clock as expected.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 arch/arm/plat-mxc/clock.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
index 2daec3c..3189275 100644
--- a/arch/arm/plat-mxc/clock.c
+++ b/arch/arm/plat-mxc/clock.c
@@ -53,10 +53,14 @@ static void __clk_disable(struct clk *clk)
 	if (clk == NULL || IS_ERR(clk))
 		return;
 
+	if (!clk->usecount) {
+		WARN(1, "Trying to disable a clock with 0 usecount\n");
+		return;
+	}
+
 	__clk_disable(clk->parent);
 	__clk_disable(clk->secondary);
 
-	WARN_ON(!clk->usecount);
 	if (!(--clk->usecount) && clk->disable)
 		clk->disable(clk);
 }
-- 
1.6.6.1

[PATCH 2/2 v2] [ARM] [IMX]: Fix clock use counter from underflow on multiple clk_disable().

[Uwe Kleine-König]

Hi Vladimir,

On Wed, Mar 17, 2010 at 02:03:31PM +0300, Vladimir Zapolskiy wrote:
> If clk_disable() is called for already disabled clock clk->usecount
> value is decremented anyway. This leads to a problem that sequent
> clk_enable() call doesn't enable the clock as expected.
> 
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/plat-mxc/clock.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
> index 2daec3c..3189275 100644
> --- a/arch/arm/plat-mxc/clock.c
> +++ b/arch/arm/plat-mxc/clock.c
> @@ -53,10 +53,14 @@ static void __clk_disable(struct clk *clk)
>  	if (clk == NULL || IS_ERR(clk))
>  		return;
>  
> +	if (!clk->usecount) {
> +		WARN(1, "Trying to disable a clock with 0 usecount\n");
> +		return;
> +	}
> +
The advantage of just using

	WARN(!clk->usecount, "Trying to disable a disabled clock\n");

is that the error isn't caught and so is more likely to be fixed :-)
I'n not a native English speaker, but I think "Trying to disable a clock
with 0 usecount" isn't a proper English sentence.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[Uwe Kleine-König]

On Wed, Mar 17, 2010 at 01:34:50PM +0300, Vladimir Zapolskiy wrote:
> Hello Uwe,
> 
> __clk_enable() and __clk_disable() are recursive with another arguments,
> that means it is hardly possible to remove the checks from them.
ah, at least the check for NULL cannot go away.  However IS_ERR(clk)
should hardly matter, shouldn't it?

Maybe the generated code is more optimal when doing:

	static void __clk_disable(struct clk *clk)
	{
		WARN_ON(!clk->usecount);

		if (clk->parent)
			__clk_disable(clk->parent);

		if (clk->secondary)
			__clk_disable(clk->secondary);

		if (!(--clk->usecount) && clk->disable)
			clk->disable(clk);
	}

	void clk_disable(struct clk *clk)
	{
		BUG_ON(clk == NULL || IS_ERR(clk));
		mutex_lock(&clocks_mutex);
		__clk_disable(clk);
		mutex_unlock(&clocks_mutex);
	}
 
So unless the compiler optimizes well, this reduces the calls of
__clk_disable from three to one for a clock without parent and
secondary.

While at it I wonder if it isn't more correct in __clk_disable to
disable the clock first and only then parent and secondary?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/2] [ARM] [IMX]: Removed superfluous checks for argument validity.

[Vladimir Zapolskiy]

Hi Uwe,

2010/3/18 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>
> On Wed, Mar 17, 2010 at 01:34:50PM +0300, Vladimir Zapolskiy wrote:
> > Hello Uwe,
> >
> > __clk_enable() and __clk_disable() are recursive with another arguments,
> > that means it is hardly possible to remove the checks from them.
> ah, at least the check for NULL cannot go away. ?However IS_ERR(clk)
> should hardly matter, shouldn't it?
>
> Maybe the generated code is more optimal when doing:
>
> ? ? ? ?static void __clk_disable(struct clk *clk)
> ? ? ? ?{
> ? ? ? ? ? ? ? ?WARN_ON(!clk->usecount);
>
> ? ? ? ? ? ? ? ?if (clk->parent)
> ? ? ? ? ? ? ? ? ? ? ? ?__clk_disable(clk->parent);
>
> ? ? ? ? ? ? ? ?if (clk->secondary)
> ? ? ? ? ? ? ? ? ? ? ? ?__clk_disable(clk->secondary);
>
> ? ? ? ? ? ? ? ?if (!(--clk->usecount) && clk->disable)
> ? ? ? ? ? ? ? ? ? ? ? ?clk->disable(clk);
> ? ? ? ?}
>
> ? ? ? ?void clk_disable(struct clk *clk)
> ? ? ? ?{
> ? ? ? ? ? ? ? ?BUG_ON(clk == NULL || IS_ERR(clk));
> ? ? ? ? ? ? ? ?mutex_lock(&clocks_mutex);
> ? ? ? ? ? ? ? ?__clk_disable(clk);
> ? ? ? ? ? ? ? ?mutex_unlock(&clocks_mutex);
> ? ? ? ?}
>
> So unless the compiler optimizes well, this reduces the calls of
> __clk_disable from three to one for a clock without parent and
> secondary.
>

I hope I got your view on optimization of calls, because I just wanted
to reduce 6 LOCs with the patch :)
Anyway better to do both optimizations at the same time.

> While at it I wonder if it isn't more correct in __clk_disable to
> disable the clock first and only then parent and secondary?!
>

Looks like a reasonable assumption.

> Best regards
> Uwe
>

Best wishes,
Vladimir

[PATCH 2/2 v2] [ARM] [IMX]: Fix clock use counter from underflow on multiple clk_disable().

[Vladimir Zapolskiy]

Hi Uwe

2010/3/18 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> Hi Vladimir,
>
> On Wed, Mar 17, 2010 at 02:03:31PM +0300, Vladimir Zapolskiy wrote:
>> If clk_disable() is called for already disabled clock clk->usecount
>> value is decremented anyway. This leads to a problem that sequent
>> clk_enable() call doesn't enable the clock as expected.
>>
>> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> ---
>> ?arch/arm/plat-mxc/clock.c | ? ?6 +++++-
>> ?1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
>> index 2daec3c..3189275 100644
>> --- a/arch/arm/plat-mxc/clock.c
>> +++ b/arch/arm/plat-mxc/clock.c
>> @@ -53,10 +53,14 @@ static void __clk_disable(struct clk *clk)
>> ? ? ? if (clk == NULL || IS_ERR(clk))
>> ? ? ? ? ? ? ? return;
>>
>> + ? ? if (!clk->usecount) {
>> + ? ? ? ? ? ? WARN(1, "Trying to disable a clock with 0 usecount\n");
>> + ? ? ? ? ? ? return;
>> + ? ? }
>> +
> The advantage of just using
>
> ? ? ? ?WARN(!clk->usecount, "Trying to disable a disabled clock\n");
>
> is that the error isn't caught and so is more likely to be fixed :-)
> I'n not a native English speaker, but I think "Trying to disable a clock
> with 0 usecount" isn't a proper English sentence.
>

I see, thank you. But why don't you use BUG() here, if this testifies
to a higher level bug and after such a call the whole kernel can be
unmaintainable on run-time?

> Best regards
> Uwe
>

Best wishes,
Vladimir

[PATCH 1/2 v2] imx: optimize __clk_enable() and __clk_disable() functions.

[Vladimir Zapolskiy]

To remove unnecessary calls of functions with invalid argumets make
the checks before a call.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
---
 arch/arm/plat-mxc/clock.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
index 323ff8c..ec4af90 100644
--- a/arch/arm/plat-mxc/clock.c
+++ b/arch/arm/plat-mxc/clock.c
@@ -50,24 +50,25 @@ static DEFINE_MUTEX(clocks_mutex);
 
 static void __clk_disable(struct clk *clk)
 {
-	if (clk == NULL || IS_ERR(clk))
-		return;
-
-	__clk_disable(clk->parent);
-	__clk_disable(clk->secondary);
-
 	WARN_ON(!clk->usecount);
+
 	if (!(--clk->usecount) && clk->disable)
 		clk->disable(clk);
+
+	if (clk->secondary)
+		__clk_disable(clk->secondary);
+
+	if (clk->parent)
+		__clk_disable(clk->parent);
 }
 
 static int __clk_enable(struct clk *clk)
 {
-	if (clk == NULL || IS_ERR(clk))
-		return -EINVAL;
+	if (clk->parent)
+		__clk_enable(clk->parent);
 
-	__clk_enable(clk->parent);
-	__clk_enable(clk->secondary);
+	if (clk->secondary)
+		__clk_enable(clk->secondary);
 
 	if (clk->usecount++ == 0 && clk->enable)
 		clk->enable(clk);
@@ -99,9 +100,7 @@ EXPORT_SYMBOL(clk_enable);
  */
 void clk_disable(struct clk *clk)
 {
-	if (clk == NULL || IS_ERR(clk))
-		return;
-
+	BUG_ON(clk == NULL || IS_ERR(clk));
 	mutex_lock(&clocks_mutex);
 	__clk_disable(clk);
 	mutex_unlock(&clocks_mutex);
-- 
1.6.6.1

[PATCH 1/2 v2] imx: optimize __clk_enable() and __clk_disable() functions.

[Vladimir Zapolskiy]

On Thu, Mar 18, 2010 at 4:27 PM, Vladimir Zapolskiy
<vzapolskiy@gmail.com> wrote:
> To remove unnecessary calls of functions with invalid argumets make
> the checks before a call.
>
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> ---
> ?arch/arm/plat-mxc/clock.c | ? 25 ++++++++++++-------------
> ?1 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/clock.c b/arch/arm/plat-mxc/clock.c
> index 323ff8c..ec4af90 100644
> --- a/arch/arm/plat-mxc/clock.c
> +++ b/arch/arm/plat-mxc/clock.c
> @@ -50,24 +50,25 @@ static DEFINE_MUTEX(clocks_mutex);
>
> ?static void __clk_disable(struct clk *clk)
> ?{
> - ? ? ? if (clk == NULL || IS_ERR(clk))
> - ? ? ? ? ? ? ? return;
> -
> - ? ? ? __clk_disable(clk->parent);
> - ? ? ? __clk_disable(clk->secondary);
> -
> ? ? ? ?WARN_ON(!clk->usecount);
> +
> ? ? ? ?if (!(--clk->usecount) && clk->disable)
> ? ? ? ? ? ? ? ?clk->disable(clk);
> +
> + ? ? ? if (clk->secondary)
> + ? ? ? ? ? ? ? __clk_disable(clk->secondary);
> +
> + ? ? ? if (clk->parent)
> + ? ? ? ? ? ? ? __clk_disable(clk->parent);
> ?}
>
> ?static int __clk_enable(struct clk *clk)
> ?{
> - ? ? ? if (clk == NULL || IS_ERR(clk))
> - ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? if (clk->parent)
> + ? ? ? ? ? ? ? __clk_enable(clk->parent);
>
> - ? ? ? __clk_enable(clk->parent);
> - ? ? ? __clk_enable(clk->secondary);
> + ? ? ? if (clk->secondary)
> + ? ? ? ? ? ? ? __clk_enable(clk->secondary);
>
> ? ? ? ?if (clk->usecount++ == 0 && clk->enable)
> ? ? ? ? ? ? ? ?clk->enable(clk);
> @@ -99,9 +100,7 @@ EXPORT_SYMBOL(clk_enable);
> ?*/
> ?void clk_disable(struct clk *clk)
> ?{
> - ? ? ? if (clk == NULL || IS_ERR(clk))
> - ? ? ? ? ? ? ? return;
> -
> + ? ? ? BUG_ON(clk == NULL || IS_ERR(clk));
> ? ? ? ?mutex_lock(&clocks_mutex);
> ? ? ? ?__clk_disable(clk);
> ? ? ? ?mutex_unlock(&clocks_mutex);
> --
> 1.6.6.1
>
>
One potential issue is still exists, there is no valid checks and
unbinding of return values in __clk_enable() recursive calls.

With best wishes,
Vladimir