* [PATCH 01/10] NCR5380: Clear all unissued commands on host reset
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 03/10] NCR5380: Have NCR5380_select() return a bool Finn Thain
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
From: Hannes Reinecke <hare@suse.de>
When doing a host reset we should be clearing all outstanding
commands, not just the command triggering the reset.
Signed-off-by: Hannes Reinecke <hare@suse.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Ondrey Zary <linux@rainbow-software.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 90ea0f5d9bdb..80c20ab4fc53 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 03/10] NCR5380: Have NCR5380_select() return a bool
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
2018-09-27 1:17 ` [PATCH 01/10] NCR5380: Clear all unissued commands on host reset Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 09/10] NCR5380: Handle BUS FREE during reselection Finn Thain
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 46 +++++++++++++++++++++-------------------------
drivers/scsi/NCR5380.h | 2 +-
2 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index c6d10780febe..7be1ba2b9e4e 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
}
/*
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index 31096a0b0fdd..efca509b92b0 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -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 *,
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 09/10] NCR5380: Handle BUS FREE during reselection
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
2018-09-27 1:17 ` [PATCH 01/10] NCR5380: Clear all unissued commands on host reset Finn Thain
2018-09-27 1:17 ` [PATCH 03/10] NCR5380: Have NCR5380_select() return a bool Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 05/10] NCR5380: Use DRIVER_SENSE to indicate valid sense data Finn Thain
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 419033643015..b9a3eb0647e4 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 05/10] NCR5380: Use DRIVER_SENSE to indicate valid sense data
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (2 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 09/10] NCR5380: Handle BUS FREE during reselection Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 10/10] NCR5380: Check for bus reset Finn Thain
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5226164cfa65..e96a48b9e86c 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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);
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 10/10] NCR5380: Check for bus reset
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (3 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 05/10] NCR5380: Use DRIVER_SENSE to indicate valid sense data Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 08/10] NCR5380: Don't call dsprintk() following reselection interrupt Finn Thain
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 74 ++++++++++++++++++++++++++++++--------------------
1 file changed, 45 insertions(+), 29 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index b9a3eb0647e4..8429c855701f 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 08/10] NCR5380: Don't call dsprintk() following reselection interrupt
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (4 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 10/10] NCR5380: Check for bus reset Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 06/10] NCR5380: Check for invalid reselection target Finn Thain
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5826421146ba..419033643015 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 06/10] NCR5380: Check for invalid reselection target
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (5 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 08/10] NCR5380: Don't call dsprintk() following reselection interrupt Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 07/10] NCR5380: Don't clear busy flag when abort fails Finn Thain
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index e96a48b9e86c..3058b68b6740 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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");
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 07/10] NCR5380: Don't clear busy flag when abort fails
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (6 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 06/10] NCR5380: Check for invalid reselection target Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 02/10] NCR5380: Reduce goto statements in NCR5380_select() Finn Thain
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
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(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 3058b68b6740..5826421146ba 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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);
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 02/10] NCR5380: Reduce goto statements in NCR5380_select()
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (7 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 07/10] NCR5380: Don't clear busy flag when abort fails Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-27 1:17 ` [PATCH 04/10] NCR5380: Withhold disconnect privilege for REQUEST SENSE Finn Thain
2018-09-28 6:29 ` [PATCH 00/10] NCR5380: Various improvements Martin K. Petersen
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
Replace a 'goto' statement with a simple 'return' where possible.
This improves readability. No functional change.
Tested-by: Michael Schmitz <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 80c20ab4fc53..c6d10780febe 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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",
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 04/10] NCR5380: Withhold disconnect privilege for REQUEST SENSE
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (8 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 02/10] NCR5380: Reduce goto statements in NCR5380_select() Finn Thain
@ 2018-09-27 1:17 ` Finn Thain
2018-09-28 6:29 ` [PATCH 00/10] NCR5380: Various improvements Martin K. Petersen
10 siblings, 0 replies; 12+ messages in thread
From: Finn Thain @ 2018-09-27 1:17 UTC (permalink / raw)
To: James E.J. Bottomley, Martin K. Petersen
Cc: Michael Schmitz, Hannes Reinecke, Ondrey Zary, linux-scsi,
linux-kernel
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 <schmitzmic@gmail.com>
Tested-by: Michael Schmitz <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 7be1ba2b9e4e..5226164cfa65 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -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;
--
2.16.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 00/10] NCR5380: Various improvements
2018-09-27 1:17 [PATCH 00/10] NCR5380: Various improvements Finn Thain
` (9 preceding siblings ...)
2018-09-27 1:17 ` [PATCH 04/10] NCR5380: Withhold disconnect privilege for REQUEST SENSE Finn Thain
@ 2018-09-28 6:29 ` Martin K. Petersen
10 siblings, 0 replies; 12+ messages in thread
From: Martin K. Petersen @ 2018-09-28 6:29 UTC (permalink / raw)
To: Finn Thain
Cc: James E.J. Bottomley, Martin K. Petersen, Michael Schmitz,
Hannes Reinecke, Ondrey Zary, linux-scsi, linux-kernel
Finn,
> 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!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 12+ messages in thread