From: Finn Thain <fthain@telegraphics.com.au>
To: "James E.J. Bottomley" <JBottomley@odin.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
Michael Schmitz <schmitzmic@gmail.com>,
<linux-m68k@vger.kernel.org>, <linux-scsi@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: [PATCH 4/6] ncr5380: Forget aborted commands
Date: Tue, 23 Feb 2016 10:07:07 +1100 [thread overview]
Message-ID: <20160222230705.004684812@telegraphics.com.au> (raw)
In-Reply-To: 20160222230703.994951661@telegraphics.com.au
[-- Attachment #1: ncr5380-always-forget-aborted-commands --]
[-- Type: text/plain, Size: 9445 bytes --]
The list structures and related logic used in the NCR5380 driver mean that
a command cannot be queued twice (i.e. can't appear on more than one queue
and can't appear on the same queue more than once).
The abort handler must forget the command so that the mid-layer can re-use
it. E.g. the ML may send it back to the LLD via via scsi_eh_get_sense().
Fix this and also fix two error paths, so that commands get forgotten iff
completed.
Fixes: 8b00c3d5d40d ("ncr5380: Implement new eh_abort_handler")
Tested-by: Michael Schmitz <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/scsi/NCR5380.c | 62 +++++++++++--------------------------------
drivers/scsi/atari_NCR5380.c | 62 +++++++++++--------------------------------
2 files changed, 34 insertions(+), 90 deletions(-)
Index: linux/drivers/scsi/NCR5380.c
===================================================================
--- linux.orig/drivers/scsi/NCR5380.c 2016-02-23 10:06:58.000000000 +1100
+++ linux/drivers/scsi/NCR5380.c 2016-02-23 10:07:00.000000000 +1100
@@ -1796,6 +1796,7 @@ static void NCR5380_information_transfer
do_abort(instance);
cmd->result = DID_ERROR << 16;
complete_cmd(instance, cmd);
+ hostdata->connected = NULL;
return;
#endif
case PHASE_DATAIN:
@@ -1845,7 +1846,6 @@ static void NCR5380_information_transfer
sink = 1;
do_abort(instance);
cmd->result = DID_ERROR << 16;
- complete_cmd(instance, cmd);
/* XXX - need to source or sink data here, as appropriate */
} else
cmd->SCp.this_residual -= transfersize - len;
@@ -2294,14 +2294,14 @@ static bool list_del_cmd(struct list_hea
* [disconnected -> connected ->]...
* [autosense -> connected ->] done
*
- * If cmd is unissued then just remove it.
- * If cmd is disconnected, try to select the target.
- * If cmd is connected, try to send an abort message.
- * If cmd is waiting for autosense, give it a chance to complete but check
- * that it isn't left connected.
* If cmd was not found at all then presumably it has already been completed,
* in which case return SUCCESS to try to avoid further EH measures.
+ *
* If the command has not completed yet, we must not fail to find it.
+ * We have no option but to forget the aborted command (even if it still
+ * lacks sense data). The mid-layer may re-issue a command that is in error
+ * recovery (see scsi_send_eh_cmnd), but the logic and data structures in
+ * this driver are such that a command can appear on one queue only.
*
* The lock protects driver data structures, but EH handlers also use it
* to serialize their own execution and prevent their own re-entry.
@@ -2327,6 +2327,7 @@ static int NCR5380_abort(struct scsi_cmn
"abort: removed %p from issue queue\n", cmd);
cmd->result = DID_ABORT << 16;
cmd->scsi_done(cmd); /* No tag or busy flag to worry about */
+ goto out;
}
if (hostdata->selecting == cmd) {
@@ -2344,6 +2345,8 @@ static int NCR5380_abort(struct scsi_cmn
/* Can't call NCR5380_select() and send ABORT because that
* means releasing the lock. Need a bus reset.
*/
+ set_host_byte(cmd, DID_ERROR);
+ complete_cmd(instance, cmd);
result = FAILED;
goto out;
}
@@ -2351,45 +2354,9 @@ static int NCR5380_abort(struct scsi_cmn
if (hostdata->connected == cmd) {
dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p is connected\n", cmd);
hostdata->connected = NULL;
- if (do_abort(instance)) {
- set_host_byte(cmd, DID_ERROR);
- complete_cmd(instance, cmd);
- result = FAILED;
- goto out;
- }
- set_host_byte(cmd, DID_ABORT);
#ifdef REAL_DMA
hostdata->dma_len = 0;
#endif
- if (cmd->cmnd[0] == REQUEST_SENSE)
- complete_cmd(instance, cmd);
- else {
- struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd);
-
- /* Perform autosense for this command */
- list_add(&ncmd->list, &hostdata->autosense);
- }
- }
-
- if (list_find_cmd(&hostdata->autosense, cmd)) {
- dsprintk(NDEBUG_ABORT, instance,
- "abort: found %p on sense queue\n", cmd);
- spin_unlock_irqrestore(&hostdata->lock, flags);
- queue_work(hostdata->work_q, &hostdata->main_task);
- msleep(1000);
- spin_lock_irqsave(&hostdata->lock, flags);
- if (list_del_cmd(&hostdata->autosense, cmd)) {
- dsprintk(NDEBUG_ABORT, instance,
- "abort: removed %p from sense queue\n", cmd);
- set_host_byte(cmd, DID_ABORT);
- complete_cmd(instance, cmd);
- goto out;
- }
- }
-
- if (hostdata->connected == cmd) {
- dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p is connected\n", cmd);
- hostdata->connected = NULL;
if (do_abort(instance)) {
set_host_byte(cmd, DID_ERROR);
complete_cmd(instance, cmd);
@@ -2397,9 +2364,14 @@ static int NCR5380_abort(struct scsi_cmn
goto out;
}
set_host_byte(cmd, DID_ABORT);
-#ifdef REAL_DMA
- hostdata->dma_len = 0;
-#endif
+ complete_cmd(instance, cmd);
+ goto out;
+ }
+
+ 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);
}
Index: linux/drivers/scsi/atari_NCR5380.c
===================================================================
--- linux.orig/drivers/scsi/atari_NCR5380.c 2016-02-23 10:06:58.000000000 +1100
+++ linux/drivers/scsi/atari_NCR5380.c 2016-02-23 10:07:00.000000000 +1100
@@ -1907,6 +1907,7 @@ static void NCR5380_information_transfer
do_abort(instance);
cmd->result = DID_ERROR << 16;
complete_cmd(instance, cmd);
+ hostdata->connected = NULL;
return;
#endif
case PHASE_DATAIN:
@@ -1964,7 +1965,6 @@ static void NCR5380_information_transfer
sink = 1;
do_abort(instance);
cmd->result = DID_ERROR << 16;
- complete_cmd(instance, cmd);
/* XXX - need to source or sink data here, as appropriate */
} else {
#ifdef REAL_DMA
@@ -2489,14 +2489,14 @@ static bool list_del_cmd(struct list_hea
* [disconnected -> connected ->]...
* [autosense -> connected ->] done
*
- * If cmd is unissued then just remove it.
- * If cmd is disconnected, try to select the target.
- * If cmd is connected, try to send an abort message.
- * If cmd is waiting for autosense, give it a chance to complete but check
- * that it isn't left connected.
* If cmd was not found at all then presumably it has already been completed,
* in which case return SUCCESS to try to avoid further EH measures.
+ *
* If the command has not completed yet, we must not fail to find it.
+ * We have no option but to forget the aborted command (even if it still
+ * lacks sense data). The mid-layer may re-issue a command that is in error
+ * recovery (see scsi_send_eh_cmnd), but the logic and data structures in
+ * this driver are such that a command can appear on one queue only.
*
* The lock protects driver data structures, but EH handlers also use it
* to serialize their own execution and prevent their own re-entry.
@@ -2522,6 +2522,7 @@ static int NCR5380_abort(struct scsi_cmn
"abort: removed %p from issue queue\n", cmd);
cmd->result = DID_ABORT << 16;
cmd->scsi_done(cmd); /* No tag or busy flag to worry about */
+ goto out;
}
if (hostdata->selecting == cmd) {
@@ -2539,6 +2540,8 @@ static int NCR5380_abort(struct scsi_cmn
/* Can't call NCR5380_select() and send ABORT because that
* means releasing the lock. Need a bus reset.
*/
+ set_host_byte(cmd, DID_ERROR);
+ complete_cmd(instance, cmd);
result = FAILED;
goto out;
}
@@ -2546,45 +2549,9 @@ static int NCR5380_abort(struct scsi_cmn
if (hostdata->connected == cmd) {
dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p is connected\n", cmd);
hostdata->connected = NULL;
- if (do_abort(instance)) {
- set_host_byte(cmd, DID_ERROR);
- complete_cmd(instance, cmd);
- result = FAILED;
- goto out;
- }
- set_host_byte(cmd, DID_ABORT);
#ifdef REAL_DMA
hostdata->dma_len = 0;
#endif
- if (cmd->cmnd[0] == REQUEST_SENSE)
- complete_cmd(instance, cmd);
- else {
- struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd);
-
- /* Perform autosense for this command */
- list_add(&ncmd->list, &hostdata->autosense);
- }
- }
-
- if (list_find_cmd(&hostdata->autosense, cmd)) {
- dsprintk(NDEBUG_ABORT, instance,
- "abort: found %p on sense queue\n", cmd);
- spin_unlock_irqrestore(&hostdata->lock, flags);
- queue_work(hostdata->work_q, &hostdata->main_task);
- msleep(1000);
- spin_lock_irqsave(&hostdata->lock, flags);
- if (list_del_cmd(&hostdata->autosense, cmd)) {
- dsprintk(NDEBUG_ABORT, instance,
- "abort: removed %p from sense queue\n", cmd);
- set_host_byte(cmd, DID_ABORT);
- complete_cmd(instance, cmd);
- goto out;
- }
- }
-
- if (hostdata->connected == cmd) {
- dsprintk(NDEBUG_ABORT, instance, "abort: cmd %p is connected\n", cmd);
- hostdata->connected = NULL;
if (do_abort(instance)) {
set_host_byte(cmd, DID_ERROR);
complete_cmd(instance, cmd);
@@ -2592,9 +2559,14 @@ static int NCR5380_abort(struct scsi_cmn
goto out;
}
set_host_byte(cmd, DID_ABORT);
-#ifdef REAL_DMA
- hostdata->dma_len = 0;
-#endif
+ complete_cmd(instance, cmd);
+ goto out;
+ }
+
+ 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);
}
next prev parent reply other threads:[~2016-02-22 23:14 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-22 23:07 [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 Finn Thain
2016-02-22 23:07 ` [PATCH 1/6] ncr5380: Correctly clear command pointers and lists after bus reset Finn Thain
2016-02-22 23:07 ` [PATCH 2/6] ncr5380: Dont release lock for PIO transfer Finn Thain
2016-02-22 23:07 ` [PATCH 3/6] ncr5380: Dont re-enter NCR5380_select() Finn Thain
2016-02-22 23:07 ` Finn Thain [this message]
2016-02-22 23:07 ` [PATCH 5/6] ncr5380: Fix NCR5380_select() EH checks and result handling Finn Thain
2016-02-22 23:07 ` [PATCH 6/6] ncr5380: Call scsi_eh_prep_cmnd() and scsi_eh_restore_cmnd() as and when appropriate Finn Thain
2016-03-01 2:31 ` [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 Martin K. Petersen
2016-03-01 3:32 ` Finn Thain
2016-03-01 13:00 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160222230705.004684812@telegraphics.com.au \
--to=fthain@telegraphics.com.au \
--cc=JBottomley@odin.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=schmitzmic@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox