[PATCH 0/8] qla2xxx: Bug Fixes for the driver

[PATCH 0/8] qla2xxx: Bug Fixes for the driver

[Himanshu Madhani]

Hi Martin,

This series contains bug fixes for the driver. 

Patch 1 and 7 resolves discovery issue in the driver where
older target device which does not understand FC-NVMe PRLI
will go in strange state when driver issues PRLI with NVMe
bit set. To fix the issue we will now restart from PLOGI state
and send FCP PRLI to discover FCP LUNs.

Patches 2-6 are various SRB leak and driver unload hang
observed with the latest code.

Please apply these patches to 5.5/scsi-queue at your earliest
convenience. 

Thanks,
Himanshu

Arun Easi (2):
  qla2xxx: Fix memory leak when sending I/O fails
  qla2xxx: Fix device connect issues in P2P configuration

Himanshu Madhani (1):
  qla2xxx: Update driver version to 10.01.00.21-k

Quinn Tran (5):
  qla2xxx: Retry PLOGI on FC-NVMe PRLI failure
  qla2xxx: Do command completion on abort timeout
  qla2xxx: Fix SRB leak on switch command timeout
  qla2xxx: Fix driver unload hang
  qla2xxx: Fix double scsi_done for abort path

 drivers/scsi/qla2xxx/qla_def.h     |   6 +-
 drivers/scsi/qla2xxx/qla_gbl.h     |   1 +
 drivers/scsi/qla2xxx/qla_gs.c      |   2 +-
 drivers/scsi/qla2xxx/qla_init.c    |  77 +++++++++++-----------
 drivers/scsi/qla2xxx/qla_iocb.c    |  11 ++--
 drivers/scsi/qla2xxx/qla_isr.c     |   5 ++
 drivers/scsi/qla2xxx/qla_mbx.c     |   4 --
 drivers/scsi/qla2xxx/qla_mid.c     |  11 ++--
 drivers/scsi/qla2xxx/qla_nvme.c    |   4 +-
 drivers/scsi/qla2xxx/qla_os.c      | 128 ++++++++++++++++++++-----------------
 drivers/scsi/qla2xxx/qla_version.h |   2 +-
 11 files changed, 137 insertions(+), 114 deletions(-)

-- 
2.12.0

[PATCH 1/8] qla2xxx: Retry PLOGI on FC-NVMe PRLI failure

[Himanshu Madhani]

From: Quinn Tran <qutran@marvell.com>

Current code will send PRLI with FC-NVMe bit set for the
targets which supports only FCP. This may result into issue
with targets which do not understand NVMe and will go into
strange state. This patch would restart the login process
by going back to PLOGI state. The PLOGI state will force the
target to respond to correct PRLI request.

Fixes: c76ae845ea836 ("scsi: qla2xxx: Add error handling for PLOGI ELS passthrough")
Cc: stable@vger.kernel.org # 5.4
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 37 ++++++++-----------------------------
 drivers/scsi/qla2xxx/qla_iocb.c |  6 +++++-
 2 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 7cb7545de962..5db8ad832893 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -1864,42 +1864,21 @@ qla24xx_handle_prli_done_event(struct scsi_qla_host *vha, struct event_arg *ea)
 		 * FCP/NVMe port
 		 */
 		if (NVME_FCP_TARGET(ea->fcport)) {
-			if (vha->hw->fc4_type_priority == FC4_PRIORITY_NVME)
-				ea->fcport->fc4_type &= ~FS_FC4TYPE_NVME;
-			else
-				ea->fcport->fc4_type &= ~FS_FC4TYPE_FCP;
 			ql_dbg(ql_dbg_disc, vha, 0x2118,
 				"%s %d %8phC post %s prli\n",
 				__func__, __LINE__, ea->fcport->port_name,
 				(ea->fcport->fc4_type & FS_FC4TYPE_NVME) ?
 				"NVMe" : "FCP");
-			qla24xx_post_prli_work(vha, ea->fcport);
-			break;
+			if (vha->hw->fc4_type_priority == FC4_PRIORITY_NVME)
+				ea->fcport->fc4_type &= ~FS_FC4TYPE_NVME;
+			else
+				ea->fcport->fc4_type &= ~FS_FC4TYPE_FCP;
 		}
 
-		/* at this point both PRLI NVME & PRLI FCP failed */
-		if (N2N_TOPO(vha->hw)) {
-			if (ea->fcport->n2n_link_reset_cnt < 3) {
-				ea->fcport->n2n_link_reset_cnt++;
-				/*
-				 * remote port is not sending Plogi. Reset
-				 * link to kick start his state machine
-				 */
-				set_bit(N2N_LINK_RESET, &vha->dpc_flags);
-			} else {
-				ql_log(ql_log_warn, vha, 0x2119,
-				    "%s %d %8phC Unable to reconnect\n",
-				    __func__, __LINE__, ea->fcport->port_name);
-			}
-		} else {
-			/*
-			 * switch connect. login failed. Take connection
-			 * down and allow relogin to retrigger
-			 */
-			ea->fcport->flags &= ~FCF_ASYNC_SENT;
-			ea->fcport->keep_nport_handle = 0;
-			qlt_schedule_sess_for_deletion(ea->fcport);
-		}
+		ea->fcport->flags &= ~FCF_ASYNC_SENT;
+		ea->fcport->keep_nport_handle = 0;
+		ea->fcport->logout_on_delete = 1;
+		qlt_schedule_sess_for_deletion(ea->fcport);
 		break;
 	}
 }
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index eeb526411536..2b675da34bda 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2764,6 +2764,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
 			ea.sp = sp;
 			qla24xx_handle_plogi_done_event(vha, &ea);
 			break;
+
 		case CS_IOCB_ERROR:
 			switch (fw_status[1]) {
 			case LSC_SCODE_PORTID_USED:
@@ -2834,6 +2835,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
 				    fw_status[0], fw_status[1], fw_status[2]);
 
 				fcport->flags &= ~FCF_ASYNC_SENT;
+				fcport->disc_state = DSC_LOGIN_FAILED;
 				set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
 				break;
 			}
@@ -2846,6 +2848,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
 			    fw_status[0], fw_status[1], fw_status[2]);
 
 			sp->fcport->flags &= ~FCF_ASYNC_SENT;
+			sp->fcport->disc_state = DSC_LOGIN_FAILED;
 			set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
 			break;
 		}
@@ -2881,11 +2884,12 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode,
 		return -ENOMEM;
 	}
 
+	fcport->flags |= FCF_ASYNC_SENT;
+	fcport->disc_state = DSC_LOGIN_PEND;
 	elsio = &sp->u.iocb_cmd;
 	ql_dbg(ql_dbg_io, vha, 0x3073,
 	    "Enter: PLOGI portid=%06x\n", fcport->d_id.b24);
 
-	fcport->flags |= FCF_ASYNC_SENT;
 	sp->type = SRB_ELS_DCMD;
 	sp->name = "ELS_DCMD";
 	sp->fcport = fcport;
-- 
2.12.0

[PATCH 2/8] qla2xxx: Do command completion on abort timeout

