[PATCH] x86/hpet: prevent boot hang when hpet=force used on old ICH

[PATCH] x86/hpet: prevent boot hang when hpet=force used on old ICH

[Len Brown]

From: Len Brown <len.brown@intel.com>

Linux tells ICH4 users that they can (manually) invoke
"hpet=force" to enable the undocumented HPET.
The HPET becomes available for both clocksource and clockevents.

But as of ff69f2bba67bd45514923aaedbf40fe351787c59
(acpi: fix of pmtimer overflow that make Cx states time incorrect)
the hpet may be used for idle accounting, and
hpet=force on an ICH4 box hangs boot.

It turns out that reading the HPET timer immediately
after return from C3 hangs.

Well, the reason we enable the HPET on these systems
is primarily for clockevents -- the HPET has a longer
maximum timer duration than the PIT.  HPET as a clocksource
is less interesting.

So for "hpet=force" on old ICH, allow HPET clockevents,
but do not allow HPET as a clocksource.

http://bugzilla.kernel.org/show_bug.cgi?id=13087

Signed-off-by: Len Brown <len.brown@intel.com>
---
Ingo/Peter/Thomas,
This regression was filed against ACPI, but the fix is under x86.
I'd wait for Venki's ack on anything related to HPET,
and I can send via your tree or yours, just let me know.

thanks,
-Len

 arch/x86/include/asm/hpet.h |    1 +
 arch/x86/kernel/hpet.c      |    5 +++++
 arch/x86/kernel/quirks.c    |    4 ++++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index 1c22cb0..39d4e86 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -66,6 +66,7 @@
 extern unsigned long hpet_address;
 extern unsigned long force_hpet_address;
 extern int hpet_force_user;
+extern int hpet_is_forced_old_ich(void);
 extern int is_hpet_enabled(void);
 extern int hpet_enable(void);
 extern void hpet_disable(void);
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 3f0019e..8b061c7 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -752,6 +752,11 @@ static int hpet_clocksource_register(void)
 	u64 start, now;
 	cycle_t t1;
 
+	if (hpet_is_forced_old_ich()) {
+		printk(KERN_WARNING "hpet: hpet=force on old ICH enables clockevents,"
+				" but not clocksource.\n");
+		return -ENODEV;
+	}
 	/* Start the counter */
 	hpet_restart_counter();
 
diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index e95022e..9e4d73d 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -253,6 +253,10 @@ static void old_ich_force_enable_hpet(struct pci_dev *dev)
 	dev_printk(KERN_DEBUG, &dev->dev, "Failed to force enable HPET\n");
 }
 
