Re: [PATCH] powerpc: Fix race in the pasemi timebase calibration

[Olof Johansson <olof@lixom.net>]

On Wed, Aug 22, 2007 at 11:27:33AM +1000, Paul Mackerras wrote:
> Olof Johansson writes:
> 
> > Make sure the new timebase value is available by the time take_timebase
> > completes. Otherwise take_timebase might race with give_timebase,
> > causing severe badness when the value later is modified (think looong
> > hang trying to catch up with a very large number of lost ticks).
> 
> OK.
> 
> > @@ -61,6 +62,7 @@ static void __devinit pas_give_timebase(
> >  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_LOWER | (tb & 0xffffffff));
> >  	mtspr(SPRN_TBCTL, TBCTL_UPDATE_UPPER | (tb >> 32));
> >  	mtspr(SPRN_TBCTL, TBCTL_RESTART);
> > +	timebase_avail = 1;
> 
> No memory barrier before setting timebase_avail?  Shouldn't there be
> one?

Technically there's no previous memory access to put that barrier up
against since they're all SPR ops, but an isync after the mtspr would
be warranted.

> Actually I don't understand that code at all.  Your give_timebase
> seems to freeze the timebase, read it, set it to the same value and
> restart, all without synchronizing with the other cpu, and your
> take_timebase does nothing except print the timebase.  How does that
> work?

The TBCTL functions control the TBs of all cores. I.e. current
give_timebase will push out the current TB of the booting core to all
others in the system.

And yes, I had misunderstood the timebase calibration back when I
implemented it, not realizing we do a give+take for each cpu coming
up. It should really look more like the pseries implementation, only
using TBCTL to freeze/thaw and do the handover manually. That'll be CPU
hotplug-proof as well.

New patch reworking all of this coming. Thanks for the reality check.

-Olof