OMAP3: disable idle early in the suspend sequence

PATCH Superseded Jean Pihet 4 participants 11 messages Nov 22, 2010 – Dec 8, 2010

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP power management support

lore

Share & tools

lore

Jean Pihet

author

2010-11-22 10:45 UTC #0 lore raw

PATCH 1/1 OMAP3: disable idle early in the suspend sequence

Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.

The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).

Tested with RET and OFF on Beagle and OMAP3EVM.

Signed-off-by Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/pm34xx.c +2 -2

index 75c0cd1..022fdff 100644
@@ -508,7 +508,6 @@ static suspend_state_t suspend_state;
 
 static int omap3_pm_prepare(void)
 {
-	disable_hlt();
 	return 0;
 }

@@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
 
 static void omap3_pm_finish(void)
 {
-	enable_hlt();
 }
 
 /* Hooks to enable / disable UART interrupts during suspend */
 static int omap3_pm_begin(suspend_state_t state)
 {
+	disable_hlt();
 	suspend_state = state;
 	omap_uart_enable_irqs(0);
 	return 0;

@@ -591,6 +590,7 @@ static void omap3_pm_end(void)
 {
 	suspend_state = PM_SUSPEND_ON;
 	omap_uart_enable_irqs(1);
+	enable_hlt();
 	return;
 }

Signature · 1 line

1.7.2.3

Rajendra Nayak

2010-11-22 10:53 UTC #1 lore raw

Quoted 2 lines

-----Original Message-----
From: [email protected]

[mailto:[email protected]] On Behalf Of Jean Pihet

Quoted 8 lines

Sent: Monday, November 22, 2010 4:15 PM
To: [email protected]
Cc: Jean Pihet; Kevin Hilman
Subject: [PATCH] OMAP3: disable idle early in the suspend sequence
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.

Can you elaborate more on what kind of issues were seen?

Quoted 55 lines

The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).
Tested with RET and OFF on Beagle and OMAP3EVM.
Signed-off-by Jean Pihet <[email protected]>
Cc Kevin Hilman <[email protected]>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/pm34xx.c +2 -3
index 75c0cd1..022fdff 100644
@@ -508,7 +508,6 @@ static suspend_state_t suspend_state;
 
 static int omap3_pm_prepare(void)
 {
-	disable_hlt();
 	return 0;
 }
 
@@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
 
 static void omap3_pm_finish(void)
 {
-	enable_hlt();
 }
 
 /* Hooks to enable / disable UART interrupts during suspend */
 static int omap3_pm_begin(suspend_state_t state)
 {
+	disable_hlt();
 	suspend_state = state;
 	omap_uart_enable_irqs(0);
 	return 0;
@@ -591,6 +590,7 @@ static void omap3_pm_end(void)
 {
 	suspend_state = PM_SUSPEND_ON;
 	omap_uart_enable_irqs(1);
+	enable_hlt();
 	return;
 }
 
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jean Pihet

author

2010-11-22 11:30 UTC #2 lore raw

Quoted 8 lines · Rajendra Nayak

Quoted 2 lines
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of Jean Pihet
Quoted 5 lines
Sent: Monday, November 22, 2010 4:15 PM
To: [email protected]
Cc: Jean Pihet; Kevin Hilman
Subject: [PATCH] OMAP3: disable idle early in the suspend sequence

Quoted 5 lines

Quoted 3 lines
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
Can you elaborate more on what kind of issues were seen?

Trying to get the PRCM registers dump after a suspend/resume does not show the correct registers values, cf. Kevin's patch at
http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=commitdiff;h=9fc4891d4a21d2b644a463d62c77ef97da55f091.

Digging a bit further I found out that the idle routine is called >50 times while the suspend/resume sequence still is on-going. The root cause is because disable_hlt is called from omap3_pm_prepare which runs after omap3_pm_begin (and the same issue in the resume sequence). This leaves a time window for idle to kick-in while the suspend sequence is busy saving/restoring the system state. This is a potential bug that just waits to show up soon or later, especially if more code is added in the suspend prepare and finish functions.

Does the changelog need an update?

Regards,
Jean

Rajendra Nayak

2010-11-22 12:35 UTC #3 lore raw

Quoted 27 lines

-----Original Message-----
From: Jean Pihet [mailto:[email protected]]
Sent: Monday, November 22, 2010 5:01 PM
To: Rajendra Nayak
Cc: [email protected]; Jean Pihet-XID; Kevin Hilman
Subject: Re: [PATCH] OMAP3: disable idle early in the suspend sequence
Quoted 8 lines · Rajendra Nayak
Quoted 2 lines
-----Original Message-----
From: [email protected]
[mailto:[email protected]] On Behalf Of Jean Pihet
Quoted 5 lines
Sent: Monday, November 22, 2010 4:15 PM
To: [email protected]
Cc: Jean Pihet; Kevin Hilman
Subject: [PATCH] OMAP3: disable idle early in the suspend sequence
Quoted 5 lines
Quoted 3 lines
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
Can you elaborate more on what kind of issues were seen?
Trying to get the PRCM registers dump after a suspend/resume does not show the correct registers values, cf. Kevin's patch at
http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-
pm.git;a=commitdiff;h=9fc4891d4a21d2b644a463d62c77ef97da55f091.

Quoted 8 lines

Digging a bit further I found out that the idle routine is called >50 times while the suspend/resume sequence still is on-going. The root cause is because disable_hlt is called from omap3_pm_prepare which runs after omap3_pm_begin (and the same issue in the resume sequence). This leaves a time window for idle to kick-in while the suspend sequence is busy saving/restoring the system state. This is a potential bug that just waits to show up soon or later, especially if more code is added in the suspend prepare and finish functions.

Ok. Thanks. Basically we had some similar issues on OMAP4. I am yet to root
cause, but the initial hunch was there was a race between idle and suspend and I did pretty much the same change in pm44xx.c.
That fixed the issue but only partially, so am still in the process of debugging
it further to see what exactly is the issue. So was just curious to know what
issues you ran into on OMAP3.
Will update once I know whats happening on OMAP4.

Quoted 2 lines

Does the changelog need an update?

No, The changelog looks fine to me.

Thanks,
Rajendra

Quoted 3 lines

Regards,
Jean

Felipe Balbi

2010-11-22 11:10 UTC #4 lore raw

In reply to Jean Pihet

Hi,

Quoted 9 lines · Jean Pihet

Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).

Quoted 4 lines

Tested with RET and OFF on Beagle and OMAP3EVM.
Signed-off-by Jean Pihet <[email protected]>
Cc Kevin Hilman <[email protected]>

This patch will look weird on git log, it's going to be From: Jean Pihet <[email protected]> and Signed-off-by: Jean Pihet <[email protected]>. Maybe you should change user.email variable on your .gitconfig and use envelope sender as your newoldbits.com address ?

Signature · 1 line

balbi

Jean Pihet

author

2010-11-22 11:38 UTC #5 lore raw

Hi,

Quoted 15 lines · Felipe Balbi

Hi,
Quoted 12 lines · Jean Pihet
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).
Tested with RET and OFF on Beagle and OMAP3EVM.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Jean Pihet <[email protected]>
Cc Kevin Hilman <[email protected]>
This patch will look weird on git log, it's going to be From: Jean Pihet <[email protected]> and Signed-off-by: Jean Pihet <[email protected]>. Maybe you should change user.email variable on your .gitconfig and use envelope sender as your newoldbits.com address ?

That is exactly what I am trying to achieve but it does not work ;-(

git config -l shows:
user.name=Jean Pihet
[email protected]
[email protected]

The original patch has the ti.com address in both the From: and
Signed-off-by: fields, but the From: on the ML still shows otherwise. Weird...

Any idea how to fix that?

Thanks!
Jean

Quoted 4 lines

--
balbi

Felipe Balbi

2010-11-22 11:45 UTC #6 lore raw

Hi,

Quoted 5 lines · Jean Pihet

The original patch has the ti.com address in both the From: and
Signed-off-by: fields, but the From: on the ML still shows otherwise. Weird...
Any idea how to fix that?

how about setting sendemail.envelopesender ??

Signature · 1 line

balbi

Kevin Hilman

2010-11-22 17:51 UTC #7 lore raw

In reply to Jean Pihet

Quoted 10 lines · Jean Pihet

Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).
Tested with RET and OFF on Beagle and OMAP3EVM.

I think the description could have a little more detail. Something like:

Idle path should be disabled during the entire suspend/resume sequence. Currently it is disabled in ->prepare() and re-enabled in ->finish(), but the suspend sequence starts with ->begin() and ends with ->end(), leaving windows where the suspend/resume sequence is still underway and idle code could execute.

To fix, move idle disable and enable into ->begin() and ->end() respectively to ensure idle path is disabled for the entire suspend/resume sequence.

Quoted 20 lines

Signed-off-by Jean Pihet <[email protected]>

Cc Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/pm34xx.c +0 -1

index 75c0cd1..022fdff 100644
@@ -508,7 +508,6 @@ static suspend_state_t suspend_state;
 
 static int omap3_pm_prepare(void)
 {
-	disable_hlt();
 	return 0;
 }

arch/arm/mach-omap2/pm34xx.c static int omap3_pm_enter(suspend_state_t unused) · line 578

Quoted 5 lines

arch/arm/mach-omap2/pm34xx.c +0 -1

@@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
 
 static void omap3_pm_finish(void)
 {
-	enable_hlt();
 }

Might as well remove these empty functions now.

Quoted 8 lines

arch/arm/mach-omap2/pm34xx.c +1 -0

 /* Hooks to enable / disable UART interrupts during suspend */
 static int omap3_pm_begin(suspend_state_t state)
 {
+	disable_hlt();
 	suspend_state = state;
 	omap_uart_enable_irqs(0);
 	return 0;

arch/arm/mach-omap2/pm34xx.c static void omap3_pm_end(void) · line 595

Quoted 6 lines

arch/arm/mach-omap2/pm34xx.c +1 -0

@@ -591,6 +590,7 @@ static void omap3_pm_end(void)
 {
 	suspend_state = PM_SUSPEND_ON;
 	omap_uart_enable_irqs(1);
+	enable_hlt();
 	return;
 }

Kevin

Jean Pihet

author

2010-11-22 19:40 UTC #8 lore raw

Quoted 21 lines · Kevin Hilman

Quoted 10 lines · Jean Pihet
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).
Tested with RET and OFF on Beagle and OMAP3EVM.
I think the description could have a little more detail. Something like:
Idle path should be disabled during the entire suspend/resume sequence. Currently it is disabled in ->prepare() and re-enabled in ->finish(), but the suspend sequence starts with ->begin() and ends with ->end(), leaving windows where the suspend/resume sequence is still underway and idle code could execute.

Quoted 4 lines

To fix, move idle disable and enable into ->begin() and ->end() respectively to ensure idle path is disabled for the entire suspend/resume sequence.

Ok thx for the suggestion.

Quoted 10 lines

arch/arm/mach-omap2/pm34xx.c static int omap3_pm_enter(suspend_state_t unused) · line 578
Quoted 7 lines
arch/arm/mach-omap2/pm34xx.c +0 -1
@@ -576,12 +575,12 @@ static int omap3_pm_enter(suspend_state_t unused)
 
  static void omap3_pm_finish(void)
  {
-     enable_hlt();
  }
Might as well remove these empty functions now.

Quoted 17 lines

Quoted 8 lines

arch/arm/mach-omap2/pm34xx.c +1 -0

  /* Hooks to enable / disable UART interrupts during suspend */
  static int omap3_pm_begin(suspend_state_t state)
  {
+     disable_hlt();
       suspend_state = state;
       omap_uart_enable_irqs(0);
       return 0;

arch/arm/mach-omap2/pm34xx.c static void omap3_pm_end(void) · line 595

Quoted 6 lines

arch/arm/mach-omap2/pm34xx.c +1 -0

@@ -591,6 +590,7 @@ static void omap3_pm_end(void)
  {
       suspend_state = PM_SUSPEND_ON;
       omap_uart_enable_irqs(1);
+     enable_hlt();
       return;
  }

Kevin

Ok resent!

Thx,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kevin Hilman

2010-12-08 01:11 UTC #9 lore raw

In reply to Jean Pihet

Hi Jean,

Quoted 9 lines · Jean Pihet

Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).

Quoted 7 lines

Tested with RET and OFF on Beagle and OMAP3EVM.
Signed-off-by Jean Pihet <[email protected]>
Cc Kevin Hilman <[email protected]>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Can you update this to do similar for OMAP2 and OMAP4?

Thanks,

Kevin

Jean Pihet

author

2010-12-08 09:11 UTC #10 lore raw

Quoted 15 lines · Kevin Hilman

Hi Jean,
Quoted 11 lines · Jean Pihet
Some bad interaction between the idle and the suspend paths has been
noticed: the idle code is called during the suspend enter and exit sequences. This could cause corruption or lock-up of resources.
The solution is to move the call to disable_hlt at the very beginning of the suspend sequence (in omap3_pm_begin instead of omap3_pm_prepare), and the call to enable_hlt at the very end of the suspend sequence (in omap3_pm_end instead of omap3_pm_finish).
Tested with RET and OFF on Beagle and OMAP3EVM.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Jean Pihet <[email protected]>
Cc Kevin Hilman <[email protected]>
---
arch/arm/mach-omap2/pm34xx.c | 4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Can you update this to do similar for OMAP2 and OMAP4?

Done!
It needs some testing on OMAP2 and OMAP4 boards since it has been compile tested only.

Quoted 5 lines

Thanks,
Kevin

Thanks,
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html