[PATCH 0/7] NCR5380 drivers: fixes and improvements

[PATCH 0/7] NCR5380 drivers: fixes and improvements

[Finn Thain]

Among other improvements, this patch series fixes a data corruption bug
in the mac_scsi driver and a bug in the EH abort routine in the core
5380 driver.

For consistency I have ignored certain checkpatch.pl complaints about
the indentation in mac_scsi.c. The remaining complaints seem to be
false positives.

Some of these patches are not trivial to backport. Those patches have
been nominated for recent -stable branches only.


Finn Thain (7):
  Revert "scsi: ncr5380: Increase register polling limit"
  scsi: NCR5380: Always re-enable reselection interrupt
  scsi: NCR5380: Handle PDMA failure reliably
  scsi: mac_scsi: Increase PIO/PDMA transfer length threshold
  scsi: mac_scsi: Fix pseudo DMA implementation, take 2
  scsi: mac_scsi: Enable PDMA on Mac IIfx
  scsi: mac_scsi: Treat Last Byte Sent time-out as failure

 arch/m68k/include/asm/mac_pdma.h | 179 ++++++++++++++++++++++
 arch/m68k/mac/config.c           |  10 +-
 drivers/scsi/NCR5380.c           |  18 +--
 drivers/scsi/NCR5380.h           |   2 +-
 drivers/scsi/mac_scsi.c          | 249 +++++++++++--------------------
 5 files changed, 280 insertions(+), 178 deletions(-)
 create mode 100644 arch/m68k/include/asm/mac_pdma.h

-- 
2.21.0

[PATCH 6/7] scsi: mac_scsi: Enable PDMA on Mac IIfx

[Finn Thain]

Add support for Apple's custom "SCSI DMA" chip. This patch doesn't make
use of its DMA capability. Just the PDMA capability is sufficient to
improve sequential read throughput by a factor of 5.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Joshua Thompson <funaho@jurai.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/config.c  | 10 +++++++--
 drivers/scsi/mac_scsi.c | 47 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 39835ca5a474..611f73bfc87c 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -911,6 +911,10 @@ static const struct resource mac_scsi_iifx_rsrc[] __initconst = {
 		.flags = IORESOURCE_MEM,
 		.start = 0x50008000,
 		.end   = 0x50009FFF,
+	}, {
+		.flags = IORESOURCE_MEM,
+		.start = 0x50008000,
+		.end   = 0x50009FFF,
 	},
 };
 
@@ -1012,10 +1016,12 @@ int __init mac_platform_init(void)
 	case MAC_SCSI_IIFX:
 		/* Addresses from The Guide to Mac Family Hardware.
 		 * $5000 8000 - $5000 9FFF: SCSI DMA
+		 * $5000 A000 - $5000 BFFF: Alternate SCSI
 		 * $5000 C000 - $5000 DFFF: Alternate SCSI (DMA)
 		 * $5000 E000 - $5000 FFFF: Alternate SCSI (Hsk)
-		 * The SCSI DMA custom IC embeds the 53C80 core. mac_scsi does
-		 * not make use of its DMA or hardware handshaking logic.
+		 * The A/UX header file sys/uconfig.h says $50F0 8000.
+		 * The "SCSI DMA" custom IC embeds the 53C80 core and
+		 * supports Programmed IO, DMA and PDMA (hardware handshake).
 		 */
 		platform_device_register_simple("mac_scsi", 0,
 			mac_scsi_iifx_rsrc, ARRAY_SIZE(mac_scsi_iifx_rsrc));
diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index e83b47a7e4b5..2e503f06ac99 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -22,6 +22,7 @@
 #include <asm/hwtest.h>
 #include <asm/io.h>
 #include <asm/mac_pdma.h>
+#include <asm/macintosh.h>
 #include <asm/macints.h>
 #include <asm/setup.h>
 
@@ -90,11 +91,22 @@ static int __init mac_scsi_setup(char *str)
 __setup("mac5380=", mac_scsi_setup);
 #endif /* !MODULE */
 
+/* The "SCSI DMA" chip on the IIfx implements this register. */
+#define CTRL_REG                0x8
+#define CTRL_INTERRUPTS_ENABLE  BIT(1)
+#define CTRL_HANDSHAKE_MODE     BIT(3)
+
+static inline void write_ctrl_reg(struct NCR5380_hostdata *hostdata, u32 value)
+{
+	out_be32(hostdata->io + (CTRL_REG << 4), value);
+}
+
 static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
                                 unsigned char *dst, int len)
 {
 	u8 __iomem *s = hostdata->pdma_io + (INPUT_DATA_REG << 4);
 	unsigned char *d = dst;
+	int result = 0;
 
 	hostdata->pdma_residual = len;
 
@@ -103,6 +115,10 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
 		int bytes;
 
+		if (macintosh_config->ident == MAC_MODEL_IIFX)
+			write_ctrl_reg(hostdata, CTRL_HANDSHAKE_MODE |
+			                         CTRL_INTERRUPTS_ENABLE);
+
 		bytes = mac_pdma_recv(s, d, min(hostdata->pdma_residual, 512));
 
 		if (bytes > 0) {
@@ -111,7 +127,7 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 		}
 
 		if (hostdata->pdma_residual == 0)
-			return 0;
+			goto out;
 
 		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
 		                           BUS_AND_STATUS_REG, BASR_ACK,
@@ -119,7 +135,7 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 			scmd_printk(KERN_DEBUG, hostdata->connected,
 			            "%s: !REQ and !ACK\n", __func__);
 		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
-			return 0;
+			goto out;
 
 		if (bytes == 0)
 			udelay(MAC_PDMA_DELAY);
@@ -130,13 +146,18 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 		dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host,
 		         "%s: bus error (%d/%d)\n", __func__, d - dst, len);
 		NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-		return -1;
+		result = -1;
+		goto out;
 	}
 
 	scmd_printk(KERN_ERR, hostdata->connected,
 	            "%s: phase mismatch or !DRQ\n", __func__);
 	NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-	return -1;
+	result = -1;
+out:
+	if (macintosh_config->ident == MAC_MODEL_IIFX)
+		write_ctrl_reg(hostdata, CTRL_INTERRUPTS_ENABLE);
+	return result;
 }
 
 static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
