isci: Handle timed-out request terminations correctly

In the situation where a termination of an I/O times-out,
make sure that the linkage from the request to the task
is severed completely.  Also make sure that the selection
of tasks to terminate occurs under scic_lock.
Signed-off-by: Jeff Skirvin <jeffrey.d.skirvin@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>

drivers/scsi/isci/request.c

@@ -2741,6 +2741,15 @@ static void isci_request_io_request_complete(struct isci_host *isci_host,
 		spin_unlock(&request->state_lock);
 		break;
 
+	case dead:
+		/* This was a terminated request that timed-out during the
+		 * termination process.  There is no task to complete to
+		 * libsas.
+		 */
+		complete_to_host = isci_perform_normal_io_completion;
+		spin_unlock(&request->state_lock);
+		break;
+
 	default:
 
 		/* The request is done from an SCU HW perspective. */

drivers/scsi/isci/task.c

@@ -567,31 +567,33 @@ static enum isci_request_status isci_task_validate_request_to_abort(
 	return old_state;
 }
 
+/**
+* isci_request_cleanup_completed_loiterer() - This function will take care of
+*    the final cleanup on any request which has been explicitly terminated.
+* @isci_host: This parameter specifies the ISCI host object
+* @isci_device: This is the device to which the request is pending.
+* @isci_request: This parameter specifies the terminated request object.
+* @task: This parameter is the libsas I/O request.
+*/
 static void isci_request_cleanup_completed_loiterer(
-	struct isci_host *isci_host,
+	struct isci_host          *isci_host,
 	struct isci_remote_device *isci_device,
-	struct isci_request *isci_request)
+	struct isci_request       *isci_request,
+	struct sas_task           *task)
 {
-	struct sas_task     *task;
-	unsigned long       flags;
-
-	task = (isci_request->ttype == io_task)
-		? isci_request_access_task(isci_request)
-		: NULL;
+	unsigned long flags;
 
 	dev_dbg(&isci_host->pdev->dev,
 		"%s: isci_device=%p, request=%p, task=%p\n",
 		__func__, isci_device, isci_request, task);
 
-	spin_lock_irqsave(&isci_host->scic_lock, flags);
-	list_del_init(&isci_request->dev_node);
-	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
-
 	if (task != NULL) {
 
 		spin_lock_irqsave(&task->task_state_lock, flags);
 		task->lldd_task = NULL;
 
+		task->task_state_flags &= ~SAS_TASK_NEED_DEV_RESET;
+
 		isci_set_task_doneflags(task);
 
 		/* If this task is not in the abort path, call task_done. */
@@ -602,61 +604,16 @@ static void isci_request_cleanup_completed_loiterer(
 		} else
 			spin_unlock_irqrestore(&task->task_state_lock, flags);
 	}
-	isci_request_free(isci_host, isci_request);
-}
-
-/**
-* @isci_termination_timed_out(): this function will deal with a request for
-* which the wait for termination has timed-out.
-*
-* @isci_host    This SCU.
-* @isci_request The I/O request being terminated.
-*/
-static void
-isci_termination_timed_out(
-	struct isci_host    * host,
-	struct isci_request * request
-	)
-{
-	unsigned long state_flags;
-
-	dev_warn(&host->pdev->dev,
-		"%s: host = %p; request = %p\n",
-		__func__, host, request);
 
-	/* At this point, the request to terminate
-	* has timed out. The best we can do is to
-	* have the request die a silent death
-	* if it ever completes.
-	*/
-	spin_lock_irqsave(&request->state_lock, state_flags);
-
-	if (request->status == started) {
-
-		/* Set the request state to "dead",
-		* and clear the task pointer so that an actual
-		* completion event callback doesn't do
-		* anything.
-		*/
-		request->status = dead;
-
-		/* Clear the timeout completion event pointer.*/
-		request->io_request_completion = NULL;
-
-		if (request->ttype == io_task) {
-
-			/* Break links with the sas_task. */
-			if (request->ttype_ptr.io_task_ptr != NULL) {
+	if (isci_request != NULL) {
+		spin_lock_irqsave(&isci_host->scic_lock, flags);
+		list_del_init(&isci_request->dev_node);
+		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 
-				request->ttype_ptr.io_task_ptr->lldd_task = NULL;
-				request->ttype_ptr.io_task_ptr            = NULL;
-			}
-		}
+		isci_request_free(isci_host, isci_request);
 	}
-	spin_unlock_irqrestore(&request->state_lock, state_flags);
 }
 
-
 /**
  * isci_terminate_request_core() - This function will terminate the given
  *    request, and wait for it to complete.  This function must only be called
@@ -666,7 +623,6 @@ isci_termination_timed_out(
  * @isci_device: The target.
  * @isci_request: The I/O request to be terminated.
  *
- *
  */
 static void isci_terminate_request_core(
 	struct isci_host *isci_host,
@@ -677,9 +633,10 @@ static void isci_terminate_request_core(
 	bool was_terminated         = false;
 	bool needs_cleanup_handling = false;
 	enum isci_request_status request_status;
-	unsigned long flags;
-	unsigned long timeout_remaining;
-
+	unsigned long     flags;
+	unsigned long     termination_completed = 1;
+	struct completion *io_request_completion;
+	struct sas_task   *task;
 
 	dev_dbg(&isci_host->pdev->dev,
 		"%s: device = %p; request = %p\n",
@@ -687,6 +644,12 @@ static void isci_terminate_request_core(
 
 	spin_lock_irqsave(&isci_host->scic_lock, flags);
 
+	io_request_completion = isci_request->io_request_completion;
+
+	task = (isci_request->ttype == io_task)
+		? isci_request_access_task(isci_request)
+		: NULL;
+
 	/* Note that we are not going to control
 	* the target to abort the request.
 	*/
@@ -715,119 +678,112 @@ static void isci_terminate_request_core(
 		dev_err(&isci_host->pdev->dev,
 			"%s: scic_controller_terminate_request"
 			" returned = 0x%x\n",
-			__func__,
-			status);
-		/* Clear the completion pointer from the request. */
+			__func__, status);
+
 		isci_request->io_request_completion = NULL;
 
 	} else {
 		if (was_terminated) {
 			dev_dbg(&isci_host->pdev->dev,
-				"%s: before completion wait (%p)\n",
-				__func__,
-				isci_request->io_request_completion);
+				"%s: before completion wait (%p/%p)\n",
+				__func__, isci_request, io_request_completion);
 
 			/* Wait here for the request to complete. */
-			#define TERMINATION_TIMEOUT_MSEC 50
-			timeout_remaining
+			#define TERMINATION_TIMEOUT_MSEC 500
+			termination_completed
 				= wait_for_completion_timeout(
-				   isci_request->io_request_completion,
+				   io_request_completion,
 				   msecs_to_jiffies(TERMINATION_TIMEOUT_MSEC));
 
-			if (!timeout_remaining) {
+			if (!termination_completed) {
+
+				/* The request to terminate has timed out.  */
+				spin_lock_irqsave(&isci_host->scic_lock,
+						  flags);
+
+				/* Check for state changes. */
+				if (!isci_request->terminated) {
+
+					/* The best we can do is to have the
+					 * request die a silent death if it
+					 * ever really completes.
+					 *
+					 * Set the request state to "dead",
+					 * and clear the task pointer so that
+					 * an actual completion event callback
+					 * doesn't do anything.
+					 */
+					isci_request->status = dead;
+					isci_request->io_request_completion
+						= NULL;
+
+					if (isci_request->ttype == io_task) {
+
+						/* Break links with the
+						* sas_task.
+						*/
+						isci_request->ttype_ptr.io_task_ptr
+							= NULL;
+					}
+				} else
+					termination_completed = 1;
+
+				spin_unlock_irqrestore(&isci_host->scic_lock,
+						       flags);
 
-				isci_termination_timed_out(isci_host,
-							   isci_request);
+				if (!termination_completed) {
 
-				dev_err(&isci_host->pdev->dev,
-					"%s: *** Timeout waiting for "
-					"termination(%p/%p)\n",
-					__func__,
-					isci_request->io_request_completion,
-					isci_request);
+					dev_err(&isci_host->pdev->dev,
+						"%s: *** Timeout waiting for "
+						"termination(%p/%p)\n",
+						__func__, io_request_completion,
+						isci_request);
 
-			} else
+					/* The request can no longer be referenced
+					 * safely since it may go away if the
+					 * termination every really does complete.
+					 */
+					isci_request = NULL;
+				}
+			}
+			if (termination_completed)
 				dev_dbg(&isci_host->pdev->dev,
-					"%s: after completion wait (%p)\n",
-					__func__,
-					isci_request->io_request_completion);
+					"%s: after completion wait (%p/%p)\n",
+					__func__, isci_request, io_request_completion);
 		}
-		/* Clear the completion pointer from the request. */
-		isci_request->io_request_completion = NULL;
 
-		/* Peek at the status of the request.  This will tell
-		* us if there was special handling on the request such that it
-		* needs to be detached and freed here.
-		*/
-		spin_lock_irqsave(&isci_request->state_lock, flags);
-		request_status = isci_request_get_state(isci_request);
-
-		if ((isci_request->ttype == io_task) /* TMFs are in their own thread */
-		    && ((request_status == aborted)
-			|| (request_status == aborting)
-			|| (request_status == terminating)
-			|| (request_status == completed)
-			|| (request_status == dead)
-			)
-		    ) {
-
-			/* The completion routine won't free a request in
-			* the aborted/aborting/etc. states, so we do
-			* it here.
-			*/
-			needs_cleanup_handling = true;
-		}
-		spin_unlock_irqrestore(&isci_request->state_lock, flags);
-
-		if (needs_cleanup_handling)
-			isci_request_cleanup_completed_loiterer(
-				isci_host, isci_device, isci_request
-				);
-	}
-}
+		if (termination_completed) {
 
-static void isci_terminate_request(
-	struct isci_host *isci_host,
-	struct isci_remote_device *isci_device,
-	struct isci_request *isci_request,
-	enum isci_request_status new_request_state)
-{
-	enum isci_request_status old_state;
-	DECLARE_COMPLETION_ONSTACK(request_completion);
+			isci_request->io_request_completion = NULL;
 
-	/* Change state to "new_request_state" if it is currently "started" */
-	old_state = isci_request_change_started_to_newstate(
-		isci_request,
-		&request_completion,
-		new_request_state
-		);
+			/* Peek at the status of the request.  This will tell
+			 * us if there was special handling on the request such that it
+			 * needs to be detached and freed here.
+			 */
+			spin_lock_irqsave(&isci_request->state_lock, flags);
+			request_status = isci_request_get_state(isci_request);
+
+			if ((isci_request->ttype == io_task) /* TMFs are in their own thread */
+			    && ((request_status == aborted)
+				|| (request_status == aborting)
+				|| (request_status == terminating)
+				|| (request_status == completed)
+				|| (request_status == dead)
+				)
+			    ) {
+
+				/* The completion routine won't free a request in
+				 * the aborted/aborting/etc. states, so we do
+				 * it here.
+				 */
+				needs_cleanup_handling = true;
+			}
+			spin_unlock_irqrestore(&isci_request->state_lock, flags);
 
-	if ((old_state == started) ||
-	    (old_state == completed) ||
-	    (old_state == aborting)) {
-
-		/* If the old_state is started:
-		 * This request was not already being aborted. If it had been,
-		 * then the aborting I/O (ie. the TMF request) would not be in
-		 * the aborting state, and thus would be terminated here.  Note
-		 * that since the TMF completion's call to the kernel function
-		 * "complete()" does not happen until the pending I/O request
-		 * terminate fully completes, we do not have to implement a
-		 * special wait here for already aborting requests - the
-		 * termination of the TMF request will force the request
-		 * to finish it's already started terminate.
-		 *
-		 * If old_state == completed:
-		 * This request completed from the SCU hardware perspective
-		 * and now just needs cleaning up in terms of freeing the
-		 * request and potentially calling up to libsas.
-		 *
-		 * If old_state == aborting:
-		 * This request has already gone through a TMF timeout, but may
-		 * not have been terminated; needs cleaning up at least.
-		 */
-		isci_terminate_request_core(isci_host, isci_device,
-					    isci_request);
+		}
+		if (needs_cleanup_handling)
+			isci_request_cleanup_completed_loiterer(
+				isci_host, isci_device, isci_request, task);
 	}
 }
 
@@ -850,9 +806,8 @@ void isci_terminate_pending_requests(
 	struct isci_request *request;
 	struct isci_request *next_request;
 	unsigned long       flags;
-	struct list_head    aborted_request_list;
-
-	INIT_LIST_HEAD(&aborted_request_list);
+	enum isci_request_status old_state;
+	DECLARE_COMPLETION_ONSTACK(request_completion);
 
 	dev_dbg(&isci_host->pdev->dev,
 		"%s: isci_device = %p (new request state = %d)\n",
@@ -860,31 +815,62 @@ void isci_terminate_pending_requests(
 
 	spin_lock_irqsave(&isci_host->scic_lock, flags);
 
-	/* Move all of the pending requests off of the device list. */
-	list_splice_init(&isci_device->reqs_in_process,
-			 &aborted_request_list);
-
-	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
-
-	/* Iterate through the now-local list. */
+	/* Iterate through the list. */
 	list_for_each_entry_safe(request, next_request,
-				 &aborted_request_list, dev_node) {
+				 &isci_device->reqs_in_process, dev_node) {
 
-		dev_warn(&isci_host->pdev->dev,
-			"%s: isci_device=%p request=%p; task=%p\n",
-			__func__,
-			isci_device, request,
-			((request->ttype == io_task)
-				? isci_request_access_task(request)
-				: NULL));
+		init_completion(&request_completion);
 
-		/* Mark all still pending I/O with the selected next
-		* state, terminate and free it.
+		/* Change state to "new_request_state" if it is currently
+		* "started".
 		*/
-		isci_terminate_request(isci_host, isci_device,
-				       request, new_request_state
-				       );
+		old_state = isci_request_change_started_to_newstate(
+					request,
+					&request_completion,
+					new_request_state);
+
+		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
+
+		if ((old_state == started) ||
+		    (old_state == completed) ||
+		    (old_state == aborting)) {
+
+			dev_warn(&isci_host->pdev->dev,
+				 "%s: isci_device=%p request=%p; task=%p "
+				 "old_state=%d\n",
+				 __func__,
+				 isci_device, request,
+				 ((request->ttype == io_task)
+					? isci_request_access_task(request)
+					: NULL),
+				 old_state);
+
+			/* If the old_state is started:
+			 * This request was not already being aborted. If it had been,
+			 * then the aborting I/O (ie. the TMF request) would not be in
+			 * the aborting state, and thus would be terminated here.  Note
+			 * that since the TMF completion's call to the kernel function
+			 * "complete()" does not happen until the pending I/O request
+			 * terminate fully completes, we do not have to implement a
+			 * special wait here for already aborting requests - the
+			 * termination of the TMF request will force the request
+			 * to finish it's already started terminate.
+			 *
+			 * If old_state == completed:
+			 * This request completed from the SCU hardware perspective
+			 * and now just needs cleaning up in terms of freeing the
+			 * request and potentially calling up to libsas.
+			 *
+			 * If old_state == aborting:
+			 * This request has already gone through a TMF timeout, but may
+			 * not have been terminated; needs cleaning up at least.
+			 */
+			isci_terminate_request_core(isci_host, isci_device,
+						    request);
+		}
+		spin_lock_irqsave(&isci_host->scic_lock, flags);
 	}
+	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 }
 
 /**
@@ -1257,9 +1243,9 @@ int isci_task_abort_task(struct sas_task *task)
 	if (ret == TMF_RESP_FUNC_COMPLETE) {
 		old_request->complete_in_target = true;
 
-		/* Clean up the request on our side, and wait for the aborted I/O to
-		* complete.
-		*/
+		/* 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);
 	}