Re: [git pull] timer fix

[Ingo Molnar]

* 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/