@@ -144,6 +165,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 {
 	unsigned char *s = src;
 	u8 __iomem *d = hostdata->pdma_io + (OUTPUT_DATA_REG << 4);
+	int result = 0;
 
 	hostdata->pdma_residual = len;
 
@@ -152,6 +174,10 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
 		int bytes;
 
+		if (macintosh_config->ident == MAC_MODEL_IIFX)
+			write_ctrl_reg(hostdata, CTRL_HANDSHAKE_MODE |
+			                         CTRL_INTERRUPTS_ENABLE);
+
 		bytes = mac_pdma_send(s, d, min(hostdata->pdma_residual, 512));
 
 		if (bytes > 0) {
@@ -165,7 +191,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 			                          TCR_LAST_BYTE_SENT, HZ / 64) < 0)
 				scmd_printk(KERN_ERR, hostdata->connected,
 				            "%s: Last Byte Sent timeout\n", __func__);
-			return 0;
+			goto out;
 		}
 
 		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
@@ -174,7 +200,7 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 			scmd_printk(KERN_DEBUG, hostdata->connected,
 			            "%s: !REQ and !ACK\n", __func__);
 		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
-			return 0;
+			goto out;
 
 		if (bytes == 0)
 			udelay(MAC_PDMA_DELAY);
@@ -185,13 +211,18 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 		dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host,
 		         "%s: bus error (%d/%d)\n", __func__, s - src, len);
 		NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-		return -1;
+		result = -1;
+		goto out;
 	}
 
 	scmd_printk(KERN_ERR, hostdata->connected,
 	            "%s: phase mismatch or !DRQ\n", __func__);
 	NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-	return -1;
+	result = -1;
+out:
+	if (macintosh_config->ident == MAC_MODEL_IIFX)
+		write_ctrl_reg(hostdata, CTRL_INTERRUPTS_ENABLE);
+	return result;
 }
 
 static int macscsi_dma_xfer_len(struct NCR5380_hostdata *hostdata,
-- 
2.21.0

Re: [PATCH 6/7] scsi: mac_scsi: Enable PDMA on Mac IIfx

[Geert Uytterhoeven]

Hi Finn,

On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> Add support for Apple's custom "SCSI DMA" chip. This patch doesn't make
> use of its DMA capability. Just the PDMA capability is sufficient to
> improve sequential read throughput by a factor of 5.
>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Cc: Joshua Thompson <funaho@jurai.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

> ---
>  arch/m68k/mac/config.c  | 10 +++++++--

For the  m68k change to go in through the SCSI tree:
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH 7/7] scsi: mac_scsi: Treat Last Byte Sent time-out as failure

[Finn Thain]

A system bus error during a PDMA send operation can result in bytes being
lost. Theoretically that could cause the target to remain in DATA OUT
phase and the initiator (expecting a phase change) would time-out waiting
for the Last Byte Sent flag. Should that happen, fail the transfer so the
core driver will stop using PDMA with this target.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/mac_scsi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index 2e503f06ac99..68d4665112b5 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -188,9 +188,12 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 		if (hostdata->pdma_residual == 0) {
 			if (NCR5380_poll_politely(hostdata, TARGET_COMMAND_REG,
 			                          TCR_LAST_BYTE_SENT,
-			                          TCR_LAST_BYTE_SENT, HZ / 64) < 0)
+			                          TCR_LAST_BYTE_SENT,
+			                          HZ / 64) < 0) {
 				scmd_printk(KERN_ERR, hostdata->connected,
 				            "%s: Last Byte Sent timeout\n", __func__);
+				result = -1;
+			}
 			goto out;
 		}
 
-- 
2.21.0

[PATCH 3/7] scsi: NCR5380: Handle PDMA failure reliably

[Finn Thain]

A PDMA error is handled in the core driver by setting the device's
'borken' flag and aborting the command. Unfortunately, do_abort() is not
dependable. Perform a SCSI bus reset instead, to make sure that the
command fails and gets retried.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: stable@vger.kernel.org # v4.20+
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 08e3ea8159b3..d9fa9cf2fd8b 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1761,10 +1761,8 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 						scmd_printk(KERN_INFO, cmd,
 							"switching to slow handshake\n");
 						cmd->device->borken = 1;