[Himanshu Madhani]

From: Quinn Tran <qutran@marvell.com>

On switch, fabric and mgt command timeout, driver
send Abort to tell FW to return the original command.
If abort is timeout, then return both Abort and
original command for cleanup.

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: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_def.h  |  1 +
 drivers/scsi/qla2xxx/qla_init.c | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 721ee7f09b39..ef9bb3c7ad6f 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -604,6 +604,7 @@ typedef struct srb {
 	const char *name;
 	int iocbs;
 	struct qla_qpair *qpair;
+	struct srb *cmd_sp;
 	struct list_head elem;
 	u32 gen1;	/* scratch */
 	u32 gen2;	/* scratch */
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 5db8ad832893..7fdbe041cc19 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -101,8 +101,22 @@ static void qla24xx_abort_iocb_timeout(void *data)
 	u32 handle;
 	unsigned long flags;
 
+	if (sp->cmd_sp)
+		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
+		    "Abort timeout - cmd hdl=%x, cmd type=%x hdl=%x, type=%x\n",
+		    sp->cmd_sp->handle, sp->cmd_sp->type,
+		    sp->handle, sp->type);
+	else
+		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
+		    "Abort timeout 2 - hdl=%x, type=%x\n",
+		    sp->handle, sp->type);
+
 	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
 	for (handle = 1; handle < qpair->req->num_outstanding_cmds; handle++) {
+		if (sp->cmd_sp && (qpair->req->outstanding_cmds[handle] ==
+		    sp->cmd_sp))
+			qpair->req->outstanding_cmds[handle] = NULL;
+
 		/* removing the abort */
 		if (qpair->req->outstanding_cmds[handle] == sp) {
 			qpair->req->outstanding_cmds[handle] = NULL;
@@ -111,6 +125,9 @@ static void qla24xx_abort_iocb_timeout(void *data)
 	}
 	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
 
+	if (sp->cmd_sp)
+		sp->cmd_sp->done(sp->cmd_sp, QLA_OS_TIMER_EXPIRED);
+
 	abt->u.abt.comp_status = CS_TIMEOUT;
 	sp->done(sp, QLA_OS_TIMER_EXPIRED);
 }
@@ -142,6 +159,7 @@ static int qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
 	sp->type = SRB_ABT_CMD;
 	sp->name = "abort";
 	sp->qpair = cmd_sp->qpair;
+	sp->cmd_sp = cmd_sp;
 	if (wait)
 		sp->flags = SRB_WAKEUP_ON_COMP;
 
-- 
2.12.0

[PATCH 3/8] qla2xxx: Fix SRB leak on switch command timeout

[Himanshu Madhani]

From: Quinn Tran <qutran@marvell.com>

when GPSC/GPDB switch command fails, driver just returns
without doing a proper cleanup. This patch fixes this memory
leak by calling sp->free() in the error path.

Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_gs.c   |  2 +-
 drivers/scsi/qla2xxx/qla_init.c | 11 +++++------
 drivers/scsi/qla2xxx/qla_mbx.c  |  4 ----
 drivers/scsi/qla2xxx/qla_mid.c  | 11 ++++-------
 drivers/scsi/qla2xxx/qla_os.c   |  7 ++++++-
 5 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index 7a00272ca380..67230688b05e 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -3010,7 +3010,7 @@ static void qla24xx_async_gpsc_sp_done(srb_t *sp, int res)
 	fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
 
 	if (res == QLA_FUNCTION_TIMEOUT)
-		return;
+		goto done;
 
 	if (res == (DID_ERROR << 16)) {
 		/* entry status error */
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 7fdbe041cc19..bddb26baedd2 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -1151,19 +1151,18 @@ static void qla24xx_async_gpdb_sp_done(srb_t *sp, int res)
 	    "Async done-%s res %x, WWPN %8phC mb[1]=%x mb[2]=%x \n",
 	    sp->name, res, fcport->port_name, mb[1], mb[2]);
 
-	if (res == QLA_FUNCTION_TIMEOUT) {
-		dma_pool_free(sp->vha->hw->s_dma_pool, sp->u.iocb_cmd.u.mbx.in,
-			sp->u.iocb_cmd.u.mbx.in_dma);
-		return;
-	}
-
 	fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
+
+	if (res == QLA_FUNCTION_TIMEOUT)
+		goto done;
+
 	memset(&ea, 0, sizeof(ea));
 	ea.fcport = fcport;
 	ea.sp = sp;
 
 	qla24xx_handle_gpdb_event(vha, &ea);
 
+done:
 	dma_pool_free(ha->s_dma_pool, sp->u.iocb_cmd.u.mbx.in,
 		sp->u.iocb_cmd.u.mbx.in_dma);
 
diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
index 04175c91af0e..0cf94f05f008 100644
--- a/drivers/scsi/qla2xxx/qla_mbx.c
+++ b/drivers/scsi/qla2xxx/qla_mbx.c
@@ -6288,17 +6288,13 @@ int qla24xx_send_mb_cmd(struct scsi_qla_host *vha, mbx_cmd_t *mcp)
 	case  QLA_SUCCESS:
 		ql_dbg(ql_dbg_mbx, vha, 0x119d, "%s: %s done.\n",
 		    __func__, sp->name);
-		sp->free(sp);
 		break;
 	default:
 		ql_dbg(ql_dbg_mbx, vha, 0x119e, "%s: %s Failed. %x.\n",
 		    __func__, sp->name, rval);
-		sp->free(sp);
 		break;
 	}
 
-	return rval;
-
 done_free_sp:
 	sp->free(sp);
 done:
diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
index 6afad68e5ba2..bd62c4595b73 100644
--- a/drivers/scsi/qla2xxx/qla_mid.c
+++ b/drivers/scsi/qla2xxx/qla_mid.c
@@ -944,7 +944,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
 
 	sp = qla2x00_get_sp(base_vha, NULL, GFP_KERNEL);
 	if (!sp)
-		goto done;
+		return rval;
 
 	sp->type = SRB_CTRL_VP;
 	sp->name = "ctrl_vp";
@@ -960,7 +960,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
 		ql_dbg(ql_dbg_async, vha, 0xffff,
 		    "%s: %s Failed submission. %x.\n",
 		    __func__, sp->name, rval);
-		goto done_free_sp;
+		goto done;
 	}
 
 	ql_dbg(ql_dbg_vport, vha, 0x113f, "%s hndl %x submitted\n",
@@ -978,16 +978,13 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
 	case QLA_SUCCESS:
 		ql_dbg(ql_dbg_vport, vha, 0xffff, "%s: %s done.\n",
 		    __func__, sp->name);
-		goto done_free_sp;
+		break;
 	default:
 		ql_dbg(ql_dbg_vport, vha, 0xffff, "%s: %s Failed. %x.\n",
 		    __func__, sp->name, rval);
-		goto done_free_sp;
+		break;
 	}
 done:
