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

Groups > linux.kernel > #1340074 > unrolled thread

[PATCH 0/6] ncr5380: Exception handling fixes for v4.5

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 Finn Thain <fthain@telegraphics.com.au> - 2016-02-23 00:20 +0100
    [PATCH 4/6] ncr5380: Forget aborted commands Finn Thain <fthain@telegraphics.com.au> - 2016-02-23 00:20 +0100
    Re: [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-03-01 03:40 +0100
      Re: [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 Finn Thain <fthain@telegraphics.com.au> - 2016-03-01 04:40 +0100
        Re: [PATCH 0/6] ncr5380: Exception handling fixes for v4.5 "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-03-01 14:10 +0100

#1340074 — [PATCH 0/6] ncr5380: Exception handling fixes for v4.5

These patches fix some exception handling and autosense bugs that I
accidentally introduced in v4.5-rc1.

The error recovery and autosense code in these drivers has been unstable
for a long time. Despite that, v4.5-rc1 shows a regression in as much as
it exposes a bug in the aranym emulator. This leads to error recovery,
which can crash.

Also, Michael Schmitz reported some crashes involving abort handling
for a certain target device. And Dan Carpenter found a NULL pointer deref
in the new bus reset code.

Error recovery and autosense are stable with these patches.

I tested them using a Domex 3191D PCI card. Errors during IO were
simulated by sending bus resets and unplugging/replugging the SCSI
cables. Some of these patches fix bugs that only affect more capable
hardware (like Atari). Thanks to Michael Schmitz for patiently testing
those.

Please review this series for v4.5.

---
 drivers/scsi/NCR5380.c       |  133 +++++++++++++++++++------------------------
 drivers/scsi/atari_NCR5380.c |  133 +++++++++++++++++++------------------------
 2 files changed, 118 insertions(+), 148 deletions(-)

[toc] | [next] | [standalone]

#1340075 — [PATCH 4/6] ncr5380: Forget aborted commands

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);
 	}
 

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

#1346309

>>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:

Finn> These patches fix some exception handling and autosense bugs that
Finn> I accidentally introduced in v4.5-rc1.

Finn> drivers/scsi/NCR5380.c       |  133 +++++++++++++++++++------------------------
Finn> drivers/scsi/atari_NCR5380.c |  133 +++++++++++++++++++------------------------
Finn> 2 files changed, 118 insertions(+), 148 deletions(-)

This is a pretty big lump of changes for a 4.5 bug fix!

Given the limited exposure based on the nature of the affected hardware
I have queued them for 4.6.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

#1346346

On Mon, 29 Feb 2016, Martin K. Petersen wrote:

> >>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:
> 
> Finn> These patches fix some exception handling and autosense bugs that
> Finn> I accidentally introduced in v4.5-rc1.
> 
> Finn> drivers/scsi/NCR5380.c       |  133 +++++++++++++++++++------------------------
> Finn> drivers/scsi/atari_NCR5380.c |  133 +++++++++++++++++++------------------------
> Finn> 2 files changed, 118 insertions(+), 148 deletions(-)
> 
> This is a pretty big lump of changes for a 4.5 bug fix!

I'm sure this is a lot of rework when compared to the high standard set by 
well-funded corporate contributions.

However, I don't have anyone paying me to write and execute thorough test 
plans for error paths on a wide variety of different hardware platforms.

Nonetheless, these error paths have now been thoroughly tested on several 
platforms.

Please keep in mind that these fixes are all rework of the changes I made 
in -rc1. So this submission is
   2 files changed, 118 insertions(+), 148 deletions(-)
which is all rework of the previous submission, which was
   18 files changed, 2940 insertions(+), 3688 deletions(-)

Better than 5% rate of rework. Bugs-per-line-of-code should drop quite 
quickly at that rate, presuming fixes get merged.

> 
> Given the limited exposure based on the nature of the affected hardware 
> I have queued them for 4.6.
> 

The affected hardware doesn't matter (though it is likely outside of any 
commercial support contract). It's the affected users that interest me.

-- 

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

#1346603

>>>>> "Finn" == Finn Thain <fthain@telegraphics.com.au> writes:

Finn,

>> This is a pretty big lump of changes for a 4.5 bug fix!

Finn> Please keep in mind that these fixes are all rework of the changes
Finn> I made in -rc1. So this submission is 2 files changed, 118
Finn> insertions(+), 148 deletions(-) which is all rework of the
Finn> previous submission, which was 18 files changed, 2940
Finn> insertions(+), 3688 deletions(-)

I am well aware of and appreciate the huge amount of work you have done.

My concern is merely that Linus is going to get pretty upset with such a
big delta this late in the 4.5 cycle. By comparison, our other fixes at
this time are in the 1-5 line bucket!

I think putting the fixes into 4.6 and tagging them for stable is a
better (less emperor penguin wrath-inducing) approach.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web