Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703691 > unrolled thread

[PATCH 0/5] esp_scsi, mac_esp: Various fixes and cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] esp_scsi, mac_esp: Various fixes and cleanups Finn Thain <fthain@telegraphics.com.au> - 2017-08-04 07:50 +0200
    [PATCH 2/5] scsi/mac_esp: Fix PIO transfers for MESSAGE IN phase Finn Thain <fthain@telegraphics.com.au> - 2017-08-04 07:50 +0200
    [PATCH 1/5] scsi/mac_esp: Avoid type warning from sparse Finn Thain <fthain@telegraphics.com.au> - 2017-08-04 07:50 +0200
    [PATCH 3/5] scsi/esp_scsi: Clean up control flow and dead code Finn Thain <fthain@telegraphics.com.au> - 2017-08-04 07:50 +0200
    Re: [PATCH 0/5] esp_scsi, mac_esp: Various fixes and cleanups "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-08-11 02:10 +0200

#1703691 — [PATCH 0/5] esp_scsi, mac_esp: Various fixes and cleanups

This series has been tested on m68k Macs (ESP236 equivalent).

Some more testing with different targets and devices (FAS236 etc)
might be nice. Being that the esp_scsi fixes are on error paths,
more review may actually be more valuable than more testing...


Finn Thain (5):
  scsi/mac_esp: Avoid type warning from sparse
  scsi/mac_esp: Fix PIO transfers for MESSAGE IN phase
  scsi/esp_scsi: Clean up control flow and dead code
  scsi/esp_scsi: Avoid sending ABORT TASK SET messages
  scsi/esp_scsi: Always clear msg_out_len after MESSAGE OUT phase

 drivers/scsi/esp_scsi.c | 53 +++++++++++++++++++++----------------------------
 drivers/scsi/esp_scsi.h |  1 -
 drivers/scsi/mac_esp.c  | 37 +++++++++++++++++-----------------
 3 files changed, 42 insertions(+), 49 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1703692 — [PATCH 2/5] scsi/mac_esp: Fix PIO transfers for MESSAGE IN phase

When in MESSAGE IN phase, the ESP device does not automatically
acknowledge each byte that is transferred by PIO. The mac_esp driver
neglects to explicitly ack them, which causes a timeout during messages
larger than one byte (e.g. tag bytes during reconnect). Fix this with an
ESP_CMD_MOK command after each byte.

The MESSAGE IN phase is also different in that each byte transferred
raises ESP_INTR_FDONE. So don't exit the transfer loop for this interrupt,
for this phase.

That resolves the "Reconnect IRQ2 timeout" error on those Macs which use
PIO transfers instead of PDMA. This patch also improves on the weak tests
for unexpected interrupts and phase changes during PIO transfers.

Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 02507a80b35e ("[PATCH] [SCSI] mac_esp: fix PIO mode, take 2")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/mac_esp.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index 253142f3cf6f..eb551f3cc471 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -349,25 +349,23 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 {
 	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) {
-			unsigned int n;
-
-			n = mac_esp_wait_for_fifo(esp);
-			if (!n)
+			if (!mac_esp_wait_for_fifo(esp))
 				break;
 
-			if (n > esp_count)
-				n = esp_count;
-			esp_count -= n;
-
-			MAC_ESP_PIO_LOOP("%2@,%0@+", n);
+			*dst++ = esp_read8(ESP_FDATA);
+			--esp_count;
 
 			if (!esp_count)
 				break;
@@ -375,14 +373,17 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			if (mac_esp_wait_for_intr(esp))
 				break;
 
-			if (((esp->sreg & ESP_STAT_PMASK) != ESP_DIP) &&
-			    ((esp->sreg & ESP_STAT_PMASK) != ESP_MIP))
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
 				break;
 
 			esp->ireg = esp_read8(ESP_INTRPT);
-			if ((esp->ireg & (ESP_INTR_DC | ESP_INTR_BSERV)) !=
-			    ESP_INTR_BSERV)
+			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);
 		}
