lkml.org 
[lkml]   [2009]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [git pull] timer fix

    * Linus Torvalds <torvalds@linux-foundation.org> 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


    \
     
     \ /
      Last update: 2009-02-04 23:31    [W:0.039 / U:1.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site