On Tue, 6 Feb 2007 17:44:59 +0100 Jiri Bohac <[EMAIL PROTECTED]> wrote:
> 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; > Well it still seems a bit dodgy to me - I'd have thought it'd be possible (and nicer) to come up with a version which is safe to call at any time. Are you sure this won't cause a kexec'ed kernel to lock up for five minutes, for example? Anyway, I'll let Andi worry about this one. Please send him a signed-off and fully changelogged patch and still cc myself, thanks. - 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/