-	return rval;
-
-done_free_sp:
 	sp->free(sp);
 	return rval;
 }
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 588e0d27f151..2e7a4a2d6c5a 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -996,7 +996,7 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
 		ql_dbg(ql_dbg_io + ql_dbg_verbose, vha, 0x3078,
 		    "Start scsi failed rval=%d for cmd=%p.\n", rval, cmd);
 		if (rval == QLA_INTERFACE_ERROR)
-			goto qc24_fail_command;
+			goto qc24_free_sp_fail_command;
 		goto qc24_host_busy_free_sp;
 	}
 
@@ -1008,6 +1008,11 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
 qc24_target_busy:
 	return SCSI_MLQUEUE_TARGET_BUSY;
 
+qc24_free_sp_fail_command:
+	sp->free(sp);
+	CMD_SP(cmd) = NULL;
+	qla2xxx_rel_qpair_sp(sp->qpair, sp);
+
 qc24_fail_command:
 	cmd->scsi_done(cmd);
 
-- 
2.12.0

[PATCH 4/8] qla2xxx: Fix driver unload hang

[Himanshu Madhani]

From: Quinn Tran <qutran@marvell.com>

This patch fixes driver unload hang by removing msleep()

Fixes: d74595278f4ab ("scsi: qla2xxx: Add multiple queue pair functionality.")
Cc: stable@vger.kernel.org
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index bddb26baedd2..ff4528702b4e 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -9009,8 +9009,6 @@ int qla2xxx_delete_qpair(struct scsi_qla_host *vha, struct qla_qpair *qpair)
 	struct qla_hw_data *ha = qpair->hw;
 
 	qpair->delete_in_progress = 1;
-	while (atomic_read(&qpair->ref_count))
-		msleep(500);
 
 	ret = qla25xx_delete_req_que(vha, qpair->req);
 	if (ret != QLA_SUCCESS)
-- 
2.12.0

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

[Himanshu Madhani]

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

[PATCH 6/8] qla2xxx: Fix memory leak when sending I/O fails

[Himanshu Madhani]

From: Arun Easi <aeasi@marvell.com>

On heavy loads, a memory leak of the srb_t structure is observed.
This would make the qla2xxx_srbs cache gobble up memory.

Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
Cc: stable@vger.kernel.org # 5.2
Signed-off-by: Arun Easi <aeasi@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_os.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index b59d579834ea..b513008042fb 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -909,6 +909,8 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 
 qc24_host_busy_free_sp:
 	sp->free(sp);
+	CMD_SP(cmd) = NULL;
+	qla2x00_rel_sp(sp);
 
 qc24_target_busy:
 	return SCSI_MLQUEUE_TARGET_BUSY;
@@ -992,6 +994,8 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
 
 qc24_host_busy_free_sp:
 	sp->free(sp);
+	CMD_SP(cmd) = NULL;
+	qla2xxx_rel_qpair_sp(sp->qpair, sp);
 
 qc24_target_busy:
 	return SCSI_MLQUEUE_TARGET_BUSY;
-- 
2.12.0

[PATCH 7/8] qla2xxx: Fix device connect issues in P2P configuration

[Himanshu Madhani]

From: Arun Easi <aeasi@marvell.com>

P2P need to take the alternate plogi route.

Signed-off-by: Arun Easi <aeasi@marvell.com>
Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_gbl.h  | 1 +
 drivers/scsi/qla2xxx/qla_init.c | 9 +++++++++
 drivers/scsi/qla2xxx/qla_iocb.c | 5 ++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
index d11416dcee4e..5b163ad85c34 100644
--- a/drivers/scsi/qla2xxx/qla_gbl.h
+++ b/drivers/scsi/qla2xxx/qla_gbl.h
@@ -917,4 +917,5 @@ int qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode);
 
 /* nvme.c */
 void qla_nvme_unregister_remote_port(struct fc_port *fcport);
+void qla_handle_els_plogi_done(scsi_qla_host_t *vha, struct event_arg *ea);
 #endif /* _QLA_GBL_H */
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index ff4528702b4e..6bb4ddd90b6e 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -1717,6 +1717,15 @@ void qla24xx_handle_relogin_event(scsi_qla_host_t *vha,
 	qla24xx_fcport_handle_login(vha, fcport);
 }
 
+void qla_handle_els_plogi_done(scsi_qla_host_t *vha,
+				      struct event_arg *ea)
+{
+	ql_dbg(ql_dbg_disc, vha, 0x2118,
+	    "%s %d %8phC post PRLI\n",
+	    __func__, __LINE__, ea->fcport->port_name);
+	qla24xx_post_prli_work(vha, ea->fcport);
+}
+
 /*
  * RSCN(s) came in for this fcport, but the RSCN(s) was not able
  * to be consumed by the fcport
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 2b675da34bda..b25f87ff8cde 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2760,9 +2760,8 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
 		case CS_COMPLETE:
 			memset(&ea, 0, sizeof(ea));
 			ea.fcport = fcport;
-			ea.data[0] = MBS_COMMAND_COMPLETE;
-			ea.sp = sp;
-			qla24xx_handle_plogi_done_event(vha, &ea);
+			ea.rc = res;
+			qla_handle_els_plogi_done(vha, &ea);
 			break;
 
 		case CS_IOCB_ERROR:
-- 
2.12.0

[PATCH 8/8] qla2xxx: Update driver version to 10.01.00.21-k

[Himanshu Madhani]

Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
---
 drivers/scsi/qla2xxx/qla_version.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_version.h b/drivers/scsi/qla2xxx/qla_version.h
index 225e401b62fa..03bd3b712b77 100644
--- a/drivers/scsi/qla2xxx/qla_version.h
+++ b/drivers/scsi/qla2xxx/qla_version.h
@@ -7,7 +7,7 @@
 /*
  * Driver version
  */
-#define QLA2XXX_VERSION      "10.01.00.20-k"
+#define QLA2XXX_VERSION      "10.01.00.21-k"
 
 #define QLA_DRIVER_MAJOR_VER	10
 #define QLA_DRIVER_MINOR_VER	1
-- 
2.12.0

