[Cbe-oss-dev] [PATCH 2:9] spufs: Fix spu status read in spu_stopped()

[Luke Browning]

On Thu, 2008-06-05 at 14:14 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2008-06-04 at 13:29 +0000, Luke Browning wrote:
> > On Wed, 2008-06-04 at 11:49 +0800, Jeremy Kerr wrote:
> > > Hi Luke,
> > > 
> > > > +	/* Ensure that all spu status bits have been updated */
> > > > +	eieio();
> > > > +
> > > > +top:
> > > > +	*stat = ctx->ops->status_read(ctx);
> > > 
> > > should the eieio() be repeated for each status read? (ie, have the top: 
> > > label before the eieio)
> > 
> > No.  eieio() must be reliable.  Otherwise it is worthless.
> 
> Ugh ?
> 
> What do you mean ?
> 
> If you needed that eieio at all in the first place, then yes, you
> would probably need it only once, but this has nothing to do with
> being "reliable" :-) It's purely due to the fact that the loop isn't
> doing any relevant operations that might be re-ordered with that load,
> and so subsequent loads from the same place don't need another eieio.
> 

That is true also, but I assumed Jeremy knew that.  The loop is a simple
read loop with no writes taking place.  The eieio() is needed to
synchronize the caller with state transitions in the hardware that are
still in flight so to speak.  If the eieio() instruction does not
provide this guarantee, it is a hardware bug.  There are no subsequent
instructions in the loop that would cause a state transtion.  The 
eieio() is required in this case even without the loop.

> However, I strongly suspect that status_read implementation uses
> one of the in_* accessors which will already do the necessary barrier.
> 
> 

I would not expect an eieio() for a read.  I would for a write.

static inline unsigned in_be32(const volatile unsigned *addr)
{
        unsigned ret;

        __asm__ __volatile__("lwz%U1%X1 %0,%1; twi 0,%0,0; isync"
                             : "=r" (ret) : "m" (*addr));
        return ret;
}

Luke