Skip to contentSkip to conversation

NCR5380: Various improvements

PATCH Applied Finn Thain 2 participants 12 messages Sep 27, 2018 – Sep 28, 2018

Subsystems (2)
SCSI Ncr 5380 SCSI drivers
lore
Share & tools
lore

Finn Thain

author
#0 lore raw

cover NCR5380: Various improvements

[This patch series is being re-sent unchanged due to email problems.]

This series addresses issues which became apparent when Michael Schmitz tried to use an AztecMonster II SATA/SCSI adapter with a 5380 host.

That target seems to have some bugs which thoroughly exercise driver error paths and EH.

The patch by Hannes Reinecke was cherry-picked from his 'eh-reset.v5' branch.

Finn Thain (9):
NCR5380: Reduce goto statements in NCR5380_select()
NCR5380: Have NCR5380_select() return a bool
NCR5380: Withhold disconnect privilege for REQUEST SENSE
NCR5380: Use DRIVER_SENSE to indicate valid sense data
NCR5380: Check for invalid reselection target
NCR5380: Don't clear busy flag when abort fails
NCR5380: Don't call dsprintk() following reselection interrupt
NCR5380: Handle BUS FREE during reselection
NCR5380: Check for bus reset

Hannes Reinecke (1):
NCR5380: Clear all unissued commands on host reset

 drivers/scsi/NCR5380.c | 167 +++++++++++++++++++++++++++++--------------------
 drivers/scsi/NCR5380.h |   2 +-
 2 files changed, 101 insertions(+), 68 deletions(-)
Signature · 1 line
2.16.4
author
#1 lore raw

PATCH 1/10 NCR5380: Clear all unissued commands on host reset

From: Hannes Reinecke <[email protected]>

When doing a host reset we should be clearing all outstanding commands, not just the command triggering the reset.

Signed-off-by Hannes Reinecke <[email protected]>
Reviewed-by Johannes Thumshirn <[email protected]>
Cc Ondrey Zary <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/NCR5380.c +5 -2
index 90ea0f5d9bdb..80c20ab4fc53 100644
@@ -2308,7 +2308,7 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 	spin_lock_irqsave(&hostdata->lock, flags);
 
 #if (NDEBUG & NDEBUG_ANY)
-	scmd_printk(KERN_INFO, cmd, __func__);
+	shost_printk(KERN_INFO, instance, __func__);
 #endif
 	NCR5380_dprint(NDEBUG_ANY, instance);
 	NCR5380_dprint_phase(NDEBUG_ANY, instance);

@@ -2326,10 +2326,13 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 	 * commands!
 	 */
 
-	if (list_del_cmd(&hostdata->unissued, cmd)) {
+	list_for_each_entry(ncmd, &hostdata->unissued, list) {
+		struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
+
 		cmd->result = DID_RESET << 16;
 		cmd->scsi_done(cmd);
 	}
+	INIT_LIST_HEAD(&hostdata->unissued);
 
 	if (hostdata->selecting) {
 		hostdata->selecting->result = DID_RESET << 16;
Signature · 1 line
2.16.4
author
#2 lore raw

PATCH 2/10 NCR5380: Reduce goto statements in NCR5380_select()

In reply to Finn Thain

Replace a 'goto' statement with a simple 'return' where possible. This improves readability. No functional change.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)
drivers/scsi/NCR5380.c +12 -9
index 80c20ab4fc53..c6d10780febe 100644
@@ -984,7 +984,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	if (!hostdata->selecting) {
 		/* Command was aborted */
 		NCR5380_write(MODE_REG, MR_BASE);
-		goto out;
+		return NULL;
 	}
 	if (err < 0) {
 		NCR5380_write(MODE_REG, MR_BASE);

@@ -1033,7 +1033,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	if (!hostdata->selecting) {
 		NCR5380_write(MODE_REG, MR_BASE);
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-		goto out;
+		return NULL;
 	}
 
 	dsprintk(NDEBUG_ARBITRATION, instance, "won arbitration\n");

@@ -1116,13 +1116,16 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 		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) {
-			cmd->result = DID_BAD_TARGET << 16;
-			complete_cmd(instance, cmd);
-			dsprintk(NDEBUG_SELECTION, instance, "target did not respond within 250ms\n");
-			cmd = NULL;
-		}
+		if (!hostdata->selecting)
+			return NULL;
+
+		cmd->result = DID_BAD_TARGET << 16;
+		complete_cmd(instance, cmd);
+		dsprintk(NDEBUG_SELECTION, instance,
+			"target did not respond within 250ms\n");
+		cmd = NULL;
 		goto out;
 	}
 

