[PATCH] x86: fix hpet timer reinit for x86_64

[PATCH] x86: fix hpet timer reinit for x86_64

[Pavel Emelyanov]

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.

The solution is to cut the upper 32-bit from the hpet_t1_cmp
variable to make the comparison to HPET_COUNTER correct.

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..ec319d1 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -1075,7 +1075,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 ((long)(hpet_readl(HPET_COUNTER) - (u32)hpet_t1_cmp) > 0);
 
 	if (lost_ints) {
 		if (hpet_rtc_flags & RTC_PIE)

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Vitaly Mayatskikh]

At Wed, 04 Feb 2009 13:40:31 +0300, Pavel Emelyanov wrote:
> 
> 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.
> 
> The solution is to cut the upper 32-bit from the hpet_t1_cmp
> variable to make the comparison to HPET_COUNTER correct.

Why not to use u32 as a type for hpet_t1_cmp then?
--
wbr, Vitaly

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Kirill Korotaev]

On 2/4/09 2:39 PM, "Vitaly Mayatskikh" <v.mayatskih@gmail.com> wrote:

> At Wed, 04 Feb 2009 13:40:31 +0300, Pavel Emelyanov wrote:
>> 
>> 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.
>> 
>> The solution is to cut the upper 32-bit from the hpet_t1_cmp
>> variable to make the comparison to HPET_COUNTER correct.
> 
> Why not to use u32 as a type for hpet_t1_cmp then?
> --
> wbr, Vitaly

Pavel, I agree, better please submit original fix
replacing unsigned long to unsigned int (or u32) in definition of
hpet_t1_cmp.

Thanks,
Kirill

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Pavel Emelyanov]

Vitaly Mayatskikh wrote:
> At Wed, 04 Feb 2009 13:40:31 +0300, Pavel Emelyanov wrote:
>> 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.
>>
>> The solution is to cut the upper 32-bit from the hpet_t1_cmp
>> variable to make the comparison to HPET_COUNTER correct.
> 
> Why not to use u32 as a type for hpet_t1_cmp then?

That's the matter of taste from my POV, so I can re-send the patch
if x86 maintainers agree with your version.

> --
> wbr, Vitaly
> 

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Ingo Molnar]

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

> 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.
> 
> The solution is to cut the upper 32-bit from the hpet_t1_cmp
> variable to make the comparison to HPET_COUNTER correct.
> 
> Reported-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

applied to tip/timers/urgent, thanks Pavel!

I'm wondering, what symptoms were seen due to the bug? Did system time stop, 
or did it become erratic, or were there outright lockups?

	Ingo

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Kirill Korotaev]

On 2/4/09 4:49 PM, "Ingo Molnar" <mingo@elte.hu> wrote:

> 
> 
> * Pavel Emelyanov <xemul@openvz.org> wrote:
> 
>> 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.
>> 
>> The solution is to cut the upper 32-bit from the hpet_t1_cmp
>> variable to make the comparison to HPET_COUNTER correct.
>> 
>> Reported-by: Kirill Korotaev <dev@openvz.org>
>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> 
> applied to tip/timers/urgent, thanks Pavel!
> 
> I'm wondering, what symptoms were seen due to the bug? Did system time stop,
> or did it become erratic, or were there outright lockups?

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
counter gets wrap around).

Regards,
Kirill

Re: [PATCH] x86: fix hpet timer reinit for x86_64

[Ingo Molnar]

* Kirill Korotaev <dev@parallels.com> wrote:

> On 2/4/09 4:49 PM, "Ingo Molnar" <mingo@elte.hu> wrote:
> 
> > 
> > 
> > * Pavel Emelyanov <xemul@openvz.org> wrote:
> > 
> >> 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.
> >> 
> >> The solution is to cut the upper 32-bit from the hpet_t1_cmp
> >> variable to make the comparison to HPET_COUNTER correct.
> >> 
> >> Reported-by: Kirill Korotaev <dev@openvz.org>
> >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> > 
> > applied to tip/timers/urgent, thanks Pavel!
> > 
> > I'm wondering, what symptoms were seen due to the bug? Did system time stop,
> > or did it become erratic, or were there outright lockups?
> 
> 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
> counter gets wrap around).

okay. Could you guys please put this info in the commit log on the 
resubmitted patch? Thanks,

	Ingo