[PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

[PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

[Fan Wu]

From: fwu <fwu@marvell.com>

1. On ARM platform, "nohlt" can be used to prevent core from idle
   process, returning immediately.
2. There are two interface, exported for other modules, named
   disable_hlt and enable_hlt and used to enable/disable the
   cpuidle mechanism by increasing/decreasing "hlt_counter".
   So, the more "hlt_counter" is, the more user want to disable
   cpuidle. Disable_hlt and enable_hlt are paired operation,
   when you first call disable_hlt and then enable_hlt, the
   semantics are right, but if you call enable_hlt and
   then disable_hlt, it is wrong.
3. Change "hlt_counter > 0" can fix the problem.
   The judgement whethere the cpuidle is disabled need to check
   whether the "hlt_counter > 0" rather than "hlt_counter != 0".

Change-Id: Ibd5ea805b0c01fe326833d333ce5d72e0447430a
Signed-off-by: fwu <fwu@marvell.com>
Signed-off-by: YiLu Mao <ylmao@marvell.com>
Signed-off-by: Ning Jiang <ning.jiang@marvell.com>
---
 arch/arm/kernel/process.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 693b744..4dc2fee 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -204,7 +204,7 @@ void cpu_idle(void)
 #ifdef CONFIG_PL310_ERRATA_769419
 			wmb();
 #endif
-			if (hlt_counter) {
+			if (hlt_counter > 0) {
 				local_irq_enable();
 				cpu_relax();
 			} else if (!need_resched()) {
-- 
1.7.0.4

[PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

[Russell King - ARM Linux]

On Thu, Sep 27, 2012 at 02:36:01PM +0800, Fan Wu wrote:
> From: fwu <fwu@marvell.com>
> 
> 1. On ARM platform, "nohlt" can be used to prevent core from idle
>    process, returning immediately.
> 2. There are two interface, exported for other modules, named
>    disable_hlt and enable_hlt and used to enable/disable the
>    cpuidle mechanism by increasing/decreasing "hlt_counter".
>    So, the more "hlt_counter" is, the more user want to disable
>    cpuidle. Disable_hlt and enable_hlt are paired operation,
>    when you first call disable_hlt and then enable_hlt, the
>    semantics are right, but if you call enable_hlt and
>    then disable_hlt, it is wrong.
> 3. Change "hlt_counter > 0" can fix the problem.
>    The judgement whethere the cpuidle is disabled need to check
>    whether the "hlt_counter > 0" rather than "hlt_counter != 0".

NAK.  The bug is that you're calling enable_hlt() without first calling
disable_hlt().  That is something you _must_ _not_ do.

If the count starts off zero, and a driver calls disable_hlt(), another
driver _must_ _not_ override that by then calling enable_hlt().

[PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

[Fan Wu]

Thanks a lot for reviewing. 

So, you mean it is driver's responsibility to make sure the "disable and enable" function are paired before using it, which I think is NOT OK for current code. 

1. If we want different users have chance to sync about the counter, 
	I think we may try the following ways
		1). add one interface (like exported function) for other modules or driver to get the current counter value .
		2). add constraint in "enable and disable" function to avoid the possible situation that the counter is less/more than "0".
2. If we want the "nohlt" is OK for every driver and module without sync or similar operation,
	We may just remove "enable and disable" interface directly, which will cause the "nohlt" change will only be the init interface and cannot be changed any more after kernel bootup.

I have patches to try above possible ways. 
However, I still think my current way(My filed patch) is easier and better way? 

What's your suggestion ? 

Thanks a lot !


Best Regards.
Fan Wu(??)
Ext: 9853


-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
Sent: 2012?9?27? 16:53
To: Fan Wu
Cc: Nicolas Pitre; Will Deacon; H Hartley Sweeten; Tony Lindgren; linux-arm-kernel at lists.infradead.org; Lu Mao; Ning Jiang
Subject: Re: [PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

On Thu, Sep 27, 2012 at 02:36:01PM +0800, Fan Wu wrote:
> From: fwu <fwu@marvell.com>
> 
> 1. On ARM platform, "nohlt" can be used to prevent core from idle
>    process, returning immediately.
> 2. There are two interface, exported for other modules, named
>    disable_hlt and enable_hlt and used to enable/disable the
>    cpuidle mechanism by increasing/decreasing "hlt_counter".
>    So, the more "hlt_counter" is, the more user want to disable
>    cpuidle. Disable_hlt and enable_hlt are paired operation,
>    when you first call disable_hlt and then enable_hlt, the
>    semantics are right, but if you call enable_hlt and
>    then disable_hlt, it is wrong.
> 3. Change "hlt_counter > 0" can fix the problem.
>    The judgement whethere the cpuidle is disabled need to check
>    whether the "hlt_counter > 0" rather than "hlt_counter != 0".

NAK.  The bug is that you're calling enable_hlt() without first calling
disable_hlt().  That is something you _must_ _not_ do.

If the count starts off zero, and a driver calls disable_hlt(), another
driver _must_ _not_ override that by then calling enable_hlt().

[PATCH] ARM:CPUIDLE: Fix wrongly used idle control counter

[Russell King - ARM Linux]

On Thu, Sep 27, 2012 at 02:19:49AM -0700, Fan Wu wrote:
> Thanks a lot for reviewing. 
> 
> So, you mean it is driver's responsibility to make sure the "disable
> and enable" function are paired before using it, which I think is
> NOT OK for current code. 

It is, and always has been.

> 1. If we want different users have chance to sync about the counter, 
> 	I think we may try the following ways
> 		1). add one interface (like exported function) for other
>		 modules or driver to get the current counter value .

That is broken.

> 		2). add constraint in "enable and disable" function to
>		 avoid the possible situation that the counter is
>		 less/more than "0".

Err what?

> 2. If we want the "nohlt" is OK for every driver and module without
> sync or similar operation,
> 	We may just remove "enable and disable" interface directly,
> which will cause the "nohlt" change will only be the init interface
> and cannot be changed any more after kernel bootup.

WTF.

No.  Look, it is _VERY_ simple.

Drivers must _not_ call enable_hlt() without first having called
disable_hlt() - and the number of enable_hlt()s must _NEVER_ be more
than the number of times you've called disable_hlt().

That's exactly the same with other subsystems - eg, you must not call
enable_irq() without having first called disable_irq(), and you must
not call enable_irq() more times than you've called disable_irq().

It is senseless to export the nohlt count - doing so will _create_ bugs
because if a driver were to ever use this value (which is a cumulative
value) then it can call enable_hlt() more times than it's called
disable_hlt() because of the interaction with another driver.

When drivers comply with the interface, the "nohlt" command line argument
works - it calls disable_hlt() once without a following enable_hlt(),
which means that hlt (or rather, putting the CPU into a low power mode)
is disabled at boot time and never enabled.

So, if you have a driver which is calling enable_hlt() without first
having called disable_hlt(), fix that driver.  There is nothing wrong
in the generic ARM code, and the generic ARM code can't help you work
around the broken driver.