@@ -1155,7 +1158,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	}
 	if (!hostdata->selecting) {
 		do_abort(instance);
-		goto out;
+		return NULL;
 	}
 
 	dsprintk(NDEBUG_SELECTION, instance, "target %d selected, going into MESSAGE OUT phase.\n",
Signature · 1 line
2.16.4
author
#3 lore raw

PATCH 3/10 NCR5380: Have NCR5380_select() return a bool

In reply to Finn Thain

The return value is taken to mean "retry" or "don't retry". Change it to bool to improve readability. Fix related comments. No functional change.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 46 +++++++++++++++++++++-------------------------
 drivers/scsi/NCR5380.h |  2 +-
 2 files changed, 22 insertions(+), 26 deletions(-)
drivers/scsi/NCR5380.c +21 -25
index c6d10780febe..7be1ba2b9e4e 100644
@@ -902,20 +902,16 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, void *dev_id)
 	return IRQ_RETVAL(handled);
 }
 
-/*
- * Function : int NCR5380_select(struct Scsi_Host *instance,
- * struct scsi_cmnd *cmd)
- *
- * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command,
- * including ARBITRATION, SELECTION, and initial message out for
- * IDENTIFY and queue messages.
+/**
+ * NCR5380_select - attempt arbitration and selection for a given command
+ * @instance: the Scsi_Host instance
+ * @cmd: the scsi_cmnd to execute
  *
- * Inputs : instance - instantiation of the 5380 driver on which this
- * target lives, cmd - SCSI command to execute.
+ * This routine establishes an I_T_L nexus for a SCSI command. This involves
+ * ARBITRATION, SELECTION and MESSAGE OUT phases and an IDENTIFY message.
  *
- * Returns cmd if selection failed but should be retried,
- * NULL if selection failed and should not be retried, or
- * NULL if selection succeeded (hostdata->connected == cmd).
+ * Returns true if the operation should be retried.
+ * Returns false if it should not be retried.
  *
  * Side effects :
  * If bus busy, arbitration failed, etc, NCR5380_select() will exit

@@ -923,16 +919,15 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, void *dev_id)
  * SELECT_ENABLE will be set appropriately, the NCR5380
  * will cease to drive any SCSI bus signals.
  *
- * If successful : I_T_L or I_T_L_Q nexus will be established,
- * instance->connected will be set to cmd.
+ * If successful : the I_T_L nexus will be established, and
+ * hostdata->connected will be set to cmd.
  * SELECT interrupt will be disabled.
  *
  * If failed (no target) : cmd->scsi_done() will be called, and the
  * cmd->result host byte set to DID_BAD_TARGET.
  */
 
-static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
-                                        struct scsi_cmnd *cmd)
+static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	__releases(&hostdata->lock) __acquires(&hostdata->lock)
 {
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);

@@ -940,6 +935,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	unsigned char *data;
 	int len;
 	int err;
+	bool ret = true;
 
 	NCR5380_dprint(NDEBUG_ARBITRATION, instance);
 	dsprintk(NDEBUG_ARBITRATION, instance, "starting arbitration, id = %d\n",

@@ -948,7 +944,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	/*
 	 * Arbitration and selection phases are slow and involve dropping the
 	 * lock, so we have to watch out for EH. An exception handler may
-	 * change 'selecting' to NULL. This function will then return NULL
+	 * change 'selecting' to NULL. This function will then return false
 	 * so that the caller will forget about 'cmd'. (During information
 	 * transfer phases, EH may change 'connected' to NULL.)
 	 */

@@ -984,7 +980,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	if (!hostdata->selecting) {
 		/* Command was aborted */
 		NCR5380_write(MODE_REG, MR_BASE);
-		return NULL;
+		return false;
 	}
 	if (err < 0) {
 		NCR5380_write(MODE_REG, MR_BASE);

@@ -1033,7 +1029,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	if (!hostdata->selecting) {
 		NCR5380_write(MODE_REG, MR_BASE);
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
-		return NULL;
+		return false;
 	}
 
 	dsprintk(NDEBUG_ARBITRATION, instance, "won arbitration\n");

@@ -1119,13 +1115,13 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 
 		/* Can't touch cmd if it has been reclaimed by the scsi ML */
 		if (!hostdata->selecting)
-			return NULL;
+			return false;
 
 		cmd->result = DID_BAD_TARGET << 16;
 		complete_cmd(instance, cmd);
 		dsprintk(NDEBUG_SELECTION, instance,
 			"target did not respond within 250ms\n");
-		cmd = NULL;
+		ret = false;
 		goto out;
 	}
 

@@ -1158,7 +1154,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 	}
 	if (!hostdata->selecting) {
 		do_abort(instance);
-		return NULL;
+		return false;
 	}
 
 	dsprintk(NDEBUG_SELECTION, instance, "target %d selected, going into MESSAGE OUT phase.\n",

@@ -1174,7 +1170,7 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 		cmd->result = DID_ERROR << 16;
 		complete_cmd(instance, cmd);
 		dsprintk(NDEBUG_SELECTION, instance, "IDENTIFY message transfer failed\n");
-		cmd = NULL;
+		ret = false;
 		goto out;
 	}
 

