mac_esp, zorro_esp, esp_scsi: Various improvements

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so the chip's Transfer Counter register is only 16 bits wide (not 24). A larger transfer cannot work and will theoretically result in a failed command and a "DMA length is zero" error.

Fixes 3109e5ae0311

Signed-off-by Finn Thain <[email protected]>

Cc Michael Schmitz <[email protected]>

Tested-by Michael Schmitz <[email protected]>

---
 drivers/scsi/zorro_esp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/scsi/zorro_esp.c +1 -7

index bb70882e6b56..be79127db594 100644
@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
 					u32 dma_len)
 {
-	return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
+	return dma_len > 0xFFFF ? 0xFFFF : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)

@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }

@@ -529,7 +528,6 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }

@@ -574,7 +572,6 @@ static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
 	scsi_esp_cmd(esp, ESP_CMD_DMA);
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	scsi_esp_cmd(esp, cmd);
 }

@@ -599,7 +596,6 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */

@@ -649,7 +645,6 @@ static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */

@@ -691,7 +686,6 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
 
 	zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
 	zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-	zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
 
 	if (write) {
 		/* DMA receive */

The core driver, esp_scsi, does not use the ESP_CONFIG2_FENAB bit, so the chip's Transfer Counter register is only 16 bits wide (not 24). A larger transfer cannot work and will theoretically result in a failed command and a "DMA length is zero" error.

Fixes 3109e5ae0311

Signed-off-by Finn Thain <[email protected]>

Cc Michael Schmitz <[email protected]>

Tested-by Michael Schmitz <[email protected]>

drivers/scsi/zorro_esp.c +1 -1

@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
                                        u32 dma_len)
 {
-       return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
+       return dma_len > 0xFFFF ? 0xFFFF : dma_len;
 }
 
 static void zorro_esp_reset_dma(struct esp *esp)

Quoted 2 lines

Fixes 3109e5ae0311

Fixes 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro

NCR53C9x boards")

$ git help fixes
'fixes' is aliased to 'show --format='Fixes: %h ("%s")' -s'

BTW, if you use vim, add

noremap ;gs "zyiw:exe "new \| setlocal buftype=nofile
bufhidden=hide noswapfile syntax=git \| 0r ! git show ".@z."" \| :0<CR><CR>

to .vimrc, and type ";gs" when the cursor is positioned on a commit ID.

Quoted 14 lines

Signed-off-by Finn Thain <[email protected]>

Cc Michael Schmitz <[email protected]>

Tested-by Michael Schmitz <[email protected]>

drivers/scsi/zorro_esp.c +1 -1

@@ -245,7 +245,7 @@ static int fastlane_esp_irq_pending(struct esp *esp)
 static u32 zorro_esp_dma_length_limit(struct esp *esp, u32 dma_addr,
                                        u32 dma_len)
 {
-       return dma_len > 0xFFFFFF ? 0xFFFFFF : dma_len;
+       return dma_len > 0xFFFF ? 0xFFFF : dma_len;
 }
 

drivers/scsi/zorro_esp.c static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr, · line 487

Quoted 6 lines

drivers/scsi/zorro_esp.c +0 -1

 static void zorro_esp_reset_dma(struct esp *esp)

@@ -484,7 +484,6 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
        scsi_esp_cmd(esp, ESP_CMD_DMA);
        zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
        zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
-       zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);

Since all these sub-drivers seem to support 24-bit transfers, perhaps this is something to be fixed in the esp_scsi core?

If a target disconnects during a PIO data transfer the command may fail when the target reconnects:

scsi host1: DMA length is zero!
scsi host1: cur adr[04380000] len[00000000]

The scsi bus is then reset. This happens because the residual reached zero before the transfer was completed.

The usual residual calculation relies on the Transfer Count registers which works for DMA transfers but not for PIO transfers. Fix the problem by storing the PIO transfer residual and using that to correctly calculate bytes_sent.

Fixes 6fe07aaffbf0

Quoted 2 lines

