Re: Warnings: pm branch

2009-09-30 Thread Kevin Hilman 
On Wed, Aug 12, 2009 at 7:38 AM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 Hemanth V heman...@ti.com writes:

 - Original Message -
 From: Kevin Hilman khil...@deeprootsystems.com
  /* enter the state and update stats */
 @@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
  /* give the governor an opportunity to reflect on the outcome */
  if (cpuidle_curr_governor-reflect)
  cpuidle_curr_governor-reflect(dev);
 +
 + return;
 +

 ... I think you want to drop this return.  If it returns here, it
 will still not enable IRQs.  I think it should just fall through
 to the enable and return.

 Since omap3_enter_idle returns with interrupts enabled, I had
 added this return. We could remove it also for safety purposes.

 OK.  I think you should post to linux-pm for comment, and possibly
 raise this as a question.

 You can add:

 Signed-off-by: Kevin Hilman khil...@deeprootsystems.com

Hemanth,

I've reworked/simplified this patch slightly (see below) and will send
to linux-pm shortly.

Kevin


This one is against PM branch:

commit 808854375b94017ba996a467a082a38730fff434
Author: Kevin Hilman khil...@deeprootsystems.com
Date:   Wed Sep 30 09:57:40 2009 -0700

CPUidle: always return with interrupts enabled

In the case where cpuidle_idle_call() returns before changing state
due to a need_resched(), it was returning with IRQs disabled.

This patche ensures IRQs are (re)enabled before returning.

Reported-by: Hemanth V heman...@ti.com
Signed-off-by: Kevin Hilman khil...@deeprootsystems.com

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8504a21..910c49d 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -75,8 +75,11 @@ static void cpuidle_idle_call(void)
 #endif
/* ask the governor for the next state */
next_state = cpuidle_curr_governor-select(dev);
-   if (need_resched())
+   if (need_resched()) {
+   local_irq_enable();
return;
+   }
+
target_state = dev-states[next_state];

/* enter the state and update stats */
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warnings: pm branch

2009-08-12 Thread Hemanth V 
- Original Message - 
From: Kevin Hilman khil...@deeprootsystems.com

To: Pandita, Vikram vikram.pand...@ti.com
Cc: linux-omap@vger.kernel.org
Sent: Tuesday, August 11, 2009 8:57 PM
Subject: Re: Warnings: pm branch



Pandita, Vikram vikram.pand...@ti.com writes:


Has anyone send these warning logs on the pm branch on kevin's tree [1]
This seems to be some issue in CpuIdle path with interrupts?

4WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
dModules linked in:Modules linked in:
[c0031344] [c0031344] (unwind_backtrace+0x0/0xdc) 
(unwind_backtrace+0x0/0xdc

) from [c0057a08] from [c0057a08] (warn_slowpath_common+0x48/0x60)
(warn_slowpath_common+0x48/0x60)
[c0057a08] [c0057a08] (warn_slowpath_common+0x48/0x60) 
(warn_slowpath_common

+0x48/0x60) from [c002d204] from [c002d204] (cpu_idle+0x60/0x88)
(cpu_idle+0x60/0x88)
[c002d204] [c002d204] (cpu_idle+0x60/0x88) (cpu_idle+0x60/0x88) from 
[c0008

a70] from [c0008a70] (start_kernel+0x234/0x28c)
(start_kernel+0x234/0x28c)
[c0008a70] [c0008a70] (start_kernel+0x234/0x28c) 
(start_kernel+0x234/0x28c)

from [80008034] from [80008034] (0x80008034)
(0x80008034)



Yes, I've seen it, but have yet to debug it.

This warning is from the generic ARM idle loop reporting that the OMAP
idle path is returning with IRQs disabled.

Kevin


I did some more debugging on this. I added the below instrumentation to 
local_irq_disable to capture

the PC of the last call to local_irq_disable.

extern unsigned long hem_pc;
register unsigned long current_pc asm (pc);

#define local_irq_disable() \
   do { hem_pc = current_pc;raw_local_irq_disable(); 
trace_hardirqs_off(); } while (0)



When I set a breakpoint  at the instruction pointing to 
WARN_ON(irqs_disabled()) using
lauterbach and dump hem_pc, I see that the last call to irq_disable is 
actuall from cpu_idle itself.

Looks like some recursion is happening. Does anyone have any inputs on this.

