Re: [git pull] timer fix

From: Ingo Molnar
Date: Wed Feb 04 2009 - 17:28:18 EST



* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed, 4 Feb 2009, Ingo Molnar wrote:
> >
> > Pavel Emelyanov (1):
> > x86: fix hpet timer reinit for x86_64
> >
> >
> > arch/x86/kernel/hpet.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > 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);
>
> This is bordering on not being correct.

yeah, i had to look twice. The only reason i left it that way was because i
couldnt reproduce the problem and hpet is hellishly fragile and this patch
was tested so i chickened out.

OTOH that fragility is partly because such constructs have piled up so you
very much have a valid point ...

We'll clean this up. I've already added the clean 32-bit casts - which also
has another advantage: it does not actually trust the hw to always return
32-bit values - it explicitly cuts to 32 bits and does signed arithmetics on
that. Will also do the helper function cleanup to abstract the counter
arithmetics away.

> In particular, think about when HPET_COUNTER or hpet_t1_cmp overflows in
> 32 bits, and what you want to happen. If you do the subtract add test in
> 64 bits, it will simply do the wrong thing. Think what happens if
> hpet_t1_cmp is actually _larger_ than HPET_COUNTER, but overflowed in 32
> bits, and you're now looking at:
>
> (long) (0xffffffff - 0x00000001)
>
> which is actually > 0, so the thing will continue to loop INCORRECTLY. It
> should have stopped (and _would_ have stopped on 32-bit x86).

yeah, allowing that to happen is just wrong.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/