Fixes 6fe07aaffbf0

Fixes 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")

Quoted 8 lines

Tested-by Stan Johnson <[email protected]>

Signed-off-by Finn Thain <[email protected]>

Tested-by Michael Schmitz <[email protected]>

drivers/scsi/esp_scsi.h +0 -0

@@ -540,6 +540,8 @@ struct esp {
 

drivers/scsi/esp_scsi.h struct esp { · line 544

Quoted 4 lines

drivers/scsi/esp_scsi.h +2 -0

        void                    *dma;
        int                     dmarev;
+
+       int                     send_cmd_residual;

unsigned int?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

drivers/scsi/esp_scsi.c static void esp_maybe_execute_command(struct esp *esp) · line 805

Quoted 1 line

drivers/scsi/esp_scsi.c +1 -0

+	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);

I think lp should always be non-NULL here.

Do we really need to make that distinction?

drivers/scsi/esp_scsi.c static void esp_maybe_execute_command(struct esp *esp) · line 805

Quoted 1 line

drivers/scsi/esp_scsi.c +1 -0

+	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);

I think lp should always be non-NULL here.

Quoted 4 lines · Christoph Hellwig

drivers/scsi/esp_scsi.c static void esp_maybe_execute_command(struct esp *esp) · line 805

Quoted 1 line

drivers/scsi/esp_scsi.c +1 -0

+	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);

I think lp should always be non-NULL here.

It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess that's true for scanning of targets.

Is it true in general that queuecommand for a device will not occur before its slave_alloc and not after its slave_destroy invocation?

(I'm thinking of queuecommand via the other command submission paths, like ioctl.)

Quoted 4 lines · Christoph Hellwig

drivers/scsi/esp_scsi.c static void esp_maybe_execute_command(struct esp *esp) · line 805

Quoted 1 line

drivers/scsi/esp_scsi.c +1 -0

+	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);

I think lp should always be non-NULL here.

It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess that's true for scanning of targets.

Is it true in general that queuecommand for a device will not occur before its slave_alloc and not after its slave_destroy invocation?

As a temporary measure, the code to implement PIO transfers was duplicated in zorro_esp and mac_esp. Now that it has stabilized move the common code into the core driver but don't build it unless needed.

This replaces the inline assembler with more portable writesb() calls. Optimizing the m68k writesb() implementation is a separate patch.

Tested-by Stan Johnson <[email protected]>

Signed-off-by Finn Thain <[email protected]>

Tested-by Michael Schmitz <[email protected]>

Quoted 7 lines

drivers/scsi/Kconfig +0 -0

@@ -42,6 +42,10 @@ config SCSI_DMA
        bool
        default n
 

drivers/scsi/Kconfig config SCSI_DMA · line 47

Quoted 3 lines

drivers/scsi/Kconfig +3 -0

+config SCSI_ESP_PIO
+       bool
+       default n

"default n" is the default, so please drop this line.

As a temporary measure, the code to implement PIO transfers was duplicated in zorro_esp and mac_esp. Now that it has stabilized move the common code into the core driver but don't build it unless needed.

This replaces the inline assembler with more portable writesb() calls. Optimizing the m68k writesb() implementation is a separate patch.

Tested-by Stan Johnson <[email protected]>

Signed-off-by Finn Thain <[email protected]>

Tested-by Michael Schmitz <[email protected]>

---
Changed since v1:
- Use shost_printk() instead of pr_err().
- Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.

---
  drivers/scsi/Kconfig     |   6 +
  drivers/scsi/esp_scsi.c  | 128 +++++++++++++++++++++
  drivers/scsi/esp_scsi.h  |   5 +
  drivers/scsi/mac_esp.c   | 173 +----------------------------
  drivers/scsi/zorro_esp.c | 232 ++++++---------------------------------
  5 files changed, 179 insertions(+), 365 deletions(-)

drivers/scsi/Kconfig +6 -0

