bogus barriers in sym53c8xx_2?

bogus barriers in sym53c8xx_2?

[David Mosberger]

In drivers/scsi/sym53c8xx_2/sym_misc.h we find:

#elif	defined	__ia64__
#define __READ_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
#define __WRITE_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")

based on the comments and the other implementations, these barriers
are bogus and the "mf.a" should be dropped.

Anyone know who wrote this code originally and why the mf.a was added?

mf.a is very slow and should be avoided except were truly needed.

	--david

Re: bogus barriers in sym53c8xx_2?

[Matthew Wilcox]

On Tue, Aug 19, 2003 at 04:49:53PM -0700, David Mosberger wrote:
> In drivers/scsi/sym53c8xx_2/sym_misc.h we find:
> 
> #elif	defined	__ia64__
> #define __READ_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
> #define __WRITE_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
> 
> based on the comments and the other implementations, these barriers
> are bogus and the "mf.a" should be dropped.
> 
> Anyone know who wrote this code originally and why the mf.a was added?
> 
> mf.a is very slow and should be avoided except were truly needed.

I'm sure Gerard must have written it originally.  It's there
in the earliest version of the sym2 driver I can find --
sym-2.1.16a-for-linux-2.4.13.patch.gz.  A similar barrier is there in
the sym1 driver (drivers/scsi/sym53c8xx_defs.h).  It seems to have been
introduced around 2.4.3 (symbios driver version 1.6b -> 1.7.3a-20010304)

So you're looking for a patch which looks something like this:

- #define __READ_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
- #define __WRITE_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
+ #define __READ_BARRIER()	__asm__ volatile("mf" : : : "memory")
+ #define __WRITE_BARRIER()	__asm__ volatile("mf" : : : "memory")

Or really, might be better to just define them to rmb() and wmb()?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: bogus barriers in sym53c8xx_2?

[Anton Blanchard]

> I'm sure Gerard must have written it originally.  It's there
> in the earliest version of the sym2 driver I can find --
> sym-2.1.16a-for-linux-2.4.13.patch.gz.  A similar barrier is there in
> the sym1 driver (drivers/scsi/sym53c8xx_defs.h).  It seems to have been
> introduced around 2.4.3 (symbios driver version 1.6b -> 1.7.3a-20010304)
> 
> So you're looking for a patch which looks something like this:
> 
> - #define __READ_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
> - #define __WRITE_BARRIER()	__asm__ volatile("mf.a; mf" : : : "memory")
> + #define __READ_BARRIER()	__asm__ volatile("mf" : : : "memory")
> + #define __WRITE_BARRIER()	__asm__ volatile("mf" : : : "memory")
> 
> Or really, might be better to just define them to rmb() and wmb()?

I suspect so, the powerpc ones are overkill too:

#define __READ_BARRIER()        __asm__ volatile("eieio; sync" : : : "memory")
#define __WRITE_BARRIER()       __asm__ volatile("eieio; sync" : : : "memory")

Anton

Re: bogus barriers in sym53c8xx_2?

[David Mosberger]

I didn't see any followup.  Gerard, do you have any comments?
I'm inclined to drop the #ifdef __ia64 part.  Also, I think
Matthew's suggestion to use rmb()/wmb() for the generic case
is a good one (though it makes no difference for ia64).

	--david

>>>>> On Wed, 20 Aug 2003 13:43:18 +1000, Anton Blanchard <anton@samba.org> said:

  >> I'm sure Gerard must have written it originally.  It's there in
  >> the earliest version of the sym2 driver I can find --
  >> sym-2.1.16a-for-linux-2.4.13.patch.gz.  A similar barrier is
  >> there in the sym1 driver (drivers/scsi/sym53c8xx_defs.h).  It
  >> seems to have been introduced around 2.4.3 (symbios driver
  >> version 1.6b -> 1.7.3a-20010304)

  >> So you're looking for a patch which looks something like this:

  >> - #define __READ_BARRIER() __asm__ volatile("mf.a; mf" : : :
  >> "memory") - #define __WRITE_BARRIER() __asm__ volatile("mf.a; mf"
  >> : : : "memory") + #define __READ_BARRIER() __asm__ volatile("mf"
  >> : : : "memory") + #define __WRITE_BARRIER() __asm__ volatile("mf"
  >> : : : "memory")

  >> Or really, might be better to just define them to rmb() and
  >> wmb()?

  Anton> I suspect so, the powerpc ones are overkill too:

  Anton> #define __READ_BARRIER() __asm__ volatile("eieio; sync" : : :
  Anton> "memory") #define __WRITE_BARRIER() __asm__ volatile("eieio;
  Anton> sync" : : : "memory")