+int hpet_is_forced_old_ich() {
+	return (force_hpet_resume_type == OLD_ICH_FORCE_HPET_RESUME);
+}
+
 /*
  * Undocumented chipset features. Make sure that the user enforced
  * this.
-- 
1.6.3.rc1.34.g0be9b

Re: [PATCH] x86/hpet: prevent boot hang when hpet=force used on old ICH

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> Linux tells ICH4 users that they can (manually) invoke
> "hpet=force" to enable the undocumented HPET.
> The HPET becomes available for both clocksource and clockevents.
> 
> But as of ff69f2bba67bd45514923aaedbf40fe351787c59
> (acpi: fix of pmtimer overflow that make Cx states time incorrect)
> the hpet may be used for idle accounting, and
> hpet=force on an ICH4 box hangs boot.
> 
> It turns out that reading the HPET timer immediately
> after return from C3 hangs.
> 
> Well, the reason we enable the HPET on these systems
> is primarily for clockevents -- the HPET has a longer
> maximum timer duration than the PIT.  HPET as a clocksource
> is less interesting.
> 
> So for "hpet=force" on old ICH, allow HPET clockevents,
> but do not allow HPET as a clocksource.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=13087
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> Ingo/Peter/Thomas,
> This regression was filed against ACPI, but the fix is under x86.
> I'd wait for Venki's ack on anything related to HPET,
> and I can send via your tree or yours, just let me know.

Looks good, thanks!

There's two minor details which you or Venki might want to fix when 
this is resubmitted with Venki's ack:

> +	if (hpet_is_forced_old_ich()) {
> +		printk(KERN_WARNING "hpet: hpet=force on old ICH enables clockevents,"
> +				" but not clocksource.\n");
> +		return -ENODEV;

that should be in a single line, to make git grep work when someone 
greps for a line out of a syslog:

	git grep "hpet: hpet=force on old ICH enables clockevents, but not clocksource."

( The string can be shortened to fit nicely in a line, and/or 
  defined separately or checkpatch can be ignored. )

> +++ b/arch/x86/kernel/quirks.c
> @@ -253,6 +253,10 @@ static void old_ich_force_enable_hpet(struct pci_dev *dev)
>  	dev_printk(KERN_DEBUG, &dev->dev, "Failed to force enable HPET\n");
>  }
>  
> +int hpet_is_forced_old_ich() {
> +	return (force_hpet_resume_type == OLD_ICH_FORCE_HPET_RESUME);
> +}

the function needs a proper prototype and the curly brace should 
start on the next line i guess.

Thanks,

	Ingo

Re: [PATCH] x86/hpet: prevent boot hang when hpet=force used on old ICH

[Len Brown]

Ingo,
Thanks for the reply.

But please disregard that patch -- we've cooked up a better one
that allows ICH4M to continue to use hpet=force, even for clocksource.
(and it is an ACPI patch rather than an x86 patch:-)

However, I'll also send a 2nd patch to stop soliciting more hpet=force
users, since technically we shouldn't be encouraging the use of
this un-documented and lightly-validated hardware.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] x86/hpet: prevent boot hang when hpet=force used on old ICH

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> Ingo,
> Thanks for the reply.
> 
> But please disregard that patch -- we've cooked up a better one
> that allows ICH4M to continue to use hpet=force, even for clocksource.
> (and it is an ACPI patch rather than an x86 patch:-)

ok.

> However, I'll also send a 2nd patch to stop soliciting more 
> hpet=force users, since technically we shouldn't be encouraging 
> the use of this un-documented and lightly-validated hardware.

agreed - i've applied this patch.

	Ingo

[PATCH] ACPI/hpet: prevent boot hang when hpet=force used on ICH-4M

[Len Brown]

From: Len Brown <len.brown@intel.com>

Linux tells ICH4 users that they can (manually) invoke
"hpet=force" to enable the undocumented ICH-4M HPET.
The HPET becomes available for both clocksource and clockevents.

But as of ff69f2bba67bd45514923aaedbf40fe351787c59
(acpi: fix of pmtimer overflow that make Cx states time incorrect)
the HPET may be used via clocksource for idle accounting, and
hpet=force on an ICH4 box hangs boot.

It turns out that touching the MMIO HPET withing
the ARB_DIS part of C3 will hang the hardware.

The fix is to simply move the timer access outside
the ARB_DIS region.  This is a no-op on modern hardware
because ARB_DIS is no longer used.

http://bugzilla.kernel.org/show_bug.cgi?id=13087

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/acpi/processor_idle.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 6fe1214..ea23c64 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -955,6 +955,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	 */
 	acpi_state_timer_broadcast(pr, cx, 1);
 
+	kt1 = ktime_get_real();
 	/*
 	 * disable bus master
 	 * bm_check implies we need ARB_DIS
@@ -976,10 +977,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		ACPI_FLUSH_CPU_CACHE();
 	}
 
-	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	kt2 = ktime_get_real();
-	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {
@@ -988,6 +986,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		c3_cpu_count--;
 		spin_unlock(&c3_lock);
 	}
+	kt2 = ktime_get_real();
+	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
-- 
1.6.3.rc1.34.g0be9b

Re: [PATCH] ACPI/hpet: prevent boot hang when hpet=force used on ICH-4M

[Pallipadi, Venkatesh]

Acked-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

On Thu, 2009-04-23 at 16:18 -0700, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> Linux tells ICH4 users that they can (manually) invoke
> "hpet=force" to enable the undocumented ICH-4M HPET.
> The HPET becomes available for both clocksource and clockevents.
> 
> But as of ff69f2bba67bd45514923aaedbf40fe351787c59
> (acpi: fix of pmtimer overflow that make Cx states time incorrect)
> the HPET may be used via clocksource for idle accounting, and
> hpet=force on an ICH4 box hangs boot.
> 
> It turns out that touching the MMIO HPET withing
> the ARB_DIS part of C3 will hang the hardware.
> 
> The fix is to simply move the timer access outside
> the ARB_DIS region.  This is a no-op on modern hardware
> because ARB_DIS is no longer used.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=13087
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  drivers/acpi/processor_idle.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 6fe1214..ea23c64 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -955,6 +955,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	 */
>  	acpi_state_timer_broadcast(pr, cx, 1);
>  
> +	kt1 = ktime_get_real();
>  	/*
>  	 * disable bus master
>  	 * bm_check implies we need ARB_DIS
> @@ -976,10 +977,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		ACPI_FLUSH_CPU_CACHE();
>  	}
>  
> -	kt1 = ktime_get_real();
>  	acpi_idle_do_entry(cx);
> -	kt2 = ktime_get_real();
> -	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
>  	/* Re-enable bus master arbitration */
>  	if (pr->flags.bm_check && pr->flags.bm_control) {
> @@ -988,6 +986,8 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		c3_cpu_count--;
>  		spin_unlock(&c3_lock);
>  	}
> +	kt2 = ktime_get_real();
> +	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
>  #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
>  	/* TSC could halt in idle, so notify users */

[PATCH] HPET: stop soliciting hpet=force users on ICH4M

[Len Brown]