@@ -402,14 +403,14 @@ static void mac_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
 			if (mac_esp_wait_for_intr(esp))
 				break;
 
-			if (((esp->sreg & ESP_STAT_PMASK) != ESP_DOP) &&
-			    ((esp->sreg & ESP_STAT_PMASK) != ESP_MOP))
+			if ((esp->sreg & ESP_STAT_PMASK) != phase)
 				break;
 
 			esp->ireg = esp_read8(ESP_INTRPT);
-			if ((esp->ireg & (ESP_INTR_DC | ESP_INTR_BSERV)) !=
-			    ESP_INTR_BSERV)
+			if (esp->ireg & ~ESP_INTR_BSERV) {
+				mep->error = 1;
 				break;
+			}
 
 			n = MAC_ESP_FIFO_SIZE -
 			    (esp_read8(ESP_FFLAGS) & ESP_FF_FBYTES);
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1703693 — [PATCH 1/5] scsi/mac_esp: Avoid type warning from sparse

Avoid the following warning from "make C=1":

  CHECK   drivers/scsi/mac_esp.c
drivers/scsi/mac_esp.c:357:30: warning: incorrect type in initializer (different address spaces)
drivers/scsi/mac_esp.c:357:30:    expected unsigned char [usertype] *fifo
drivers/scsi/mac_esp.c:357:30:    got void [noderef] <asn:2>*

Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/mac_esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mac_esp.c b/drivers/scsi/mac_esp.c
index cdb61eaa2d1f..253142f3cf6f 100644
--- a/drivers/scsi/mac_esp.c
+++ b/drivers/scsi/mac_esp.c
@@ -348,7 +348,7 @@ 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 *fifo = esp->regs + ESP_FDATA * 16;
+	u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
 
 	cmd &= ~ESP_CMD_DMA;
 	mep->error = 0;
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1703694 — [PATCH 3/5] scsi/esp_scsi: Clean up control flow and dead code

This patch improves readability. There are no functional changes.

Since this touches on a questionable ESP_INTR_DC conditional, add some
commentary to help others who may (as I did) find themselves chasing an
"Invalid Command" error after the device flags this condition.

This cleanup also eliminates a warning from "make W=1":
drivers/scsi/esp_scsi.c: In function 'esp_finish_select':
drivers/scsi/esp_scsi.c:1233:5: warning: variable 'orig_select_state' set but not used [-Wunused-but-set-variable]
  u8 orig_select_state;

Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/scsi/esp_scsi.c | 40 ++++++++++++++--------------------------
 drivers/scsi/esp_scsi.h |  1 -
 2 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 71cb05b1c3eb..93fef2b3d357 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -597,14 +597,12 @@ static int esp_alloc_lun_tag(struct esp_cmd_entry *ent,
 
 		lp->non_tagged_cmd = ent;
 		return 0;
-	} else {
-		/* Tagged command, see if blocked by a
-		 * non-tagged one.
-		 */
-		if (lp->non_tagged_cmd || lp->hold)
-			return -EBUSY;
 	}
 
+	/* Tagged command. Check that it isn't blocked by a non-tagged one. */
+	if (lp->non_tagged_cmd || lp->hold)
+		return -EBUSY;
+
 	BUG_ON(lp->tagged_cmds[ent->orig_tag[1]]);
 
 	lp->tagged_cmds[ent->orig_tag[1]] = ent;
@@ -1210,12 +1208,6 @@ static int esp_reconnect(struct esp *esp)
 
 	esp->active_cmd = ent;
 
