All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: linux-scsi@vger.kernel.org
Cc: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Subject: [PATCH 06/11] isci: Fix task management for SMP, SATA and on dev remove.
Date: Thu, 27 Oct 2011 15:05:16 -0700	[thread overview]
Message-ID: <20111027220516.4941.22954.stgit@localhost6.localdomain6> (raw)
In-Reply-To: <20111027220346.4941.43260.stgit@localhost6.localdomain6>

From: Jeff Skirvin <jeffrey.d.skirvin@intel.com>

libsas uses the LLDD abort task interface to handle I/O timeouts
in the SATA/STP and SMP discovery paths, so this change will terminate
STP/SMP requests. Also, if the device is gone, the lldd will prevent
libsas from further escalations in the error handler.

Signed-off-by: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/scsi/isci/remote_device.c |   47 ----------
 drivers/scsi/isci/remote_device.h |    2 
 drivers/scsi/isci/task.c          |  169 ++++++++++++++-----------------------
 drivers/scsi/isci/task.h          |   33 +------
 4 files changed, 67 insertions(+), 184 deletions(-)

diff --git a/drivers/scsi/isci/remote_device.c b/drivers/scsi/isci/remote_device.c
index fbf9ce2..20c77ed 100644
--- a/drivers/scsi/isci/remote_device.c
+++ b/drivers/scsi/isci/remote_device.c
@@ -1438,53 +1438,6 @@ int isci_remote_device_found(struct domain_device *domain_dev)
 
 	return status == SCI_SUCCESS ? 0 : -ENODEV;
 }
-/**
- * isci_device_is_reset_pending() - This function will check if there is any
- *    pending reset condition on the device.
- * @request: This parameter is the isci_device object.
- *
- * true if there is a reset pending for the device.
- */
-bool isci_device_is_reset_pending(
-	struct isci_host *isci_host,
-	struct isci_remote_device *isci_device)
-{
-	struct isci_request *isci_request;
-	struct isci_request *tmp_req;
-	bool reset_is_pending = false;
-	unsigned long flags;
-
-	dev_dbg(&isci_host->pdev->dev,
-		"%s: isci_device = %p\n", __func__, isci_device);
-
-	spin_lock_irqsave(&isci_host->scic_lock, flags);
-
-	/* Check for reset on all pending requests. */
-	list_for_each_entry_safe(isci_request, tmp_req,
-				 &isci_device->reqs_in_process, dev_node) {
-		dev_dbg(&isci_host->pdev->dev,
-			"%s: isci_device = %p request = %p\n",
-			__func__, isci_device, isci_request);
-
-		if (isci_request->ttype == io_task) {
-			struct sas_task *task = isci_request_access_task(
-				isci_request);
-
-			spin_lock(&task->task_state_lock);
-			if (task->task_state_flags & SAS_TASK_NEED_DEV_RESET)
-				reset_is_pending = true;
-			spin_unlock(&task->task_state_lock);
-		}
-	}
-
-	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
-
-	dev_dbg(&isci_host->pdev->dev,
-		"%s: isci_device = %p reset_is_pending = %d\n",
-		__func__, isci_device, reset_is_pending);
-
-	return reset_is_pending;
-}
 
 /**
  * isci_device_clear_reset_pending() - This function will clear if any pending
diff --git a/drivers/scsi/isci/remote_device.h b/drivers/scsi/isci/remote_device.h
index e1747ea..bee6dd2 100644
--- a/drivers/scsi/isci/remote_device.h
+++ b/drivers/scsi/isci/remote_device.h
@@ -132,8 +132,6 @@ void isci_remote_device_nuke_requests(struct isci_host *ihost,
 				      struct isci_remote_device *idev);
 void isci_remote_device_gone(struct domain_device *domain_dev);
 int isci_remote_device_found(struct domain_device *domain_dev);
-bool isci_device_is_reset_pending(struct isci_host *ihost,
-				  struct isci_remote_device *idev);
 void isci_device_clear_reset_pending(struct isci_host *ihost,
 				     struct isci_remote_device *idev);
 /**
diff --git a/drivers/scsi/isci/task.c b/drivers/scsi/isci/task.c
index c1f439b..ec85beb 100644
--- a/drivers/scsi/isci/task.c
+++ b/drivers/scsi/isci/task.c
@@ -920,22 +920,14 @@ int isci_task_lu_reset(struct domain_device *domain_device, u8 *lun)
 		"%s: domain_device=%p, isci_host=%p; isci_device=%p\n",
 		 __func__, domain_device, isci_host, isci_device);
 
-	if (isci_device)
-		set_bit(IDEV_EH, &isci_device->flags);
+	if (!isci_device) {
+		/* If the device is gone, stop the escalations. */
+		dev_dbg(&isci_host->pdev->dev, "%s: No dev\n", __func__);
 