-						sink = 1;
-						do_abort(instance);
-						cmd->result = DID_ERROR << 16;
-						/* XXX - need to source or sink data here, as appropriate */
+						do_reset(instance);
+						bus_reset_cleanup(instance);
 					}
 				} else {
 					/* Transfer a small chunk so that the
-- 
2.21.0

[PATCH 4/7] scsi: mac_scsi: Increase PIO/PDMA transfer length threshold

[Finn Thain]

Some targets introduce delays when handshaking the response to certain
commands. For example, a disk may send a 96-byte response to an INQUIRY
command (or a 24-byte response to a MODE SENSE command) too slowly.

Apparently the first 12 or 14 bytes are handshaked okay but then the
system bus error timeout is reached while transferring the next word.

Since the scsi bus phase hasn't changed, the driver then sets the target
borken flag to prevent further PDMA transfers. The driver also logs the
warning, "switching to slow handshake".

Raise the PDMA threshold to 512 bytes so that PIO transfers will be used
for these commands. This default is sufficiently low that PDMA will still
be used for READ and WRITE commands.

The existing threshold (16 bytes) was chosen more or less at random.
However, best performance requires the threshold to be as low as possible.
Those systems that don't need the PIO workaround at all may benefit from
mac_scsi.setup_use_pdma=1

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: stable@vger.kernel.org # v4.14+
Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/mac_scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index 8b4b5b1a13d7..ba1afcaadae8 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -52,7 +52,7 @@ static int setup_cmd_per_lun = -1;
 module_param(setup_cmd_per_lun, int, 0);
 static int setup_sg_tablesize = -1;
 module_param(setup_sg_tablesize, int, 0);
-static int setup_use_pdma = -1;
+static int setup_use_pdma = 512;
 module_param(setup_use_pdma, int, 0);
 static int setup_hostid = -1;
 module_param(setup_hostid, int, 0);
@@ -305,7 +305,7 @@ static int macscsi_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                 struct scsi_cmnd *cmd)
 {
 	if (hostdata->flags & FLAG_NO_PSEUDO_DMA ||
-	    cmd->SCp.this_residual < 16)
+	    cmd->SCp.this_residual < setup_use_pdma)
 		return 0;
 
 	return cmd->SCp.this_residual;
-- 
2.21.0

[PATCH 1/7] Revert "scsi: ncr5380: Increase register polling limit"

[Finn Thain]

This reverts commit 4822827a69d7cd3bc5a07b7637484ebd2cf88db6.

The purpose of that commit was to suppress a timeout warning message
which appeared to be caused by target latency. But suppressing the warning
is undesirable as the warning may indicate a messed up transfer count.

Another problem with that commit is that 15 ms is too long to keep
interrupts disabled as interrupt latency can cause system clock drift
and other problems.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: stable@vger.kernel.org
Fixes: 4822827a69d7 ("scsi: ncr5380: Increase register polling limit")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index efca509b92b0..5935fd6d1a05 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -235,7 +235,7 @@ struct NCR5380_cmd {
 #define NCR5380_PIO_CHUNK_SIZE		256
 
 /* Time limit (ms) to poll registers when IRQs are disabled, e.g. during PDMA */
-#define NCR5380_REG_POLL_TIME		15
+#define NCR5380_REG_POLL_TIME		10
 
 static inline struct scsi_cmnd *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr)
 {
-- 
2.21.0

[PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Finn Thain]

A system bus error during a PDMA transfer can mess up the calculation of
the transfer residual (the PDMA handshaking hardware lacks a byte
counter). This results in data corruption.

The algorithm in this patch anticipates a bus error by starting each
transfer with a MOVE.B instruction. If a bus error is caught the transfer
will be retried. If a bus error is caught later in the transfer (for a
MOVE.W instruction) the transfer gets failed and subsequent requests for
that target will use PIO instead of PDMA.

This avoids the "!REQ and !ACK" error so the severity level of that
message is reduced to KERN_DEBUG.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: stable@vger.kernel.org # v4.14+
Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
Reported-by: Chris Jones <chris@martin-jones.com>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
 drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------
 2 files changed, 226 insertions(+), 154 deletions(-)
 create mode 100644 arch/m68k/include/asm/mac_pdma.h

diff --git a/arch/m68k/include/asm/mac_pdma.h b/arch/m68k/include/asm/mac_pdma.h
new file mode 100644
index 000000000000..44e2fb6d4b1c
--- /dev/null
+++ b/arch/m68k/include/asm/mac_pdma.h
@@ -0,0 +1,179 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/* Copyright (C) 2019 Finn Thain */
+
+#ifndef _ASM_MAC_PDMA_H_
+#define _ASM_MAC_PDMA_H_
+
+#include <linux/delay.h>
+
+/*
+ * According to "Inside Macintosh: Devices", Mac OS requires disk drivers to
+ * specify the number of bytes between the delays expected from a SCSI target.
+ * This allows the operating system to "prevent bus errors when a target fails
+ * to deliver the next byte within the processor bus error timeout period."
+ * Linux SCSI drivers lack knowledge of the timing behaviour of SCSI targets
+ * so bus errors are unavoidable.
+ *
+ * If a MOVE.B instruction faults, we assume that zero bytes were transferred
+ * and simply retry. That assumption probably depends on target behaviour but
+ * seems to hold up okay. The NOP provides synchronization: without it the
+ * fault can sometimes occur after the program counter has moved past the
+ * offending instruction. Post-increment addressing can't be used.
+ */
+
+#define MOVE_BYTE(operands) \
+	asm volatile ( \
+		"1:     moveb " operands "     \n" \
+		"11:    nop                    \n" \
+		"       addq #1,%0             \n" \
+		"       subq #1,%1             \n" \
+		"40:                           \n" \
+		"                              \n" \
+		".section .fixup,\"ax\"        \n" \
+		".even                         \n" \
+		"90:    movel #1, %2           \n" \
+		"       jra 40b                \n" \
+		".previous                     \n" \
+		"                              \n" \
+		".section __ex_table,\"a\"     \n" \
+		".align  4                     \n" \
+		".long   1b,90b                \n" \
+		".long  11b,90b                \n" \
+		".previous                     \n" \
+		: "+a" (addr), "+r" (n), "+r" (result) : "a" (io))
+
+/*
+ * If a MOVE.W (or MOVE.L) instruction faults, it cannot be retried because
+ * the residual byte count would be uncertain. In that situation the MOVE_WORD
+ * macro clears n in the fixup section to abort the transfer.
+ */
+
+#define MOVE_WORD(operands) \
+	asm volatile ( \
+		"1:     movew " operands "     \n" \
+		"11:    nop                    \n" \
+		"       subq #2,%1             \n" \
+		"40:                           \n" \
+		"                              \n" \
+		".section .fixup,\"ax\"        \n" \
+		".even                         \n" \
+		"90:    movel #0, %1           \n" \
+		"       movel #2, %2           \n" \
+		"       jra 40b                \n" \
+		".previous                     \n" \
+		"                              \n" \
+		".section __ex_table,\"a\"     \n" \
+		".align  4                     \n" \
+		".long   1b,90b                \n" \
+		".long  11b,90b                \n" \
+		".previous                     \n" \
+		: "+a" (addr), "+r" (n), "+r" (result) : "a" (io))
+
+#define MOVE_16_WORDS(operands) \
+	asm volatile ( \
+		"1:     movew " operands "     \n" \
+		"2:     movew " operands "     \n" \
+		"3:     movew " operands "     \n" \
+		"4:     movew " operands "     \n" \
+		"5:     movew " operands "     \n" \
+		"6:     movew " operands "     \n" \
+		"7:     movew " operands "     \n" \
+		"8:     movew " operands "     \n" \
+		"9:     movew " operands "     \n" \
+		"10:    movew " operands "     \n" \
+		"11:    movew " operands "     \n" \
+		"12:    movew " operands "     \n" \
+		"13:    movew " operands "     \n" \
+		"14:    movew " operands "     \n" \
+		"15:    movew " operands "     \n" \
+		"16:    movew " operands "     \n" \
+		"17:    nop                    \n" \
+		"       subl  #32,%1           \n" \
+		"40:                           \n" \
+		"                              \n" \
+		".section .fixup,\"ax\"        \n" \
+		".even                         \n" \
+		"90:    movel #0, %1           \n" \
+		"       movel #2, %2           \n" \
+		"       jra 40b                \n" \
+		".previous                     \n" \
+		"                              \n" \
+		".section __ex_table,\"a\"     \n" \
+		".align  4                     \n" \
+		".long   1b,90b                \n" \
+		".long   2b,90b                \n" \
+		".long   3b,90b                \n" \
+		".long   4b,90b                \n" \
+		".long   5b,90b                \n" \
+		".long   6b,90b                \n" \
+		".long   7b,90b                \n" \
+		".long   8b,90b                \n" \
+		".long   9b,90b                \n" \
+		".long  10b,90b                \n" \
+		".long  11b,90b                \n" \
+		".long  12b,90b                \n" \
+		".long  13b,90b                \n" \
+		".long  14b,90b                \n" \
+		".long  15b,90b                \n" \
+		".long  16b,90b                \n" \
+		".long  17b,90b                \n" \
+		".previous                     \n" \
+		: "+a" (addr), "+r" (n), "+r" (result) : "a" (io))
+
+#define MAC_PDMA_DELAY		32
+
+static inline int mac_pdma_recv(void __iomem *io, unsigned char *start, int n)
+{
+	unsigned char *addr = start;
+	int result = 0;
+
+	if (n >= 1) {
+		MOVE_BYTE("%3@,%0@");
+		if (result)
+			goto out;
+	}
+	if (n >= 1 && ((unsigned long)addr & 1)) {
+		MOVE_BYTE("%3@,%0@");
+		if (result)
+			goto out;
+	}
+	while (n >= 32)
+		MOVE_16_WORDS("%3@,%0@+");
+	while (n >= 2)
+		MOVE_WORD("%3@,%0@+");
+	if (result)
+		return start - addr; /* Negated to indicate uncertain length */
+	if (n == 1)
+		MOVE_BYTE("%3@,%0@");
+out:
+	return addr - start;
+}
+
+static inline int mac_pdma_send(unsigned char *start, void __iomem *io, int n)
+{
+	unsigned char *addr = start;
+	int result = 0;
+
+	if (n >= 1) {
+		MOVE_BYTE("%0@,%3@");
+		if (result)
+			goto out;
+	}
+	if (n >= 1 && ((unsigned long)addr & 1)) {
+		MOVE_BYTE("%0@,%3@");
+		if (result)
+			goto out;
+	}
+	while (n >= 32)
+		MOVE_16_WORDS("%0@+,%3@");
+	while (n >= 2)
+		MOVE_WORD("%0@+,%3@");
+	if (result)
+		return start - addr; /* Negated to indicate uncertain length */
+	if (n == 1)
+		MOVE_BYTE("%0@,%3@");
+out:
+	return addr - start;
+}
+
+#endif /* _ASM_MAC_PDMA_H_ */
diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index ba1afcaadae8..e83b47a7e4b5 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -21,6 +21,7 @@
 
 #include <asm/hwtest.h>
 #include <asm/io.h>
+#include <asm/mac_pdma.h>
 #include <asm/macints.h>
 #include <asm/setup.h>
 
@@ -89,101 +90,47 @@ static int __init mac_scsi_setup(char *str)
 __setup("mac5380=", mac_scsi_setup);
 #endif /* !MODULE */
 
-/* Pseudo DMA asm originally by Ove Edlund */
-
-#define CP_IO_TO_MEM(s,d,n)				\
-__asm__ __volatile__					\
-    ("    cmp.w  #4,%2\n"				\
-     "    bls    8f\n"					\
-     "    move.w %1,%%d0\n"				\
-     "    neg.b  %%d0\n"				\
-     "    and.w  #3,%%d0\n"				\
-     "    sub.w  %%d0,%2\n"				\
-     "    bra    2f\n"					\
-     " 1: move.b (%0),(%1)+\n"				\
-     " 2: dbf    %%d0,1b\n"				\
-     "    move.w %2,%%d0\n"				\
-     "    lsr.w  #5,%%d0\n"				\
-     "    bra    4f\n"					\
-     " 3: move.l (%0),(%1)+\n"				\
-     "31: move.l (%0),(%1)+\n"				\
-     "32: move.l (%0),(%1)+\n"				\
-     "33: move.l (%0),(%1)+\n"				\
-     "34: move.l (%0),(%1)+\n"				\
-     "35: move.l (%0),(%1)+\n"				\
-     "36: move.l (%0),(%1)+\n"				\
-     "37: move.l (%0),(%1)+\n"				\
-     " 4: dbf    %%d0,3b\n"				\
-     "    move.w %2,%%d0\n"				\
-     "    lsr.w  #2,%%d0\n"				\
-     "    and.w  #7,%%d0\n"				\
-     "    bra    6f\n"					\
-     " 5: move.l (%0),(%1)+\n"				\
-     " 6: dbf    %%d0,5b\n"				\
-     "    and.w  #3,%2\n"				\
-     "    bra    8f\n"					\
-     " 7: move.b (%0),(%1)+\n"				\
-     " 8: dbf    %2,7b\n"				\
-     "    moveq.l #0, %2\n"				\
-     " 9: \n"						\
-     ".section .fixup,\"ax\"\n"				\
-     "    .even\n"					\
-     "91: moveq.l #1, %2\n"				\
-     "    jra 9b\n"					\
-     "94: moveq.l #4, %2\n"				\
-     "    jra 9b\n"					\
-     ".previous\n"					\
-     ".section __ex_table,\"a\"\n"			\
-     "   .align 4\n"					\
-     "   .long  1b,91b\n"				\
-     "   .long  3b,94b\n"				\
-     "   .long 31b,94b\n"				\
-     "   .long 32b,94b\n"				\
-     "   .long 33b,94b\n"				\
-     "   .long 34b,94b\n"				\
-     "   .long 35b,94b\n"				\
-     "   .long 36b,94b\n"				\
-     "   .long 37b,94b\n"				\
-     "   .long  5b,94b\n"				\
-     "   .long  7b,91b\n"				\
-     ".previous"					\
-     : "=a"(s), "=a"(d), "=d"(n)			\
-     : "0"(s), "1"(d), "2"(n)				\
-     : "d0")
-
 static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
                                 unsigned char *dst, int len)
 {
 	u8 __iomem *s = hostdata->pdma_io + (INPUT_DATA_REG << 4);
 	unsigned char *d = dst;
-	int n = len;
-	int transferred;
+
+	hostdata->pdma_residual = len;
 
 	while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 	                              BASR_DRQ | BASR_PHASE_MATCH,
 	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
-		CP_IO_TO_MEM(s, d, n);
+		int bytes;
+
+		bytes = mac_pdma_recv(s, d, min(hostdata->pdma_residual, 512));
 
-		transferred = d - dst - n;
-		hostdata->pdma_residual = len - transferred;
+		if (bytes > 0) {
+			d += bytes;
+			hostdata->pdma_residual -= bytes;
+		}
 
-		/* No bus error. */
-		if (n == 0)
+		if (hostdata->pdma_residual == 0)
 			return 0;
 
-		/* Target changed phase early? */
 		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
-		                           BUS_AND_STATUS_REG, BASR_ACK, BASR_ACK, HZ / 64) < 0)
-			scmd_printk(KERN_ERR, hostdata->connected,
+		                           BUS_AND_STATUS_REG, BASR_ACK,
+		                           BASR_ACK, HZ / 64) < 0)
+			scmd_printk(KERN_DEBUG, hostdata->connected,
 			            "%s: !REQ and !ACK\n", __func__);
 		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
 			return 0;
 