@@ -1189,13 +1185,13 @@ static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *instance,
 
 	initialize_SCp(cmd);
 
-	cmd = NULL;
+	ret = false;
 
 out:
 	if (!hostdata->selecting)
 		return NULL;
 	hostdata->selecting = NULL;
-	return cmd;
+	return ret;
 }
 
 /*
drivers/scsi/NCR5380.h +1 -1
index 31096a0b0fdd..efca509b92b0 100644
@@ -275,7 +275,7 @@ static irqreturn_t NCR5380_intr(int irq, void *dev_id);
 static void NCR5380_main(struct work_struct *work);
 static const char *NCR5380_info(struct Scsi_Host *instance);
 static void NCR5380_reselect(struct Scsi_Host *instance);
-static struct scsi_cmnd *NCR5380_select(struct Scsi_Host *, struct scsi_cmnd *);
+static bool NCR5380_select(struct Scsi_Host *, struct scsi_cmnd *);
 static int NCR5380_transfer_dma(struct Scsi_Host *instance, unsigned char *phase, int *count, unsigned char **data);
 static int NCR5380_transfer_pio(struct Scsi_Host *instance, unsigned char *phase, int *count, unsigned char **data);
 static int NCR5380_poll_politely2(struct NCR5380_hostdata *,
Signature · 1 line
2.16.4
author
#4 lore raw

PATCH 4/10 NCR5380: Withhold disconnect privilege for REQUEST SENSE

In reply to Finn Thain

This is mostly needed because an AztecMonster II target has been observed disconnecting REQUEST SENSE commands and then failing to reselect properly.

Suggested-by Michael Schmitz <[email protected]>
Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/scsi/NCR5380.c +3 -1
index 7be1ba2b9e4e..5226164cfa65 100644
@@ -936,6 +936,8 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 	int len;
 	int err;
 	bool ret = true;
+	bool can_disconnect = instance->irq != NO_IRQ &&
+			      cmd->cmnd[0] != REQUEST_SENSE;
 
 	NCR5380_dprint(NDEBUG_ARBITRATION, instance);
 	dsprintk(NDEBUG_ARBITRATION, instance, "starting arbitration, id = %d\n",

@@ -1159,7 +1161,7 @@ static bool NCR5380_select(struct Scsi_Host *instance, struct scsi_cmnd *cmd)
 
 	dsprintk(NDEBUG_SELECTION, instance, "target %d selected, going into MESSAGE OUT phase.\n",
 	         scmd_id(cmd));
-	tmp[0] = IDENTIFY(((instance->irq == NO_IRQ) ? 0 : 1), cmd->device->lun);
+	tmp[0] = IDENTIFY(can_disconnect, cmd->device->lun);
 
 	len = 1;
 	data = tmp;
Signature · 1 line
2.16.4
author
#5 lore raw

PATCH 5/10 NCR5380: Use DRIVER_SENSE to indicate valid sense data

In reply to Finn Thain

When sense data is valid, call set_driver_byte(cmd, DRIVER_SENSE). Otherwise some callers of scsi_execute() will ignore sense data. Don't set DID_ERROR or DID_RESET just because sense data is missing.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/scsi/NCR5380.c +4 -5
index 5226164cfa65..e96a48b9e86c 100644
@@ -513,11 +513,12 @@ static void complete_cmd(struct Scsi_Host *instance,
 
 	if (hostdata->sensing == cmd) {
 		/* Autosense processing ends here */