-	/* If there is a device reset pending on any request in the
-	 * device's list, fail this LUN reset request in order to
-	 * escalate to the device reset.
-	 */
-	if (!isci_device ||
-	    isci_device_is_reset_pending(isci_host, isci_device)) {
-		dev_dbg(&isci_host->pdev->dev,
-			 "%s: No dev (%p), or "
-			 "RESET PENDING: domain_device=%p\n",
-			 __func__, isci_device, domain_device);
-		ret = TMF_RESP_FUNC_FAILED;
+		ret = TMF_RESP_FUNC_COMPLETE;
 		goto out;
 	}
+	set_bit(IDEV_EH, &isci_device->flags);
 
 	/* Send the task management part of the reset. */
 	if (sas_protocol_ata(domain_device->tproto)) {
@@ -1044,7 +1036,7 @@ int isci_task_abort_task(struct sas_task *task)
 	struct isci_tmf           tmf;
 	int                       ret = TMF_RESP_FUNC_FAILED;
 	unsigned long             flags;
-	bool                      any_dev_reset = false;
+	int                       perform_termination = 0;
 
 	/* Get the isci_request reference from the task.  Note that
 	 * this check does not depend on the pending request list
@@ -1066,89 +1058,34 @@ int isci_task_abort_task(struct sas_task *task)
 	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 
 	dev_dbg(&isci_host->pdev->dev,
-		"%s: task = %p\n", __func__, task);
-
-	if (!isci_device || !old_request)
-		goto out;
+		"%s: dev = %p, task = %p, old_request == %p\n",
+		__func__, isci_device, task, old_request);
 
-	set_bit(IDEV_EH, &isci_device->flags);
-
-	/* This version of the driver will fail abort requests for
-	 * SATA/STP.  Failing the abort request this way will cause the
-	 * SCSI error handler thread to escalate to LUN reset
-	 */
-	if (sas_protocol_ata(task->task_proto)) {
-		dev_dbg(&isci_host->pdev->dev,
-			    " task %p is for a STP/SATA device;"
-			    " returning TMF_RESP_FUNC_FAILED\n"
-			    " to cause a LUN reset...\n", task);
-		goto out;
-	}
-
-	dev_dbg(&isci_host->pdev->dev,
-		"%s: old_request == %p\n", __func__, old_request);
-
-	any_dev_reset = isci_device_is_reset_pending(isci_host, isci_device);
-
-	spin_lock_irqsave(&task->task_state_lock, flags);
-
-	any_dev_reset = any_dev_reset || (task->task_state_flags & SAS_TASK_NEED_DEV_RESET);
+	if (isci_device)
+		set_bit(IDEV_EH, &isci_device->flags);
 
-	/* If the extraction of the request reference from the task
-	 * failed, then the request has been completed (or if there is a
-	 * pending reset then this abort request function must be failed
-	 * in order to escalate to the target reset).
+	/* Device reset conditions signalled in task_state_flags are the
+	 * responsbility of libsas to observe at the start of the error
+	 * handler thread.
 	 */
-	if ((old_request == NULL) || any_dev_reset) {
-
-		/* If the device reset task flag is set, fail the task
-		 * management request.  Otherwise, the original request
-		 * has completed.
-		 */
-		if (any_dev_reset) {
-
-			/* Turn off the task's DONE to make sure this
-			 * task is escalated to a target reset.
-			 */
-			task->task_state_flags &= ~SAS_TASK_STATE_DONE;
-
-			/* Make the reset happen as soon as possible. */
-			task->task_state_flags |= SAS_TASK_NEED_DEV_RESET;
-
-			spin_unlock_irqrestore(&task->task_state_lock, flags);
-
-			/* Fail the task management request in order to
-			 * escalate to the target reset.
-			 */
-			ret = TMF_RESP_FUNC_FAILED;
-
-			dev_dbg(&isci_host->pdev->dev,
-				"%s: Failing task abort in order to "
-				"escalate to target reset because\n"
-				"SAS_TASK_NEED_DEV_RESET is set for "
-				"task %p on dev %p\n",
-				__func__, task, isci_device);
-
-
-		} else {
-			/* The request has already completed and there
-			 * is nothing to do here other than to set the task
-			 * done bit, and indicate that the task abort function
-			 * was sucessful.
-			 */
-			isci_set_task_doneflags(task);
-
-			spin_unlock_irqrestore(&task->task_state_lock, flags);
+	if (!isci_device || !old_request) {
+		/* The request has already completed and there
+		* is nothing to do here other than to set the task
+		* done bit, and indicate that the task abort function
+		* was sucessful.
+		*/
+		spin_lock_irqsave(&task->task_state_lock, flags);
+		task->task_state_flags |= SAS_TASK_STATE_DONE;
+		task->task_state_flags &= ~(SAS_TASK_AT_INITIATOR |
+					    SAS_TASK_STATE_PENDING);
+		spin_unlock_irqrestore(&task->task_state_lock, flags);
 
-			ret = TMF_RESP_FUNC_COMPLETE;
+		ret = TMF_RESP_FUNC_COMPLETE;
 
-			dev_dbg(&isci_host->pdev->dev,
-				"%s: abort task not needed for %p\n",
-				__func__, task);
-		}
+		dev_dbg(&isci_host->pdev->dev,
+			"%s: abort task not needed for %p\n",
+			__func__, task);
 		goto out;
-	} else {
-		spin_unlock_irqrestore(&task->task_state_lock, flags);
 	}
 
 	spin_lock_irqsave(&isci_host->scic_lock, flags);
@@ -1177,24 +1114,44 @@ int isci_task_abort_task(struct sas_task *task)
 		goto out;
 	}
 	if (task->task_proto == SAS_PROTOCOL_SMP ||
+	    sas_protocol_ata(task->task_proto) ||
 	    test_bit(IREQ_COMPLETE_IN_TARGET, &old_request->flags)) {
 
 		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 
 		dev_dbg(&isci_host->pdev->dev,
-			"%s: SMP request (%d)"
+			"%s: %s request"
 			" or complete_in_target (%d), thus no TMF\n",
-			__func__, (task->task_proto == SAS_PROTOCOL_SMP),
+			__func__,
+			((task->task_proto == SAS_PROTOCOL_SMP)
+				? "SMP"
+				: (sas_protocol_ata(task->task_proto)
+					? "SATA/STP"
+					: "<other>")
+			 ),
 			test_bit(IREQ_COMPLETE_IN_TARGET, &old_request->flags));
 
-		/* Set the state on the task. */
-		isci_task_all_done(task);
-
-		ret = TMF_RESP_FUNC_COMPLETE;
+		if (test_bit(IREQ_COMPLETE_IN_TARGET, &old_request->flags)) {
+			spin_lock_irqsave(&task->task_state_lock, flags);
+			task->task_state_flags |= SAS_TASK_STATE_DONE;
+			task->task_state_flags &= ~(SAS_TASK_AT_INITIATOR |
+						    SAS_TASK_STATE_PENDING);
+			spin_unlock_irqrestore(&task->task_state_lock, flags);
+			ret = TMF_RESP_FUNC_COMPLETE;
+		} else {
+			spin_lock_irqsave(&task->task_state_lock, flags);
+			task->task_state_flags &= ~(SAS_TASK_AT_INITIATOR |
+						    SAS_TASK_STATE_PENDING);
+			spin_unlock_irqrestore(&task->task_state_lock, flags);
+		}
 
-		/* Stopping and SMP devices are not sent a TMF, and are not
-		 * reset, but the outstanding I/O request is terminated below.
+		/* STP and SMP devices are not sent a TMF, but the
+		 * outstanding I/O request is terminated below.  This is
+		 * because SATA/STP and SMP discovery path timeouts directly
+		 * call the abort task interface for cleanup.
 		 */
+		perform_termination = 1;
+
 	} else {
 		/* Fill in the tmf stucture */
 		isci_task_build_abort_task_tmf(&tmf, isci_tmf_ssp_task_abort,
@@ -1203,22 +1160,24 @@ int isci_task_abort_task(struct sas_task *task)
 
 		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 
-		#define ISCI_ABORT_TASK_TIMEOUT_MS 500 /* half second timeout. */
+		#define ISCI_ABORT_TASK_TIMEOUT_MS 500 /* 1/2 second timeout */
 		ret = isci_task_execute_tmf(isci_host, isci_device, &tmf,
 					    ISCI_ABORT_TASK_TIMEOUT_MS);
 
-		if (ret != TMF_RESP_FUNC_COMPLETE)
+		if (ret == TMF_RESP_FUNC_COMPLETE)
+			perform_termination = 1;
+		else
 			dev_dbg(&isci_host->pdev->dev,
-				"%s: isci_task_send_tmf failed\n",
-				__func__);
+				"%s: isci_task_send_tmf failed\n", __func__);
 	}
-	if (ret == TMF_RESP_FUNC_COMPLETE) {
+	if (perform_termination) {
 		set_bit(IREQ_COMPLETE_IN_TARGET, &old_request->flags);
 
 		/* Clean up the request on our side, and wait for the aborted
 		 * I/O to complete.
 		 */
-		isci_terminate_request_core(isci_host, isci_device, old_request);
+		isci_terminate_request_core(isci_host, isci_device,
+					    old_request);
 	}
 
 	/* Make sure we do not leave a reference to aborted_io_completion */
diff --git a/drivers/scsi/isci/task.h b/drivers/scsi/isci/task.h
index c9ccd0b..bc78c0a 100644
--- a/drivers/scsi/isci/task.h
+++ b/drivers/scsi/isci/task.h
@@ -226,35 +226,6 @@ enum isci_completion_selection {
 	isci_perform_error_io_completion        /* Use sas_task_abort */
 };
 
-static inline void isci_set_task_doneflags(
-	struct sas_task *task)
-{
-	/* Since no futher action will be taken on this task,
-	 * make sure to mark it complete from the lldd perspective.
-	 */
-	task->task_state_flags |= SAS_TASK_STATE_DONE;
-	task->task_state_flags &= ~SAS_TASK_AT_INITIATOR;
-	task->task_state_flags &= ~SAS_TASK_STATE_PENDING;
-}
-/**
- * isci_task_all_done() - This function clears the task bits to indicate the
- *    LLDD is done with the task.
- *
- *
- */
-static inline void isci_task_all_done(
-	struct sas_task *task)
-{
-	unsigned long flags;
-
-	/* Since no futher action will be taken on this task,
-	 * make sure to mark it complete from the lldd perspective.
-	 */
-	spin_lock_irqsave(&task->task_state_lock, flags);
-	isci_set_task_doneflags(task);
-	spin_unlock_irqrestore(&task->task_state_lock, flags);
-}
-
 /**
  * isci_task_set_completion_status() - This function sets the completion status
  *    for the request.
@@ -336,7 +307,9 @@ isci_task_set_completion_status(
 		/* Fall through to the normal case... */
 	case isci_perform_normal_io_completion:
 		/* Normal notification (task_done) */
-		isci_set_task_doneflags(task);
+		task->task_state_flags |= SAS_TASK_STATE_DONE;
+		task->task_state_flags &= ~(SAS_TASK_AT_INITIATOR |
+					    SAS_TASK_STATE_PENDING);
 		break;
 	default:
 		WARN_ONCE(1, "unknown task_notification_selection: %d\n",


  parent reply	other threads:[~2011-10-27 22:05 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-10-27 22:04 [PATCH 00/11] isci fixes: hotplug Dan Williams
2011-10-27 22:04 ` [PATCH 01/11] isci: Lookup device references through requests in completions Dan Williams
2011-10-27 22:04 ` [PATCH 02/11] isci: Immediately fail I/O to removed devices Dan Williams
2011-10-27 22:05 ` [PATCH 03/11] isci: Fix tag leak in tasks and terminated requests Dan Williams
2011-10-27 22:05 ` [PATCH 04/11] isci: Handle task request timeouts correctly Dan Williams
2011-10-27 22:05 ` [PATCH 05/11] isci: No task_done callbacks in error handler paths Dan Williams
2011-10-27 22:05 ` Dan Williams [this message]
2011-10-27 22:05 ` [PATCH 07/11] isci: Remove redundant isci_request.ttype field Dan Williams
2011-10-31  9:23   ` James Bottomley
2011-10-27 22:05 ` [PATCH 08/11] isci: No need to manage the pending reset bit on pending requests Dan Williams
2011-10-27 22:05 ` [PATCH 09/11] isci: Fix hard reset timeout conditions Dan Williams
2011-10-27 22:05 ` [PATCH 10/11] isci: revert bcn filtering Dan Williams
2011-10-27 22:05 ` [PATCH 11/11] isci: overriding max_concurr_spinup oem parameter by max(oem, user) Dan Williams

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=20111027220516.4941.22954.stgit@localhost6.localdomain6 \
    --to=dan.j.williams@intel.com \
    --cc=jeffrey.d.skirvin@intel.com \
    --cc=linux-scsi@vger.kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.