On Thu, Feb 01, 2007 at 06:34:50PM -0800, Andrew Morton wrote: > On Thu, 01 Feb 2007 10:59:53 +0100 [EMAIL PROTECTED] wrote: > > > Fix a race in the initialization of HPET, which might result in a > > 5 minute lockup on boot. > > > > What race? Please always describe bugs when fixing them.
If the value of the HPET_T0_CMP register is reached and exceeded by the value of the HPET_COUNTER register after HPET_T0_CMP is read into trigger, but before the first iteration of the while, the while loop will iterate "endlessly" until the HPET overlaps eventually (in as much as 5 minutes). > > > > Index: linux-2.6.20-rc5/arch/x86_64/kernel/apic.c > > =================================================================== > > --- linux-2.6.20-rc5.orig/arch/x86_64/kernel/apic.c > > +++ linux-2.6.20-rc5/arch/x86_64/kernel/apic.c > > @@ -764,10 +767,12 @@ static void setup_APIC_timer(unsigned in > > > > /* wait for irq slice */ > > if (vxtime.hpet_address && hpet_use_timer) { > > - int trigger = hpet_readl(HPET_T0_CMP); > > - while (hpet_readl(HPET_COUNTER) >= trigger) > > - /* do nothing */ ; > > - while (hpet_readl(HPET_COUNTER) < trigger) > > + int trigger; > > + do > > + trigger = hpet_readl(HPET_T0_CMP); > > + while (hpet_readl(HPET_COUNTER) >= trigger); > > + > > Is this signedness-safe and wraparound-safe? It might be better to make > `trigger' unsigned and do > > while (hpet_readl(HPET_COUNTER) - trigger >= 0) Yes, making trigger unsigned is a good idea (although having it signed would probably never cause any problem, because this is called during boot and it takes ~2 minutes for HPET to overflow the s32) But no, looping while the unsigned result is >= 0 does not seem that good an idea to me ;-) An updated patch follows. It is still not wraparound safe (a lockup would still happen if it's called ~5 minutes after boot, but this should never happen -- it's called early during boot) --- linux-2.6.20-rc5.orig/arch/x86_64/kernel/apic.c 2007-02-06 16:56:00.000000000 +0100 +++ linux-2.6.20-rc5/arch/x86_64/kernel/apic.c 2007-02-06 17:26:42.000000000 +0100 @@ -764,10 +764,12 @@ static void setup_APIC_timer(unsigned in /* wait for irq slice */ if (vxtime.hpet_address && hpet_use_timer) { - int trigger = hpet_readl(HPET_T0_CMP); - while (hpet_readl(HPET_COUNTER) >= trigger) - /* do nothing */ ; - while (hpet_readl(HPET_COUNTER) < trigger) + u32 trigger; + do + trigger = hpet_readl(HPET_T0_CMP); + while (hpet_readl(HPET_COUNTER) >= trigger); + + while (hpet_readl(HPET_COUNTER) < trigger) /* do nothing */ ; } else { int c1, c2; -- Jiri Bohac <[EMAIL PROTECTED]> SUSE Labs, SUSE CZ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/