[PATCH] x86: fix hpet timer reinit for x86_64 (v2)

[PATCH] x86: fix hpet timer reinit for x86_64 (v2)

[Pavel Emelyanov]

Sorry for late response - took some time to re-check this...

There's a small problem with hpet_rtc_reinit function - it checks
for the
	hpet_readl(HPET_COUNTER) - hpet_t1_cmp > 0
to continue increasing both the HPET_T1_CMP (register) and the 
hpet_t1_cmp (variable).

But since the HPET_COUNTER is always 32-bit, if the hpet_t1_cmp
is 64-bit this condition will always be FALSE once the latter hits
the 32-bit boundary, and we can have a situation, when we don't
increase the HPET_T1_CMP register high enough.

The result - timer stops ticking, since HPET_T1_CMP becomes less, 
than the COUNTER and never increased again. Symptoms were observed
in user space. An application doing read() on /dev/rtc blocked 
sometimes for a long periods of time (as turned out - until the
counter gets wrap around).

The fix is to properly check for one counter being ahead of another
the way it's done for jiffies.

Plush, change the hpet_t1_cmp to u32 to save 4 bytes from .bss ;)

Reported-by: Kirill Korotaev <dev@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 64d5ad0..388254f 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
 static int hpet_prev_update_sec;
 static struct rtc_time hpet_alarm_time;
 static unsigned long hpet_pie_count;
-static unsigned long hpet_t1_cmp;
+static u32 hpet_t1_cmp;
 static unsigned long hpet_default_delta;
 static unsigned long hpet_pie_delta;
 static unsigned long hpet_pie_limit;
@@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit;
 static rtc_irq_handler irq_handler;
 
 /*
+ * Check that the hpet counter c1 is ahead of the c2
+ */
+static inline int hpet_cnt_ahead(u32 c1, u32 c2)
+{
+	return (s32)(c2 - c1) < 0;
+}
+
+/*
  * Registers a IRQ handler.
  */
 int hpet_register_irq_handler(rtc_irq_handler handler)
@@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void)
 		hpet_t1_cmp += delta;
 		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
 		lost_ints++;
-	} while ((long)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
+	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));
 
 	if (lost_ints) {
 		if (hpet_rtc_flags & RTC_PIE)

[PATCH] x86: clean up hpet timer reinit

[Ingo Molnar]

* Pavel Emelyanov <xemul@openvz.org> wrote:

> Sorry for late response - took some time to re-check this...

Applied to tip:timers/urgent, thanks Pavel!

Note, since i've already queued up the minimal fix i've created a delta 
cleanup patch from your v2 patch - see it below. (It is the exact same end 
result in terms of code, just a nicer splitup.)

	Ingo

------------------->
>From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001
From: Pavel Emelyanov <xemul@openvz.org>
Date: Wed, 4 Feb 2009 13:40:31 +0300
Subject: [PATCH] x86: clean up hpet timer reinit

Implement Linus's suggestion: introduce the hpet_cnt_ahead()
helper function to compare hpet time values - like other
wrapping counter comparisons are abstracted away elsewhere.
(jiffies, ktime_t, etc.)

Reported-by: Kirill Korotaev <dev@openvz.org>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/hpet.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index c761f91..388254f 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
 static int hpet_prev_update_sec;
 static struct rtc_time hpet_alarm_time;
 static unsigned long hpet_pie_count;
-static unsigned long hpet_t1_cmp;
+static u32 hpet_t1_cmp;
 static unsigned long hpet_default_delta;
 static unsigned long hpet_pie_delta;
 static unsigned long hpet_pie_limit;
@@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit;
 static rtc_irq_handler irq_handler;
 
 /*
+ * Check that the hpet counter c1 is ahead of the c2
+ */
+static inline int hpet_cnt_ahead(u32 c1, u32 c2)
+{
+	return (s32)(c2 - c1) < 0;
+}
+
+/*
  * Registers a IRQ handler.
  */
 int hpet_register_irq_handler(rtc_irq_handler handler)
@@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void)
 		hpet_t1_cmp += delta;
 		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
 		lost_ints++;
