diff mbox series

[5/8] qla2xxx: Fix double scsi_done for abort path

Message ID 20191105150657.8092-6-hmadhani@marvell.com (mailing list archive)
State Mainlined
Commit f45bca8c5052e8c59bab64ee90c44441678b9a52
Headers show
Series qla2xxx: Bug Fixes for the driver | expand

Commit Message

Himanshu Madhani Nov. 5, 2019, 3:06 p.m. UTC
From: Quinn Tran <qutran@marvell.com>

Current code assume abort will remove the original command from the
active list where scsi_done will not be call. Instead, the eh_abort
thread will do the scsi_done. That is not the case.  Instead, we
have a double scsi_done calls triggering use after free.

Abort will tell FW to release the command from FW possesion. The
original command will return to ULP with error in its normal fashion via
scsi_done.  eh_abort path would wait for the original command
completion before returning.  eh_abort path will not perform the
scsi_done call.

Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
Cc: stable@vger.kernel.org # 5.2
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Arun Easi <aeasi@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_def.h  |   5 +-
 drivers/scsi/qla2xxx/qla_isr.c  |   5 ++
 drivers/scsi/qla2xxx/qla_nvme.c |   4 +-
 drivers/scsi/qla2xxx/qla_os.c   | 117 +++++++++++++++++++++-------------------
 4 files changed, 72 insertions(+), 59 deletions(-)

Comments