-	if (ent->flags & ESP_CMD_FLAG_ABORT) {
-		esp->msg_out[0] = ABORT_TASK_SET;
-		esp->msg_out_len = 1;
-		scsi_esp_cmd(esp, ESP_CMD_SATN);
-	}
-
 	esp_event(esp, ESP_EVENT_CHECK_PHASE);
 	esp_restore_pointers(esp, ent);
 	esp->flags |= ESP_FLAG_QUICKIRQ_CHECK;
@@ -1230,9 +1222,6 @@ static int esp_finish_select(struct esp *esp)
 {
 	struct esp_cmd_entry *ent;
 	struct scsi_cmnd *cmd;
-	u8 orig_select_state;
-
-	orig_select_state = esp->select_state;
 
 	/* No longer selecting.  */
 	esp->select_state = ESP_SELECT_NONE;
@@ -1745,7 +1734,6 @@ static int esp_process_event(struct esp *esp)
 			return 0;
 		}
 		goto again;
-		break;
 
 	case ESP_EVENT_DATA_IN:
 		write = 1;
@@ -1956,12 +1944,14 @@ static int esp_process_event(struct esp *esp)
 		} else {
 			if (esp->msg_out_len > 1)
 				esp->ops->dma_invalidate(esp);
-		}
 
-		if (!(esp->ireg & ESP_INTR_DC)) {
-			if (esp->rev != FASHME)
+			/* XXX if the chip went into disconnected mode,
+			 * we can't run the phase state machine anyway.
+			 */
+			if (!(esp->ireg & ESP_INTR_DC))
 				scsi_esp_cmd(esp, ESP_CMD_NULL);
 		}
+
 		esp_event(esp, ESP_EVENT_CHECK_PHASE);
 		goto again;
 	case ESP_EVENT_MSGIN:
@@ -2022,7 +2012,6 @@ static int esp_process_event(struct esp *esp)
 		}
 		esp_schedule_reset(esp);
 		return 0;
-		break;
 
 	case ESP_EVENT_RESET:
 		scsi_esp_cmd(esp, ESP_CMD_RS);
@@ -2033,7 +2022,6 @@ static int esp_process_event(struct esp *esp)
 			     "Unexpected event %x, resetting\n", esp->event);
 		esp_schedule_reset(esp);
 		return 0;
-		break;
 	}
 	return 1;
 }
@@ -2170,14 +2158,14 @@ static void __esp_interrupt(struct esp *esp)
 
 		esp_schedule_reset(esp);
 	} else {
-		if (!(esp->ireg & ESP_INTR_RSEL)) {
-			/* Some combination of FDONE, BSERV, DC.  */
-			if (esp->select_state != ESP_SELECT_NONE)
-				intr_done = esp_finish_select(esp);
-		} else if (esp->ireg & ESP_INTR_RSEL) {
+		if (esp->ireg & ESP_INTR_RSEL) {
 			if (esp->active_cmd)
 				(void) esp_finish_select(esp);
 			intr_done = esp_reconnect(esp);
+		} else {
+			/* Some combination of FDONE, BSERV, DC. */
+			if (esp->select_state != ESP_SELECT_NONE)
+				intr_done = esp_finish_select(esp);
 		}
 	}
 	while (!intr_done)
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index 84dcbe4a6268..7e8932ae91f8 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -281,7 +281,6 @@ struct esp_cmd_entry {
 
 	u8			flags;
 #define ESP_CMD_FLAG_WRITE	0x01 /* DMA is a write */
-#define ESP_CMD_FLAG_ABORT	0x02 /* being aborted */
 #define ESP_CMD_FLAG_AUTOSENSE	0x04 /* Doing automatic REQUEST_SENSE */
 #define ESP_CMD_FLAG_RESIDUAL	0x08 /* AM53c974 BLAST residual */
 
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1709131

Finn,

> This series has been tested on m68k Macs (ESP236 equivalent).
>
> Some more testing with different targets and devices (FAS236 etc)
> might be nice. Being that the esp_scsi fixes are on error paths, more
> review may actually be more valuable than more testing...

Applied to 4.14/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web