Re: [PATCH 1/8] qla2xxx: Retry PLOGI on FC-NVMe PRLI failure

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> Current code will send PRLI with FC-NVMe bit set for the
> targets which supports only FCP. This may result into issue
> with targets which do not understand NVMe and will go into
> strange state. This patch would restart the login process
> by going back to PLOGI state. The PLOGI state will force the
> target to respond to correct PRLI request.
> 
> Fixes: c76ae845ea836 ("scsi: qla2xxx: Add error handling for PLOGI ELS passthrough")
> Cc: stable@vger.kernel.org # 5.4
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_init.c | 37 ++++++++-----------------------------
>  drivers/scsi/qla2xxx/qla_iocb.c |  6 +++++-
>  2 files changed, 13 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 7cb7545de962..5db8ad832893 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1864,42 +1864,21 @@ qla24xx_handle_prli_done_event(struct scsi_qla_host *vha, struct event_arg *ea)
>  		 * FCP/NVMe port
>  		 */
>  		if (NVME_FCP_TARGET(ea->fcport)) {
> -			if (vha->hw->fc4_type_priority == FC4_PRIORITY_NVME)
> -				ea->fcport->fc4_type &= ~FS_FC4TYPE_NVME;
> -			else
> -				ea->fcport->fc4_type &= ~FS_FC4TYPE_FCP;
>  			ql_dbg(ql_dbg_disc, vha, 0x2118,
>  				"%s %d %8phC post %s prli\n",
>  				__func__, __LINE__, ea->fcport->port_name,
>  				(ea->fcport->fc4_type & FS_FC4TYPE_NVME) ?
>  				"NVMe" : "FCP");
> -			qla24xx_post_prli_work(vha, ea->fcport);
> -			break;
> +			if (vha->hw->fc4_type_priority == FC4_PRIORITY_NVME)
> +				ea->fcport->fc4_type &= ~FS_FC4TYPE_NVME;
> +			else
> +				ea->fcport->fc4_type &= ~FS_FC4TYPE_FCP;
>  		}
>  
> -		/* at this point both PRLI NVME & PRLI FCP failed */
> -		if (N2N_TOPO(vha->hw)) {
> -			if (ea->fcport->n2n_link_reset_cnt < 3) {
> -				ea->fcport->n2n_link_reset_cnt++;
> -				/*
> -				 * remote port is not sending Plogi. Reset
> -				 * link to kick start his state machine
> -				 */
> -				set_bit(N2N_LINK_RESET, &vha->dpc_flags);
> -			} else {
> -				ql_log(ql_log_warn, vha, 0x2119,
> -				    "%s %d %8phC Unable to reconnect\n",
> -				    __func__, __LINE__, ea->fcport->port_name);
> -			}
> -		} else {
> -			/*
> -			 * switch connect. login failed. Take connection
> -			 * down and allow relogin to retrigger
> -			 */
> -			ea->fcport->flags &= ~FCF_ASYNC_SENT;
> -			ea->fcport->keep_nport_handle = 0;
> -			qlt_schedule_sess_for_deletion(ea->fcport);
> -		}
> +		ea->fcport->flags &= ~FCF_ASYNC_SENT;
> +		ea->fcport->keep_nport_handle = 0;
> +		ea->fcport->logout_on_delete = 1;
> +		qlt_schedule_sess_for_deletion(ea->fcport);
>  		break;
>  	}
>  }
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index eeb526411536..2b675da34bda 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -2764,6 +2764,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
>  			ea.sp = sp;
>  			qla24xx_handle_plogi_done_event(vha, &ea);
>  			break;
> +
>  		case CS_IOCB_ERROR:
>  			switch (fw_status[1]) {
>  			case LSC_SCODE_PORTID_USED:
> @@ -2834,6 +2835,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
>  				    fw_status[0], fw_status[1], fw_status[2]);
>  
>  				fcport->flags &= ~FCF_ASYNC_SENT;
> +				fcport->disc_state = DSC_LOGIN_FAILED;
>  				set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
>  				break;
>  			}
> @@ -2846,6 +2848,7 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
>  			    fw_status[0], fw_status[1], fw_status[2]);
>  
>  			sp->fcport->flags &= ~FCF_ASYNC_SENT;
> +			sp->fcport->disc_state = DSC_LOGIN_FAILED;
>  			set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
>  			break;
>  		}
> @@ -2881,11 +2884,12 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode,
>  		return -ENOMEM;
>  	}
>  
> +	fcport->flags |= FCF_ASYNC_SENT;
> +	fcport->disc_state = DSC_LOGIN_PEND;
>  	elsio = &sp->u.iocb_cmd;
>  	ql_dbg(ql_dbg_io, vha, 0x3073,
>  	    "Enter: PLOGI portid=%06x\n", fcport->d_id.b24);
>  
> -	fcport->flags |= FCF_ASYNC_SENT;
>  	sp->type = SRB_ELS_DCMD;
>  	sp->name = "ELS_DCMD";
>  	sp->fcport = fcport;

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 2/8] qla2xxx: Do command completion on abort timeout

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> On switch, fabric and mgt command timeout, driver
> send Abort to tell FW to return the original command.
> If abort is timeout, then return both Abort and
> original command for cleanup.
> 
> 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: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h  |  1 +
>  drivers/scsi/qla2xxx/qla_init.c | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 721ee7f09b39..ef9bb3c7ad6f 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -604,6 +604,7 @@ typedef struct srb {
>  	const char *name;
>  	int iocbs;
>  	struct qla_qpair *qpair;
> +	struct srb *cmd_sp;
>  	struct list_head elem;
>  	u32 gen1;	/* scratch */
>  	u32 gen2;	/* scratch */
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 5db8ad832893..7fdbe041cc19 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -101,8 +101,22 @@ static void qla24xx_abort_iocb_timeout(void *data)
>  	u32 handle;
>  	unsigned long flags;
>  
> +	if (sp->cmd_sp)
> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
> +		    "Abort timeout - cmd hdl=%x, cmd type=%x hdl=%x, type=%x\n",
> +		    sp->cmd_sp->handle, sp->cmd_sp->type,
> +		    sp->handle, sp->type);
> +	else
> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
> +		    "Abort timeout 2 - hdl=%x, type=%x\n",
> +		    sp->handle, sp->type);
> +
>  	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
>  	for (handle = 1; handle < qpair->req->num_outstanding_cmds; handle++) {
> +		if (sp->cmd_sp && (qpair->req->outstanding_cmds[handle] ==
> +		    sp->cmd_sp))
> +			qpair->req->outstanding_cmds[handle] = NULL;
> +
>  		/* removing the abort */
>  		if (qpair->req->outstanding_cmds[handle] == sp) {
>  			qpair->req->outstanding_cmds[handle] = NULL;
> @@ -111,6 +125,9 @@ static void qla24xx_abort_iocb_timeout(void *data)
>  	}
>  	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
>  
> +	if (sp->cmd_sp)
> +		sp->cmd_sp->done(sp->cmd_sp, QLA_OS_TIMER_EXPIRED);
> +
>  	abt->u.abt.comp_status = CS_TIMEOUT;
>  	sp->done(sp, QLA_OS_TIMER_EXPIRED);
>  }
> @@ -142,6 +159,7 @@ static int qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
>  	sp->type = SRB_ABT_CMD;
>  	sp->name = "abort";
>  	sp->qpair = cmd_sp->qpair;
> +	sp->cmd_sp = cmd_sp;
>  	if (wait)
>  		sp->flags = SRB_WAKEUP_ON_COMP;
>  

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 3/8] qla2xxx: Fix SRB leak on switch command timeout

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> when GPSC/GPDB switch command fails, driver just returns
> without doing a proper cleanup. This patch fixes this memory
> leak by calling sp->free() in the error path.
> 
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_gs.c   |  2 +-
>  drivers/scsi/qla2xxx/qla_init.c | 11 +++++------
>  drivers/scsi/qla2xxx/qla_mbx.c  |  4 ----
>  drivers/scsi/qla2xxx/qla_mid.c  | 11 ++++-------
>  drivers/scsi/qla2xxx/qla_os.c   |  7 ++++++-
>  5 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
> index 7a00272ca380..67230688b05e 100644
> --- a/drivers/scsi/qla2xxx/qla_gs.c
> +++ b/drivers/scsi/qla2xxx/qla_gs.c
> @@ -3010,7 +3010,7 @@ static void qla24xx_async_gpsc_sp_done(srb_t *sp, int res)
>  	fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
>  
>  	if (res == QLA_FUNCTION_TIMEOUT)
> -		return;
> +		goto done;
>  
>  	if (res == (DID_ERROR << 16)) {
>  		/* entry status error */
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 7fdbe041cc19..bddb26baedd2 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1151,19 +1151,18 @@ static void qla24xx_async_gpdb_sp_done(srb_t *sp, int res)
>  	    "Async done-%s res %x, WWPN %8phC mb[1]=%x mb[2]=%x \n",
>  	    sp->name, res, fcport->port_name, mb[1], mb[2]);
>  
> -	if (res == QLA_FUNCTION_TIMEOUT) {
> -		dma_pool_free(sp->vha->hw->s_dma_pool, sp->u.iocb_cmd.u.mbx.in,
> -			sp->u.iocb_cmd.u.mbx.in_dma);
> -		return;
> -	}
> -
>  	fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
> +
> +	if (res == QLA_FUNCTION_TIMEOUT)
> +		goto done;
> +
>  	memset(&ea, 0, sizeof(ea));
>  	ea.fcport = fcport;
>  	ea.sp = sp;
>  
>  	qla24xx_handle_gpdb_event(vha, &ea);
>  
> +done:
>  	dma_pool_free(ha->s_dma_pool, sp->u.iocb_cmd.u.mbx.in,
>  		sp->u.iocb_cmd.u.mbx.in_dma);
>  
> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 04175c91af0e..0cf94f05f008 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -6288,17 +6288,13 @@ int qla24xx_send_mb_cmd(struct scsi_qla_host *vha, mbx_cmd_t *mcp)
>  	case  QLA_SUCCESS:
>  		ql_dbg(ql_dbg_mbx, vha, 0x119d, "%s: %s done.\n",
>  		    __func__, sp->name);
> -		sp->free(sp);
>  		break;
>  	default:
>  		ql_dbg(ql_dbg_mbx, vha, 0x119e, "%s: %s Failed. %x.\n",
>  		    __func__, sp->name, rval);
> -		sp->free(sp);
>  		break;
>  	}
>  
> -	return rval;
> -
>  done_free_sp:
>  	sp->free(sp);
>  done:
> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 6afad68e5ba2..bd62c4595b73 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -944,7 +944,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>  
>  	sp = qla2x00_get_sp(base_vha, NULL, GFP_KERNEL);
>  	if (!sp)
> -		goto done;
> +		return rval;
>  
>  	sp->type = SRB_CTRL_VP;
>  	sp->name = "ctrl_vp";
> @@ -960,7 +960,7 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>  		ql_dbg(ql_dbg_async, vha, 0xffff,
>  		    "%s: %s Failed submission. %x.\n",
>  		    __func__, sp->name, rval);
> -		goto done_free_sp;
> +		goto done;
>  	}
>  
>  	ql_dbg(ql_dbg_vport, vha, 0x113f, "%s hndl %x submitted\n",
> @@ -978,16 +978,13 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>  	case QLA_SUCCESS:
>  		ql_dbg(ql_dbg_vport, vha, 0xffff, "%s: %s done.\n",
>  		    __func__, sp->name);
> -		goto done_free_sp;
> +		break;
>  	default:
>  		ql_dbg(ql_dbg_vport, vha, 0xffff, "%s: %s Failed. %x.\n",
>  		    __func__, sp->name, rval);
> -		goto done_free_sp;
> +		break;
>  	}
>  done:
> -	return rval;
> -
> -done_free_sp:
>  	sp->free(sp);
>  	return rval;
>  }
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 588e0d27f151..2e7a4a2d6c5a 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -996,7 +996,7 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
>  		ql_dbg(ql_dbg_io + ql_dbg_verbose, vha, 0x3078,
>  		    "Start scsi failed rval=%d for cmd=%p.\n", rval, cmd);
>  		if (rval == QLA_INTERFACE_ERROR)
> -			goto qc24_fail_command;
> +			goto qc24_free_sp_fail_command;
>  		goto qc24_host_busy_free_sp;
>  	}
>  
> @@ -1008,6 +1008,11 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
>  qc24_target_busy:
>  	return SCSI_MLQUEUE_TARGET_BUSY;
>  
> +qc24_free_sp_fail_command:
> +	sp->free(sp);
> +	CMD_SP(cmd) = NULL;
> +	qla2xxx_rel_qpair_sp(sp->qpair, sp);
> +
>  qc24_fail_command:
>  	cmd->scsi_done(cmd);
>  

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 4/8] qla2xxx: Fix driver unload hang

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> This patch fixes driver unload hang by removing msleep()
> 
> Fixes: d74595278f4ab ("scsi: qla2xxx: Add multiple queue pair functionality.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_init.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index bddb26baedd2..ff4528702b4e 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -9009,8 +9009,6 @@ int qla2xxx_delete_qpair(struct scsi_qla_host *vha, struct qla_qpair *qpair)
>  	struct qla_hw_data *ha = qpair->hw;
>  
>  	qpair->delete_in_progress = 1;
> -	while (atomic_read(&qpair->ref_count))
> -		msleep(500);
>  
>  	ret = qla25xx_delete_req_que(vha, qpair->req);
>  	if (ret != QLA_SUCCESS)

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

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

[Ewan D. Milne]

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>

Re: [PATCH 6/8] qla2xxx: Fix memory leak when sending I/O fails

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Arun Easi <aeasi@marvell.com>
> 
> On heavy loads, a memory leak of the srb_t structure is observed.
> This would make the qla2xxx_srbs cache gobble up memory.
> 
> Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
> Cc: stable@vger.kernel.org # 5.2
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_os.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index b59d579834ea..b513008042fb 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -909,6 +909,8 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  
>  qc24_host_busy_free_sp:
>  	sp->free(sp);
> +	CMD_SP(cmd) = NULL;
> +	qla2x00_rel_sp(sp);
>  
>  qc24_target_busy:
>  	return SCSI_MLQUEUE_TARGET_BUSY;
> @@ -992,6 +994,8 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
>  
>  qc24_host_busy_free_sp:
>  	sp->free(sp);
> +	CMD_SP(cmd) = NULL;
> +	qla2xxx_rel_qpair_sp(sp->qpair, sp);
>  
>  qc24_target_busy:
>  	return SCSI_MLQUEUE_TARGET_BUSY;

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 7/8] qla2xxx: Fix device connect issues in P2P configuration

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> From: Arun Easi <aeasi@marvell.com>
> 
> P2P need to take the alternate plogi route.
> 
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_gbl.h  | 1 +
>  drivers/scsi/qla2xxx/qla_init.c | 9 +++++++++
>  drivers/scsi/qla2xxx/qla_iocb.c | 5 ++---
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_gbl.h b/drivers/scsi/qla2xxx/qla_gbl.h
> index d11416dcee4e..5b163ad85c34 100644
> --- a/drivers/scsi/qla2xxx/qla_gbl.h
> +++ b/drivers/scsi/qla2xxx/qla_gbl.h
> @@ -917,4 +917,5 @@ int qla2x00_set_data_rate(scsi_qla_host_t *vha, uint16_t mode);
>  
>  /* nvme.c */
>  void qla_nvme_unregister_remote_port(struct fc_port *fcport);
> +void qla_handle_els_plogi_done(scsi_qla_host_t *vha, struct event_arg *ea);
>  #endif /* _QLA_GBL_H */
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index ff4528702b4e..6bb4ddd90b6e 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -1717,6 +1717,15 @@ void qla24xx_handle_relogin_event(scsi_qla_host_t *vha,
>  	qla24xx_fcport_handle_login(vha, fcport);
>  }
>  
> +void qla_handle_els_plogi_done(scsi_qla_host_t *vha,
> +				      struct event_arg *ea)
> +{
> +	ql_dbg(ql_dbg_disc, vha, 0x2118,
> +	    "%s %d %8phC post PRLI\n",
> +	    __func__, __LINE__, ea->fcport->port_name);
> +	qla24xx_post_prli_work(vha, ea->fcport);
> +}
> +
>  /*
>   * RSCN(s) came in for this fcport, but the RSCN(s) was not able
>   * to be consumed by the fcport
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 2b675da34bda..b25f87ff8cde 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -2760,9 +2760,8 @@ static void qla2x00_els_dcmd2_sp_done(srb_t *sp, int res)
>  		case CS_COMPLETE:
>  			memset(&ea, 0, sizeof(ea));
>  			ea.fcport = fcport;
> -			ea.data[0] = MBS_COMMAND_COMPLETE;
> -			ea.sp = sp;
> -			qla24xx_handle_plogi_done_event(vha, &ea);
> +			ea.rc = res;
> +			qla_handle_els_plogi_done(vha, &ea);
>  			break;
>  
>  		case CS_IOCB_ERROR:

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 8/8] qla2xxx: Update driver version to 10.01.00.21-k

[Ewan D. Milne]

On Tue, 2019-11-05 at 07:06 -0800, Himanshu Madhani wrote:
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>  drivers/scsi/qla2xxx/qla_version.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_version.h b/drivers/scsi/qla2xxx/qla_version.h
> index 225e401b62fa..03bd3b712b77 100644
> --- a/drivers/scsi/qla2xxx/qla_version.h
> +++ b/drivers/scsi/qla2xxx/qla_version.h
> @@ -7,7 +7,7 @@
>  /*
>   * Driver version
>   */
> -#define QLA2XXX_VERSION      "10.01.00.20-k"
> +#define QLA2XXX_VERSION      "10.01.00.21-k"
>  
>  #define QLA_DRIVER_MAJOR_VER	10
>  #define QLA_DRIVER_MINOR_VER	1