-	} while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
+	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));
 
 	if (lost_ints) {
 		if (hpet_rtc_flags & RTC_PIE)

Re: [PATCH] x86: clean up hpet timer reinit

[Daniel Forrest]

On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote:
> 
> * Pavel Emelyanov <xemul@openvz.org> wrote:
> 
> > Sorry for late response - took some time to re-check this...
> 
> Applied to tip:timers/urgent, thanks Pavel!
> 
> Note, since i've already queued up the minimal fix i've created a delta 
> cleanup patch from your v2 patch - see it below. (It is the exact same end 
> result in terms of code, just a nicer splitup.)
> 
> 	Ingo
> 
> ------------------->
> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001
> From: Pavel Emelyanov <xemul@openvz.org>
> Date: Wed, 4 Feb 2009 13:40:31 +0300
> Subject: [PATCH] x86: clean up hpet timer reinit
> 
> Implement Linus's suggestion: introduce the hpet_cnt_ahead()
> helper function to compare hpet time values - like other
> wrapping counter comparisons are abstracted away elsewhere.
> (jiffies, ktime_t, etc.)
> 
> Reported-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/hpet.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> index c761f91..388254f 100644
> --- a/arch/x86/kernel/hpet.c
> +++ b/arch/x86/kernel/hpet.c
> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
>  static int hpet_prev_update_sec;
>  static struct rtc_time hpet_alarm_time;
>  static unsigned long hpet_pie_count;
> -static unsigned long hpet_t1_cmp;
> +static u32 hpet_t1_cmp;
>  static unsigned long hpet_default_delta;
>  static unsigned long hpet_pie_delta;
>  static unsigned long hpet_pie_limit;
> @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit;
>  static rtc_irq_handler irq_handler;
>  
>  /*
> + * Check that the hpet counter c1 is ahead of the c2
> + */
> +static inline int hpet_cnt_ahead(u32 c1, u32 c2)
> +{
> +	return (s32)(c2 - c1) < 0;
> +}
> +
> +/*
>   * Registers a IRQ handler.
>   */
>  int hpet_register_irq_handler(rtc_irq_handler handler)
> @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void)
>  		hpet_t1_cmp += delta;
>  		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
>  		lost_ints++;
> -	} while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
> +	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));

These are not equivalent for the case where the values are equal.

Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)"

Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0"

Shouldn't it be:

+	} while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp));

Or am I missing something?

>  
>  	if (lost_ints) {
>  		if (hpet_rtc_flags & RTC_PIE)
> --

-- 
Daniel K. Forrest		Space Science and
dan.forrest@ssec.wisc.edu	Engineering Center
(608) 890 - 0558		University of Wisconsin, Madison

Re: [PATCH] x86: clean up hpet timer reinit

[Pavel Emelyanov]

Daniel Forrest wrote:
> On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote:
>> * Pavel Emelyanov <xemul@openvz.org> wrote:
>>
>>> Sorry for late response - took some time to re-check this...
>> Applied to tip:timers/urgent, thanks Pavel!
>>
>> Note, since i've already queued up the minimal fix i've created a delta 
>> cleanup patch from your v2 patch - see it below. (It is the exact same end 
>> result in terms of code, just a nicer splitup.)
>>
>> 	Ingo
>>
>> ------------------->
>> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001
>> From: Pavel Emelyanov <xemul@openvz.org>
>> Date: Wed, 4 Feb 2009 13:40:31 +0300
>> Subject: [PATCH] x86: clean up hpet timer reinit
>>
>> Implement Linus's suggestion: introduce the hpet_cnt_ahead()
>> helper function to compare hpet time values - like other
>> wrapping counter comparisons are abstracted away elsewhere.
>> (jiffies, ktime_t, etc.)
>>
>> Reported-by: Kirill Korotaev <dev@openvz.org>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>> ---
>>  arch/x86/kernel/hpet.c |   12 ++++++++++--
>>  1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
>> index c761f91..388254f 100644
>> --- a/arch/x86/kernel/hpet.c
>> +++ b/arch/x86/kernel/hpet.c
>> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
>>  static int hpet_prev_update_sec;
>>  static struct rtc_time hpet_alarm_time;
>>  static unsigned long hpet_pie_count;
>> -static unsigned long hpet_t1_cmp;
>> +static u32 hpet_t1_cmp;
>>  static unsigned long hpet_default_delta;
>>  static unsigned long hpet_pie_delta;
>>  static unsigned long hpet_pie_limit;
>> @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit;
>>  static rtc_irq_handler irq_handler;
>>  
>>  /*
>> + * Check that the hpet counter c1 is ahead of the c2
>> + */
>> +static inline int hpet_cnt_ahead(u32 c1, u32 c2)
>> +{
>> +	return (s32)(c2 - c1) < 0;
>> +}
>> +
>> +/*
>>   * Registers a IRQ handler.
>>   */
>>  int hpet_register_irq_handler(rtc_irq_handler handler)
>> @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void)
>>  		hpet_t1_cmp += delta;
>>  		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
>>  		lost_ints++;
>> -	} while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
>> +	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));
> 
> These are not equivalent for the case where the values are equal.
> 
> Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)"
> 
> Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0"
> 
> Shouldn't it be:
> 
> +	} while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp));
> 
> Or am I missing something?