+		if (bytes == 0)
+			udelay(MAC_PDMA_DELAY);
+
+		if (bytes >= 0)
+			continue;
+
 		dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host,
-		         "%s: bus error (%d/%d)\n", __func__, transferred, len);
+		         "%s: bus error (%d/%d)\n", __func__, d - dst, len);
 		NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-		d = dst + transferred;
-		n = len - transferred;
+		return -1;
 	}
 
 	scmd_printk(KERN_ERR, hostdata->connected,
@@ -192,93 +139,27 @@ static inline int macscsi_pread(struct NCR5380_hostdata *hostdata,
 	return -1;
 }
 
-
-#define CP_MEM_TO_IO(s,d,n)				\
-__asm__ __volatile__					\
-    ("    cmp.w  #4,%2\n"				\
-     "    bls    8f\n"					\
-     "    move.w %0,%%d0\n"				\
-     "    neg.b  %%d0\n"				\
-     "    and.w  #3,%%d0\n"				\
-     "    sub.w  %%d0,%2\n"				\
-     "    bra    2f\n"					\
-     " 1: move.b (%0)+,(%1)\n"				\
-     " 2: dbf    %%d0,1b\n"				\
-     "    move.w %2,%%d0\n"				\
-     "    lsr.w  #5,%%d0\n"				\
-     "    bra    4f\n"					\
-     " 3: move.l (%0)+,(%1)\n"				\
-     "31: move.l (%0)+,(%1)\n"				\
-     "32: move.l (%0)+,(%1)\n"				\
-     "33: move.l (%0)+,(%1)\n"				\
-     "34: move.l (%0)+,(%1)\n"				\
-     "35: move.l (%0)+,(%1)\n"				\
-     "36: move.l (%0)+,(%1)\n"				\
-     "37: move.l (%0)+,(%1)\n"				\
-     " 4: dbf    %%d0,3b\n"				\
-     "    move.w %2,%%d0\n"				\
-     "    lsr.w  #2,%%d0\n"				\
-     "    and.w  #7,%%d0\n"				\
-     "    bra    6f\n"					\
-     " 5: move.l (%0)+,(%1)\n"				\
-     " 6: dbf    %%d0,5b\n"				\
-     "    and.w  #3,%2\n"				\
-     "    bra    8f\n"					\
-     " 7: move.b (%0)+,(%1)\n"				\
-     " 8: dbf    %2,7b\n"				\
-     "    moveq.l #0, %2\n"				\
-     " 9: \n"						\
-     ".section .fixup,\"ax\"\n"				\
-     "    .even\n"					\
-     "91: moveq.l #1, %2\n"				\
-     "    jra 9b\n"					\
-     "94: moveq.l #4, %2\n"				\
-     "    jra 9b\n"					\
-     ".previous\n"					\
-     ".section __ex_table,\"a\"\n"			\
-     "   .align 4\n"					\
-     "   .long  1b,91b\n"				\
-     "   .long  3b,94b\n"				\
-     "   .long 31b,94b\n"				\
-     "   .long 32b,94b\n"				\
-     "   .long 33b,94b\n"				\
-     "   .long 34b,94b\n"				\
-     "   .long 35b,94b\n"				\
-     "   .long 36b,94b\n"				\
-     "   .long 37b,94b\n"				\
-     "   .long  5b,94b\n"				\
-     "   .long  7b,91b\n"				\
-     ".previous"					\
-     : "=a"(s), "=a"(d), "=d"(n)			\
-     : "0"(s), "1"(d), "2"(n)				\
-     : "d0")
-
 static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
                                  unsigned char *src, int len)
 {
 	unsigned char *s = src;
 	u8 __iomem *d = hostdata->pdma_io + (OUTPUT_DATA_REG << 4);
-	int n = len;
-	int transferred;
+
+	hostdata->pdma_residual = len;
 
 	while (!NCR5380_poll_politely(hostdata, BUS_AND_STATUS_REG,
 	                              BASR_DRQ | BASR_PHASE_MATCH,
 	                              BASR_DRQ | BASR_PHASE_MATCH, HZ / 64)) {
-		CP_MEM_TO_IO(s, d, n);
+		int bytes;
 
-		transferred = s - src - n;
-		hostdata->pdma_residual = len - transferred;
+		bytes = mac_pdma_send(s, d, min(hostdata->pdma_residual, 512));
 
-		/* Target changed phase early? */
-		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
-		                           BUS_AND_STATUS_REG, BASR_ACK, BASR_ACK, HZ / 64) < 0)
-			scmd_printk(KERN_ERR, hostdata->connected,
-			            "%s: !REQ and !ACK\n", __func__);
-		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
-			return 0;
+		if (bytes > 0) {
+			s += bytes;
+			hostdata->pdma_residual -= bytes;
+		}
 