Reviewed-by: Ewan D. Milne <emilne@redhat.com>

Re: [PATCH 2/8] qla2xxx: Do command completion on abort timeout

[Bart Van Assche]

On 11/5/19 7:06 AM, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> On switch, fabric and mgt command timeout, driver
> send Abort to tell FW to return the original command.
> If abort is timeout, then return both Abort and
> original command for cleanup.
> 
> 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: Himanshu Madhani <hmadhani@marvell.com>
> ---
>   drivers/scsi/qla2xxx/qla_def.h  |  1 +
>   drivers/scsi/qla2xxx/qla_init.c | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 721ee7f09b39..ef9bb3c7ad6f 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -604,6 +604,7 @@ typedef struct srb {
>   	const char *name;
>   	int iocbs;
>   	struct qla_qpair *qpair;
> +	struct srb *cmd_sp;
>   	struct list_head elem;
>   	u32 gen1;	/* scratch */
>   	u32 gen2;	/* scratch */
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 5db8ad832893..7fdbe041cc19 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -101,8 +101,22 @@ static void qla24xx_abort_iocb_timeout(void *data)
>   	u32 handle;
>   	unsigned long flags;
>   
> +	if (sp->cmd_sp)
> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
> +		    "Abort timeout - cmd hdl=%x, cmd type=%x hdl=%x, type=%x\n",
> +		    sp->cmd_sp->handle, sp->cmd_sp->type,
> +		    sp->handle, sp->type);
> +	else
> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
> +		    "Abort timeout 2 - hdl=%x, type=%x\n",
> +		    sp->handle, sp->type);
> +
>   	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
>   	for (handle = 1; handle < qpair->req->num_outstanding_cmds; handle++) {
> +		if (sp->cmd_sp && (qpair->req->outstanding_cmds[handle] ==
> +		    sp->cmd_sp))
> +			qpair->req->outstanding_cmds[handle] = NULL;
> +
>   		/* removing the abort */
>   		if (qpair->req->outstanding_cmds[handle] == sp) {
>   			qpair->req->outstanding_cmds[handle] = NULL;
> @@ -111,6 +125,9 @@ static void qla24xx_abort_iocb_timeout(void *data)
>   	}
>   	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
>   
> +	if (sp->cmd_sp)
> +		sp->cmd_sp->done(sp->cmd_sp, QLA_OS_TIMER_EXPIRED);
> +
>   	abt->u.abt.comp_status = CS_TIMEOUT;
>   	sp->done(sp, QLA_OS_TIMER_EXPIRED);
>   }
> @@ -142,6 +159,7 @@ static int qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
>   	sp->type = SRB_ABT_CMD;
>   	sp->name = "abort";
>   	sp->qpair = cmd_sp->qpair;
> +	sp->cmd_sp = cmd_sp;
>   	if (wait)
>   		sp->flags = SRB_WAKEUP_ON_COMP;
>   