When comparator it equal to counter (the corner case we're talking about) the
hpet_cnt_ahead will return false and the loop will go on shifting the cmp,
which is what we need here.

>>  
>>  	if (lost_ints) {
>>  		if (hpet_rtc_flags & RTC_PIE)
>> --
> 

Re: [PATCH] x86: clean up hpet timer reinit

[Daniel Forrest]

On Fri, Feb 06, 2009 at 08:00:00PM +0300, Pavel Emelyanov wrote:
> Daniel Forrest wrote:
> > On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote:
> >> * Pavel Emelyanov <xemul@openvz.org> wrote:
> >>
> >>> Sorry for late response - took some time to re-check this...
> >> Applied to tip:timers/urgent, thanks Pavel!
> >>
> >> Note, since i've already queued up the minimal fix i've created a delta 
> >> cleanup patch from your v2 patch - see it below. (It is the exact same end 
> >> result in terms of code, just a nicer splitup.)
> >>
> >> 	Ingo
> >>
> >> ------------------->
> >> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001
> >> From: Pavel Emelyanov <xemul@openvz.org>
> >> Date: Wed, 4 Feb 2009 13:40:31 +0300
> >> Subject: [PATCH] x86: clean up hpet timer reinit
> >>
> >> Implement Linus's suggestion: introduce the hpet_cnt_ahead()
> >> helper function to compare hpet time values - like other
> >> wrapping counter comparisons are abstracted away elsewhere.
> >> (jiffies, ktime_t, etc.)
> >>
> >> Reported-by: Kirill Korotaev <dev@openvz.org>
> >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> >> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> >> ---
> >>  arch/x86/kernel/hpet.c |   12 ++++++++++--
> >>  1 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> >> index c761f91..388254f 100644
> >> --- a/arch/x86/kernel/hpet.c
> >> +++ b/arch/x86/kernel/hpet.c
> >> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags;
> >>  static int hpet_prev_update_sec;
> >>  static struct rtc_time hpet_alarm_time;
> >>  static unsigned long hpet_pie_count;
> >> -static unsigned long hpet_t1_cmp;
> >> +static u32 hpet_t1_cmp;
> >>  static unsigned long hpet_default_delta;
> >>  static unsigned long hpet_pie_delta;
> >>  static unsigned long hpet_pie_limit;
> >> @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit;
> >>  static rtc_irq_handler irq_handler;
> >>  
> >>  /*
> >> + * Check that the hpet counter c1 is ahead of the c2
> >> + */
> >> +static inline int hpet_cnt_ahead(u32 c1, u32 c2)
> >> +{
> >> +	return (s32)(c2 - c1) < 0;
> >> +}
> >> +
> >> +/*
> >>   * Registers a IRQ handler.
> >>   */
> >>  int hpet_register_irq_handler(rtc_irq_handler handler)
> >> @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void)
> >>  		hpet_t1_cmp += delta;
> >>  		hpet_writel(hpet_t1_cmp, HPET_T1_CMP);
> >>  		lost_ints++;
> >> -	} while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
> >> +	} while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER)));
> > 
> > These are not equivalent for the case where the values are equal.
> > 
> > Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)"
> > 
> > Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0"
> > 
> > Shouldn't it be:
> > 
> > +	} while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp));
> > 
> > Or am I missing something?
> 
> When comparator it equal to counter (the corner case we're talking about) the
> hpet_cnt_ahead will return false and the loop will go on shifting the cmp,
> which is what we need here.
> 

Okay, as long as that is what is intended.  My point was just that the
code being replaced did not loop when the counts were equal so it
looked like an unintended change.

> >>  
> >>  	if (lost_ints) {
> >>  		if (hpet_rtc_flags & RTC_PIE)
> >> --

-- 
Daniel K. Forrest		Space Science and
dan.forrest@ssec.wisc.edu	Engineering Center
(608) 890 - 0558		University of Wisconsin, Madison