-		/* No bus error. */
-		if (n == 0) {
+		if (hostdata->pdma_residual == 0) {
 			if (NCR5380_poll_politely(hostdata, TARGET_COMMAND_REG,
 			                          TCR_LAST_BYTE_SENT,
 			                          TCR_LAST_BYTE_SENT, HZ / 64) < 0)
@@ -287,17 +168,29 @@ static inline int macscsi_pwrite(struct NCR5380_hostdata *hostdata,
 			return 0;
 		}
 
+		if (NCR5380_poll_politely2(hostdata, STATUS_REG, SR_REQ, SR_REQ,
+		                           BUS_AND_STATUS_REG, BASR_ACK,
+		                           BASR_ACK, HZ / 64) < 0)
+			scmd_printk(KERN_DEBUG, hostdata->connected,
+			            "%s: !REQ and !ACK\n", __func__);
+		if (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_PHASE_MATCH))
+			return 0;
+
+		if (bytes == 0)
+			udelay(MAC_PDMA_DELAY);
+
+		if (bytes >= 0)
+			continue;
+
 		dsprintk(NDEBUG_PSEUDO_DMA, hostdata->host,
-		         "%s: bus error (%d/%d)\n", __func__, transferred, len);
+		         "%s: bus error (%d/%d)\n", __func__, s - src, len);
 		NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-		s = src + transferred;
