Re: [bisected] Clocksource tsc unstable git

[Markus Trippelsdorf <markus@trippelsdorf.de>]

On Fri, Nov 05, 2010 at 05:09:19PM +0100, Borislav Petkov wrote:
> On Tue, Nov 02, 2010 at 11:26:54AM -0400, Thomas Gleixner wrote:
> > Ok, I'm waiting for Boris to get us the confirmation from HW folks.
> 
> Ok, I don't have much but it could be something. We could use the
> minimum tick u16 value in the ACPI HPET table, offset 53:
> 
> Main Counter Minimum   2 53 Unit: Clock tick
> Clock_tick in Periodic      The minimum clock ticks can be set without lost
> Mode                        interrupts while the counter is programmed to operate in
> *Note#3                     periodic mode
> 
> ...
> 
> * Note #3: This field is written by BIOS and may be chipset and/or
> platform dependent. This indicates the minimum value that must be used
> for any counter programmed in periodic mode to avoid lost interrupts.
> For any counter x that has been configured for periodic mode, the number
> can be programmed in any Tx_Compare Register must be greater than P,
> where P = (Minimum Period) / (Main counter period) in order to avoid
> lost interrupts.
> 
> However, we don't know (yet) whether this can be used in the
> non-periodic mode too. My gut feeling says yes but I wouldn't trust it.
> 
> We went and collected that value a bunch of systems and it looks like it
> would need more massaging (read: capping) since some BIOSen simply write
> crap in it. It ranges from
> 
> - 0x37ee on old nVidia and Intel boards (this is definitely crap, I
> can't imagine a minimum ticks value of 14318 for a HPET but who knows)
> 
> - 0x1000 on a HP machine (also fishy)
> 
> - 0x10, 0x14 on current SBxxx boards
> 
> - 0x80 on newer Intel boards

"min tick: 20" on my machine. Given that 12 is running stable here, maybe
the value is a bit too large?

> and it looks like 0x80 is also the default value which we can use as a
> capping value. Btw, I've been running the following on the machine in
> question for a while now and no lockups so far.
> 
> Opinions?
> 
> ---
>  arch/x86/include/asm/hpet.h |    1 +
>  arch/x86/kernel/acpi/boot.c |   12 +++++++++---
>  arch/x86/kernel/hpet.c      |    3 ++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
> index 2c392d6..01d9480 100644
> --- a/arch/x86/include/asm/hpet.h
> +++ b/arch/x86/include/asm/hpet.h
> @@ -68,6 +68,7 @@ extern unsigned long force_hpet_address;
>  extern u8 hpet_blockid;
>  extern int hpet_force_user;
>  extern u8 hpet_msi_disable;
> +extern u16 hpet_min_tick;
>  extern int is_hpet_enabled(void);
>  extern int hpet_enable(void);
>  extern void hpet_disable(void);
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 71232b9..37b93e8 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -772,9 +772,6 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
>  		hpet_address >>= 32;
>  	}
>  #endif
> -	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
> -	       hpet_tbl->id, hpet_address);
> -
>  	/*
>  	 * Allocate and initialize the HPET firmware resource for adding into
>  	 * the resource tree during the lateinit timeframe.
> @@ -790,6 +787,15 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
>  	hpet_res->start = hpet_address;
>  	hpet_res->end = hpet_address + (1 * 1024) - 1;
>  
> +	hpet_min_tick = hpet_tbl->minimum_tick;
> +	if (hpet_min_tick > 0x80) {
> +		printk(KERN_WARNING PREFIX "Limiting HPET read delay to 0x80.\n");
> +		hpet_min_tick = 0x80;
> +	}
> +
> +	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx min tick: %d\n",
> +		hpet_tbl->id, hpet_address, hpet_min_tick);
> +
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index e49c3b9..a0b790a 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -40,6 +40,7 @@ u8					hpet_msi_disable;
>  static unsigned long			hpet_num_timers;
>  #endif
>  static void __iomem			*hpet_virt_address;
> +u16					hpet_min_tick = 0x80;

Wouldn't:

+u16                                  hpet_min_tick;

make more sense, if you want to use the value detected in boot.c?

>  struct hpet_dev {
>  	struct clock_event_device	evt;
> @@ -408,7 +409,7 @@ static int hpet_next_event(unsigned long delta,
>  	 */
>  	res = (s32)(cnt - hpet_readl(HPET_COUNTER));
>  
> -	return res < 8 ? -ETIME : 0;
> +	return res < hpet_min_tick ? -ETIME : 0;
>  }
>  
>  static void hpet_legacy_set_mode(enum clock_event_mode mode,

-- 
Markus