-		if ((cmd->result & 0xff) != SAM_STAT_GOOD) {
+		if (status_byte(cmd->result) != GOOD) {
 			scsi_eh_restore_cmnd(cmd, &hostdata->ses);
-			set_host_byte(cmd, DID_ERROR);
-		} else
+		} else {
 			scsi_eh_restore_cmnd(cmd, &hostdata->ses);
+			set_driver_byte(cmd, DRIVER_SENSE);
+		}
 		hostdata->sensing = NULL;
 	}
 

@@ -2273,7 +2274,6 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 	if (list_del_cmd(&hostdata->autosense, cmd)) {
 		dsprintk(NDEBUG_ABORT, instance,
 		         "abort: removed %p from sense queue\n", cmd);
-		set_host_byte(cmd, DID_ERROR);
 		complete_cmd(instance, cmd);
 	}
 

@@ -2352,7 +2352,6 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 	list_for_each_entry(ncmd, &hostdata->autosense, list) {
 		struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
 
-		set_host_byte(cmd, DID_RESET);
 		cmd->scsi_done(cmd);
 	}
 	INIT_LIST_HEAD(&hostdata->autosense);
Signature · 1 line
2.16.4
author
#6 lore raw

PATCH 6/10 NCR5380: Check for invalid reselection target

In reply to Finn Thain

The X3T9.2 specification (draft) says, under "6.1.4.1 RESELECTION", that "the initiator shall not respond to a RESELECTION phase if other than two SCSI ID bits are on the DATA BUS." This issue (too many bits set) has been observed in the wild, so add a check.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/scsi/NCR5380.c +5 -0
index e96a48b9e86c..3058b68b6740 100644
@@ -2016,6 +2016,11 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 	NCR5380_write(MODE_REG, MR_BASE);
 
 	target_mask = NCR5380_read(CURRENT_SCSI_DATA_REG) & ~(hostdata->id_mask);
+	if (!target_mask || target_mask & (target_mask - 1)) {
+		shost_printk(KERN_WARNING, instance,
+			     "reselect: bad target_mask 0x%02x\n", target_mask);
+		return;
+	}
 
 	dsprintk(NDEBUG_RESELECTION, instance, "reselect\n");
Signature · 1 line
2.16.4
author
#7 lore raw

PATCH 7/10 NCR5380: Don't clear busy flag when abort fails

In reply to Finn Thain

When NCR5380_abort() returns FAILED, the driver forgets that the target is still busy. Hence, further commands may be sent to the target, which may fail during selection and produce the error message, "reselection after won arbitration?". Prevent this by leaving the busy flag set when NCR5380_abort() fails.

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

---
Consistent with the rest of NCR5380.c, this patch triggers some "line over 80 characters" messages from checkpatch.pl. I haven't addressed those complaints because IMHO the cure is worse than the disease. Refactoring to reduce indentation levels would be a lot of churn.

---
 drivers/scsi/NCR5380.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/scsi/NCR5380.c +10 -4
index 3058b68b6740..5826421146ba 100644
@@ -522,8 +522,6 @@ static void complete_cmd(struct Scsi_Host *instance,
 		hostdata->sensing = NULL;
 	}
 
-	hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
-
 	cmd->scsi_done(cmd);
 }
 

@@ -1713,6 +1711,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 				cmd->result = DID_ERROR << 16;
 				complete_cmd(instance, cmd);
 				hostdata->connected = NULL;
+				hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
 				return;
 #endif
 			case PHASE_DATAIN:

@@ -1795,6 +1794,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 					         cmd, scmd_id(cmd), cmd->device->lun);
 
 					hostdata->connected = NULL;
+					hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
 
 					cmd->result &= ~0xffff;
 					cmd->result |= cmd->SCp.Status;

@@ -1953,6 +1953,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 				NCR5380_transfer_pio(instance, &phase, &len, &data);
 				if (msgout == ABORT) {
 					hostdata->connected = NULL;
+					hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
 					cmd->result = DID_ERROR << 16;
 					complete_cmd(instance, cmd);
 					maybe_release_dma_irq(instance);

@@ -2108,13 +2109,16 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 		dsprintk(NDEBUG_RESELECTION | NDEBUG_QUEUES, instance,
 		         "reselect: removed %p from disconnected queue\n", tmp);
 	} else {
+		int target = ffs(target_mask) - 1;
+
 		shost_printk(KERN_ERR, instance, "target bitmask 0x%02x lun %d not in disconnected queue.\n",
 		             target_mask, lun);
 		/*
 		 * Since we have an established nexus that we can't do anything
 		 * with, we must abort it.
 		 */
-		do_abort(instance);
+		if (do_abort(instance) == 0)
+			hostdata->busy[target] &= ~(1 << lun);
 		return;
 	}
 