-		n = len - transferred;
+		return -1;
 	}
 
 	scmd_printk(KERN_ERR, hostdata->connected,
 	            "%s: phase mismatch or !DRQ\n", __func__);
 	NCR5380_dprint(NDEBUG_PSEUDO_DMA, hostdata->host);
-
 	return -1;
 }
 
-- 
2.21.0

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Geert Uytterhoeven]

Hi Finn,

On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> A system bus error during a PDMA transfer can mess up the calculation of
> the transfer residual (the PDMA handshaking hardware lacks a byte
> counter). This results in data corruption.
>
> The algorithm in this patch anticipates a bus error by starting each
> transfer with a MOVE.B instruction. If a bus error is caught the transfer
> will be retried. If a bus error is caught later in the transfer (for a
> MOVE.W instruction) the transfer gets failed and subsequent requests for
> that target will use PIO instead of PDMA.
>
> This avoids the "!REQ and !ACK" error so the severity level of that
> message is reduced to KERN_DEBUG.
>
> Cc: Michael Schmitz <schmitzmic@gmail.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: stable@vger.kernel.org # v4.14+
> Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
> Reported-by: Chris Jones <chris@martin-jones.com>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

> ---
>  arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
>  drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------

Why have you moved the PDMA implementation to a header file under
arch/m68k/? Do you intend to reuse it by other drivers?

If not, please keep it in the driver, so (a) you don't need an ack from
me ;-), and (b) your change may be easier to review.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Finn Thain]

On Sun, 2 Jun 2019, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au> 
> wrote:
> > A system bus error during a PDMA transfer can mess up the calculation 
> > of the transfer residual (the PDMA handshaking hardware lacks a byte 
> > counter). This results in data corruption.
> >
> > The algorithm in this patch anticipates a bus error by starting each 
> > transfer with a MOVE.B instruction. If a bus error is caught the 
> > transfer will be retried. If a bus error is caught later in the 
> > transfer (for a MOVE.W instruction) the transfer gets failed and 
> > subsequent requests for that target will use PIO instead of PDMA.
> >
> > This avoids the "!REQ and !ACK" error so the severity level of that 
> > message is reduced to KERN_DEBUG.
> >
> > Cc: Michael Schmitz <schmitzmic@gmail.com>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: stable@vger.kernel.org # v4.14+
> > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
> > Reported-by: Chris Jones <chris@martin-jones.com>
> > Tested-by: Stan Johnson <userm57@yahoo.com>
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Thanks for your patch!
> 
> > ---
> >  arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
> >  drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------
> 
> Why have you moved the PDMA implementation to a header file under 
> arch/m68k/? Do you intend to reuse it by other drivers?
> 

There are a couple of reasons: the mac_esp driver also uses PDMA and the 
NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is 
still out-of-tree, and it is unclear whether the mac_esp driver will ever 
benefit from this code.

> If not, please keep it in the driver, so (a) you don't need an ack from 
> me ;-), and (b) your change may be easier to review.
> 

I take your wink to mean that you don't want to ask the SCSI maintainers 
to review m68k asm. Putting aside the code review process for a moment, do 
you have an opinion on the most logical way to organise this sort of code, 
from the point-of-view of maintainability, re-usability, readability etc.?

Thanks.

-- 

> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Geert Uytterhoeven]

Hi Finn,

On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Sun, 2 Jun 2019, Geert Uytterhoeven wrote:
> > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au>
> > wrote:
> > > A system bus error during a PDMA transfer can mess up the calculation
> > > of the transfer residual (the PDMA handshaking hardware lacks a byte
> > > counter). This results in data corruption.
> > >
> > > The algorithm in this patch anticipates a bus error by starting each
> > > transfer with a MOVE.B instruction. If a bus error is caught the
> > > transfer will be retried. If a bus error is caught later in the
> > > transfer (for a MOVE.W instruction) the transfer gets failed and
> > > subsequent requests for that target will use PIO instead of PDMA.
> > >
> > > This avoids the "!REQ and !ACK" error so the severity level of that
> > > message is reduced to KERN_DEBUG.
> > >
> > > Cc: Michael Schmitz <schmitzmic@gmail.com>
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Cc: stable@vger.kernel.org # v4.14+
> > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
> > > Reported-by: Chris Jones <chris@martin-jones.com>
> > > Tested-by: Stan Johnson <userm57@yahoo.com>
> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> >
> > Thanks for your patch!
> >
> > > ---
> > >  arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
> > >  drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------
> >
> > Why have you moved the PDMA implementation to a header file under
> > arch/m68k/? Do you intend to reuse it by other drivers?
> >
>
> There are a couple of reasons: the mac_esp driver also uses PDMA and the
> NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is
> still out-of-tree, and it is unclear whether the mac_esp driver will ever
> benefit from this code.

So you do have future sharing in mind...

> > If not, please keep it in the driver, so (a) you don't need an ack from
> > me ;-), and (b) your change may be easier to review.
>
> I take your wink to mean that you don't want to ask the SCSI maintainers
> to review m68k asm. Putting aside the code review process for a moment, do

I meant that apart from the code containing m68k assembler source, it
is not related to arch/m68k/, and thus belongs to the driver.
There are several other drivers that contain pieces of assembler code.

> you have an opinion on the most logical way to organise this sort of code,
> from the point-of-view of maintainability, re-usability, readability etc.?

If the code is used by multiple SCSI drivers, you can move it to a header
file under drivers/scsi/.
If the code is shared by drivers belonging to multiple subsystems, you can
move it to a header file under include/linux/.

Anyone who has a better solution?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Finn Thain]