Ewan Milne Nov. 5, 2019, 3:20 p.m. UTC | #1
On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> Current code assume abort will remove the original command from the
> active list where scsi_done will not be call. Instead, the eh_abort
> thread will do the scsi_done. That is not the case.  Instead, we
> have a double scsi_done calls triggering use after free.
> 
> Abort will tell FW to release the command from FW possesion. The
> original command will return to ULP with error in its normal fashion via
> scsi_done.  eh_abort path would wait for the original command
> completion before returning.  eh_abort path will not perform the
> scsi_done call.
> 
> Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
> Cc: stable@vger.kernel.org # 5.2
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h  |   5 +-
>  drivers/scsi/qla2xxx/qla_isr.c  |   5 ++
>  drivers/scsi/qla2xxx/qla_nvme.c |   4 +-
>  drivers/scsi/qla2xxx/qla_os.c   | 117 +++++++++++++++++++++-------------------
>  4 files changed, 72 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index ef9bb3c7ad6f..2a9e6a9a8c9d 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -591,13 +591,16 @@ typedef struct srb {
>  	 */
>  	uint8_t cmd_type;
>  	uint8_t pad[3];
> -	atomic_t ref_count;
>  	struct kref cmd_kref;	/* need to migrate ref_count over to this */
>  	void *priv;
>  	wait_queue_head_t nvme_ls_waitq;
>  	struct fc_port *fcport;
>  	struct scsi_qla_host *vha;
>  	unsigned int start_timer:1;
> +	unsigned int abort:1;
> +	unsigned int aborted:1;
> +	unsigned int completed:1;
> +
>  	uint32_t handle;
>  	uint16_t flags;
>  	uint16_t type;
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index acef3d73983c..1b8f297449cf 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -2487,6 +2487,11 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
>  		return;
>  	}
>  
> +	if (sp->abort)
> +		sp->aborted = 1;
> +	else
> +		sp->completed = 1;
> +
>  	if (sp->cmd_type != TYPE_SRB) {
>  		req->outstanding_cmds[handle] = NULL;
>  		ql_dbg(ql_dbg_io, vha, 0x3015,
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 6cc19e060afc..941aa53363f5 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -224,8 +224,8 @@ static void qla_nvme_abort_work(struct work_struct *work)
>  
>  	if (ha->flags.host_shutting_down) {
>  		ql_log(ql_log_info, sp->fcport->vha, 0xffff,
> -		    "%s Calling done on sp: %p, type: 0x%x, sp->ref_count: 0x%x\n",
> -		    __func__, sp, sp->type, atomic_read(&sp->ref_count));
> +		    "%s Calling done on sp: %p, type: 0x%x\n",
> +		    __func__, sp, sp->type);
>  		sp->done(sp, 0);
>  		goto out;
>  	}
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2e7a4a2d6c5a..b59d579834ea 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -698,11 +698,6 @@ void qla2x00_sp_compl(srb_t *sp, int res)
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>  	struct completion *comp = sp->comp;
>  
> -	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
> -		return;
> -
> -	atomic_dec(&sp->ref_count);
> -
>  	sp->free(sp);
>  	cmd->result = res;
>  	CMD_SP(cmd) = NULL;
> @@ -794,11 +789,6 @@ void qla2xxx_qpair_sp_compl(srb_t *sp, int res)
>  	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
>  	struct completion *comp = sp->comp;
>  
> -	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
> -		return;
> -
> -	atomic_dec(&sp->ref_count);
> -
>  	sp->free(sp);
>  	cmd->result = res;
>  	CMD_SP(cmd) = NULL;
> @@ -903,7 +893,7 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  	sp->u.scmd.cmd = cmd;
>  	sp->type = SRB_SCSI_CMD;
> -	atomic_set(&sp->ref_count, 1);
> +
>  	CMD_SP(cmd) = (void *)sp;
>  	sp->free = qla2x00_sp_free_dma;
>  	sp->done = qla2x00_sp_compl;
> @@ -985,11 +975,9 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
>  
>  	sp->u.scmd.cmd = cmd;
>  	sp->type = SRB_SCSI_CMD;
> -	atomic_set(&sp->ref_count, 1);
>  	CMD_SP(cmd) = (void *)sp;
>  	sp->free = qla2xxx_qpair_sp_free_dma;
>  	sp->done = qla2xxx_qpair_sp_compl;
> -	sp->qpair = qpair;
>  
>  	rval = ha->isp_ops->start_scsi_mq(sp);
>  	if (rval != QLA_SUCCESS) {
> @@ -1187,16 +1175,6 @@ qla2x00_wait_for_chip_reset(scsi_qla_host_t *vha)
>  	return return_status;
>  }
>  
> -static int
> -sp_get(struct srb *sp)
> -{
> -	if (!refcount_inc_not_zero((refcount_t *)&sp->ref_count))
> -		/* kref get fail */
> -		return ENXIO;
> -	else
> -		return 0;
> -}
> -
>  #define ISP_REG_DISCONNECT 0xffffffffU
>  /**************************************************************************
>  * qla2x00_isp_reg_stat
> @@ -1252,6 +1230,9 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  	uint64_t lun;
>  	int rval;
>  	struct qla_hw_data *ha = vha->hw;
> +	uint32_t ratov_j;
> +	struct qla_qpair *qpair;
> +	unsigned long flags;
>  
>  	if (qla2x00_isp_reg_stat(ha)) {
>  		ql_log(ql_log_info, vha, 0x8042,
> @@ -1264,13 +1245,26 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  		return ret;
>  
>  	sp = scsi_cmd_priv(cmd);
> +	qpair = sp->qpair;
>  
> -	if (sp->fcport && sp->fcport->deleted)
> +	if ((sp->fcport && sp->fcport->deleted) || !qpair)
>  		return SUCCESS;
>  
> -	/* Return if the command has already finished. */
> -	if (sp_get(sp))
> +	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
> +	if (sp->completed) {
> +		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
>  		return SUCCESS;
> +	}
> +
> +	if (sp->abort || sp->aborted) {
> +		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
> +		return FAILED;
> +	}
> +
> +	sp->abort = 1;
> +	sp->comp = &comp;
> +	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
> +
>  
>  	id = cmd->device->id;
>  	lun = cmd->device->lun;
> @@ -1279,47 +1273,37 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
>  	    "Aborting from RISC nexus=%ld:%d:%llu sp=%p cmd=%p handle=%x\n",
>  	    vha->host_no, id, lun, sp, cmd, sp->handle);
>  
> +	/*
> +	 * Abort will release the original Command/sp from FW. Let the
> +	 * original command call scsi_done. In return, he will wakeup
> +	 * this sleeping thread.
> +	 */
>  	rval = ha->isp_ops->abort_command(sp);
> +
>  	ql_dbg(ql_dbg_taskm, vha, 0x8003,
>  	       "Abort command mbx cmd=%p, rval=%x.\n", cmd, rval);
>  
> +	/* Wait for the command completion. */
> +	ratov_j = ha->r_a_tov/10 * 4 * 1000;
> +	ratov_j = msecs_to_jiffies(ratov_j);
>  	switch (rval) {
>  	case QLA_SUCCESS:
> -		/*
> -		 * The command has been aborted. That means that the firmware
> -		 * won't report a completion.
> -		 */
> -		sp->done(sp, DID_ABORT << 16);
> -		ret = SUCCESS;
> -		break;
> -	case QLA_FUNCTION_PARAMETER_ERROR: {
> -		/* Wait for the command completion. */
> -		uint32_t ratov = ha->r_a_tov/10;
> -		uint32_t ratov_j = msecs_to_jiffies(4 * ratov * 1000);
> -
> -		WARN_ON_ONCE(sp->comp);
> -		sp->comp = &comp;
>  		if (!wait_for_completion_timeout(&comp, ratov_j)) {
>  			ql_dbg(ql_dbg_taskm, vha, 0xffff,
>  			    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
> -			    __func__, ha->r_a_tov);
> +			    __func__, ha->r_a_tov/10);
>  			ret = FAILED;
>  		} else {
>  			ret = SUCCESS;
>  		}
>  		break;
> -	}
>  	default:
> -		/*
> -		 * Either abort failed or abort and completion raced. Let
> -		 * the SCSI core retry the abort in the former case.
> -		 */
>  		ret = FAILED;
>  		break;
>  	}
>  
>  	sp->comp = NULL;
> -	atomic_dec(&sp->ref_count);
> +
>  	ql_log(ql_log_info, vha, 0x801c,
>  	    "Abort command issued nexus=%ld:%d:%llu -- %x.\n",
>  	    vha->host_no, id, lun, ret);
> @@ -1711,32 +1695,53 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
>  	scsi_qla_host_t *vha = qp->vha;
>  	struct qla_hw_data *ha = vha->hw;
>  	int rval;
> +	bool ret_cmd;
> +	uint32_t ratov_j;
>  
> -	if (sp_get(sp))
> +	if (qla2x00_chip_is_down(vha)) {
> +		sp->done(sp, res);
>  		return;
> +	}
>  
>  	if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS ||
>  	    (sp->type == SRB_SCSI_CMD && !ha->flags.eeh_busy &&
>  	     !test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) &&
>  	     !qla2x00_isp_reg_stat(ha))) {
> +		if (sp->comp) {
> +			sp->done(sp, res);
> +			return;
> +		}
> +
>  		sp->comp = &comp;
> +		sp->abort =  1;
>  		spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
> -		rval = ha->isp_ops->abort_command(sp);
>  
> +		rval = ha->isp_ops->abort_command(sp);
> +		/* Wait for command completion. */
> +		ret_cmd = false;
> +		ratov_j = ha->r_a_tov/10 * 4 * 1000;
> +		ratov_j = msecs_to_jiffies(ratov_j);
>  		switch (rval) {
>  		case QLA_SUCCESS:
> -			sp->done(sp, res);
> +			if (wait_for_completion_timeout(&comp, ratov_j)) {
> +				ql_dbg(ql_dbg_taskm, vha, 0xffff,
> +				    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
> +				    __func__, ha->r_a_tov/10);
> +				ret_cmd = true;
> +			}
> +			/* else FW return SP to driver */
>  			break;
> -		case QLA_FUNCTION_PARAMETER_ERROR:
> -			wait_for_completion(&comp);
> +		default:
> +			ret_cmd = true;
>  			break;
>  		}
>  
>  		spin_lock_irqsave(qp->qp_lock_ptr, *flags);
> -		sp->comp = NULL;
> +		if (ret_cmd && (!sp->completed || !sp->aborted))
> +			sp->done(sp, res);
> +	} else {
> +		sp->done(sp, res);
>  	}
> -
> -	atomic_dec(&sp->ref_count);
>  }
>  
>  static void
> @@ -1758,7 +1763,6 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>  	for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
>  		sp = req->outstanding_cmds[cnt];
>  		if (sp) {
> -			req->outstanding_cmds[cnt] = NULL;
>  			switch (sp->cmd_type) {
>  			case TYPE_SRB:
>  				qla2x00_abort_srb(qp, sp, res, &flags);
> @@ -1780,6 +1784,7 @@ __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
>  			default:
>  				break;
>  			}
> +			req->outstanding_cmds[cnt] = NULL;
>  		}
>  	}
>  	spin_unlock_irqrestore(qp->qp_lock_ptr, flags);

Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Bart Van Assche Nov. 7, 2019, 6:01 p.m. UTC | #2
On 11/5/19 7:06 AM, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> Current code assume abort will remove the original command from the
> active list where scsi_done will not be call. Instead, the eh_abort
> thread will do the scsi_done. That is not the case.  Instead, we
> have a double scsi_done calls triggering use after free.
> 
> Abort will tell FW to release the command from FW possesion. The
> original command will return to ULP with error in its normal fashion via
> scsi_done.  eh_abort path would wait for the original command
> completion before returning.  eh_abort path will not perform the
> scsi_done call.
> 
> Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")

The intent of commit 219d27d7147e0 was only to fix race conditions and
not to change when scsi_done() is called. Are the double scsi_done()
calls perhaps the result of a recent firmware change? If so, does this
patch break compatibility with firmware versions that do not report an
error status for aborted commands?

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index ef9bb3c7ad6f..2a9e6a9a8c9d 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -591,13 +591,16 @@  typedef struct srb {
 	 */
 	uint8_t cmd_type;
 	uint8_t pad[3];
-	atomic_t ref_count;
 	struct kref cmd_kref;	/* need to migrate ref_count over to this */
 	void *priv;
 	wait_queue_head_t nvme_ls_waitq;
 	struct fc_port *fcport;
 	struct scsi_qla_host *vha;
 	unsigned int start_timer:1;
+	unsigned int abort:1;
+	unsigned int aborted:1;
+	unsigned int completed:1;
+
 	uint32_t handle;
 	uint16_t flags;
 	uint16_t type;
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index acef3d73983c..1b8f297449cf 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2487,6 +2487,11 @@  qla2x00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
 		return;
 	}
 
+	if (sp->abort)
+		sp->aborted = 1;
+	else
+		sp->completed = 1;
+
 	if (sp->cmd_type != TYPE_SRB) {
 		req->outstanding_cmds[handle] = NULL;
 		ql_dbg(ql_dbg_io, vha, 0x3015,
diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
index 6cc19e060afc..941aa53363f5 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.c
+++ b/drivers/scsi/qla2xxx/qla_nvme.c
@@ -224,8 +224,8 @@  static void qla_nvme_abort_work(struct work_struct *work)
 
 	if (ha->flags.host_shutting_down) {
 		ql_log(ql_log_info, sp->fcport->vha, 0xffff,
-		    "%s Calling done on sp: %p, type: 0x%x, sp->ref_count: 0x%x\n",
-		    __func__, sp, sp->type, atomic_read(&sp->ref_count));
+		    "%s Calling done on sp: %p, type: 0x%x\n",
+		    __func__, sp, sp->type);
 		sp->done(sp, 0);
 		goto out;
 	}
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 2e7a4a2d6c5a..b59d579834ea 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -698,11 +698,6 @@  void qla2x00_sp_compl(srb_t *sp, int res)
 	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
 	struct completion *comp = sp->comp;
 
-	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
-		return;
-
-	atomic_dec(&sp->ref_count);
-
 	sp->free(sp);
 	cmd->result = res;
 	CMD_SP(cmd) = NULL;
@@ -794,11 +789,6 @@  void qla2xxx_qpair_sp_compl(srb_t *sp, int res)
 	struct scsi_cmnd *cmd = GET_CMD_SP(sp);
 	struct completion *comp = sp->comp;
 
-	if (WARN_ON_ONCE(atomic_read(&sp->ref_count) == 0))
-		return;
-
-	atomic_dec(&sp->ref_count);
-
 	sp->free(sp);
 	cmd->result = res;
 	CMD_SP(cmd) = NULL;
@@ -903,7 +893,7 @@  qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 	sp->u.scmd.cmd = cmd;
 	sp->type = SRB_SCSI_CMD;
-	atomic_set(&sp->ref_count, 1);
+
 	CMD_SP(cmd) = (void *)sp;
 	sp->free = qla2x00_sp_free_dma;
 	sp->done = qla2x00_sp_compl;
@@ -985,11 +975,9 @@  qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
 
 	sp->u.scmd.cmd = cmd;
 	sp->type = SRB_SCSI_CMD;
-	atomic_set(&sp->ref_count, 1);
 	CMD_SP(cmd) = (void *)sp;
 	sp->free = qla2xxx_qpair_sp_free_dma;
 	sp->done = qla2xxx_qpair_sp_compl;
-	sp->qpair = qpair;
 
 	rval = ha->isp_ops->start_scsi_mq(sp);
 	if (rval != QLA_SUCCESS) {
@@ -1187,16 +1175,6 @@  qla2x00_wait_for_chip_reset(scsi_qla_host_t *vha)
 	return return_status;
 }
 
-static int
-sp_get(struct srb *sp)
-{
-	if (!refcount_inc_not_zero((refcount_t *)&sp->ref_count))
-		/* kref get fail */
-		return ENXIO;
-	else
-		return 0;
-}
-
 #define ISP_REG_DISCONNECT 0xffffffffU
 /**************************************************************************
 * qla2x00_isp_reg_stat
@@ -1252,6 +1230,9 @@  qla2xxx_eh_abort(struct scsi_cmnd *cmd)
 	uint64_t lun;
 	int rval;
 	struct qla_hw_data *ha = vha->hw;
+	uint32_t ratov_j;
+	struct qla_qpair *qpair;
+	unsigned long flags;
 
 	if (qla2x00_isp_reg_stat(ha)) {
 		ql_log(ql_log_info, vha, 0x8042,
@@ -1264,13 +1245,26 @@  qla2xxx_eh_abort(struct scsi_cmnd *cmd)
 		return ret;
 
 	sp = scsi_cmd_priv(cmd);
+	qpair = sp->qpair;
 
-	if (sp->fcport && sp->fcport->deleted)
+	if ((sp->fcport && sp->fcport->deleted) || !qpair)
 		return SUCCESS;
 
-	/* Return if the command has already finished. */
-	if (sp_get(sp))
+	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
+	if (sp->completed) {
+		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
 		return SUCCESS;
+	}
+
+	if (sp->abort || sp->aborted) {
+		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
+		return FAILED;
+	}
+
+	sp->abort = 1;
+	sp->comp = &comp;
+	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
+
 
 	id = cmd->device->id;
 	lun = cmd->device->lun;
@@ -1279,47 +1273,37 @@  qla2xxx_eh_abort(struct scsi_cmnd *cmd)
 	    "Aborting from RISC nexus=%ld:%d:%llu sp=%p cmd=%p handle=%x\n",
 	    vha->host_no, id, lun, sp, cmd, sp->handle);
 
+	/*
+	 * Abort will release the original Command/sp from FW. Let the
+	 * original command call scsi_done. In return, he will wakeup
+	 * this sleeping thread.
+	 */
 	rval = ha->isp_ops->abort_command(sp);
+
 	ql_dbg(ql_dbg_taskm, vha, 0x8003,
 	       "Abort command mbx cmd=%p, rval=%x.\n", cmd, rval);
 
+	/* Wait for the command completion. */
+	ratov_j = ha->r_a_tov/10 * 4 * 1000;
+	ratov_j = msecs_to_jiffies(ratov_j);
 	switch (rval) {
 	case QLA_SUCCESS:
-		/*
-		 * The command has been aborted. That means that the firmware
-		 * won't report a completion.
-		 */
-		sp->done(sp, DID_ABORT << 16);
-		ret = SUCCESS;
-		break;
-	case QLA_FUNCTION_PARAMETER_ERROR: {
-		/* Wait for the command completion. */
-		uint32_t ratov = ha->r_a_tov/10;
-		uint32_t ratov_j = msecs_to_jiffies(4 * ratov * 1000);
-
-		WARN_ON_ONCE(sp->comp);
-		sp->comp = &comp;
 		if (!wait_for_completion_timeout(&comp, ratov_j)) {
 			ql_dbg(ql_dbg_taskm, vha, 0xffff,
 			    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
-			    __func__, ha->r_a_tov);
+			    __func__, ha->r_a_tov/10);
 			ret = FAILED;
 		} else {
 			ret = SUCCESS;
 		}
 		break;
-	}
 	default:
-		/*
-		 * Either abort failed or abort and completion raced. Let
-		 * the SCSI core retry the abort in the former case.
-		 */
 		ret = FAILED;
 		break;
 	}
 
 	sp->comp = NULL;
-	atomic_dec(&sp->ref_count);
+
 	ql_log(ql_log_info, vha, 0x801c,
 	    "Abort command issued nexus=%ld:%d:%llu -- %x.\n",
 	    vha->host_no, id, lun, ret);
@@ -1711,32 +1695,53 @@  static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
 	scsi_qla_host_t *vha = qp->vha;
 	struct qla_hw_data *ha = vha->hw;
 	int rval;
+	bool ret_cmd;
+	uint32_t ratov_j;
 
-	if (sp_get(sp))
+	if (qla2x00_chip_is_down(vha)) {
+		sp->done(sp, res);
 		return;
+	}
 
 	if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS ||
 	    (sp->type == SRB_SCSI_CMD && !ha->flags.eeh_busy &&
 	     !test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) &&
 	     !qla2x00_isp_reg_stat(ha))) {
+		if (sp->comp) {
+			sp->done(sp, res);
+			return;
+		}
+
 		sp->comp = &comp;
+		sp->abort =  1;
 		spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
-		rval = ha->isp_ops->abort_command(sp);
 
+		rval = ha->isp_ops->abort_command(sp);
+		/* Wait for command completion. */
+		ret_cmd = false;
+		ratov_j = ha->r_a_tov/10 * 4 * 1000;
+		ratov_j = msecs_to_jiffies(ratov_j);
 		switch (rval) {
 		case QLA_SUCCESS:
-			sp->done(sp, res);
+			if (wait_for_completion_timeout(&comp, ratov_j)) {
+				ql_dbg(ql_dbg_taskm, vha, 0xffff,
+				    "%s: Abort wait timer (4 * R_A_TOV[%d]) expired\n",
+				    __func__, ha->r_a_tov/10);
+				ret_cmd = true;
+			}
+			/* else FW return SP to driver */
 			break;
-		case QLA_FUNCTION_PARAMETER_ERROR:
-			wait_for_completion(&comp);
+		default:
+			ret_cmd = true;
 			break;
 		}
 
 		spin_lock_irqsave(qp->qp_lock_ptr, *flags);
-		sp->comp = NULL;
+		if (ret_cmd && (!sp->completed || !sp->aborted))
+			sp->done(sp, res);
+	} else {
+		sp->done(sp, res);
 	}
-
-	atomic_dec(&sp->ref_count);
 }
 
 static void
@@ -1758,7 +1763,6 @@  __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
 	for (cnt = 1; cnt < req->num_outstanding_cmds; cnt++) {
 		sp = req->outstanding_cmds[cnt];
 		if (sp) {
-			req->outstanding_cmds[cnt] = NULL;
 			switch (sp->cmd_type) {
 			case TYPE_SRB:
 				qla2x00_abort_srb(qp, sp, res, &flags);
@@ -1780,6 +1784,7 @@  __qla2x00_abort_all_cmds(struct qla_qpair *qp, int res)
 			default:
 				break;
 			}
+			req->outstanding_cmds[cnt] = NULL;
 		}
 	}
 	spin_unlock_irqrestore(qp->qp_lock_ptr, flags);