All of lore.kernel.org
 help / color / mirror / Atom feed
* [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM
@ 2007-01-21 19:13 Adrian Bunk
  2007-01-22 15:18 ` Alan
  0 siblings, 1 reply; 5+ messages in thread
From: Adrian Bunk @ 2007-01-21 19:13 UTC (permalink / raw)
  To: Andrew Morton; +Cc: James.Bottomley, linux-scsi, linux-kernel

Using assembler code for performance in drivers might have been a good 
idea 15 years ago when this code was written, but with today's compilers 
that's unlikely to be an advantage.

Besides this, it also hurts the readability.

Simply use the C code that was already there as an alternative.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

This patch was already sent on:
- 6 Jan 2007

 drivers/scsi/Makefile  |    2 
 drivers/scsi/seagate.c |  148 -----------------------------------------
 2 files changed, 1 insertion(+), 149 deletions(-)

--- linux-2.6.20-rc3-mm1/drivers/scsi/Makefile.old	2007-01-05 23:01:12.000000000 +0100
+++ linux-2.6.20-rc3-mm1/drivers/scsi/Makefile	2007-01-05 23:01:51.000000000 +0100
@@ -16,7 +16,7 @@
 
 CFLAGS_aha152x.o =   -DAHA152X_STAT -DAUTOCONF
 CFLAGS_gdth.o    = # -DDEBUG_GDTH=2 -D__SERIAL__ -D__COM2__ -DGDTH_STATISTICS
-CFLAGS_seagate.o =   -DARBITRATE -DPARITY -DSEAGATE_USE_ASM
+CFLAGS_seagate.o =   -DARBITRATE -DPARITY
 
 subdir-$(CONFIG_PCMCIA)		+= pcmcia
 
--- linux-2.6.20-rc3-mm1/drivers/scsi/seagate.c.old	2007-01-05 23:02:03.000000000 +0100
+++ linux-2.6.20-rc3-mm1/drivers/scsi/seagate.c	2007-01-05 23:03:11.000000000 +0100
@@ -60,10 +60,6 @@
  * -DPARITY  
  *      This will enable parity.
  *
- * -DSEAGATE_USE_ASM
- *      Will use older seagate assembly code. should be (very small amount)
- *      Faster.
- *
  * -DSLOW_RATE=50
  *      Will allow compatibility with broken devices that don't
  *      handshake fast enough (ie, some CD ROM's) for the Seagate
@@ -132,10 +128,6 @@
 #error Please use -DCONTROLLER=SEAGATE or -DCONTROLLER=FD to override controller type
 #endif
 
-#ifndef __i386__
-#undef SEAGATE_USE_ASM
-#endif
-
 /*
 	Thanks to Brian Antoine for the example code in his Messy-Loss ST-01
 		driver, and Mitsugu Suzuki for information on the ST-01
@@ -539,9 +531,6 @@
 #ifdef PARITY
 		" PARITY"
 #endif
-#ifdef SEAGATE_USE_ASM
-		" SEAGATE_USE_ASM"
-#endif
 #ifdef SLOW_RATE
 		" SLOW_RATE"
 #endif
@@ -1139,30 +1128,7 @@
 						 data);
 
 			/* SJT: Start. Fast Write */
-#ifdef SEAGATE_USE_ASM
-					__asm__ ("cld\n\t"
-#ifdef FAST32
-						 "shr $2, %%ecx\n\t"
-						 "1:\t"
-						 "lodsl\n\t"
-						 "movl %%eax, (%%edi)\n\t"
-#else
-						 "1:\t"
-						 "lodsb\n\t"
-						 "movb %%al, (%%edi)\n\t"
-#endif
-						 "loop 1b;"
-				      /* output */ :
-				      /* input */ :"D" (st0x_dr),
-						 "S"
-						 (data),
-						 "c" (SCint->transfersize)
-/* clobbered */
-				      :	 "eax", "ecx",
-						 "esi");
-#else				/* SEAGATE_USE_ASM */
 					memcpy_toio(st0x_dr, data, transfersize);
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End */
 					len -= transfersize;
 					data += transfersize;
@@ -1175,49 +1141,6 @@
 					 */
 
 /* SJT: Start. Slow Write. */
-#ifdef SEAGATE_USE_ASM
-
-					int __dummy_1, __dummy_2;
-
-/*
- *      We loop as long as we are in a data out phase, there is data to send, 
- *      and BSY is still active.
- */
-/* Local variables : len = ecx , data = esi, 
-                     st0x_cr_sr = ebx, st0x_dr =  edi
-*/
-					__asm__ (
-							/* Test for any data here at all. */
-							"orl %%ecx, %%ecx\n\t"
-							"jz 2f\n\t" "cld\n\t"
-/*                    "movl st0x_cr_sr, %%ebx\n\t"  */
-/*                    "movl st0x_dr, %%edi\n\t"  */
-							"1:\t"
-							"movb (%%ebx), %%al\n\t"
-							/* Test for BSY */
-							"test $1, %%al\n\t"
-							"jz 2f\n\t"
-							/* Test for data out phase - STATUS & REQ_MASK should be 
-							   REQ_DATAOUT, which is 0. */
-							"test $0xe, %%al\n\t"
-							"jnz 2f\n\t"
-							/* Test for REQ */
-							"test $0x10, %%al\n\t"
-							"jz 1b\n\t"
-							"lodsb\n\t"
-							"movb %%al, (%%edi)\n\t"
-							"loop 1b\n\t" "2:\n"
-				      /* output */ :"=S" (data), "=c" (len),
-							"=b"
-							(__dummy_1),
-							"=D" (__dummy_2)
-/* input */
-				      :		"0" (data), "1" (len),
-							"2" (st0x_cr_sr),
-							"3" (st0x_dr)
-/* clobbered */
-				      :		"eax");
-#else				/* SEAGATE_USE_ASM */
 					while (len) {
 						unsigned char stat;
 
@@ -1231,7 +1154,6 @@
 							--len;
 						}
 					}
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End. */
 				}
 
@@ -1277,30 +1199,7 @@
 						 data);
 
 /* SJT: Start. Fast Read */
-#ifdef SEAGATE_USE_ASM
-					__asm__ ("cld\n\t"
-#ifdef FAST32
-						 "shr $2, %%ecx\n\t"
-						 "1:\t"
-						 "movl (%%esi), %%eax\n\t"
-						 "stosl\n\t"
-#else
-						 "1:\t"
-						 "movb (%%esi), %%al\n\t"
-						 "stosb\n\t"
-#endif
-						 "loop 1b\n\t"
-				      /* output */ :
-				      /* input */ :"S" (st0x_dr),
-						 "D"
-						 (data),
-						 "c" (SCint->transfersize)
-/* clobbered */
-				      :	 "eax", "ecx",
-						 "edi");
-#else				/* SEAGATE_USE_ASM */
 					memcpy_fromio(data, st0x_dr, len);
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End */
 					len -= transfersize;
 					data += transfersize;
@@ -1324,52 +1223,6 @@
  */
 
 /* SJT: Start. */
-#ifdef SEAGATE_USE_ASM
-
-					int __dummy_3, __dummy_4;
-
-/* Dummy clobbering variables for the new gcc-2.95 */
-
-/*
- *      We loop as long as we are in a data in phase, there is room to read, 
- *      and BSY is still active
- */
-					/* Local variables : ecx = len, edi = data
-					   esi = st0x_cr_sr, ebx = st0x_dr */
-					__asm__ (
-							/* Test for room to read */
-							"orl %%ecx, %%ecx\n\t"
-							"jz 2f\n\t" "cld\n\t"
-/*                "movl st0x_cr_sr, %%esi\n\t"  */
-/*                "movl st0x_dr, %%ebx\n\t"  */
-							"1:\t"
-							"movb (%%esi), %%al\n\t"
-							/* Test for BSY */
-							"test $1, %%al\n\t"
-							"jz 2f\n\t"
-							/* Test for data in phase - STATUS & REQ_MASK should be REQ_DATAIN, 
-							   = STAT_IO, which is 4. */
-							"movb $0xe, %%ah\n\t"
-							"andb %%al, %%ah\n\t"
-							"cmpb $0x04, %%ah\n\t"
-							"jne 2f\n\t"
-							/* Test for REQ */
-							"test $0x10, %%al\n\t"
-							"jz 1b\n\t"
-							"movb (%%ebx), %%al\n\t"
-							"stosb\n\t"
-							"loop 1b\n\t" "2:\n"
-				      /* output */ :"=D" (data), "=c" (len),
-							"=S"
-							(__dummy_3),
-							"=b" (__dummy_4)
-/* input */
-				      :		"0" (data), "1" (len),
-							"2" (st0x_cr_sr),
-							"3" (st0x_dr)
-/* clobbered */
-				      :		"eax");
-#else				/* SEAGATE_USE_ASM */
 					while (len) {
 						unsigned char stat;
 
@@ -1383,7 +1236,6 @@
 							--len;
 						}
 					}
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End. */
 #if (DEBUG & PHASE_DATAIN)
 					printk ("scsi%d: transfered -= %d\n", hostno, len);


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM
  2007-01-21 19:13 [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM Adrian Bunk
@ 2007-01-22 15:18 ` Alan
  2007-01-22 15:38   ` Adrian Bunk
  0 siblings, 1 reply; 5+ messages in thread
From: Alan @ 2007-01-22 15:18 UTC (permalink / raw)
  To: Adrian Bunk; +Cc: Andrew Morton, James.Bottomley, linux-scsi, linux-kernel

On Sun, 21 Jan 2007 20:13:00 +0100
Adrian Bunk <bunk@stusta.de> wrote:

> Using assembler code for performance in drivers might have been a good 
> idea 15 years ago when this code was written, but with today's compilers 
> that's unlikely to be an advantage.
> 
> Besides this, it also hurts the readability.
> 
> Simply use the C code that was already there as an alternative.
> 
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
					"stosb\n\t"

NAK

The C codepaths are essentially untested on this driver.

Alan

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM
  2007-01-22 15:18 ` Alan
@ 2007-01-22 15:38   ` Adrian Bunk
  2007-01-22 15:53     ` Alan
  0 siblings, 1 reply; 5+ messages in thread
From: Adrian Bunk @ 2007-01-22 15:38 UTC (permalink / raw)
  To: Alan; +Cc: Andrew Morton, James.Bottomley, linux-scsi, linux-kernel

On Mon, Jan 22, 2007 at 03:18:41PM +0000, Alan wrote:
> On Sun, 21 Jan 2007 20:13:00 +0100
> Adrian Bunk <bunk@stusta.de> wrote:
> 
> > Using assembler code for performance in drivers might have been a good 
> > idea 15 years ago when this code was written, but with today's compilers 
> > that's unlikely to be an advantage.
> > 
> > Besides this, it also hurts the readability.
> > 
> > Simply use the C code that was already there as an alternative.
> > 
> > Signed-off-by: Adrian Bunk <bunk@stusta.de>
> 					"stosb\n\t"
> 
> NAK
> 
> The C codepaths are essentially untested on this driver.

Has any part of this driver ever be tested with kernel 2.6?
Or compiled with gcc 4?

> Alan

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM
  2007-01-22 15:38   ` Adrian Bunk
@ 2007-01-22 15:53     ` Alan
  0 siblings, 0 replies; 5+ messages in thread
From: Alan @ 2007-01-22 15:53 UTC (permalink / raw)
  To: Adrian Bunk; +Cc: Andrew Morton, James.Bottomley, linux-scsi, linux-kernel

> > The C codepaths are essentially untested on this driver.
> 
> Has any part of this driver ever be tested with kernel 2.6?
> Or compiled with gcc 4?

The C code paths have never been tested at all, the asm ones certainly
worked in late 2.4, but I don't; have an ISA box any more.

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM
@ 2007-01-06  1:06 Adrian Bunk
  0 siblings, 0 replies; 5+ messages in thread
From: Adrian Bunk @ 2007-01-06  1:06 UTC (permalink / raw)
  To: James.Bottomley; +Cc: linux-scsi, linux-kernel

Using assembler code for performance in drivers might have been a good 
idea 15 years ago when this code was written, but with today's compilers 
that's unlikely to be an advantage.

Besides this, it also hurts the readability.

Simply use the C code that was already there as an alternative.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

---

 drivers/scsi/Makefile  |    2 
 drivers/scsi/seagate.c |  148 -----------------------------------------
 2 files changed, 1 insertion(+), 149 deletions(-)

--- linux-2.6.20-rc3-mm1/drivers/scsi/Makefile.old	2007-01-05 23:01:12.000000000 +0100
+++ linux-2.6.20-rc3-mm1/drivers/scsi/Makefile	2007-01-05 23:01:51.000000000 +0100
@@ -16,7 +16,7 @@
 
 CFLAGS_aha152x.o =   -DAHA152X_STAT -DAUTOCONF
 CFLAGS_gdth.o    = # -DDEBUG_GDTH=2 -D__SERIAL__ -D__COM2__ -DGDTH_STATISTICS
-CFLAGS_seagate.o =   -DARBITRATE -DPARITY -DSEAGATE_USE_ASM
+CFLAGS_seagate.o =   -DARBITRATE -DPARITY
 
 subdir-$(CONFIG_PCMCIA)		+= pcmcia
 
--- linux-2.6.20-rc3-mm1/drivers/scsi/seagate.c.old	2007-01-05 23:02:03.000000000 +0100
+++ linux-2.6.20-rc3-mm1/drivers/scsi/seagate.c	2007-01-05 23:03:11.000000000 +0100
@@ -60,10 +60,6 @@
  * -DPARITY  
  *      This will enable parity.
  *
- * -DSEAGATE_USE_ASM
- *      Will use older seagate assembly code. should be (very small amount)
- *      Faster.
- *
  * -DSLOW_RATE=50
  *      Will allow compatibility with broken devices that don't
  *      handshake fast enough (ie, some CD ROM's) for the Seagate
@@ -132,10 +128,6 @@
 #error Please use -DCONTROLLER=SEAGATE or -DCONTROLLER=FD to override controller type
 #endif
 
-#ifndef __i386__
-#undef SEAGATE_USE_ASM
-#endif
-
 /*
 	Thanks to Brian Antoine for the example code in his Messy-Loss ST-01
 		driver, and Mitsugu Suzuki for information on the ST-01
@@ -539,9 +531,6 @@
 #ifdef PARITY
 		" PARITY"
 #endif
-#ifdef SEAGATE_USE_ASM
-		" SEAGATE_USE_ASM"
-#endif
 #ifdef SLOW_RATE
 		" SLOW_RATE"
 #endif
@@ -1139,30 +1128,7 @@
 						 data);
 
 			/* SJT: Start. Fast Write */
-#ifdef SEAGATE_USE_ASM
-					__asm__ ("cld\n\t"
-#ifdef FAST32
-						 "shr $2, %%ecx\n\t"
-						 "1:\t"
-						 "lodsl\n\t"
-						 "movl %%eax, (%%edi)\n\t"
-#else
-						 "1:\t"
-						 "lodsb\n\t"
-						 "movb %%al, (%%edi)\n\t"
-#endif
-						 "loop 1b;"
-				      /* output */ :
-				      /* input */ :"D" (st0x_dr),
-						 "S"
-						 (data),
-						 "c" (SCint->transfersize)
-/* clobbered */
-				      :	 "eax", "ecx",
-						 "esi");
-#else				/* SEAGATE_USE_ASM */
 					memcpy_toio(st0x_dr, data, transfersize);
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End */
 					len -= transfersize;
 					data += transfersize;
@@ -1175,49 +1141,6 @@
 					 */
 
 /* SJT: Start. Slow Write. */
-#ifdef SEAGATE_USE_ASM
-
-					int __dummy_1, __dummy_2;
-
-/*
- *      We loop as long as we are in a data out phase, there is data to send, 
- *      and BSY is still active.
- */
-/* Local variables : len = ecx , data = esi, 
-                     st0x_cr_sr = ebx, st0x_dr =  edi
-*/
-					__asm__ (
-							/* Test for any data here at all. */
-							"orl %%ecx, %%ecx\n\t"
-							"jz 2f\n\t" "cld\n\t"
-/*                    "movl st0x_cr_sr, %%ebx\n\t"  */
-/*                    "movl st0x_dr, %%edi\n\t"  */
-							"1:\t"
-							"movb (%%ebx), %%al\n\t"
-							/* Test for BSY */
-							"test $1, %%al\n\t"
-							"jz 2f\n\t"
-							/* Test for data out phase - STATUS & REQ_MASK should be 
-							   REQ_DATAOUT, which is 0. */
-							"test $0xe, %%al\n\t"
-							"jnz 2f\n\t"
-							/* Test for REQ */
-							"test $0x10, %%al\n\t"
-							"jz 1b\n\t"
-							"lodsb\n\t"
-							"movb %%al, (%%edi)\n\t"
-							"loop 1b\n\t" "2:\n"
-				      /* output */ :"=S" (data), "=c" (len),
-							"=b"
-							(__dummy_1),
-							"=D" (__dummy_2)
-/* input */
-				      :		"0" (data), "1" (len),
-							"2" (st0x_cr_sr),
-							"3" (st0x_dr)
-/* clobbered */
-				      :		"eax");
-#else				/* SEAGATE_USE_ASM */
 					while (len) {
 						unsigned char stat;
 
@@ -1231,7 +1154,6 @@
 							--len;
 						}
 					}
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End. */
 				}
 
@@ -1277,30 +1199,7 @@
 						 data);
 
 /* SJT: Start. Fast Read */
-#ifdef SEAGATE_USE_ASM
-					__asm__ ("cld\n\t"
-#ifdef FAST32
-						 "shr $2, %%ecx\n\t"
-						 "1:\t"
-						 "movl (%%esi), %%eax\n\t"
-						 "stosl\n\t"
-#else
-						 "1:\t"
-						 "movb (%%esi), %%al\n\t"
-						 "stosb\n\t"
-#endif
-						 "loop 1b\n\t"
-				      /* output */ :
-				      /* input */ :"S" (st0x_dr),
-						 "D"
-						 (data),
-						 "c" (SCint->transfersize)
-/* clobbered */
-				      :	 "eax", "ecx",
-						 "edi");
-#else				/* SEAGATE_USE_ASM */
 					memcpy_fromio(data, st0x_dr, len);
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End */
 					len -= transfersize;
 					data += transfersize;
@@ -1324,52 +1223,6 @@
  */
 
 /* SJT: Start. */
-#ifdef SEAGATE_USE_ASM
-
-					int __dummy_3, __dummy_4;
-
-/* Dummy clobbering variables for the new gcc-2.95 */
-
-/*
- *      We loop as long as we are in a data in phase, there is room to read, 
- *      and BSY is still active
- */
-					/* Local variables : ecx = len, edi = data
-					   esi = st0x_cr_sr, ebx = st0x_dr */
-					__asm__ (
-							/* Test for room to read */
-							"orl %%ecx, %%ecx\n\t"
-							"jz 2f\n\t" "cld\n\t"
-/*                "movl st0x_cr_sr, %%esi\n\t"  */
-/*                "movl st0x_dr, %%ebx\n\t"  */
-							"1:\t"
-							"movb (%%esi), %%al\n\t"
-							/* Test for BSY */
-							"test $1, %%al\n\t"
-							"jz 2f\n\t"
-							/* Test for data in phase - STATUS & REQ_MASK should be REQ_DATAIN, 
-							   = STAT_IO, which is 4. */
-							"movb $0xe, %%ah\n\t"
-							"andb %%al, %%ah\n\t"
-							"cmpb $0x04, %%ah\n\t"
-							"jne 2f\n\t"
-							/* Test for REQ */
-							"test $0x10, %%al\n\t"
-							"jz 1b\n\t"
-							"movb (%%ebx), %%al\n\t"
-							"stosb\n\t"
-							"loop 1b\n\t" "2:\n"
-				      /* output */ :"=D" (data), "=c" (len),
-							"=S"
-							(__dummy_3),
-							"=b" (__dummy_4)
-/* input */
-				      :		"0" (data), "1" (len),
-							"2" (st0x_cr_sr),
-							"3" (st0x_dr)
-/* clobbered */
-				      :		"eax");
-#else				/* SEAGATE_USE_ASM */
 					while (len) {
 						unsigned char stat;
 
@@ -1383,7 +1236,6 @@
 							--len;
 						}
 					}
-#endif				/* SEAGATE_USE_ASM */
 /* SJT: End. */
 #if (DEBUG & PHASE_DATAIN)
 					printk ("scsi%d: transfered -= %d\n", hostno, len);


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-01-22 15:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-21 19:13 [2.6 patch] SCSI seagate.c: remove SEAGATE_USE_ASM Adrian Bunk
2007-01-22 15:18 ` Alan
2007-01-22 15:38   ` Adrian Bunk
2007-01-22 15:53     ` Alan
  -- strict thread matches above, loose matches on Subject: below --
2007-01-06  1:06 Adrian Bunk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.