Messages in this thread | | | Date | Wed, 4 Feb 2009 14:11:55 -0800 (PST) | From | Linus Torvalds <> | Subject | Re: [git pull] timer fix |
| |
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.
It may happen to _work_, but the fact is, you want a 32-bit signed compare, not a 64-bit subtract that just happens to work. So the proper fix is to just make it do
} while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0);
Otherwise you always end up depending on very subtle internal logic, and the exact types of the things involved.
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).
In contrast, look at what happens if you do the subtracting (or at least test the _result_ of the subtract) in the right size:
(s32) (0xffffffff - 0x00000001)
which becomes -2, which is not larger than 0, which means that we exit (which is correct, because the comparator value is actually ahead of the current count: 0x00000001 is _ahead_ of 0xffffffff, even if it's smaller in an "unsigned long".
So I'm not going to pull it. This cast is simply wrong.
Either cast the result of the subtract to "s32" (or "int", whatever), or cast _both_ of them to (s32) so that the subtract is done in a signed type, and then the expansion to (long) will still be right - but unnecessary - in the sign.
Linus
| |