[PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[Thomas Gleixner]

From: Udo A. Steinberg <us15@os.inf.tu-dresden.de>

The chip set doc for IHC4 says:

1.In general, software should not attempt any non-posted accesses during
arbiter disable except to the ICH4's power management registers. This
implies that interrupt handlers for any unmasked hardware interrupts and
SMI/NMI should check ARB_DIS status before reading from ICH devices.

So it's not a good idea to access ICH devices after arbiter shut down. 

Signed-off-by: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/acpi/processor_idle.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-2.6.22-rc4/drivers/acpi/processor_idle.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/acpi/processor_idle.c	2007-06-06 11:47:21.000000000 +0200
+++ linux-2.6.22-rc4/drivers/acpi/processor_idle.c	2007-06-06 11:48:21.000000000 +0200
@@ -488,6 +488,11 @@ static void acpi_processor_idle(void)
 
 	case ACPI_STATE_C3:
 
+		/* Get start time (ticks) */
+		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		/* Handle timer broadcast before bus arbiter shutdown ! */
+		acpi_state_timer_broadcast(pr, cx, 1);
+
 		if (pr->flags.bm_check) {
 			if (atomic_inc_return(&c3_cpu_count) ==
 			    num_online_cpus()) {
@@ -502,10 +507,7 @@ static void acpi_processor_idle(void)
 			ACPI_FLUSH_CPU_CACHE();
 		}
 
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
 		/* Invoke C3 */
-		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
 		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);

RE: [PATCH] ACPI: Move timer broadcast and pmtimer access before C3arbiter shutdown

[Pallipadi, Venkatesh]

Ack.

Thanks,
Venki 

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de] 
>Sent: Wednesday, June 06, 2007 2:38 AM
>To: Andrew Morton
>Cc: Pallipadi, Venkatesh; Stable Team; LKML; Len Brown; Ingo 
>Molnar; Arjan van de Ven; Andi Kleen; Udo A. Steinberg; Dave Jones
>Subject: [PATCH] ACPI: Move timer broadcast and pmtimer access 
>before C3arbiter shutdown
>
>From: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
>
>The chip set doc for IHC4 says:
>
>1.In general, software should not attempt any non-posted 
>accesses during
>arbiter disable except to the ICH4's power management registers. This
>implies that interrupt handlers for any unmasked hardware 
>interrupts and
>SMI/NMI should check ARB_DIS status before reading from ICH devices.
>
>So it's not a good idea to access ICH devices after arbiter shut down. 
>
>Signed-off-by: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
>Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>
>---
> drivers/acpi/processor_idle.c |    9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>Index: linux-2.6.22-rc4/drivers/acpi/processor_idle.c
>===================================================================
>--- linux-2.6.22-rc4.orig/drivers/acpi/processor_idle.c	
>2007-06-06 11:47:21.000000000 +0200
>+++ linux-2.6.22-rc4/drivers/acpi/processor_idle.c	
>2007-06-06 11:48:21.000000000 +0200
>@@ -488,6 +488,11 @@ static void acpi_processor_idle(void)
> 
> 	case ACPI_STATE_C3:
> 
>+		/* Get start time (ticks) */
>+		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>+		/* Handle timer broadcast before bus arbiter 
>shutdown ! */
>+		acpi_state_timer_broadcast(pr, cx, 1);
>+
> 		if (pr->flags.bm_check) {
> 			if (atomic_inc_return(&c3_cpu_count) ==
> 			    num_online_cpus()) {
>@@ -502,10 +507,7 @@ static void acpi_processor_idle(void)
> 			ACPI_FLUSH_CPU_CACHE();
> 		}
> 
>-		/* Get start time (ticks) */
>-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
> 		/* Invoke C3 */
>-		acpi_state_timer_broadcast(pr, cx, 1);
> 		acpi_cstate_enter(cx);
> 		/* Get end time (ticks) */
> 		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>

Re: [PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[Andrew Morton]

On Wed, 06 Jun 2007 11:37:53 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
> 
> The chip set doc for IHC4 says:
> 
> 1.In general, software should not attempt any non-posted accesses during
> arbiter disable except to the ICH4's power management registers. This
> implies that interrupt handlers for any unmasked hardware interrupts and
> SMI/NMI should check ARB_DIS status before reading from ICH devices.
> 
> So it's not a good idea to access ICH devices after arbiter shut down. 
> 
> Signed-off-by: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
>  drivers/acpi/processor_idle.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Index: linux-2.6.22-rc4/drivers/acpi/processor_idle.c
> ===================================================================
> --- linux-2.6.22-rc4.orig/drivers/acpi/processor_idle.c	2007-06-06 11:47:21.000000000 +0200
> +++ linux-2.6.22-rc4/drivers/acpi/processor_idle.c	2007-06-06 11:48:21.000000000 +0200
> @@ -488,6 +488,11 @@ static void acpi_processor_idle(void)
>  
>  	case ACPI_STATE_C3:
>  
> +		/* Get start time (ticks) */
> +		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
> +		/* Handle timer broadcast before bus arbiter shutdown ! */
> +		acpi_state_timer_broadcast(pr, cx, 1);
> +
>  		if (pr->flags.bm_check) {
>  			if (atomic_inc_return(&c3_cpu_count) ==
>  			    num_online_cpus()) {
> @@ -502,10 +507,7 @@ static void acpi_processor_idle(void)
>  			ACPI_FLUSH_CPU_CACHE();
>  		}
>  
> -		/* Get start time (ticks) */
> -		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>  		/* Invoke C3 */
> -		acpi_state_timer_broadcast(pr, cx, 1);
>  		acpi_cstate_enter(cx);
>  		/* Get end time (ticks) */
>  		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);

hm, this needs a bit of help to get it to work against Len's current tree.

However, if by "non-posted accesses" you're referring to that inl(), how
come the second one which was left in place isn't also a problem?

RE: [PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Andrew Morton [mailto:akpm@linux-foundation.org] 
>Sent: Wednesday, June 06, 2007 6:39 PM
>To: Thomas Gleixner
>Cc: Pallipadi, Venkatesh; Stable Team; LKML; Len Brown; Ingo 
>Molnar; Arjan van de Ven; Andi Kleen; Udo A. Steinberg; Dave Jones
>Subject: Re: [PATCH] ACPI: Move timer broadcast and pmtimer 
>access before C3 arbiter shutdown
>
>On Wed, 06 Jun 2007 11:37:53 +0200 Thomas Gleixner 
><tglx@linutronix.de> wrote:
>
>> From: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
>> 
>> The chip set doc for IHC4 says:
>> 
>> 1.In general, software should not attempt any non-posted 
>accesses during
>> arbiter disable except to the ICH4's power management registers. This
>> implies that interrupt handlers for any unmasked hardware 
>interrupts and
>> SMI/NMI should check ARB_DIS status before reading from ICH devices.
>> 
>> So it's not a good idea to access ICH devices after arbiter 
>shut down. 
>> 
>> Signed-off-by: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> 
>> ---
>>  drivers/acpi/processor_idle.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> Index: linux-2.6.22-rc4/drivers/acpi/processor_idle.c
>> ===================================================================
>> --- linux-2.6.22-rc4.orig/drivers/acpi/processor_idle.c	
>2007-06-06 11:47:21.000000000 +0200
>> +++ linux-2.6.22-rc4/drivers/acpi/processor_idle.c	
>2007-06-06 11:48:21.000000000 +0200
>> @@ -488,6 +488,11 @@ static void acpi_processor_idle(void)
>>  
>>  	case ACPI_STATE_C3:
>>  
>> +		/* Get start time (ticks) */
>> +		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>> +		/* Handle timer broadcast before bus arbiter 
>shutdown ! */
>> +		acpi_state_timer_broadcast(pr, cx, 1);
>> +
>>  		if (pr->flags.bm_check) {
>>  			if (atomic_inc_return(&c3_cpu_count) ==
>>  			    num_online_cpus()) {
>> @@ -502,10 +507,7 @@ static void acpi_processor_idle(void)
>>  			ACPI_FLUSH_CPU_CACHE();
>>  		}
>>  
>> -		/* Get start time (ticks) */
>> -		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>>  		/* Invoke C3 */
>> -		acpi_state_timer_broadcast(pr, cx, 1);
>>  		acpi_cstate_enter(cx);
>>  		/* Get end time (ticks) */
>>  		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
>
>hm, this needs a bit of help to get it to work against Len's 
>current tree.
>
>However, if by "non-posted accesses" you're referring to that 
>inl(), how
>come the second one which was left in place isn't also a problem?
>

The doc says "except to the ICH4's power management registers".
It seems acpi_gbl_FADT.xpm_timer_block.address is actually OK in this
case
as it is ACPI PM timer register.
The problem we had is the access to HPET registers
inside acpi_state_timer_broadcast() and that is the one that has to be
done
before ARB_DIS.

Thanks,
Venki

Re: [stable] [PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[Chris Wright]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> hm, this needs a bit of help to get it to work against Len's current tree.

Here's some help, compile tested only.  Udo/Thomas, was this found to
be root cause of a real bug?  I didn't want this to get lost if it's
still meant to be relevant for -stable.

thanks,
-chris
--

Subject: ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

From: Udo A. Steinberg <us15@os.inf.tu-dresden.de>

The chip set doc for IHC4 says:

1.In general, software should not attempt any non-posted accesses during
arbiter disable except to the ICH4's power management registers. This
implies that interrupt handlers for any unmasked hardware interrupts and
SMI/NMI should check ARB_DIS status before reading from ICH devices.

So it's not a good idea to access ICH devices after arbiter shut down. 

Signed-off-by: Udo A. Steinberg <us15@os.inf.tu-dresden.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[chrisw: rebase against Len's changes in -mm]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

---

 drivers/acpi/processor_idle.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 2c6a3cb..15db3e8 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -978,6 +978,11 @@ static int acpi_idle_enter_c3(struct cpuidle_device *dev,
 		return 0;
 	}
 
+	/* Get start time (ticks) */
+	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	acpi_state_timer_broadcast(pr, cx, 1);
+	acpi_idle_do_entry(cx);
+
 	/* disable bus master */
 	if (pr->flags.bm_check) {
 		spin_lock(&c3_lock);
@@ -995,10 +1000,6 @@ static int acpi_idle_enter_c3(struct cpuidle_device *dev,
 		ACPI_FLUSH_CPU_CACHE();
 	}
 
-	/* Get start time (ticks) */
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-	acpi_state_timer_broadcast(pr, cx, 1);
-	acpi_idle_do_entry(cx);
 	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
 
 	if (pr->flags.bm_check) {

Re: [stable] [PATCH] ACPI: Move timer broadcast and pmtimer access before C3 arbiter shutdown

[Thomas Gleixner]

On Mon, 2007-06-11 at 18:07 -0700, Chris Wright wrote:
> * Andrew Morton (akpm@linux-foundation.org) wrote:
> > hm, this needs a bit of help to get it to work against Len's current tree.
> 
> Here's some help, compile tested only.  Udo/Thomas, was this found to
> be root cause of a real bug?  I didn't want this to get lost if it's
> still meant to be relevant for -stable.

Chris,

I fixed this against -mm already. I don't think that it is relevant for
stable. The issue was found with Venkis hpet force enable patches on a
ICH4 system. I doubt that any of those systems has hpet enabled in the
BIOS. It should not affect later ICH chip sets. Venki, is this correct ?

	tglx

RE: [stable] [PATCH] ACPI: Move timer broadcast and pmtimer accessbefore C3 arbiter shutdown

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Thomas Gleixner [mailto:tglx@linutronix.de] 
>Sent: Tuesday, June 12, 2007 1:26 AM
>To: Chris Wright
>Cc: Andrew Morton; LKML; Andi Kleen; Udo A. Steinberg; 
>Pallipadi, Venkatesh; Dave Jones; Ingo Molnar; Arjan van de 
>Ven; Stable Team; Len Brown
>Subject: Re: [stable] [PATCH] ACPI: Move timer broadcast and 
>pmtimer accessbefore C3 arbiter shutdown
>
>On Mon, 2007-06-11 at 18:07 -0700, Chris Wright wrote:
>> * Andrew Morton (akpm@linux-foundation.org) wrote:
>> > hm, this needs a bit of help to get it to work against 
>Len's current tree.
>> 
>> Here's some help, compile tested only.  Udo/Thomas, was this found to
>> be root cause of a real bug?  I didn't want this to get lost if it's
>> still meant to be relevant for -stable.
>
>Chris,
>
>I fixed this against -mm already. I don't think that it is relevant for
>stable. The issue was found with Venkis hpet force enable patches on a
>ICH4 system. I doubt that any of those systems has hpet enabled in the
>BIOS. It should not affect later ICH chip sets. Venki, is this 
>correct ?
>

Yes. This issue is only when HPET timer is used on ICH4. And by default 
HPET is not enabled on those chipsets. So, we don't need this patch
for stable.

Thanks,
Venki

Re: [stable] [PATCH] ACPI: Move timer broadcast and pmtimer accessbefore C3 arbiter shutdown

[Chris Wright]

* Pallipadi, Venkatesh (venkatesh.pallipadi@intel.com) wrote:
> > * Thomas Gleixner (mailto:tglx@linutronix.de) wrote: 
> > I fixed this against -mm already. I don't think that it is relevant for
> > stable. The issue was found with Venkis hpet force enable patches on a
> > ICH4 system. I doubt that any of those systems has hpet enabled in the
> > BIOS. It should not affect later ICH chip sets. Venki, is this 
> > correct ?
> 
> Yes. This issue is only when HPET timer is used on ICH4. And by default 
> HPET is not enabled on those chipsets. So, we don't need this patch
> for stable.

Great, thanks.
-chris