After an abort SRB has been submitted it can happen that the command 
that should be aborted (cmd_sp) completes before the abort SRB 
completes. I think in that case sp->cmd_sp should be cleared. However, I 
don't see the code that does that in the above patch.

Since the block layer already keeps track of which commands are 
outstanding, is it really necessary to add the 'cmd_sp' pointer in 
struct srb? Has it been considered to use blk_mq_tagset_busy_iter() 
instead of iterating over qpair->req->num_outstanding_cmds in 
qla24xx_abort_iocb_timeout()?

Thanks,

Bart.

Re: [EXT] [PATCH 2/8] qla2xxx: Do command completion on abort timeout

[Himanshu Madhani]

Bart,


> On Nov 5, 2019, at 10:57 AM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 11/5/19 7:06 AM, Himanshu Madhani wrote:
>> From: Quinn Tran <qutran@marvell.com>
>> On switch, fabric and mgt command timeout, driver
>> send Abort to tell FW to return the original command.
>> If abort is timeout, then return both Abort and
>> original command for cleanup.
>> 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: Himanshu Madhani <hmadhani@marvell.com>
>> ---
>>  drivers/scsi/qla2xxx/qla_def.h  |  1 +
>>  drivers/scsi/qla2xxx/qla_init.c | 18 ++++++++++++++++++
>>  2 files changed, 19 insertions(+)
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index 721ee7f09b39..ef9bb3c7ad6f 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -604,6 +604,7 @@ typedef struct srb {
>>  	const char *name;
>>  	int iocbs;
>>  	struct qla_qpair *qpair;
>> +	struct srb *cmd_sp;
>>  	struct list_head elem;
>>  	u32 gen1;	/* scratch */
>>  	u32 gen2;	/* scratch */
>> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
>> index 5db8ad832893..7fdbe041cc19 100644
>> --- a/drivers/scsi/qla2xxx/qla_init.c
>> +++ b/drivers/scsi/qla2xxx/qla_init.c
>> @@ -101,8 +101,22 @@ static void qla24xx_abort_iocb_timeout(void *data)
>>  	u32 handle;
>>  	unsigned long flags;
>>  +	if (sp->cmd_sp)
>> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
>> +		    "Abort timeout - cmd hdl=%x, cmd type=%x hdl=%x, type=%x\n",
>> +		    sp->cmd_sp->handle, sp->cmd_sp->type,
>> +		    sp->handle, sp->type);
>> +	else
>> +		ql_dbg(ql_dbg_async, sp->vha, 0x507c,
>> +		    "Abort timeout 2 - hdl=%x, type=%x\n",
>> +		    sp->handle, sp->type);
>> +
>>  	spin_lock_irqsave(qpair->qp_lock_ptr, flags);
>>  	for (handle = 1; handle < qpair->req->num_outstanding_cmds; handle++) {
>> +		if (sp->cmd_sp && (qpair->req->outstanding_cmds[handle] ==
>> +		    sp->cmd_sp))
>> +			qpair->req->outstanding_cmds[handle] = NULL;
>> +
>>  		/* removing the abort */
>>  		if (qpair->req->outstanding_cmds[handle] == sp) {
>>  			qpair->req->outstanding_cmds[handle] = NULL;
>> @@ -111,6 +125,9 @@ static void qla24xx_abort_iocb_timeout(void *data)
>>  	}
>>  	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
>>  +	if (sp->cmd_sp)
>> +		sp->cmd_sp->done(sp->cmd_sp, QLA_OS_TIMER_EXPIRED);
>> +
>>  	abt->u.abt.comp_status = CS_TIMEOUT;
>>  	sp->done(sp, QLA_OS_TIMER_EXPIRED);
>>  }
>> @@ -142,6 +159,7 @@ static int qla24xx_async_abort_cmd(srb_t *cmd_sp, bool wait)
>>  	sp->type = SRB_ABT_CMD;
>>  	sp->name = "abort";
>>  	sp->qpair = cmd_sp->qpair;
>> +	sp->cmd_sp = cmd_sp;
>>  	if (wait)
>>  		sp->flags = SRB_WAKEUP_ON_COMP;
>>  
> 
> After an abort SRB has been submitted it can happen that the command that should be aborted (cmd_sp) completes before the abort SRB completes. I think in that case sp->cmd_sp should be cleared. However, I don't see the code that does that in the above patch.
> 

Good point there and agree that there is a small window where we are not clearing the cmd_sp when the original command is completed. However for this series, I would like to have this patch merged as-is and we’ll send a follow up patch to cover that window. We are doing more cleanup in the abort handling code path to make it more robust. This would be good candidate for that series. 

> Since the block layer already keeps track of which commands are outstanding, is it really necessary to add the 'cmd_sp' pointer in struct srb? Has it been considered to use blk_mq_tagset_busy_iter() instead of iterating over qpair->req->num_outstanding_cmds in qla24xx_abort_iocb_timeout()?
> 

Your suggestion to consider blk_mq_tagset_busy_iter() is correct for the commands from block layer. However, qpair->req->num_outstanding_cmds is the qla2xxx driver’s commend array to keep track of which commands that driver needs to process and so we do need to iterate over it and track the commands that we need to send aborts for. 

- Himanshu

> Thanks,
> 
> Bart.
> 
> 

Re: [PATCH 4/8] qla2xxx: Fix driver unload hang

[Bart Van Assche]

On 11/5/19 7:06 AM, Himanshu Madhani wrote:
> From: Quinn Tran <qutran@marvell.com>
> 
> This patch fixes driver unload hang by removing msleep()
> 
> Fixes: d74595278f4ab ("scsi: qla2xxx: Add multiple queue pair functionality.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Quinn Tran <qutran@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
>   drivers/scsi/qla2xxx/qla_init.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index bddb26baedd2..ff4528702b4e 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -9009,8 +9009,6 @@ int qla2xxx_delete_qpair(struct scsi_qla_host *vha, struct qla_qpair *qpair)
>   	struct qla_hw_data *ha = qpair->hw;
>   
>   	qpair->delete_in_progress = 1;
> -	while (atomic_read(&qpair->ref_count))
> -		msleep(500);
>   
>   	ret = qla25xx_delete_req_que(vha, qpair->req);
>   	if (ret != QLA_SUCCESS)

I think that an explanation is needed why that loop had been introduced 
and also why it is safe not to wait until qpair->ref_count drops to zero 
in qla2xxx_delete_qpair().

Thanks,

Bart.

Re: [PATCH 4/8] qla2xxx: Fix driver unload hang

[Bart Van Assche]

On 11/7/19 9:55 AM, Himanshu Madhani wrote:
> 
> 
>> On Nov 7, 2019, at 10:54 AM, Bart Van Assche <bvanassche@acm.org
>> <mailto:bvanassche@acm.org>> wrote:
>>
>> On 11/5/19 7:06 AM, Himanshu Madhani wrote:
>>> From: Quinn Tran <qutran@marvell.com <mailto:qutran@marvell.com>>
>>> This patch fixes driver unload hang by removing msleep()
>>> Fixes: d74595278f4ab ("scsi: qla2xxx: Add multiple queue pair
>>> functionality.")
>>> Cc: stable@vger.kernel.org <mailto:stable@vger.kernel.org>
>>> Signed-off-by: Quinn Tran <qutran@marvell.com
>>> <mailto:qutran@marvell.com>>
>>> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com
>>> <mailto:hmadhani@marvell.com>>
>>> ---
>>>  drivers/scsi/qla2xxx/qla_init.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>> diff --git a/drivers/scsi/qla2xxx/qla_init.c
>>> b/drivers/scsi/qla2xxx/qla_init.c
>>> index bddb26baedd2..ff4528702b4e 100644
>>> --- a/drivers/scsi/qla2xxx/qla_init.c
>>> +++ b/drivers/scsi/qla2xxx/qla_init.c
>>> @@ -9009,8 +9009,6 @@ int qla2xxx_delete_qpair(struct scsi_qla_host
>>> *vha, struct qla_qpair *qpair)
>>>  struct qla_hw_data *ha = qpair->hw;
>>>    qpair->delete_in_progress = 1;
>>> -while (atomic_read(&qpair->ref_count))
>>> -msleep(500);
>>>    ret = qla25xx_delete_req_que(vha, qpair->req);
>>>  if (ret != QLA_SUCCESS)
>>
>> I think that an explanation is needed why that loop had been
>> introduced and also why it is safe not to wait until qpair->ref_count
>> drops to zero in qla2xxx_delete_qpair().
>>
> 
> commit d74595278f4ab had drawback in design for MQ implementation in
> qla2xxx. Now that we have been making this more stable with MQ being
> default on for 5x kernel. What we discovered that after heavy IO
> workload in a cluster environment, driver unload encountered hang and
> shows following stack trace
> 
> # ps -fax | grep rmmod
> 6029 pts/0 D+ 0:00 | \_ rmmod qla2xxx
> 
> [<0>] msleep+0x29/0x30 [<0>] qla2xxx_delete_qpair+0x2c/0x160 [qla2xxx]
> [<0>] qla25xx_delete_queues+0x14b/0x1d0 [qla2xxx] [<0>]
> qla2x00_free_device+0x31/0xe0 [qla2xxx] [<0>]
> qla2x00_remove_one+0x239/0x370 [qla2xxx] [<0>]
> pci_device_remove+0x3b/0xc0 [<0>]
> device_release_driver_internal+0x18c/0x250 [<0>] driver_detach+0x39/0x6d
> [<0>] bus_remove_driver+0x77/0xc9 [<0>] pci_unregister_driver+0x2d/0xb0
> [<0>] qla2x00_module_exit+0x2d/0x90 [qla2xxx] [<0>]
> __x64_sys_delete_module+0x139/0x270 [<0>] do_syscall_64+0x5b/0x1b0 [<0>]
> entry_SYSCALL_64_after_hwframe+0x65/0xca [<0>] 0xffffffffffffffff
> 
> Removing this msleep() help resolve this stack trace. 

Hi Himanshu,

Does your answer mean that this hang has not yet been root-caused fully
and hence that it is possible this patch is only a workaround but not a
fix of the root cause?

Thanks,

Bart.

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

[Bart Van Assche]

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.

Re: [PATCH 4/8] qla2xxx: Fix driver unload hang

[Bart Van Assche]

On 11/7/19 9:58 AM, Bart Van Assche wrote:
> Does your answer mean that this hang has not yet been root-caused fully
> and hence that it is possible this patch is only a workaround but not a
> fix of the root cause?

Answering my own question: I think that a qpair refcount leak is a 
severe problem and not something that should be ignored. How about 
changing the while loop into something like the following:

	if (atomic_read(&qpair->ref_count))
		msleep(500);
	WARN_ON_ONCE(atomic_read(&qpair->ref_count));

Thanks,

Bart.

Re: [EXT] [PATCH 4/8] qla2xxx: Fix driver unload hang

[Himanshu Madhani]

Hi Bart, 

> On Nov 7, 2019, at 12:30 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 11/7/19 9:58 AM, Bart Van Assche wrote:
>> Does your answer mean that this hang has not yet been root-caused fully
>> and hence that it is possible this patch is only a workaround but not a
>> fix of the root cause?
> 
> Answering my own question: I think that a qpair refcount leak is a severe problem and not something that should be ignored. How about changing the while loop into something like the following:
> 
> 	if (atomic_read(&qpair->ref_count))
> 		msleep(500);
> 	WARN_ON_ONCE(atomic_read(&qpair->ref_count));
> 
> Thanks,
> 
> Bart.

Since we had seen this hang in a specific cluster environment and refcount leak was observed, I would like to add this patch as is and will consider your suggestion to verify if adding WARN_ON_ONCE will make any difference. If we discover that adding WARN_ON_ONCE indeed helps, then I will add a patch with fixes tag during rc window. 

Let me know if you disagree. 

Thanks,
Himanshu

Re: [EXT] [PATCH 4/8] qla2xxx: Fix driver unload hang

[Bart Van Assche]

On 11/8/19 3:38 PM, Himanshu Madhani wrote:
> Hi Bart,
> 
>> On Nov 7, 2019, at 12:30 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 11/7/19 9:58 AM, Bart Van Assche wrote:
>>> Does your answer mean that this hang has not yet been root-caused fully
>>> and hence that it is possible this patch is only a workaround but not a
>>> fix of the root cause?
>>
>> Answering my own question: I think that a qpair refcount leak is a severe problem and not something that should be ignored. How about changing the while loop into something like the following:
>>
>> 	if (atomic_read(&qpair->ref_count))
>> 		msleep(500);
>> 	WARN_ON_ONCE(atomic_read(&qpair->ref_count));
>>
>> Thanks,
>>
>> Bart.
> 
> Since we had seen this hang in a specific cluster environment and refcount leak was observed, I would like to add this patch as is and will consider your suggestion to verify if adding WARN_ON_ONCE will make any difference. If we discover that adding WARN_ON_ONCE indeed helps, then I will add a patch with fixes tag during rc window.
> 
> Let me know if you disagree.

Hi Himanshu,

Please do not suppress reports of kernel bugs but instead make sure that 
some report is provided that indicates that something went wrong and 
needs further attention.

Thanks,

Bart.

Re: [PATCH 0/8] qla2xxx: Bug Fixes for the driver

[Martin K. Petersen]

Himanshu,

> This series contains bug fixes for the driver. 

Applied to 5.5/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 6/8] qla2xxx: Fix memory leak when sending I/O fails

[Himanshu Madhani]

Hi Martin, 

> On Nov 5, 2019, at 9:06 AM, Himanshu Madhani <hmadhani@marvell.com> wrote:
> 
> From: Arun Easi <aeasi@marvell.com>
> 
> On heavy loads, a memory leak of the srb_t structure is observed.
> This would make the qla2xxx_srbs cache gobble up memory.
> 
> Fixes: 219d27d7147e0 ("scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands")
> Cc: stable@vger.kernel.org # 5.2
> Signed-off-by: Arun Easi <aeasi@marvell.com>
> Signed-off-by: Himanshu Madhani <hmadhani@marvell.com>
> ---
> drivers/scsi/qla2xxx/qla_os.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index b59d579834ea..b513008042fb 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -909,6 +909,8 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> 
> qc24_host_busy_free_sp:
> 	sp->free(sp);
> +	CMD_SP(cmd) = NULL;
> +	qla2x00_rel_sp(sp);
> 
> qc24_target_busy:
> 	return SCSI_MLQUEUE_TARGET_BUSY;
> @@ -992,6 +994,8 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
> 
> qc24_host_busy_free_sp:
> 	sp->free(sp);
> +	CMD_SP(cmd) = NULL;
> +	qla2xxx_rel_qpair_sp(sp->qpair, sp);
> 
> qc24_target_busy:
> 	return SCSI_MLQUEUE_TARGET_BUSY;
> -- 
> 2.12.0
> 

We found this patch to have introduced regression of double free. Please revert this patch in 5.5/scsi-queue and not needed with the patch 5 included in the series. 

Thanks,
Himanshu

Re: [PATCH 6/8] qla2xxx: Fix memory leak when sending I/O fails

[Martin K. Petersen]

Himanshu,

> We found this patch to have introduced regression of double
> free. Please revert this patch in 5.5/scsi-queue and not needed with
> the patch 5 included in the series.

Reverted in 5.5/scsi-queue, thanks.

-- 
Martin K. Petersen	Oracle Linux Engineering