From: Udo A. Steinberg <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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);
Ack.
Thanks,
Venki
>-----Original Message-----
>From: Thomas Gleixner [mailto:[email protected]]
>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 <[email protected]>
>
>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 <[email protected]>
>Signed-off-by: Thomas Gleixner <[email protected]>
>
>---
> 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);
>
On Wed, 06 Jun 2007 11:37:53 +0200 Thomas Gleixner <[email protected]> wrote:
> From: Udo A. Steinberg <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
>
> ---
> 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?
>-----Original Message-----
>From: Andrew Morton [mailto:[email protected]]
>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
><[email protected]> wrote:
>
>> From: Udo A. Steinberg <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>>
>> ---
>> 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
* Andrew Morton ([email protected]) 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
[chrisw: rebase against Len's changes in -mm]
Signed-off-by: Chris Wright <[email protected]>
---
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) {
On Mon, 2007-06-11 at 18:07 -0700, Chris Wright wrote:
> * Andrew Morton ([email protected]) 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
>-----Original Message-----
>From: Thomas Gleixner [mailto:[email protected]]
>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 ([email protected]) 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
* Pallipadi, Venkatesh ([email protected]) wrote:
> > * Thomas Gleixner (mailto:[email protected]) 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