[PATCH] OMAP3: disable idle early in the suspend sequence

[PATCH] OMAP3: disable idle early in the suspend sequence

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

Signed-off-by: Jean Pihet <j-pihet@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/pm34xx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 75c0cd1..022fdff 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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

RE: [PATCH] OMAP3: disable idle early in the suspend sequence

[Rajendra Nayak]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org
[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Jean Pihet
> Sent: Monday, November 22, 2010 4:15 PM
> To: linux-omap@vger.kernel.org
> 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?

>
> 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 <j-pihet@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 75c0cd1..022fdff 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Felipe Balbi]

Hi,

On Mon, Nov 22, 2010 at 11:44:57AM +0100, Jean Pihet wrote:
>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 <j-pihet@ti.com>
>Cc: Kevin Hilman <khilman@deeprootsystems.com>

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

-- 
balbi

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Jean Pihet]

On Mon, Nov 22, 2010 at 11:53 AM, Rajendra Nayak <rnayak@ti.com> wrote:
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org
> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Jean Pihet
>> Sent: Monday, November 22, 2010 4:15 PM
>> To: linux-omap@vger.kernel.org
>> 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?

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

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Jean Pihet]

Hi,

On Mon, Nov 22, 2010 at 12:11 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Nov 22, 2010 at 11:44:57AM +0100, Jean Pihet wrote:
>>
>> 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 <j-pihet@ti.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>
> This patch will look weird on git log, it's going to be From: Jean Pihet
> <jean.pihet@newoldbits.com> and Signed-off-by: Jean Pihet
> <j-pihet@ti.com>. 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
  user.email=j-pihet@ti.com
  sendemail.smtpuser=jean.pihet@newoldbits.com

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

>
> --
> balbi
>

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Felipe Balbi]

Hi,

On Mon, Nov 22, 2010 at 12:38:21PM +0100, Jean Pihet wrote:
>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 ??

-- 
balbi

RE: [PATCH] OMAP3: disable idle early in the suspend sequence

[Rajendra Nayak]

> -----Original Message-----
> From: Jean Pihet [mailto:jean.pihet@newoldbits.com]
> Sent: Monday, November 22, 2010 5:01 PM
> To: Rajendra Nayak
> Cc: linux-omap@vger.kernel.org; Jean Pihet-XID; Kevin Hilman
> Subject: Re: [PATCH] OMAP3: disable idle early in the suspend sequence
>
> On Mon, Nov 22, 2010 at 11:53 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org
> > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Jean Pihet
> >> Sent: Monday, November 22, 2010 4:15 PM
> >> To: linux-omap@vger.kernel.org
> >> 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?
>
> 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.

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.

>
> Does the changelog need an update?

No, The changelog looks fine to me.

Thanks,
Rajendra

>
> Regards,
> Jean

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Kevin Hilman]

Jean Pihet <jean.pihet@newoldbits.com> writes:

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

>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 75c0cd1..022fdff 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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();
>  }

Might as well remove these empty functions now.

>  /* 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;
>  }

Kevin

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Jean Pihet]

On Mon, Nov 22, 2010 at 6:51 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Jean Pihet <jean.pihet@newoldbits.com> writes:
>
>> 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.
>
Ok thx for the suggestion.

>>
>> @@ -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.
>
>>  /* 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;
>>  }
>
> Kevin
>

Ok resent!

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

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Kevin Hilman]

Hi Jean,

Jean Pihet <jean.pihet@newoldbits.com> writes:

> 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 <j-pihet@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  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

Re: [PATCH] OMAP3: disable idle early in the suspend sequence

[Jean Pihet]

On Wed, Dec 8, 2010 at 2:11 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Hi Jean,
>
> Jean Pihet <jean.pihet@newoldbits.com> writes:
>
>> 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 <j-pihet@ti.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>>  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.

>
> Thanks,
>
> Kevin
>

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