On Mon, 3 Jun 2019, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Sun, 2 Jun 2019, Geert Uytterhoeven wrote:
> > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au>
> > > wrote:
> > > > A system bus error during a PDMA transfer can mess up the calculation
> > > > of the transfer residual (the PDMA handshaking hardware lacks a byte
> > > > counter). This results in data corruption.
> > > >
> > > > The algorithm in this patch anticipates a bus error by starting each
> > > > transfer with a MOVE.B instruction. If a bus error is caught the
> > > > transfer will be retried. If a bus error is caught later in the
> > > > transfer (for a MOVE.W instruction) the transfer gets failed and
> > > > subsequent requests for that target will use PIO instead of PDMA.
> > > >
> > > > This avoids the "!REQ and !ACK" error so the severity level of that
> > > > message is reduced to KERN_DEBUG.
> > > >
> > > > Cc: Michael Schmitz <schmitzmic@gmail.com>
> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Cc: stable@vger.kernel.org # v4.14+
> > > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
> > > > Reported-by: Chris Jones <chris@martin-jones.com>
> > > > Tested-by: Stan Johnson <userm57@yahoo.com>
> > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > >
> > > Thanks for your patch!
> > >
> > > > ---
> > > >  arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
> > > >  drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------
> > >
> > > Why have you moved the PDMA implementation to a header file under
> > > arch/m68k/? Do you intend to reuse it by other drivers?
> > >
> >
> > There are a couple of reasons: the mac_esp driver also uses PDMA and the
> > NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is
> > still out-of-tree, and it is unclear whether the mac_esp driver will ever
> > benefit from this code.
> 
> So you do have future sharing in mind...
> 
> > > If not, please keep it in the driver, so (a) you don't need an ack from
> > > me ;-), and (b) your change may be easier to review.
> >
> > I take your wink to mean that you don't want to ask the SCSI maintainers
> > to review m68k asm. Putting aside the code review process for a moment, do
> 
> I meant that apart from the code containing m68k assembler source, it is 
> not related to arch/m68k/, and thus belongs to the driver.

That criterion seems insufficient. It could describe most of arch/m68k/mac 
(which has headers in arch/m68k/include).

> There are several other drivers that contain pieces of assembler code.
> 

Does any driver contain assembler code for multiple architectures? I was 
trying to avoid that -- though admittedly I don't yet have actual code for 
the PDMA implementation for mac_scsi for Nubus PowerMacs.

However, the existence of that out-of-tree port suggests to me that 
arch/powerpc/include/mac_scsi.h and arch/m68k/include/mac_scsi.h would be 
an appropriate layout.

But if there's no clear policy then perhaps we should ignore the whole 
question until the driver code actually becomes shared code. I don't mind 
re-working the patch to combine the two files.

-- 

> > you have an opinion on the most logical way to organise this sort of 
> > code, from the point-of-view of maintainability, re-usability, 
> > readability etc.?
> 
> If the code is used by multiple SCSI drivers, you can move it to a header
> file under drivers/scsi/.
> If the code is shared by drivers belonging to multiple subsystems, you can
> move it to a header file under include/linux/.
> 
> Anyone who has a better solution?
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Geert Uytterhoeven]

Hi Finn,

On Mon, Jun 3, 2019 at 9:40 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Mon, 3 Jun 2019, Geert Uytterhoeven wrote:
> > On Mon, Jun 3, 2019 at 1:32 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > > On Sun, 2 Jun 2019, Geert Uytterhoeven wrote:
> > > > On Sun, Jun 2, 2019 at 3:29 AM Finn Thain <fthain@telegraphics.com.au>
> > > > wrote:
> > > > > A system bus error during a PDMA transfer can mess up the calculation
> > > > > of the transfer residual (the PDMA handshaking hardware lacks a byte
> > > > > counter). This results in data corruption.
> > > > >
> > > > > The algorithm in this patch anticipates a bus error by starting each
> > > > > transfer with a MOVE.B instruction. If a bus error is caught the
> > > > > transfer will be retried. If a bus error is caught later in the
> > > > > transfer (for a MOVE.W instruction) the transfer gets failed and
> > > > > subsequent requests for that target will use PIO instead of PDMA.
> > > > >
> > > > > This avoids the "!REQ and !ACK" error so the severity level of that
> > > > > message is reduced to KERN_DEBUG.
> > > > >
> > > > > Cc: Michael Schmitz <schmitzmic@gmail.com>
> > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > Cc: stable@vger.kernel.org # v4.14+
> > > > > Fixes: 3a0f64bfa907 ("mac_scsi: Fix pseudo DMA implementation")
> > > > > Reported-by: Chris Jones <chris@martin-jones.com>
> > > > > Tested-by: Stan Johnson <userm57@yahoo.com>
> > > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > ---
> > > > >  arch/m68k/include/asm/mac_pdma.h | 179 +++++++++++++++++++++++++++
> > > > >  drivers/scsi/mac_scsi.c          | 201 ++++++++-----------------------
> > > >
> > > > Why have you moved the PDMA implementation to a header file under
> > > > arch/m68k/? Do you intend to reuse it by other drivers?
> > > >
> > >
> > > There are a couple of reasons: the mac_esp driver also uses PDMA and the
> > > NuBus PowerMac port also uses mac_scsi.c. OTOH, the NuBus PowerMac port is
> > > still out-of-tree, and it is unclear whether the mac_esp driver will ever
> > > benefit from this code.
> >
> > So you do have future sharing in mind...
> >
> > > > If not, please keep it in the driver, so (a) you don't need an ack from
> > > > me ;-), and (b) your change may be easier to review.
> > >
> > > I take your wink to mean that you don't want to ask the SCSI maintainers
> > > to review m68k asm. Putting aside the code review process for a moment, do
> >
> > I meant that apart from the code containing m68k assembler source, it is
> > not related to arch/m68k/, and thus belongs to the driver.
>
> That criterion seems insufficient. It could describe most of arch/m68k/mac
> (which has headers in arch/m68k/include).
>
> > There are several other drivers that contain pieces of assembler code.
> >
>
> Does any driver contain assembler code for multiple architectures? I was
> trying to avoid that -- though admittedly I don't yet have actual code for
> the PDMA implementation for mac_scsi for Nubus PowerMacs.
>
> However, the existence of that out-of-tree port suggests to me that
> arch/powerpc/include/mac_scsi.h and arch/m68k/include/mac_scsi.h would be
> an appropriate layout.
>
> But if there's no clear policy then perhaps we should ignore the whole
> question until the driver code actually becomes shared code. I don't mind
> re-working the patch to combine the two files.

Yep, can be handled when the need arises.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Michael Schmitz]

Hi Finn,

On 3/06/19 7:40 PM, Finn Thain wrote:
>
>> There are several other drivers that contain pieces of assembler code.
>>
> Does any driver contain assembler code for multiple architectures? I was
> trying to avoid that -- though admittedly I don't yet have actual code for
> the PDMA implementation for mac_scsi for Nubus PowerMacs.
>
I've seen that once, for one of the ESP drivers that were supported on 
both m68k and ppc (APUS, PPC upgrade to Amiga computers). But that 
driver was removed long ago (after 2.6?).

In that case, the assembly file did reside in drivers/scsi/. That still 
appears to be current practice (see drivers/scsi/arm/acornscsi-io.S).

