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

[Benjamin Herrenschmidt]

On Thu, 2008-06-05 at 15:02 +0000, Luke Browning wrote:
> 
> 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. 

I don't know about that. Unless the SPU does something special on
receiveing an eieio message on the bus, which I yet have to see
documented properly, I would expect the eieio to be only useful to
ensure the ordering of the following load with previous stores from the
CPU to the SPU control registers.

>  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.

I don't know what you call by state transition here. eieio only provides
ordering between storage accesses. It's not supposed to do anything
else.

> > 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;
> }

Well, two things here.

First is: How old is the code above ? Current kernels have:

#define DEF_MMIO_IN(name, type, insn)					\
static inline type name(const volatile type __iomem *addr)		\
{									\
	type ret;							\
	__asm__ __volatile__("sync;" insn ";twi 0,%0,0;isync"		\
 		: "=r" (ret) : "r" (addr), "m" (*addr));		\
	return ret;							\
}

#define DEF_MMIO_OUT(name, type, insn)					\
static inline void name(volatile type __iomem *addr, type val)		\
{									\
	__asm__ __volatile__("sync;" insn				\
 		: "=m" (*addr) : "r" (val), "r" (addr));		\
	IO_SET_SYNC_FLAG();					\
}

There are many reasons for that, we can discuss them later if you
want. In addition, even with the code above, I would strongly
suspect that the eieio is only needed as a barrier between previous
stores to the SPU control registers and the load. And that eieio is
present in the old code's out_* accessors. I don't think there is
any intrinsic state transition caused nor necessary due to the eieio
itself.

Ben.