void cpu_idle(void)
{

   local_fiq_enable();

   /* endless idle loop with no priority at all */
   while (1) {
   tick_nohz_stop_sched_tick(1);
   leds_event(led_idle_start);
   while (!need_resched()) {
#ifdef CONFIG_HOTPLUG_CPU
   if (cpu_is_offline(smp_processor_id()))
   cpu_die();
#endif


   local_irq_disable();



Thanks
Hemanth 


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warnings: pm branch

2009-08-12 Thread Hemanth V 
 - Original Message -
 From: Kevin Hilman khil...@deeprootsystems.com
 To: Pandita, Vikram vikram.pand...@ti.com
 Cc: linux-omap@vger.kernel.org
 Sent: Tuesday, August 11, 2009 8:57 PM
 Subject: Re: Warnings: pm branch


 Pandita, Vikram vikram.pand...@ti.com writes:

 Has anyone send these warning logs on the pm branch on kevin's tree [1]
 This seems to be some issue in CpuIdle path with interrupts?

 4WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 dModules linked in:Modules linked in:
 [c0031344] [c0031344] (unwind_backtrace+0x0/0xdc)
 (unwind_backtrace+0x0/0xdc
 ) from [c0057a08] from [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common+0x48/0x60)
 [c0057a08] [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common
 +0x48/0x60) from [c002d204] from [c002d204] (cpu_idle+0x60/0x88)
 (cpu_idle+0x60/0x88)
 [c002d204] [c002d204] (cpu_idle+0x60/0x88) (cpu_idle+0x60/0x88) from
 [c0008
 a70] from [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 [c0008a70] [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 from [80008034] from [80008034] (0x80008034)
 (0x80008034)


 Yes, I've seen it, but have yet to debug it.

 This warning is from the generic ARM idle loop reporting that the OMAP
 idle path is returning with IRQs disabled.

 Kevin

 I did some more debugging on this. I added the below instrumentation to
 local_irq_disable to capture
 the PC of the last call to local_irq_disable.

 extern unsigned long hem_pc;
 register unsigned long current_pc asm (pc);

 #define local_irq_disable() \
 do { hem_pc = current_pc;raw_local_irq_disable();
 trace_hardirqs_off(); } while (0)


 When I set a breakpoint  at the instruction pointing to
 WARN_ON(irqs_disabled()) using
 lauterbach and dump hem_pc, I see that the last call to irq_disable is
 actuall from cpu_idle itself.
 Looks like some recursion is happening. Does anyone have any inputs on this.

 void cpu_idle(void)
 {

 local_fiq_enable();

 /* endless idle loop with no priority at all */
 while (1) {
 tick_nohz_stop_sched_tick(1);
 leds_event(led_idle_start);
 while (!need_resched()) {
 #ifdef CONFIG_HOTPLUG_CPU
 if (cpu_is_offline(smp_processor_id()))
 cpu_die();
 #endif

local_irq_disable();


 Thanks
 Hemanth


Below patch seems to fix the issue.

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8504a21..3014104 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -75,8 +75,10 @@ static void cpuidle_idle_call(void)
 #endif
/* ask the governor for the next state */
next_state = cpuidle_curr_governor-select(dev);
+
if (need_resched())
-   return;
+   goto out;
+
target_state = dev-states[next_state];

/* enter the state and update stats */
@@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
/* give the governor an opportunity to reflect on the outcome */
if (cpuidle_curr_governor-reflect)
cpuidle_curr_governor-reflect(dev);
+
+   return;
+
+out:
+   local_irq_enable();
+   return;
 }

 /**


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warnings: pm branch

2009-08-12 Thread Kevin Hilman 
Hemanth V heman...@ti.com writes:

 - Original Message -
 From: Kevin Hilman khil...@deeprootsystems.com
 To: Pandita, Vikram vikram.pand...@ti.com
 Cc: linux-omap@vger.kernel.org
 Sent: Tuesday, August 11, 2009 8:57 PM
 Subject: Re: Warnings: pm branch


 Pandita, Vikram vikram.pand...@ti.com writes:

 Has anyone send these warning logs on the pm branch on kevin's tree [1]
 This seems to be some issue in CpuIdle path with interrupts?

 4WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 dModules linked in:Modules linked in:
 [c0031344] [c0031344] (unwind_backtrace+0x0/0xdc)
 (unwind_backtrace+0x0/0xdc
 ) from [c0057a08] from [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common+0x48/0x60)
 [c0057a08] [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common
 +0x48/0x60) from [c002d204] from [c002d204] (cpu_idle+0x60/0x88)
 (cpu_idle+0x60/0x88)
 [c002d204] [c002d204] (cpu_idle+0x60/0x88) (cpu_idle+0x60/0x88) from
 [c0008
 a70] from [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 [c0008a70] [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 from [80008034] from [80008034] (0x80008034)
 (0x80008034)


 Yes, I've seen it, but have yet to debug it.

 This warning is from the generic ARM idle loop reporting that the OMAP
 idle path is returning with IRQs disabled.

 Kevin

 I did some more debugging on this. I added the below instrumentation to
 local_irq_disable to capture
 the PC of the last call to local_irq_disable.

 extern unsigned long hem_pc;
 register unsigned long current_pc asm (pc);

 #define local_irq_disable() \
 do { hem_pc = current_pc;raw_local_irq_disable();
 trace_hardirqs_off(); } while (0)


 When I set a breakpoint  at the instruction pointing to
 WARN_ON(irqs_disabled()) using
 lauterbach and dump hem_pc, I see that the last call to irq_disable is
 actuall from cpu_idle itself.
 Looks like some recursion is happening. Does anyone have any inputs on this.

 void cpu_idle(void)
 {

 local_fiq_enable();

 /* endless idle loop with no priority at all */
 while (1) {
 tick_nohz_stop_sched_tick(1);
 leds_event(led_idle_start);
 while (!need_resched()) {
 #ifdef CONFIG_HOTPLUG_CPU
 if (cpu_is_offline(smp_processor_id()))
 cpu_die();
 #endif

local_irq_disable();


 Thanks
 Hemanth


 Below patch seems to fix the issue.

The question remains, who is disabling interrupts?

If this patch works, someone is disabling interrupts between the
enable in this routine and the return of this function.

Kevin

 diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
 index 8504a21..3014104 100644
 --- a/drivers/cpuidle/cpuidle.c
 +++ b/drivers/cpuidle/cpuidle.c
 @@ -75,8 +75,10 @@ static void cpuidle_idle_call(void)
  #endif
   /* ask the governor for the next state */
   next_state = cpuidle_curr_governor-select(dev);
 +
   if (need_resched())
 - return;
 + goto out;
 +
   target_state = dev-states[next_state];

   /* enter the state and update stats */
 @@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
   /* give the governor an opportunity to reflect on the outcome */
   if (cpuidle_curr_governor-reflect)
   cpuidle_curr_governor-reflect(dev);
 +
 + return;
 +
 +out:
 + local_irq_enable();
 + return;
  }

  /**
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Warnings: pm branch

2009-08-12 Thread Sripathy, Vishwanath 


-Original Message-
From: linux-omap-ow...@vger.kernel.org 
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Kevin Hilman
Sent: Wednesday, August 12, 2009 6:59 PM
To: V, Hemanth
Cc: Pandita, Vikram; linux-omap@vger.kernel.org
Subject: Re: Warnings: pm branch

Hemanth V heman...@ti.com writes:

 - Original Message -
 From: Kevin Hilman khil...@deeprootsystems.com
 To: Pandita, Vikram vikram.pand...@ti.com
 Cc: linux-omap@vger.kernel.org
 Sent: Tuesday, August 11, 2009 8:57 PM
 Subject: Re: Warnings: pm branch


 Pandita, Vikram vikram.pand...@ti.com writes:

 Has anyone send these warning logs on the pm branch on kevin's tree [1]
 This seems to be some issue in CpuIdle path with interrupts?

 4WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 WARNING: at arch/arm/kernel/process.c:171 cpu_idle+0x60/0x88()
 dModules linked in:Modules linked in:
 [c0031344] [c0031344] (unwind_backtrace+0x0/0xdc)
 (unwind_backtrace+0x0/0xdc
 ) from [c0057a08] from [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common+0x48/0x60)
 [c0057a08] [c0057a08] (warn_slowpath_common+0x48/0x60)
 (warn_slowpath_common
 +0x48/0x60) from [c002d204] from [c002d204] (cpu_idle+0x60/0x88)
 (cpu_idle+0x60/0x88)
 [c002d204] [c002d204] (cpu_idle+0x60/0x88) (cpu_idle+0x60/0x88) from
 [c0008
 a70] from [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 [c0008a70] [c0008a70] (start_kernel+0x234/0x28c)
 (start_kernel+0x234/0x28c)
 from [80008034] from [80008034] (0x80008034)
 (0x80008034)


 Yes, I've seen it, but have yet to debug it.

 This warning is from the generic ARM idle loop reporting that the OMAP
 idle path is returning with IRQs disabled.

 Kevin

 I did some more debugging on this. I added the below instrumentation to
 local_irq_disable to capture
 the PC of the last call to local_irq_disable.

 extern unsigned long hem_pc;
 register unsigned long current_pc asm (pc);

 #define local_irq_disable() \
 do { hem_pc = current_pc;raw_local_irq_disable();
 trace_hardirqs_off(); } while (0)


 When I set a breakpoint  at the instruction pointing to
 WARN_ON(irqs_disabled()) using
 lauterbach and dump hem_pc, I see that the last call to irq_disable is
 actuall from cpu_idle itself.
 Looks like some recursion is happening. Does anyone have any inputs on this.

 void cpu_idle(void)
 {

 local_fiq_enable();

 /* endless idle loop with no priority at all */
 while (1) {
 tick_nohz_stop_sched_tick(1);
 leds_event(led_idle_start);
 while (!need_resched()) {
 #ifdef CONFIG_HOTPLUG_CPU
 if (cpu_is_offline(smp_processor_id()))
 cpu_die();
 #endif

local_irq_disable();


 Thanks
 Hemanth


 Below patch seems to fix the issue.

 The question remains, who is disabling interrupts?

 If this patch works, someone is disabling interrupts between the
 enable in this routine and the return of this function.

 Kevin
It's disabled in cpu_idle function itself before calling pm_idle
Here is the snap of cpu_idle

void cpu_idle(void)
{
local_fiq_enable();

/* endless idle loop with no priority at all */
while (1) {
tick_nohz_stop_sched_tick(1);
leds_event(led_idle_start);
while (!need_resched()) {
#ifdef CONFIG_HOTPLUG_CPU
if (cpu_is_offline(smp_processor_id()))
cpu_die();
#endif

local_irq_disable();  //it's disabled here
if (hlt_counter) {
local_irq_enable();
cpu_relax();
} else {
stop_critical_timings();
pm_idle();
start_critical_timings();
/*
 * This will eventually be removed - pm_idle
 * functions should always return with IRQs
 * enabled.
 */
WARN_ON(irqs_disabled());
local_irq_enable();
}
}


 diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
 index 8504a21..3014104 100644
 --- a/drivers/cpuidle/cpuidle.c
 +++ b/drivers/cpuidle/cpuidle.c
 @@ -75,8 +75,10 @@ static void cpuidle_idle_call(void)
  #endif
   /* ask the governor for the next state */
   next_state = cpuidle_curr_governor-select(dev);
 +
   if (need_resched())
 - return;
 + goto out;
 +
   target_state = dev-states[next_state];

   /* enter the state and update stats */
 @@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
   /* give the governor an opportunity to reflect on the outcome

Re: Warnings: pm branch

2009-08-12 Thread Kevin Hilman 
Hemanth V heman...@ti.com writes:

[...]

 Below patch seems to fix the issue.

After looking closer, I think this is the right fix as the
cpuidle_idle_call() can return with interrupts disabled, but...

[...]

 diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
 index 8504a21..3014104 100644
 --- a/drivers/cpuidle/cpuidle.c
 +++ b/drivers/cpuidle/cpuidle.c
 @@ -75,8 +75,10 @@ static void cpuidle_idle_call(void)
  #endif
   /* ask the governor for the next state */
   next_state = cpuidle_curr_governor-select(dev);
 +
   if (need_resched())
 - return;
 + goto out;
 +
   target_state = dev-states[next_state];

   /* enter the state and update stats */
 @@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
   /* give the governor an opportunity to reflect on the outcome */
   if (cpuidle_curr_governor-reflect)
   cpuidle_curr_governor-reflect(dev);
 +
 + return;
 +

... I think you want to drop this return.  If it returns here, it
will still not enable IRQs.  I think it should just fall through
to the enable and return.

 +out:
 + local_irq_enable();
 + return;
  }

  /**


Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warnings: pm branch

2009-08-12 Thread Hemanth V 
- Original Message - 
From: Kevin Hilman khil...@deeprootsystems.com

 /* enter the state and update stats */
@@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
 /* give the governor an opportunity to reflect on the outcome */
 if (cpuidle_curr_governor-reflect)
 cpuidle_curr_governor-reflect(dev);
+
+ return;
+


... I think you want to drop this return.  If it returns here, it
will still not enable IRQs.  I think it should just fall through
to the enable and return.


Since omap3_enter_idle returns with interrupts enabled, I had
added this return. We could remove it also for safety purposes.

Thanks
Hemanth
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Warnings: pm branch

2009-08-12 Thread Kevin Hilman 
Hemanth V heman...@ti.com writes:

 - Original Message - 
 From: Kevin Hilman khil...@deeprootsystems.com
  /* enter the state and update stats */
 @@ -91,6 +93,12 @@ static void cpuidle_idle_call(void)
  /* give the governor an opportunity to reflect on the outcome */
  if (cpuidle_curr_governor-reflect)
  cpuidle_curr_governor-reflect(dev);
 +
 + return;
 +

 ... I think you want to drop this return.  If it returns here, it
 will still not enable IRQs.  I think it should just fall through
 to the enable and return.

 Since omap3_enter_idle returns with interrupts enabled, I had
 added this return. We could remove it also for safety purposes.
 
OK.  I think you should post to linux-pm for comment, and possibly
raise this as a question.

You can add:

Signed-off-by: Kevin Hilman khil...@deeprootsystems.com

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html