Cheers,

     Michael

Re: [PATCH 5/7] scsi: mac_scsi: Fix pseudo DMA implementation, take 2

[Finn Thain]

On Tue, 4 Jun 2019, Michael Schmitz wrote:

> Hi Finn,
> 
> On 3/06/19 7:40 PM, Finn Thain wrote:
> > 
> > > There are several other drivers that contain pieces of assembler code.
> > > 
> > Does any driver contain assembler code for multiple architectures? I was
> > trying to avoid that -- though admittedly I don't yet have actual code for
> > the PDMA implementation for mac_scsi for Nubus PowerMacs.
> > 
> I've seen that once, for one of the ESP drivers that were supported on both
> m68k and ppc (APUS, PPC upgrade to Amiga computers). But that driver was
> removed long ago (after 2.6?).
> 
> In that case, the assembly file did reside in drivers/scsi/. That still
> appears to be current practice (see drivers/scsi/arm/acornscsi-io.S).
> 

The presence of that file would be an argument for adding 
drivers/scsi/m68k/. This seems to be begging the question.

Since there's no clear policy, I'll combine the two files and avoid the 
question for now.

-- 

> Cheers,
> 
> ??? Michael
> 
> 
> 

[PATCH 2/7] scsi: NCR5380: Always re-enable reselection interrupt

[Finn Thain]

The reselection interrupt gets disabled during selection and must be
re-enabled when hostdata->connected becomes NULL. If it isn't re-enabled
a disconnected command may time-out or the target may wedge the bus while
trying to reselect the host. This can happen after a command is aborted.

Fix this by enabling the reselection interrupt in NCR5380_main() after
calls to NCR5380_select() and NCR5380_information_transfer() return.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: stable@vger.kernel.org # v4.9+
Fixes: 8b00c3d5d40d ("ncr5380: Implement new eh_abort_handler")
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/NCR5380.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index fe0535affc14..08e3ea8159b3 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -709,6 +709,8 @@ static void NCR5380_main(struct work_struct *work)
 			NCR5380_information_transfer(instance);
 			done = 0;
 		}
+		if (!hostdata->connected)
+			NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 		spin_unlock_irq(&hostdata->lock);
 		if (!done)
 			cond_resched();
@@ -1110,8 +1112,6 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 		spin_lock_irq(&hostdata->lock);
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 		NCR5380_reselect(instance);
-		if (!hostdata->connected)
-			NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 		shost_printk(KERN_ERR, instance, "reselection after won arbitration?\n");
 		goto out;
 	}
@@ -1119,7 +1119,6 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	if (err < 0) {
 		spin_lock_irq(&hostdata->lock);
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-		NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 
 		/* Can't touch cmd if it has been reclaimed by the scsi ML */
 		if (!hostdata->selecting)
@@ -1157,7 +1156,6 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	if (err < 0) {
 		shost_printk(KERN_ERR, instance, "select: REQ timeout\n");
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-		NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 		goto out;
 	}
 	if (!hostdata->selecting) {
@@ -1826,9 +1824,6 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					 */
 					NCR5380_write(TARGET_COMMAND_REG, 0);
 
-					/* Enable reselect interrupts */
-					NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
-
 					maybe_release_dma_irq(instance);
 					return;
 				case MESSAGE_REJECT:
@@ -1860,8 +1855,6 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					 */
 					NCR5380_write(TARGET_COMMAND_REG, 0);
 
-					/* Enable reselect interrupts */
-					NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 #ifdef SUN3_SCSI_VME
 					dregs->csr |= CSR_DMA_ENABLE;
 #endif
@@ -1964,7 +1957,6 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					cmd->result = DID_ERROR << 16;
 					complete_cmd(instance, cmd);
 					maybe_release_dma_irq(instance);
-					NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
 					return;
 				}
 				msgout = NOP;
-- 
2.21.0

Re: [PATCH 2/7] scsi: NCR5380: Always re-enable reselection interrupt

[Finn Thain]

On Tue, 4 Jun 2019, Sasha Levin wrote:

> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 8b00c3d5d40d ncr5380: Implement new eh_abort_handler.
> 
> The bot has tested the following trees: v5.1.6, v5.0.20, v4.19.47, v4.14.123, v4.9.180.
> 
> v5.1.6: Build OK!
> v5.0.20: Build OK!
> v4.19.47: Failed to apply! Possible dependencies:
>     6a162836997c ("scsi: NCR5380: Reduce goto statements in NCR5380_select()")
> 
> v4.14.123: Failed to apply! Possible dependencies:
>     6a162836997c ("scsi: NCR5380: Reduce goto statements in NCR5380_select()")
> 
> v4.9.180: Failed to apply! Possible dependencies:
>     6a162836997c ("scsi: NCR5380: Reduce goto statements in NCR5380_select()")
> 
> 
> How should we proceed with this patch?
> 

Please cherry-pick the dependency, as it does not alter functionality.

$ 
$ git checkout v4.9
HEAD is now at 69973b830859 Linux 4.9
$ git cherry-pick 6a162836997c
[detached HEAD 0e33d17b7b50] scsi: NCR5380: Reduce goto statements in 
NCR5380_select()
 Date: Thu Sep 27 11:17:11 2018 +1000
 1 file changed, 12 insertions(+), 9 deletions(-)
$ git cherry-pick 61f0c0f6aaf8
[detached HEAD 8ae61212c888] scsi: NCR5380: Always re-enable reselection 
interrupt
 Date: Mon Mar 25 15:45:51 2019 +1100
 1 file changed, 2 insertions(+), 10 deletions(-)
$ 

You could instead apply the patch using more fuzz...

$ 
$ git checkout v4.9
HEAD is now at 69973b830859 Linux 4.9
$ git show 61f0c0f6aaf8 | patch -p1 -F3
patching file drivers/scsi/NCR5380.c
Hunk #1 succeeded at 813 (offset 104 lines).
Hunk #2 succeeded at 1210 (offset 98 lines).
Hunk #3 succeeded at 1217 with fuzz 3 (offset 98 lines).
Hunk #4 succeeded at 1251 (offset 95 lines).
Hunk #5 succeeded at 1901 (offset 77 lines).
Hunk #6 succeeded at 1932 (offset 77 lines).
Hunk #7 succeeded at 2039 (offset 82 lines).
$ 

This also works but would seem to undermine future backporting.

Thanks.

-- 

> -- 
> Thanks,
> Sasha
>