@@ -2285,8 +2289,10 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 out:
 	if (result == FAILED)
 		dsprintk(NDEBUG_ABORT, instance, "abort: failed to abort %p\n", cmd);
-	else
+	else {
+		hostdata->busy[scmd_id(cmd)] &= ~(1 << cmd->device->lun);
 		dsprintk(NDEBUG_ABORT, instance, "abort: successfully aborted %p\n", cmd);
+	}
 
 	queue_work(hostdata->work_q, &hostdata->main_task);
 	maybe_release_dma_irq(instance);
Signature · 1 line
2.16.4
author
#8 lore raw

PATCH 8/10 NCR5380: Don't call dsprintk() following reselection interrupt

In reply to Finn Thain

The X3T9.2 specification (draft) says, under "6.1.4.1 RESELECTION",

... The reselected initiator shall then assert the BSY signal within a selection abort time of its most recent detection of being reselected; this is required for correct operation of the time-out procedure.

The selection abort time is only 200 us which may be insufficient time for a printk() call. Move the diagnostics to the error paths.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/NCR5380.c +2 -2
index 5826421146ba..419033643015 100644
@@ -2023,8 +2023,6 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 		return;
 	}
 
-	dsprintk(NDEBUG_RESELECTION, instance, "reselect\n");
-
 	/*
 	 * At this point, we have detected that our SCSI ID is on the bus,
 	 * SEL is true and BSY was false for at least one bus settle delay

@@ -2037,6 +2035,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 	NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_BSY);
 	if (NCR5380_poll_politely(hostdata,
 	                          STATUS_REG, SR_SEL, 0, 2 * HZ) < 0) {
+		shost_printk(KERN_ERR, instance, "reselect: !SEL timeout\n");
 		NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
 		return;
 	}

@@ -2048,6 +2047,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 
 	if (NCR5380_poll_politely(hostdata,
 	                          STATUS_REG, SR_REQ, SR_REQ, 2 * HZ) < 0) {
+		shost_printk(KERN_ERR, instance, "reselect: REQ timeout\n");
 		do_abort(instance);
 		return;
 	}
Signature · 1 line
2.16.4
author
#9 lore raw

PATCH 9/10 NCR5380: Handle BUS FREE during reselection

In reply to Finn Thain

The X3T9.2 specification (draft) says, under "6.1.4.2 RESELECTION time-out procedure", that a target may assert RST or go to BUS FREE phase if the initiator does not respond within 200 us. Something like this has been observed with AztecMonster II target. When it happens, all we can do is wait for the target to try again.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/scsi/NCR5380.c +3 -0
index 419033643015..b9a3eb0647e4 100644
@@ -2047,6 +2047,9 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
 
 	if (NCR5380_poll_politely(hostdata,
 	                          STATUS_REG, SR_REQ, SR_REQ, 2 * HZ) < 0) {
+		if ((NCR5380_read(STATUS_REG) & (SR_BSY | SR_SEL)) == 0)
+			/* BUS FREE phase */
+			return;
 		shost_printk(KERN_ERR, instance, "reselect: REQ timeout\n");
 		do_abort(instance);
 		return;
Signature · 1 line
2.16.4
author
#10 lore raw

PATCH 10/10 NCR5380: Check for bus reset

In reply to Finn Thain

The SR_RST bit isn't latched. Hence, detecting a bus reset isn't reliable. When it is detected, the right thing to do is to drop all connected and disconnected commands. The code for that is already present so refactor it and call it when SR_RST is set.

Tested-by Michael Schmitz <[email protected]>
Signed-off-by Finn Thain <[email protected]>
---
 drivers/scsi/NCR5380.c | 74 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 29 deletions(-)
drivers/scsi/NCR5380.c +45 -29
index b9a3eb0647e4..8429c855701f 100644
@@ -131,6 +131,7 @@
 
 static int do_abort(struct Scsi_Host *);
 static void do_reset(struct Scsi_Host *);
+static void bus_reset_cleanup(struct Scsi_Host *);
 
 /**
  * initialize_SCp - init the scsi pointer field

@@ -883,7 +884,14 @@ static irqreturn_t __maybe_unused NCR5380_intr(int irq, void *dev_id)
 			/* Probably Bus Reset */
 			NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-			dsprintk(NDEBUG_INTR, instance, "unknown interrupt\n");