index 35c909bbf8ba..81c6872f24f8 100644
@@ -42,6 +42,10 @@ config SCSI_DMA
  	bool
  	default n
 
+config SCSI_ESP_PIO
+	bool
+	default n
+
  config SCSI_NETLINK
  	bool
  	default	n

@@ -1355,6 +1359,7 @@ config SCSI_ZORRO_ESP
  	tristate "Zorro ESP SCSI support"
  	depends on ZORRO && SCSI
  	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
  	help
  	  Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
  	  expansion boards for the Amiga.

@@ -1397,6 +1402,7 @@ config SCSI_MAC_ESP
  	tristate "Macintosh NCR53c9[46] SCSI"
  	depends on MAC && SCSI
  	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
  	help
  	  This is the NCR 53c9x SCSI controller found on most of the 68040
  	  based Macintoshes.

drivers/scsi/esp_scsi.c +128 -0

index 6ccaf818357e..305a322ad13c 100644
@@ -2776,3 +2776,131 @@ MODULE_PARM_DESC(esp_debug,
 
  module_init(esp_init);
  module_exit(esp_exit);
+
+#ifdef CONFIG_SCSI_ESP_PIO
+static inline unsigned int esp_wait_for_fifo(struct esp *esp)
+{
+	int i = 500000;
+
+	do {
+		unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
+
+		if (fbytes)
+			return fbytes;
+
+		udelay(2);
+	} while (--i);
+
+	shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
+		     esp_read8(ESP_STATUS));
+	return 0;
+}
+
+static inline int esp_wait_for_intr(struct esp *esp)
+{
+	int i = 500000;
+
+	do {
+		esp->sreg = esp_read8(ESP_STATUS);
+		if (esp->sreg & ESP_STAT_INTR)
+			return 0;
+
+		udelay(2);
+	} while (--i);
+
+	shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
+		     esp->sreg);
+	return 1;
+}
+
+#define ESP_FIFO_SIZE 16
+
+void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+		      u32 dma_count, int write, u8 cmd)
+{
+	u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+	cmd &= ~ESP_CMD_DMA;
+	esp->send_cmd_error = 0;
+
+	if (write) {
+		u8 *dst = (u8 *)addr;
+		u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
+
+		scsi_esp_cmd(esp, cmd);
+
+		while (1) {
+			if (!esp_wait_for_fifo(esp))
+				break;
+
+			*dst++ = esp_read8(ESP_FDATA);
+			--esp_count;
+
+			if (!esp_count)
+				break;
+
+			if (esp_wait_for_intr(esp)) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
+				break;
+
+			esp->ireg = esp_read8(ESP_INTRPT);
+			if (esp->ireg & mask) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if (phase == ESP_MIP)
+				scsi_esp_cmd(esp, ESP_CMD_MOK);
+
+			scsi_esp_cmd(esp, ESP_CMD_TI);
+		}
+	} else {
+		unsigned int n = ESP_FIFO_SIZE;
+		u8 *src = (u8 *)addr;
+
+		scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+
+		if (n > esp_count)
+			n = esp_count;
+		writesb(esp->fifo_reg, src, n);
+		src += n;
+		esp_count -= n;
+
+		scsi_esp_cmd(esp, cmd);
+
+		while (esp_count) {
+			if (esp_wait_for_intr(esp)) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
+				break;
+
+			esp->ireg = esp_read8(ESP_INTRPT);
+			if (esp->ireg & ~ESP_INTR_BSERV) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			n = ESP_FIFO_SIZE -
+			    (esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES);
+
+			if (n > esp_count)
+				n = esp_count;
+			writesb(esp->fifo_reg, src, n);
+			src += n;
+			esp_count -= n;
+
+			scsi_esp_cmd(esp, ESP_CMD_TI);
+		}
+	}
+
+	esp->send_cmd_residual = esp_count;
+}
+EXPORT_SYMBOL(esp_send_pio_cmd);
+#endif

drivers/scsi/esp_scsi.h +1 -0

index d0c032803749..2590e5eda595 100644
@@ -431,6 +431,7 @@ struct esp_driver_ops {
  struct esp {
  	void __iomem		*regs;
  	void __iomem		*dma_regs;
+	u8 __iomem		*fifo_reg;
 
  	const struct esp_driver_ops *ops;
 

Quoted 200 lines · Finn Thain

As a temporary measure, the code to implement PIO transfers was duplicated in zorro_esp and mac_esp. Now that it has stabilized move the common code into the core driver but don't build it unless needed.

This replaces the inline assembler with more portable writesb() calls. Optimizing the m68k writesb() implementation is a separate patch.

Tested-by Stan Johnson <[email protected]>

Signed-off-by Finn Thain <[email protected]>

Tested-by Michael Schmitz <[email protected]>

---
Changed since v1:
- Use shost_printk() instead of pr_err().
- Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.

---
  drivers/scsi/Kconfig     |   6 +
  drivers/scsi/esp_scsi.c  | 128 +++++++++++++++++++++
  drivers/scsi/esp_scsi.h  |   5 +
  drivers/scsi/mac_esp.c   | 173 +----------------------------
  drivers/scsi/zorro_esp.c | 232 ++++++---------------------------------
  5 files changed, 179 insertions(+), 365 deletions(-)

drivers/scsi/Kconfig +5 -0

index 35c909bbf8ba..81c6872f24f8 100644
@@ -42,6 +42,10 @@ config SCSI_DMA
  	bool
  	default n
  +config SCSI_ESP_PIO
+	bool
+	default n
+
  config SCSI_NETLINK
  	bool
  	default	n

@@ -1355,6 +1359,7 @@ config SCSI_ZORRO_ESP
  	tristate "Zorro ESP SCSI support"
  	depends on ZORRO && SCSI
  	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
  	help
  	  Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
  	  expansion boards for the Amiga.

@@ -1397,6 +1402,7 @@ config SCSI_MAC_ESP
  	tristate "Macintosh NCR53c9[46] SCSI"
  	depends on MAC && SCSI
  	select SCSI_SPI_ATTRS
+	select SCSI_ESP_PIO
  	help
  	  This is the NCR 53c9x SCSI controller found on most of the 68040
  	  based Macintoshes.

drivers/scsi/esp_scsi.c +128 -0

index 6ccaf818357e..305a322ad13c 100644
@@ -2776,3 +2776,131 @@ MODULE_PARM_DESC(esp_debug,
    module_init(esp_init);
  module_exit(esp_exit);
+
+#ifdef CONFIG_SCSI_ESP_PIO
+static inline unsigned int esp_wait_for_fifo(struct esp *esp)
+{
+	int i = 500000;
+
+	do {
+		unsigned int fbytes = esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES;
+
+		if (fbytes)
+			return fbytes;
+
+		udelay(2);
+	} while (--i);
+
+	shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
+		     esp_read8(ESP_STATUS));
+	return 0;
+}
+
+static inline int esp_wait_for_intr(struct esp *esp)
+{
+	int i = 500000;
+
+	do {
+		esp->sreg = esp_read8(ESP_STATUS);
+		if (esp->sreg & ESP_STAT_INTR)
+			return 0;
+
+		udelay(2);
+	} while (--i);
+
+	shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
+		     esp->sreg);
+	return 1;
+}
+
+#define ESP_FIFO_SIZE 16
+
+void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+		      u32 dma_count, int write, u8 cmd)
+{
+	u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+	cmd &= ~ESP_CMD_DMA;
+	esp->send_cmd_error = 0;
+
+	if (write) {
+		u8 *dst = (u8 *)addr;
+		u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE :
 ESP_INTR_BSERV);
+
+		scsi_esp_cmd(esp, cmd);
+
+		while (1) {
+			if (!esp_wait_for_fifo(esp))
+				break;
+
+			*dst++ = esp_read8(ESP_FDATA);
+			--esp_count;
+
+			if (!esp_count)
+				break;
+
+			if (esp_wait_for_intr(esp)) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
+				break;
+
+			esp->ireg = esp_read8(ESP_INTRPT);
+			if (esp->ireg & mask) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if (phase == ESP_MIP)
+				scsi_esp_cmd(esp, ESP_CMD_MOK);
+
+			scsi_esp_cmd(esp, ESP_CMD_TI);
+		}
+	} else {
+		unsigned int n = ESP_FIFO_SIZE;
+		u8 *src = (u8 *)addr;
+
+		scsi_esp_cmd(esp, ESP_CMD_FLUSH);
+
+		if (n > esp_count)
+			n = esp_count;
+		writesb(esp->fifo_reg, src, n);
+		src += n;
+		esp_count -= n;
+
+		scsi_esp_cmd(esp, cmd);
+
+		while (esp_count) {
+			if (esp_wait_for_intr(esp)) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
+				break;
+
+			esp->ireg = esp_read8(ESP_INTRPT);
+			if (esp->ireg & ~ESP_INTR_BSERV) {
+				esp->send_cmd_error = 1;
+				break;
+			}
+
+			n = ESP_FIFO_SIZE -
+			    (esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES);
+
+			if (n > esp_count)
+				n = esp_count;
+			writesb(esp->fifo_reg, src, n);
+			src += n;
+			esp_count -= n;
+
+			scsi_esp_cmd(esp, ESP_CMD_TI);
+		}
+	}
+
+	esp->send_cmd_residual = esp_count;
+}
+EXPORT_SYMBOL(esp_send_pio_cmd);
+#endif

drivers/scsi/esp_scsi.h +1 -0

index d0c032803749..2590e5eda595 100644
@@ -431,6 +431,7 @@ struct esp_driver_ops {
  struct esp {
  	void __iomem		*regs;
  	void __iomem		*dma_regs;
+	u8 __iomem		*fifo_reg;
    	const struct esp_driver_ops *ops;
  @@ -540,6 +541,7 @@ struct esp {

Quoted 6 lines

drivers/scsi/esp_scsi.h +0 -0

  	void			*dma;

  	int			dmarev;
  +	int			send_cmd_error;
  	int			send_cmd_residual;
  };

These variables are only used within esp_send_pio_cmd(); consider making them conditional based on the config option.

drivers/scsi/esp_scsi.h extern void scsi_esp_unregister(struct esp *); · line 589

Quoted 8 lines

drivers/scsi/esp_scsi.h +2 -0

@@ -581,4 +583,7 @@ extern void scsi_esp_unregister(struct esp *);
  extern irqreturn_t scsi_esp_intr(int, void *);
  extern void scsi_esp_cmd(struct esp *, u8);
  +extern void esp_send_pio_cmd(struct esp *esp, u32 dma_addr, u32
 esp_count,
+			     u32 dma_count, int write, u8 cmd);
+
  #endif /* !(_ESP_SCSI_H) */

Same here; the declaration only makes sense when the config option is set.

Cheers,

Hannes

Quoted 24 lines · Hannes Reinecke

Quoted 23 lines · Finn Thain

As a temporary measure, the code to implement PIO transfers was duplicated in zorro_esp and mac_esp. Now that it has stabilized move the common code into the core driver but don't build it unless needed.

This replaces the inline assembler with more portable writesb() calls. Optimizing the m68k writesb() implementation is a separate patch.

Tested-by Stan Johnson <[email protected]>

Signed-off-by Finn Thain <[email protected]>

Tested-by Michael Schmitz <[email protected]>

---
Changed since v1:
- Use shost_printk() instead of pr_err().
- Add new symbol CONFIG_SCSI_ESP_PIO to Kconfig.

---
   drivers/scsi/Kconfig     |   6 +
   drivers/scsi/esp_scsi.c  | 128 +++++++++++++++++++++
   drivers/scsi/esp_scsi.h  |   5 +
   drivers/scsi/mac_esp.c   | 173 +----------------------------
   drivers/scsi/zorro_esp.c | 232 ++++++---------------------------------
   5 files changed, 179 insertions(+), 365 deletions(-)

Quoted 11 lines

Quoted 11 lines

drivers/scsi/esp_scsi.h +1 -0

index d0c032803749..2590e5eda595 100644
@@ -431,6 +431,7 @@ struct esp_driver_ops {
   struct esp {
   	void __iomem		*regs;
   	void __iomem		*dma_regs;
+	u8 __iomem		*fifo_reg;
     	const struct esp_driver_ops *ops;
   @@ -540,6 +541,7 @@ struct esp {

Quoted 9 lines

Quoted 6 lines

drivers/scsi/esp_scsi.h +0 -0

   	void			*dma;

   	int			dmarev;
   +	int			send_cmd_error;
   	int			send_cmd_residual;
   };

These variables are only used within esp_send_pio_cmd(); consider making them conditional based on the config option.

In the case of send_cmd_residual, that would mean a second #ifdef added to esp_data_bytes_sent() where it gets used. I'm happy to comply but I fear that all these #ifdefs may harm readability...

There are already other variables in struct esp that may go unused, such as dma_regs, that don't have #ifdefs to elide them. Are these also problematic in some way?

However, the function declaration really is a worry, as the actual function body only exists when the config option is enabled.

Quoted 5 lines

In the case of send_cmd_residual, that would mean a second #ifdef added to esp_data_bytes_sent() where it gets used. I'm happy to comply but I fear that all these #ifdefs may harm readability...

Quoted 4 lines

There are already other variables in struct esp that may go unused, such as dma_regs, that don't have #ifdefs to elide them. Are these also problematic in some way?

The unused fields in the struct are not so much an issue; in fact, it rather complicated things when having individual fields in the struct surrounded by CONFIG_XXX, as then the order of the fields would change depending on the configuration. Which makes it really hard to debug ..

However, the function declaration really is a worry, as the actual function body only exists when the config option is enabled. So either add a dummy function or surround the function declaration by CONFIG_ESP_PIO.
Otherwise I think Dan Carpenter and the likes are guaranteed to send you a nice mail complaining about this ...

Cheers,

Hannes

True enough. We agree that this #ifdef is undesirable. And yet when I tried it, I found an unexpected readability benefit to your suggestion:

#ifdef CONFIG_SCSI_ESP_PIO
        u8 __iomem              *fifo_reg;
        int                     send_cmd_error;
        u32                     send_cmd_residual;

#endif

This grouping does help convey the purpose of these struct members, even though the #ifdef is meant for the compiler not for the human reader.

So maybe it makes sense to group these definitions (they are all the same size):

        /* These are used by esp_scsi_send_pio_cmd() */
        u8 __iomem              *fifo_reg;
        int                     send_cmd_error;
        u32                     send_cmd_residual;

Do static checkers really complain about this? I think the validity of an extern can't be known until the final linkage is done.

However, the function declaration really is a worry, as the actual function body only exists when the config option is enabled.
So either add a dummy function or surround the function declaration by CONFIG_ESP_PIO.
Otherwise I think Dan Carpenter and the likes are guaranteed to send you a nice mail complaining about this ...

Perhaps I've misunderstood your concern here. Is it a problem that esp_scsi.ko may or may not export the function, depending on .config?

For example, if esp_scsi.ko came from a build with CONFIG_SUN3X_ESP=y && !CONFIG_SCSI_ZORRO_ESP && !CONFIG_SCSI_MAC_ESP, then it would export no esp_send_pio_cmd() symbol.

A dummy function (mentioned above) might then avoid a link error from "modprobe zorro_esp" or "modprobe mac_esp" in this scenario. (The modules would load but fail to work properly.)

This seems a bit too contrived so I'll post v3 for you to consider.

Please leave the possibly unused exports as-is.

This series has fixes and cleanup for mac_esp, zorro_esp and the core esp_scsi driver.

These improvements include elimination of duplicated code temporarily introduced for zorro_esp.