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.
Finn Thain (6):
zorro_esp: Limit DMA transfers to 65535 bytes
esp_scsi: Track residual for PIO transfers
esp_scsi: Grant disconnect privilege for untagged commands
esp_scsi: Eliminate ESP_FLAG_DOING_SLOWCMD
esp_scsi: De-duplicate PIO routines
esp_scsi: Optimize PIO loops
drivers/scsi/Kconfig | 6 +
drivers/scsi/esp_scsi.c | 194 ++++++++++++++++++++++++-------
drivers/scsi/esp_scsi.h | 10 +-
drivers/scsi/mac_esp.c | 171 +---------------------------
drivers/scsi/zorro_esp.c | 240 ++++++---------------------------------
5 files changed, 209 insertions(+), 412 deletions(-)
--
2.18.1
Avoid function calls in the inner PIO loops. On a Centris 660av this
improves throughput for sequential read transfers by about 40% and
sequential write by about 10%.
Unfortunately it is not possible to have method calls like esp_write8()
placed inline so this is always going to be slow (even with LTO).
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
Tested-by: Michael Schmitz <[email protected]>
---
Changed since v1:
- Don't touch the scsi_esp_cmd(esp, ESP_CMD_FLUSH) that's outside the loop.
---
drivers/scsi/esp_scsi.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 305a322ad13c..cdf55bd8562e 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2788,7 +2788,7 @@ static inline unsigned int esp_wait_for_fifo(struct esp *esp)
if (fbytes)
return fbytes;
- udelay(2);
+ udelay(1);
} while (--i);
shost_printk(KERN_ERR, esp->host, "FIFO is empty (sreg %02x)\n",
@@ -2805,7 +2805,7 @@ static inline int esp_wait_for_intr(struct esp *esp)
if (esp->sreg & ESP_STAT_INTR)
return 0;
- udelay(2);
+ udelay(1);
} while (--i);
shost_printk(KERN_ERR, esp->host, "IRQ timeout (sreg %02x)\n",
@@ -2833,7 +2833,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
if (!esp_wait_for_fifo(esp))
break;
- *dst++ = esp_read8(ESP_FDATA);
+ *dst++ = readb(esp->fifo_reg);
--esp_count;
if (!esp_count)
@@ -2854,9 +2854,9 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
}
if (phase == ESP_MIP)
- scsi_esp_cmd(esp, ESP_CMD_MOK);
+ esp_write8(ESP_CMD_MOK, ESP_CMD);
- scsi_esp_cmd(esp, ESP_CMD_TI);
+ esp_write8(ESP_CMD_TI, ESP_CMD);
}
} else {
unsigned int n = ESP_FIFO_SIZE;
@@ -2896,7 +2896,7 @@ void esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
src += n;
esp_count -= n;
- scsi_esp_cmd(esp, ESP_CMD_TI);
+ esp_write8(ESP_CMD_TI, ESP_CMD);
}
}
--
2.18.1
A SCSI device is not granted disconnect privilege by an esp_scsi host
unless that device has its simple_tags flag set. However, a device may
support disconnect/reselect and not support command queueing. Allow
these devices to disconnect and improve bus utilization.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
Tested-by: Michael Schmitz <[email protected]>
---
drivers/scsi/esp_scsi.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 9e5d3f7d29ae..b7c41aa9927c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -800,13 +800,9 @@ static void esp_maybe_execute_command(struct esp *esp)
build_identify:
/* If we don't have a lun-data struct yet, we're probing
- * so do not disconnect. Also, do not disconnect unless
- * we have a tag on this command.
+ * so do not disconnect.
*/
- if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
- *p++ = IDENTIFY(1, lun);
- else
- *p++ = IDENTIFY(0, lun);
+ *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
if (ent->tag[0] && esp->rev == ESP100) {
/* ESP100 lacks select w/atn3 command, use select
--
2.18.1
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(-)
diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 35c909bbf8ba..81c6872f24f8 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -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.
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 6ccaf818357e..305a322ad13c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -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
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index d0c032803749..2590e5eda595 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -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 {
void *dma;
int dmarev;
+ int send_cmd_error;
int send_cmd_residual;
};
@@ -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) */
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index 71879f2207e0..1becf6a195a2 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -52,7 +52,6 @@ struct mac_esp_priv {
struct esp *esp;
void __iomem *pdma_regs;
void __iomem *pdma_io;
- int error;
};
static struct esp *esp_chips[2];
static DEFINE_SPINLOCK(esp_chips_lock);
@@ -120,12 +119,11 @@ static void mac_esp_dma_invalidate(struct esp *esp)
static int mac_esp_dma_error(struct esp *esp)
{
- return MAC_ESP_GET_PRIV(esp)->error;
+ return esp->send_cmd_error;
}
static inline int mac_esp_wait_for_empty_fifo(struct esp *esp)
{
- struct mac_esp_priv *mep = MAC_ESP_GET_PRIV(esp);
int i = 500000;
do {
@@ -140,7 +138,7 @@ static inline int mac_esp_wait_for_empty_fifo(struct esp *esp)
printk(KERN_ERR PFX "FIFO is not empty (sreg %02x)\n",
esp_read8(ESP_STATUS));
- mep->error = 1;
+ esp->send_cmd_error = 1;
return 1;
}
@@ -166,7 +164,7 @@ static inline int mac_esp_wait_for_dreq(struct esp *esp)
printk(KERN_ERR PFX "PDMA timeout (sreg %02x)\n",
esp_read8(ESP_STATUS));
- mep->error = 1;
+ esp->send_cmd_error = 1;
return 1;
}
@@ -233,7 +231,7 @@ static void mac_esp_send_pdma_cmd(struct esp *esp, u32 addr, u32 esp_count,
{
struct mac_esp_priv *mep = MAC_ESP_GET_PRIV(esp);
- mep->error = 0;
+ esp->send_cmd_error = 0;
if (!write)
scsi_esp_cmd(esp, ESP_CMD_FLUSH);
@@ -271,166 +269,6 @@ static void mac_esp_send_pdma_cmd(struct esp *esp, u32 addr, u32 esp_count,
} while (esp_count);
}
-/*
- * Programmed IO routines follow.
- */
-
-static inline unsigned int mac_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);
-
- printk(KERN_ERR PFX "FIFO is empty (sreg %02x)\n",
- esp_read8(ESP_STATUS));
- return 0;
-}
-
-static inline int mac_esp_wait_for_intr(struct esp *esp)
-{
- struct mac_esp_priv *mep = MAC_ESP_GET_PRIV(esp);
- int i = 500000;
-
- do {
- esp->sreg = esp_read8(ESP_STATUS);
- if (esp->sreg & ESP_STAT_INTR)
- return 0;
-
- udelay(2);
- } while (--i);
-
- printk(KERN_ERR PFX "IRQ timeout (sreg %02x)\n", esp->sreg);
- mep->error = 1;
- return 1;
-}
-
-#define MAC_ESP_PIO_LOOP(operands, reg1) \
- asm volatile ( \
- "1: moveb " operands " \n" \
- " subqw #1,%1 \n" \
- " jbne 1b \n" \
- : "+a" (addr), "+r" (reg1) \
- : "a" (fifo))
-
-#define MAC_ESP_PIO_FILL(operands, reg1) \
- asm volatile ( \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " moveb " operands " \n" \
- " subqw #8,%1 \n" \
- " subqw #8,%1 \n" \
- : "+a" (addr), "+r" (reg1) \
- : "a" (fifo))
-
-#define MAC_ESP_FIFO_SIZE 16
-
-static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
- u32 dma_count, int write, u8 cmd)
-{
- struct mac_esp_priv *mep = MAC_ESP_GET_PRIV(esp);
- u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
- u8 phase = esp->sreg & ESP_STAT_PMASK;
-
- cmd &= ~ESP_CMD_DMA;
- mep->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 (!mac_esp_wait_for_fifo(esp))
- break;
-
- *dst++ = esp_read8(ESP_FDATA);
- --esp_count;
-
- if (!esp_count)
- break;
-
- if (mac_esp_wait_for_intr(esp))
- break;
-
- if ((esp->sreg & ESP_STAT_PMASK) != phase)
- break;
-
- esp->ireg = esp_read8(ESP_INTRPT);
- if (esp->ireg & mask) {
- mep->error = 1;
- break;
- }
-
- if (phase == ESP_MIP)
- scsi_esp_cmd(esp, ESP_CMD_MOK);
-
- scsi_esp_cmd(esp, ESP_CMD_TI);
- }
- } else {
- scsi_esp_cmd(esp, ESP_CMD_FLUSH);
-
- if (esp_count >= MAC_ESP_FIFO_SIZE)
- MAC_ESP_PIO_FILL("%0@+,%2@", esp_count);
- else
- MAC_ESP_PIO_LOOP("%0@+,%2@", esp_count);
-
- scsi_esp_cmd(esp, cmd);
-
- while (esp_count) {
- unsigned int n;
-
- if (mac_esp_wait_for_intr(esp))
- break;
-
- if ((esp->sreg & ESP_STAT_PMASK) != phase)
- break;
-
- esp->ireg = esp_read8(ESP_INTRPT);
- if (esp->ireg & ~ESP_INTR_BSERV) {
- mep->error = 1;
- break;
- }
-
- n = MAC_ESP_FIFO_SIZE -
- (esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES);
- if (n > esp_count)
- n = esp_count;
-
- if (n == MAC_ESP_FIFO_SIZE) {
- MAC_ESP_PIO_FILL("%0@+,%2@", esp_count);
- } else {
- esp_count -= n;
- MAC_ESP_PIO_LOOP("%0@+,%2@", n);
- }
-
- scsi_esp_cmd(esp, ESP_CMD_TI);
- }
- }
-
- esp->send_cmd_residual = esp_count;
-}
-
static int mac_esp_irq_pending(struct esp *esp)
{
if (esp_read8(ESP_STATUS) & ESP_STAT_INTR)
@@ -553,6 +391,7 @@ static int esp_mac_probe(struct platform_device *dev)
mep->pdma_regs = NULL;
break;
}
+ esp->fifo_reg = esp->regs + ESP_FDATA * 16;
esp->ops = &mac_esp_ops;
if (mep->pdma_io == NULL) {
@@ -560,7 +399,7 @@ static int esp_mac_probe(struct platform_device *dev)
esp_write8(0, ESP_TCLOW);
esp_write8(0, ESP_TCMED);
esp->flags = ESP_FLAG_DISABLE_SYNC;
- mac_esp_ops.send_dma_cmd = mac_esp_send_pio_cmd;
+ mac_esp_ops.send_dma_cmd = esp_send_pio_cmd;
} else {
printk(KERN_INFO PFX "using PDMA for controller %d\n", dev->id);
}
diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index be79127db594..88e2bbc509ba 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -9,8 +9,6 @@
*
* Copyright (C) 2013 Tuomas Vainikka ([email protected]) for
* Blizzard 1230 DMA and probe function fixes
- *
- * Copyright (C) 2017 Finn Thain for PIO code from Mac ESP driver adapted here
*/
/*
* ZORRO bus code from:
@@ -159,7 +157,6 @@ struct fastlane_dma_registers {
struct zorro_esp_priv {
struct esp *esp; /* our ESP instance - for Scsi_host* */
void __iomem *board_base; /* virtual address (Zorro III board) */
- int error; /* PIO error flag */
int zorro3; /* board is Zorro III */
unsigned char ctrl_data; /* shadow copy of ctrl_reg */
};
@@ -274,192 +271,29 @@ static void fastlane_esp_dma_invalidate(struct esp *esp)
z_writel(0, zep->board_base);
}
-/*
- * Programmed IO routines follow.
- */
-
-static inline unsigned int zorro_esp_wait_for_fifo(struct esp *esp)
-{
- int i = 500000;
-
- do {
- unsigned int fbytes = zorro_esp_read8(esp, ESP_FFLAGS)
- & ESP_FF_FBYTES;
-
- if (fbytes)
- return fbytes;
-
- udelay(2);
- } while (--i);
-
- pr_err("FIFO is empty (sreg %02x)\n",
- zorro_esp_read8(esp, ESP_STATUS));
- return 0;
-}
-
-static inline int zorro_esp_wait_for_intr(struct esp *esp)
-{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
- int i = 500000;
-
- do {
- esp->sreg = zorro_esp_read8(esp, ESP_STATUS);
- if (esp->sreg & ESP_STAT_INTR)
- return 0;
-
- udelay(2);
- } while (--i);
-
- pr_err("IRQ timeout (sreg %02x)\n", esp->sreg);
- zep->error = 1;
- return 1;
-}
-
-/*
- * PIO macros as used in mac_esp.c.
- * Note that addr and fifo arguments are local-scope variables declared
- * in zorro_esp_send_pio_cmd(), the macros are only used in that function,
- * and addr and fifo are referenced in each use of the macros so there
- * is no need to pass them as macro parameters.
- */
-#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
- asm volatile ( \
- "1: moveb " operands "\n" \
- " subqw #1,%1 \n" \
- " jbne 1b \n" \
- : "+a" (addr), "+r" (reg1) \
- : "a" (fifo));
-
-#define ZORRO_ESP_PIO_FILL(operands, reg1) \
- asm volatile ( \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " moveb " operands "\n" \
- " subqw #8,%1 \n" \
- " subqw #8,%1 \n" \
- : "+a" (addr), "+r" (reg1) \
- : "a" (fifo));
-
-#define ZORRO_ESP_FIFO_SIZE 16
-
-static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
- u32 dma_count, int write, u8 cmd)
-{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
- u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
- u8 phase = esp->sreg & ESP_STAT_PMASK;
-
- cmd &= ~ESP_CMD_DMA;
-
- 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 (!zorro_esp_wait_for_fifo(esp))
- break;
-
- *dst++ = zorro_esp_read8(esp, ESP_FDATA);
- --esp_count;
-
- if (!esp_count)
- break;
-
- if (zorro_esp_wait_for_intr(esp))
- break;
-
- if ((esp->sreg & ESP_STAT_PMASK) != phase)
- break;
-
- esp->ireg = zorro_esp_read8(esp, ESP_INTRPT);
- if (esp->ireg & mask) {
- zep->error = 1;
- break;
- }
-
- if (phase == ESP_MIP)
- scsi_esp_cmd(esp, ESP_CMD_MOK);
-
- scsi_esp_cmd(esp, ESP_CMD_TI);
- }
- } else { /* unused, as long as we only handle MIP here */
- scsi_esp_cmd(esp, ESP_CMD_FLUSH);
-
- if (esp_count >= ZORRO_ESP_FIFO_SIZE)
- ZORRO_ESP_PIO_FILL("%0@+,%2@", esp_count)
- else
- ZORRO_ESP_PIO_LOOP("%0@+,%2@", esp_count)
-
- scsi_esp_cmd(esp, cmd);
-
- while (esp_count) {
- unsigned int n;
-
- if (zorro_esp_wait_for_intr(esp))
- break;
-
- if ((esp->sreg & ESP_STAT_PMASK) != phase)
- break;
-
- esp->ireg = zorro_esp_read8(esp, ESP_INTRPT);
- if (esp->ireg & ~ESP_INTR_BSERV) {
- zep->error = 1;
- break;
- }
-
- n = ZORRO_ESP_FIFO_SIZE -
- (zorro_esp_read8(esp, ESP_FFLAGS) & ESP_FF_FBYTES);
- if (n > esp_count)
- n = esp_count;
-
- if (n == ZORRO_ESP_FIFO_SIZE)
- ZORRO_ESP_PIO_FILL("%0@+,%2@", esp_count)
- else {
- esp_count -= n;
- ZORRO_ESP_PIO_LOOP("%0@+,%2@", n)
- }
-
- scsi_esp_cmd(esp, ESP_CMD_TI);
- }
- }
-}
-
/* Blizzard 1230/60 SCSI-IV DMA */
static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
u32 esp_count, u32 dma_count, int write, u8 cmd)
{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
struct blz1230_dma_registers __iomem *dregs = esp->dma_regs;
u8 phase = esp->sreg & ESP_STAT_PMASK;
- zep->error = 0;
/*
* Use PIO if transferring message bytes to esp->command_block_dma.
* PIO requires a virtual address, so substitute esp->command_block
* for addr.
*/
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ /* Clear the results of a possible prior esp->ops->send_dma_cmd() */
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
if (write)
/* DMA receive */
dma_sync_single_for_device(esp->dev, addr, esp_count,
@@ -493,18 +327,19 @@ static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
u32 esp_count, u32 dma_count, int write, u8 cmd)
{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
struct blz1230II_dma_registers __iomem *dregs = esp->dma_regs;
u8 phase = esp->sreg & ESP_STAT_PMASK;
- zep->error = 0;
/* Use PIO if transferring message bytes to esp->command_block_dma */
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
if (write)
/* DMA receive */
dma_sync_single_for_device(esp->dev, addr, esp_count,
@@ -537,18 +372,19 @@ static void zorro_esp_send_blz1230II_dma_cmd(struct esp *esp, u32 addr,
static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
u32 esp_count, u32 dma_count, int write, u8 cmd)
{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
struct blz2060_dma_registers __iomem *dregs = esp->dma_regs;
u8 phase = esp->sreg & ESP_STAT_PMASK;
- zep->error = 0;
/* Use PIO if transferring message bytes to esp->command_block_dma */
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
if (write)
/* DMA receive */
dma_sync_single_for_device(esp->dev, addr, esp_count,
@@ -586,14 +422,16 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
u8 phase = esp->sreg & ESP_STAT_PMASK;
unsigned char *ctrl_data = &zep->ctrl_data;
- zep->error = 0;
/* Use PIO if transferring message bytes to esp->command_block_dma */
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
@@ -631,18 +469,19 @@ static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
static void zorro_esp_send_cyberII_dma_cmd(struct esp *esp, u32 addr,
u32 esp_count, u32 dma_count, int write, u8 cmd)
{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
struct cyberII_dma_registers __iomem *dregs = esp->dma_regs;
u8 phase = esp->sreg & ESP_STAT_PMASK;
- zep->error = 0;
/* Use PIO if transferring message bytes to esp->command_block_dma */
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
@@ -676,14 +515,16 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
u8 phase = esp->sreg & ESP_STAT_PMASK;
unsigned char *ctrl_data = &zep->ctrl_data;
- zep->error = 0;
/* Use PIO if transferring message bytes to esp->command_block_dma */
if (phase == ESP_MIP && addr == esp->command_block_dma) {
- zorro_esp_send_pio_cmd(esp, (u32) esp->command_block,
- esp_count, dma_count, write, cmd);
+ esp_send_pio_cmd(esp, (u32)esp->command_block, esp_count,
+ dma_count, write, cmd);
return;
}
+ esp->send_cmd_error = 0;
+ esp->send_cmd_residual = 0;
+
zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
@@ -718,14 +559,7 @@ static void zorro_esp_send_fastlane_dma_cmd(struct esp *esp, u32 addr,
static int zorro_esp_dma_error(struct esp *esp)
{
- struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
-
- /* check for error in case we've been doing PIO */
- if (zep->error == 1)
- return 1;
-
- /* do nothing - there seems to be no way to check for DMA errors */
- return 0;
+ return esp->send_cmd_error;
}
/* per-board ESP driver ops */
@@ -1033,6 +867,8 @@ static int zorro_esp_probe(struct zorro_dev *z,
goto fail_unmap_fastlane;
}
+ esp->fifo_reg = esp->regs + ESP_FDATA * 4;
+
/* Check whether a Blizzard 12x0 or CyberstormII really has SCSI */
if (zdd->scsi_option) {
zorro_esp_write8(esp, (ESP_CONFIG1_PENABLE | 7), ESP_CFG1);
--
2.18.1
The concept of a 'slow command' as it appears in esp_scsi is confusing
because it could refer to an ESP command or a SCSI command. It turns out
that it refers to a particular ESP select command which the driver also
tracks as 'ESP_SELECT_MSGOUT'. For readability, it is better to use the
terminology from the datasheets.
The global ESP_FLAG_DOING_SLOWCMD flag is redundant anyway, as it can be
inferred from esp->select_state. Remove the ESP_FLAG_DOING_SLOWCMD cruft
and just use a boolean local variable.
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
Tested-by: Michael Schmitz <[email protected]>
---
drivers/scsi/esp_scsi.c | 57 +++++++++++++++++------------------------
drivers/scsi/esp_scsi.h | 3 +--
2 files changed, 24 insertions(+), 36 deletions(-)
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index b7c41aa9927c..6ccaf818357e 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -478,17 +478,6 @@ static void esp_restore_pointers(struct esp *esp, struct esp_cmd_entry *ent)
spriv->tot_residue = ent->saved_tot_residue;
}
-static void esp_check_command_len(struct esp *esp, struct scsi_cmnd *cmd)
-{
- if (cmd->cmd_len == 6 ||
- cmd->cmd_len == 10 ||
- cmd->cmd_len == 12) {
- esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
- } else {
- esp->flags |= ESP_FLAG_DOING_SLOWCMD;
- }
-}
-
static void esp_write_tgt_config3(struct esp *esp, int tgt)
{
if (esp->rev > ESP100A) {
@@ -721,6 +710,7 @@ static void esp_maybe_execute_command(struct esp *esp)
struct scsi_device *dev;
struct scsi_cmnd *cmd;
struct esp_cmd_entry *ent;
+ bool select_and_stop = false;
int tgt, lun, i;
u32 val, start_cmd;
u8 *p;
@@ -752,7 +742,8 @@ static void esp_maybe_execute_command(struct esp *esp)
esp_map_dma(esp, cmd);
esp_save_pointers(esp, ent);
- esp_check_command_len(esp, cmd);
+ if (!(cmd->cmd_len == 6 || cmd->cmd_len == 10 || cmd->cmd_len == 12))
+ select_and_stop = true;
p = esp->command_block;
@@ -793,9 +784,9 @@ static void esp_maybe_execute_command(struct esp *esp)
tp->flags &= ~ESP_TGT_CHECK_NEGO;
}
- /* Process it like a slow command. */
- if (tp->flags & (ESP_TGT_NEGO_WIDE | ESP_TGT_NEGO_SYNC))
- esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+ /* If there are multiple message bytes, use Select and Stop */
+ if (esp->msg_out_len)
+ select_and_stop = true;
}
build_identify:
@@ -808,23 +799,10 @@ static void esp_maybe_execute_command(struct esp *esp)
/* ESP100 lacks select w/atn3 command, use select
* and stop instead.
*/
- esp->flags |= ESP_FLAG_DOING_SLOWCMD;
+ select_and_stop = true;
}
- if (!(esp->flags & ESP_FLAG_DOING_SLOWCMD)) {
- start_cmd = ESP_CMD_SELA;
- if (ent->tag[0]) {
- *p++ = ent->tag[0];
- *p++ = ent->tag[1];
-
- start_cmd = ESP_CMD_SA3;
- }
-
- for (i = 0; i < cmd->cmd_len; i++)
- *p++ = cmd->cmnd[i];
-
- esp->select_state = ESP_SELECT_BASIC;
- } else {
+ if (select_and_stop) {
esp->cmd_bytes_left = cmd->cmd_len;
esp->cmd_bytes_ptr = &cmd->cmnd[0];
@@ -839,6 +817,19 @@ static void esp_maybe_execute_command(struct esp *esp)
start_cmd = ESP_CMD_SELAS;
esp->select_state = ESP_SELECT_MSGOUT;
+ } else {
+ start_cmd = ESP_CMD_SELA;
+ if (ent->tag[0]) {
+ *p++ = ent->tag[0];
+ *p++ = ent->tag[1];
+
+ start_cmd = ESP_CMD_SA3;
+ }
+
+ for (i = 0; i < cmd->cmd_len; i++)
+ *p++ = cmd->cmnd[i];
+
+ esp->select_state = ESP_SELECT_BASIC;
}
val = tgt;
if (esp->rev == FASHME)
@@ -1248,7 +1239,6 @@ static int esp_finish_select(struct esp *esp)
esp_unmap_dma(esp, cmd);
esp_free_lun_tag(ent, cmd->device->hostdata);
tp->flags &= ~(ESP_TGT_NEGO_SYNC | ESP_TGT_NEGO_WIDE);
- esp->flags &= ~ESP_FLAG_DOING_SLOWCMD;
esp->cmd_bytes_ptr = NULL;
esp->cmd_bytes_left = 0;
} else {
@@ -1299,9 +1289,8 @@ static int esp_finish_select(struct esp *esp)
esp_flush_fifo(esp);
}
- /* If we are doing a slow command, negotiation, etc.
- * we'll do the right thing as we transition to the
- * next phase.
+ /* If we are doing a Select And Stop command, negotiation, etc.
+ * we'll do the right thing as we transition to the next phase.
*/
esp_event(esp, ESP_EVENT_CHECK_PHASE);
return 0;
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index db4b6ea94caa..d0c032803749 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -490,7 +490,6 @@ struct esp {
u32 flags;
#define ESP_FLAG_DIFFERENTIAL 0x00000001
#define ESP_FLAG_RESETTING 0x00000002
-#define ESP_FLAG_DOING_SLOWCMD 0x00000004
#define ESP_FLAG_WIDE_CAPABLE 0x00000008
#define ESP_FLAG_QUICKIRQ_CHECK 0x00000010
#define ESP_FLAG_DISABLE_SYNC 0x00000020
@@ -532,7 +531,7 @@ struct esp {
u32 min_period;
u32 radelay;
- /* Slow command state. */
+ /* ESP_CMD_SELAS command state */
u8 *cmd_bytes_ptr;
int cmd_bytes_left;
--
2.18.1
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(-)
diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
index bb70882e6b56..be79127db594 100644
--- a/drivers/scsi/zorro_esp.c
+++ b/drivers/scsi/zorro_esp.c
@@ -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 */
--
2.18.1
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
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
Tested-by: Michael Schmitz <[email protected]>
---
drivers/scsi/esp_scsi.c | 1 +
drivers/scsi/esp_scsi.h | 2 ++
drivers/scsi/mac_esp.c | 2 ++
3 files changed, 5 insertions(+)
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index c3fc34b9964d..9e5d3f7d29ae 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1338,6 +1338,7 @@ static int esp_data_bytes_sent(struct esp *esp, struct esp_cmd_entry *ent,
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
+ bytes_sent -= esp->send_cmd_residual;
/*
* The am53c974 has a DMA 'pecularity'. The doc states:
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 8163dca2071b..db4b6ea94caa 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -540,6 +540,8 @@ struct esp {
void *dma;
int dmarev;
+
+ int send_cmd_residual;
};
/* A front-end driver for the ESP chip should do the following in
diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index eb551f3cc471..71879f2207e0 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -427,6 +427,8 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
scsi_esp_cmd(esp, ESP_CMD_TI);
}
}
+
+ esp->send_cmd_residual = esp_count;
}
static int mac_esp_irq_pending(struct esp *esp)
--
2.18.1
Hi Finn,
thanks for spotting this!
Am 14.10.2018 um 19:12 schrieb Finn Thain:
> 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]>
Reviewed-by: Michael Schmitz <[email protected]>
> ---
> drivers/scsi/zorro_esp.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> index bb70882e6b56..be79127db594 100644
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -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 */
>
Hi Finn,
On Sun, Oct 14, 2018 at 8:36 AM Finn Thain <[email protected]> wrote:
> 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.
Thanks for your patch!
> 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.
> Signed-off-by: Finn Thain <[email protected]>
> Cc: Michael Schmitz <[email protected]>
> Tested-by: Michael Schmitz <[email protected]>
> --- a/drivers/scsi/zorro_esp.c
> +++ b/drivers/scsi/zorro_esp.c
> @@ -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);
Since all these sub-drivers seem to support 24-bit transfers, perhaps this is
something to be fixed in the esp_scsi core?
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
Hi Finn,
On Sun, Oct 14, 2018 at 8:36 AM Finn Thain <[email protected]> wrote:
> 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.
Thanks for yur patch!
> Fixes: 6fe07aaffbf0
Fixes: 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")
> Tested-by: Stan Johnson <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
> Tested-by: Michael Schmitz <[email protected]>
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -540,6 +540,8 @@ struct esp {
>
> 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
Hi Finn,
On Sun, Oct 14, 2018 at 8:35 AM Finn Thain <[email protected]> wrote:
> 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]>
Thanks for your patch!
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -42,6 +42,10 @@ config SCSI_DMA
> bool
> default n
>
> +config SCSI_ESP_PIO
> + bool
> + default n
"default n" is the default, so please drop this line.
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
> + *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
I think lp should always be non-NULL here.
On 15/10/18 04:47, Christoph Hellwig wrote:
>> + *p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> I think lp should always be non-NULL here.
That indeed appears to be the case these days.
So we can't rely on !lp to detect when probing the bus any longer. What
else would be available? Do commands used for device probing also have a
tag these days by default?
Do we really need to make that distinction?
Cheers,
Michael
On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:
>
> > Fixes: 3109e5ae0311
>
> Fixes: 3109e5ae0311 ("scsi: zorro_esp: New driver for Amiga Zorro
> NCR53C9x boards")
>
OK.
> $ 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.
>
Thanks.
> > Signed-off-by: Finn Thain <[email protected]>
> > Cc: Michael Schmitz <[email protected]>
> > Tested-by: Michael Schmitz <[email protected]>
>
> > --- a/drivers/scsi/zorro_esp.c
> > +++ b/drivers/scsi/zorro_esp.c
> > @@ -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);
>
> Since all these sub-drivers seem to support 24-bit transfers, perhaps this is
> something to be fixed in the esp_scsi core?
>
I don't think there is anything to fix in the esp_scsi core. The fact that
no-one saw the error indicates that large transfers don't occur in
practice.
If you said there is an opportunity for an enhancement, I could agree,
assuming that you also found a way to produce large IO requests. But I
doubt that such an enhancement would make a measurable improvement.
Even if the benefit was measurable, the fix under review would still be
needed for stable kernels.
BTW, Michael and I already discussed all this (it probably should have
happened on the linux-m68k list).
Please see,
$ grep -lr ESP_CONFIG2_FENAB drivers/scsi/
drivers/scsi/am53c974.c
drivers/scsi/esp_scsi.c
drivers/scsi/esp_scsi.h
The authors of am53c974.c obviously decided it wasn't wise to make this
feature mandatory (which suggests that perhaps it shouldn't go into common
code).
The comments in esp_scsi.c say that ESP_CONFIG2_FENAB has undesirable
consequences. There is information in the datasheets on this point but I
didn't follow it up because I don't see a performance issue.
--
On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:
>
> > Fixes: 6fe07aaffbf0
>
> Fixes: 6fe07aaffbf0 ("[SCSI] m68k: new mac_esp scsi driver")
>
Fixed.
> > Tested-by: Stan Johnson <[email protected]>
> > Signed-off-by: Finn Thain <[email protected]>
> > Tested-by: Michael Schmitz <[email protected]>
>
> > --- a/drivers/scsi/esp_scsi.h
> > +++ b/drivers/scsi/esp_scsi.h
> > @@ -540,6 +540,8 @@ struct esp {
> >
> > void *dma;
> > int dmarev;
> > +
> > + int send_cmd_residual;
>
> unsigned int?
>
My first thought was to use u32, same as esp_count. But it turns out that
the end result really is an int --
static int esp_data_bytes_sent(struct esp *esp, struct esp_cmd_entry *ent,
struct scsi_cmnd *cmd)
{
int fifo_cnt, ecount, bytes_sent, flush_fifo;
...
bytes_sent = esp->data_dma_len;
bytes_sent -= ecount;
bytes_sent -= esp->send_cmd_residual;
...
return bytes_sent;
}
Apparently over/underflow is a real possibility, because there is a test
for this in esp_process_event().
--
> 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
>
On Mon, 15 Oct 2018, Michael Schmitz wrote:
>
> Do we really need to make that distinction?
>
esp_reconnect() dereferences lp, so !lp has to inhibit disconnection.
At least, that's my reading. I can't see any other reason for the lp
conditional.
--
On Sun, 14 Oct 2018, Christoph Hellwig wrote:
> > + *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.)
--
On Sun, 14 Oct 2018, Geert Uytterhoeven wrote:
>
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -42,6 +42,10 @@ config SCSI_DMA
> > bool
> > default n
> >
> > +config SCSI_ESP_PIO
> > + bool
> > + default n
>
> "default n" is the default, so please drop this line.
>
Done. Thanks.
--
On Mon, 15 Oct 2018, Finn Thain wrote:
> On Sun, 14 Oct 2018, Christoph Hellwig wrote:
>
> > > + *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.)
>
Nevermind. From a closer reading of the Documentation, I see that it is
true in general.
--
On Mon, Oct 15, 2018 at 10:13:37AM +1100, Finn Thain wrote:
> On Sun, 14 Oct 2018, Christoph Hellwig wrote:
>
> > > + *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?
Yes.
On 10/14/18 8:12 AM, Finn Thain wrote:
> 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(-)
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 35c909bbf8ba..81c6872f24f8 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -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.
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index 6ccaf818357e..305a322ad13c 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -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
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index d0c032803749..2590e5eda595 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -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 {
> 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.
> @@ -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
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On Mon, 15 Oct 2018, Hannes Reinecke wrote:
> On 10/14/18 8:12 AM, Finn Thain wrote:
> > 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(-)
> >
> > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> > index 35c909bbf8ba..81c6872f24f8 100644
> > --- a/drivers/scsi/Kconfig
> > +++ b/drivers/scsi/Kconfig
> > @@ -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.
> > diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> > index 6ccaf818357e..305a322ad13c 100644
> > --- a/drivers/scsi/esp_scsi.c
> > +++ b/drivers/scsi/esp_scsi.c
> > @@ -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
> > diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> > index d0c032803749..2590e5eda595 100644
> > --- a/drivers/scsi/esp_scsi.h
> > +++ b/drivers/scsi/esp_scsi.h
> > @@ -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 {
> > 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?
Thanks for your review.
--
> > @@ -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
>
On 10/15/18 8:25 AM, Finn Thain wrote:
> On Mon, 15 Oct 2018, Hannes Reinecke wrote:
>
>> On 10/14/18 8:12 AM, Finn Thain wrote:
>>> 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(-)
>>>
[ .. ]
>>> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
>>> index d0c032803749..2590e5eda595 100644
>>> --- a/drivers/scsi/esp_scsi.h
>>> +++ b/drivers/scsi/esp_scsi.h
>>> @@ -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 {
>>> 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?
>
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
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
On Mon, Oct 15, 2018 at 12:58:42PM +0200, Hannes Reinecke wrote:
> However, the function declaration really is a worry, as the actual function
> body only exists when the config option is enabled.
Having a prototype without an implementation is no problem at all.
We have this case all over the kernel, either intentionally to keep
things simple or due to bitrot.
On Mon, 15 Oct 2018, Hannes Reinecke wrote:
> >
> > 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?
> >
> 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 ..
>
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;
> 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 ...
>
Do static checkers really complain about this? I think the validity of an
extern can't be known until the final linkage is done.
At that point the checker may complain that no compilation unit references
a symbol in a header.
But this would lead to false positives where a header file is shared by
separate programs which share library code but not macros.
--
> Cheers,
>
> Hannes
>
On Mon, 15 Oct 2018, Hannes Reinecke wrote:
>
> 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.
--
On Tue, Oct 16, 2018 at 10:52:26AM +1100, Finn Thain wrote:
> 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;
I like the grouping, and in fact the ifdef sounds fine to me as well.
> Do static checkers really complain about this? I think the validity of an
> extern can't be known until the final linkage is done.
None that I know of does, and as said before this patterns is very
common all over the kernel.
On Tue, Oct 16, 2018 at 04:39:07PM +1100, Finn Thain wrote:
>
> 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.
On Wed, 17 Oct 2018, Christoph Hellwig wrote:
>
> Please leave the possibly unused exports as-is.
>
I take that to mean "leave the conditional export as-is". Hence, I think
v3 (posted on the 16th) should address all of the concerns raised by
reviewers...
--
Finn,
> 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.
Had to hand-apply patch #5. Otherwise OK.
Applied to 4.20/scsi-queue. Thank you!
--
Martin K. Petersen Oracle Linux Engineering