From: Len Brown <len.brown@intel.com>

The HPET in the ICH4M is not documented in the data sheet
because it was not officially validated.

While it is fine for hackers to continue to use "hpet=force"
to enable the hardware that they have, it is not prudent to
solicit additional "hpet=force" users on this hardware.

Signed-off-by: Len Brown <len.brown@intel.com>
---
 arch/x86/kernel/quirks.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index e95022e..7563b31 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -261,8 +261,6 @@ static void old_ich_force_enable_hpet_user(struct pci_dev *dev)
 {
 	if (hpet_force_user)
 		old_ich_force_enable_hpet(dev);
-	else
-		hpet_print_force_info();
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_1,
-- 
1.6.3.rc1.34.g0be9b

Re: [PATCH] HPET: stop soliciting hpet=force users on ICH4M

[Pallipadi, Venkatesh]

Acked-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

On Thu, 2009-04-23 at 16:19 -0700, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
> 
> The HPET in the ICH4M is not documented in the data sheet
> because it was not officially validated.
> 
> While it is fine for hackers to continue to use "hpet=force"
> to enable the hardware that they have, it is not prudent to
> solicit additional "hpet=force" users on this hardware.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
>  arch/x86/kernel/quirks.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> index e95022e..7563b31 100644
> --- a/arch/x86/kernel/quirks.c
> +++ b/arch/x86/kernel/quirks.c
> @@ -261,8 +261,6 @@ static void old_ich_force_enable_hpet_user(struct pci_dev *dev)
>  {
>  	if (hpet_force_user)
>  		old_ich_force_enable_hpet(dev);
> -	else
> -		hpet_print_force_info();
>  }
>  
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_1,

Re: [PATCH] HPET: stop soliciting hpet=force users on ICH4M

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> The HPET in the ICH4M is not documented in the data sheet
> because it was not officially validated.
> 
> While it is fine for hackers to continue to use "hpet=force"
> to enable the hardware that they have, it is not prudent to
> solicit additional "hpet=force" users on this hardware.
> 
> Signed-off-by: Len Brown <len.brown@intel.com>

Applied, thanks Len!

	Ingo

[tip:x86/urgent] x86, hpet: Stop soliciting hpet=force users on ICH4M

[tip-bot for Len Brown]

Commit-ID:  d2c8604121648b744ebb127991f1c5876931885e
Gitweb:     http://git.kernel.org/tip/d2c8604121648b744ebb127991f1c5876931885e
Author:     Len Brown <len.brown@intel.com>
AuthorDate: Thu, 23 Apr 2009 19:19:42 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 24 Apr 2009 08:41:39 +0200

x86, hpet: Stop soliciting hpet=force users on ICH4M

The HPET in the ICH4M is not documented in the data sheet
because it was not officially validated.

While it is fine for hackers to continue to use "hpet=force"
to enable the hardware that they have, it is not prudent to
solicit additional "hpet=force" users on this hardware.

[ Impact: remove hpet=force syslog message on old-ICH systems ]

Signed-off-by: Len Brown <len.brown@intel.com>
Acked-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
LKML-Reference: <alpine.LFD.2.00.0904231918510.15843@localhost.localdomain>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/quirks.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index e95022e..7563b31 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -261,8 +261,6 @@ static void old_ich_force_enable_hpet_user(struct pci_dev *dev)
 {
 	if (hpet_force_user)
 		old_ich_force_enable_hpet(dev);
-	else
-		hpet_print_force_info();
 }
 
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_1,

Re: [PATCH] ACPI/hpet: prevent boot hang when hpet=force used on ICH-4M

[Ingo Molnar]

* Len Brown <lenb@kernel.org> wrote:

> From: Len Brown <len.brown@intel.com>
> 
> Linux tells ICH4 users that they can (manually) invoke
> "hpet=force" to enable the undocumented ICH-4M HPET.
> The HPET becomes available for both clocksource and clockevents.
> 
> But as of ff69f2bba67bd45514923aaedbf40fe351787c59
> (acpi: fix of pmtimer overflow that make Cx states time incorrect)
> the HPET may be used via clocksource for idle accounting, and
> hpet=force on an ICH4 box hangs boot.
> 
> It turns out that touching the MMIO HPET withing
> the ARB_DIS part of C3 will hang the hardware.
> 
> The fix is to simply move the timer access outside
> the ARB_DIS region.  This is a no-op on modern hardware
> because ARB_DIS is no longer used.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=13087

Nice fix! This moots the other half-quirk patch you wrote for hpet, 
right?

	Ingo

Re: [PATCH] ACPI/hpet: prevent boot hang when hpet=force used on ICH-4M

[Len Brown]

> > http://bugzilla.kernel.org/show_bug.cgi?id=13087
> 
> Nice fix! This moots the other half-quirk patch you wrote for hpet, 
> right?

Right.

thanks,
Len Brown, Intel Open Source Technology Center