+			if (sr & SR_RST) {
+				/* Certainly Bus Reset */
+				shost_printk(KERN_WARNING, instance,
+					     "bus reset interrupt\n");
+				bus_reset_cleanup(instance);
+			} else {
+				dsprintk(NDEBUG_INTR, instance, "unknown interrupt\n");
+			}
 #ifdef SUN3_SCSI_VME
 			dregs->csr |= CSR_DMA_ENABLE;
 #endif

@@ -2305,31 +2313,12 @@ static int NCR5380_abort(struct scsi_cmnd *cmd)
 }
 
 
-/**
- * NCR5380_host_reset - reset the SCSI host
- * @cmd: SCSI command undergoing EH
- *
- * Returns SUCCESS
- */
-
-static int NCR5380_host_reset(struct scsi_cmnd *cmd)
+static void bus_reset_cleanup(struct Scsi_Host *instance)
 {
-	struct Scsi_Host *instance = cmd->device->host;
 	struct NCR5380_hostdata *hostdata = shost_priv(instance);
 	int i;
-	unsigned long flags;
 	struct NCR5380_cmd *ncmd;
 
-	spin_lock_irqsave(&hostdata->lock, flags);
-
-#if (NDEBUG & NDEBUG_ANY)
-	shost_printk(KERN_INFO, instance, __func__);
-#endif
-	NCR5380_dprint(NDEBUG_ANY, instance);
-	NCR5380_dprint_phase(NDEBUG_ANY, instance);
-
-	do_reset(instance);
-
 	/* reset NCR registers */
 	NCR5380_write(MODE_REG, MR_BASE);
 	NCR5380_write(TARGET_COMMAND_REG, 0);

@@ -2341,14 +2330,6 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 	 * commands!
 	 */
 
-	list_for_each_entry(ncmd, &hostdata->unissued, list) {
-		struct scsi_cmnd *cmd = NCR5380_to_scmd(ncmd);
-
-		cmd->result = DID_RESET << 16;
-		cmd->scsi_done(cmd);
-	}
-	INIT_LIST_HEAD(&hostdata->unissued);
-
 	if (hostdata->selecting) {
 		hostdata->selecting->result = DID_RESET << 16;
 		complete_cmd(instance, hostdata->selecting);

@@ -2382,6 +2363,41 @@ static int NCR5380_host_reset(struct scsi_cmnd *cmd)
 
 	queue_work(hostdata->work_q, &hostdata->main_task);
 	maybe_release_dma_irq(instance);
+}
+
+/**
+ * NCR5380_host_reset - reset the SCSI host
+ * @cmd: SCSI command undergoing EH
+ *
+ * Returns SUCCESS
+ */
+
+static int NCR5380_host_reset(struct scsi_cmnd *cmd)
+{
+	struct Scsi_Host *instance = cmd->device->host;
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
+	unsigned long flags;
+	struct NCR5380_cmd *ncmd;
+
+	spin_lock_irqsave(&hostdata->lock, flags);
+
+#if (NDEBUG & NDEBUG_ANY)
+	shost_printk(KERN_INFO, instance, __func__);
+#endif
+	NCR5380_dprint(NDEBUG_ANY, instance);
+	NCR5380_dprint_phase(NDEBUG_ANY, instance);
+
+	list_for_each_entry(ncmd, &hostdata->unissued, list) {
+		struct scsi_cmnd *scmd = NCR5380_to_scmd(ncmd);
+
+		scmd->result = DID_RESET << 16;
+		scmd->scsi_done(scmd);
+	}
+	INIT_LIST_HEAD(&hostdata->unissued);
+
+	do_reset(instance);
+	bus_reset_cleanup(instance);
+
 	spin_unlock_irqrestore(&hostdata->lock, flags);
 
 	return SUCCESS;
Signature · 1 line
2.16.4
Applied
#11 lore raw
In reply to Finn Thain

Finn,

Quoted 9 lines

This series addresses issues which became apparent when Michael Schmitz tried to use an AztecMonster II SATA/SCSI adapter with a 5380 host.

That target seems to have some bugs which thoroughly exercise driver error paths and EH.

The patch by Hannes Reinecke was cherry-picked from his 'eh-reset.v5' branch.

Applied to 4.20/scsi-queue, thank you!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering