From 92d94d4d1779723fb5605f1d1fa235f86047da00 Mon Sep 17 00:00:00 2001
From: Himanshu Madhani <hmadhani@redhat.com>
Date: Thu, 1 Aug 2019 15:55:57 -0400
Subject: [PATCH 097/124] [scsi] scsi: qla2xxx: Fix race conditions in the code
for aborting SCSI commands
Message-id: <20190801155618.12650-98-hmadhani@redhat.com>
Patchwork-id: 267896
O-Subject: [RHEL 7.8 e-stor PATCH 097/118] scsi: qla2xxx: Fix race conditions in the code for aborting SCSI commands
Bugzilla: 1729270
RH-Acked-by: Jarod Wilson <jarod@redhat.com>
RH-Acked-by: Tony Camuso <tcamuso@redhat.com>
From: Bart Van Assche <bvanassche@acm.org>
Bugzilla 1729270
In the *_done() functions, instead of returning early if sp->ref_count >=
2, only decrement sp->ref_count. In qla2xxx_eh_abort(), instead of deciding
what to do based on the value of sp->ref_count, decide which action to take
depending on the completion status of the firmware abort. Remove srb.cwaitq
and use srb.comp instead. In qla2x00_abort_srb(), call
isp_ops->abort_command() directly instead of calling qla2xxx_eh_abort().
Cc: Himanshu Madhani <hmadhani@marvell.com>
Cc: Giridhar Malavali <gmalavali@marvell.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Acked-by: Himanshu Madhani <hmadhani@marvell.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
(cherry picked from commit 219d27d7147e07fe899a781bd72f9180b78c3852)
Signed-off-by: Himanshu Madhani <hmadhani@redhat.com>
Conflicts:
drivers/scsi/qla2xxx/qla_os.c
[HM: RHEL78 kernel source does not have commit 25ab0bc334b4 ]
[ ("scsi: sched/wait: Add wait_event_lock_irq_timeout for ]
[ TASK_UNINTERRUPTIBLE usage"). Since this macro was missing ]
[ commit 711a08d79f71 ("scsi: qla2xxx: Change abort wait_loop ]
[ from msleep to wait_event_timeout") was not backported. ]
[ This patch now removes code that was added by commit 711a08d79f71 ]
[ Due to skipped commit code shows deviation from upstream ]
[ in qla_os.c, qla2xxx_qpair_sp_compl() and qla2x00_sp_free_dma() ]
[ brings in source from commit 711a08d79f71, to set the cmd->result ]
[ and CMD_SP(cmd) also removes double qla2x00_rel_sp() ]
Signed-off-by: Himanshu Madhani <hmadhani@redhat.com>
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
drivers/scsi/qla2xxx/qla_nvme.c | 34 +--------
drivers/scsi/qla2xxx/qla_nvme.h | 1 -
drivers/scsi/qla2xxx/qla_os.c | 150 ++++++++++++++++------------------------
3 files changed, 62 insertions(+), 123 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
index 73d6b7833830..8ddd44bb6c7f 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.c
+++ b/drivers/scsi/qla2xxx/qla_nvme.c
@@ -137,8 +137,7 @@ static void qla_nvme_sp_ls_done(void *ptr, int res)
return;
}
- if (!atomic_dec_and_test(&sp->ref_count))
- return;
+ atomic_dec(&sp->ref_count);
if (res)
res = -EINVAL;
@@ -161,8 +160,7 @@ static void qla_nvme_sp_done(void *ptr, int res)
nvme = &sp->u.iocb_cmd;
fd = nvme->u.nvme.desc;
- if (!atomic_dec_and_test(&sp->ref_count))
- return;
+ atomic_dec(&sp->ref_count);
if (res == QLA_SUCCESS) {
fd->rcv_rsplen = nvme->u.nvme.rsp_pyld_len;
@@ -611,34 +609,6 @@ static struct nvme_fc_port_template qla_nvme_fc_transport = {
.fcprqst_priv_sz = sizeof(struct nvme_private),
};
-#define NVME_ABORT_POLLING_PERIOD 2
-static int qla_nvme_wait_on_command(srb_t *sp)
-{
- int ret = QLA_SUCCESS;
-
- wait_event_timeout(sp->nvme_ls_waitq, (atomic_read(&sp->ref_count) > 1),
- NVME_ABORT_POLLING_PERIOD*HZ);
-
- if (atomic_read(&sp->ref_count) > 1)
- ret = QLA_FUNCTION_FAILED;
-
- return ret;
-}
-
-void qla_nvme_abort(struct qla_hw_data *ha, struct srb *sp, int res)
-{
- int rval;
-
- if (ha->flags.fw_started) {
- rval = ha->isp_ops->abort_command(sp);
- if (!rval && !qla_nvme_wait_on_command(sp))
- ql_log(ql_log_warn, NULL, 0x2112,
- "timed out waiting on sp=%p\n", sp);
- } else {
- sp->done(sp, res);
- }
-}
-
static void qla_nvme_unregister_remote_port(struct work_struct *work)
{
struct fc_port *fcport = container_of(work, struct fc_port,
diff --git a/drivers/scsi/qla2xxx/qla_nvme.h b/drivers/scsi/qla2xxx/qla_nvme.h
index da8dad5ad693..0db04f0a4d5d 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.h
+++ b/drivers/scsi/qla2xxx/qla_nvme.h
@@ -145,7 +145,6 @@ struct pt_ls4_rx_unsol {
int qla_nvme_register_hba(struct scsi_qla_host *);
int qla_nvme_register_remote(struct scsi_qla_host *, struct fc_port *);
void qla_nvme_delete(struct scsi_qla_host *);
-void qla_nvme_abort(struct qla_hw_data *, struct srb *sp, int res);
void qla24xx_nvme_ls4_iocb(struct scsi_qla_host *, struct pt_ls4_request *,
struct req_que *);
void qla24xx_async_gffid_sp_done(void *, int);
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index a13798c4d178..a32074dd4727 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -728,7 +728,7 @@ qla2x00_sp_free_dma(void *ptr)
}
if (!ctx)
- goto end;
+ return;
if (sp->flags & SRB_CRC_CTX_DSD_VALID) {
/* List assured to be having elements */
@@ -753,12 +753,6 @@ qla2x00_sp_free_dma(void *ptr)
ha->gbl_dsd_avail += ctx1->dsd_use_cnt;
mempool_free(ctx1, ha->ctx_mempool);
}
-
-end:
- if (sp->type != SRB_NVME_CMD && sp->type != SRB_NVME_LS) {
- CMD_SP(cmd) = NULL;
- qla2x00_rel_sp(sp);
- }
}
void
@@ -766,6 +760,7 @@ qla2x00_sp_compl(void *ptr, int res)
{
srb_t *sp = ptr;
struct scsi_cmnd *cmd = GET_CMD_SP(sp);
+ struct completion *comp = sp->comp;
if (atomic_read(&sp->ref_count) == 0) {
ql_dbg(ql_dbg_io, sp->vha, 0x3015,
@@ -775,12 +770,15 @@ qla2x00_sp_compl(void *ptr, int res)
WARN_ON(atomic_read(&sp->ref_count) == 0);
return;
}
- if (!atomic_dec_and_test(&sp->ref_count))
- return;
+
+ atomic_dec(&sp->ref_count);
sp->free(sp);
cmd->result = res;
cmd->scsi_done(cmd);
+ if (comp)
+ complete(comp);
+ qla2x00_rel_sp(sp);
}
void
@@ -803,7 +801,7 @@ qla2xxx_qpair_sp_free_dma(void *ptr)
}
if (!ctx)
- goto end;
+ return;
if (sp->flags & SRB_CRC_CTX_DSD_VALID) {
/* List assured to be having elements */
@@ -865,10 +863,6 @@ qla2xxx_qpair_sp_free_dma(void *ptr)
dma_pool_free(ha->dl_dma_pool, ctx, ctx0->crc_ctx_dma);
sp->flags &= ~SRB_CRC_CTX_DMA_VALID;
}
-
-end:
- CMD_SP(cmd) = NULL;
- qla2xxx_rel_qpair_sp(sp->qpair, sp);
}
void
@@ -876,8 +870,7 @@ qla2xxx_qpair_sp_compl(void *ptr, int res)
{
srb_t *sp = ptr;
struct scsi_cmnd *cmd = GET_CMD_SP(sp);
-
- cmd->result = res;
+ struct completion *comp = sp->comp;
if (atomic_read(&sp->ref_count) == 0) {
ql_dbg(ql_dbg_io, sp->fcport->vha, 0x3079,
@@ -887,11 +880,16 @@ qla2xxx_qpair_sp_compl(void *ptr, int res)
WARN_ON(atomic_read(&sp->ref_count) == 0);
return;
}
- if (!atomic_dec_and_test(&sp->ref_count))
- return;
+
+ atomic_dec(&sp->ref_count);
sp->free(sp);
+ cmd->result = res;
+ CMD_SP(cmd) = NULL;
cmd->scsi_done(cmd);
+ if (comp)
+ complete(comp);
+ qla2xxx_rel_qpair_sp(sp->qpair, sp);
}
static int
@@ -1336,7 +1334,7 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
int ret;
unsigned int id, lun;
unsigned long flags;
- int rval, wait = 0;
+ int rval;
struct qla_hw_data *ha = vha->hw;
struct qla_qpair *qpair;
@@ -1349,7 +1347,6 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
ret = fc_block_scsi_eh(cmd);
if (ret != 0)
return ret;
- ret = SUCCESS;
sp = (srb_t *) CMD_SP(cmd);
if (!sp)
@@ -1360,7 +1357,7 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
return SUCCESS;
spin_lock_irqsave(qpair->qp_lock_ptr, flags);
- if (!CMD_SP(cmd)) {
+ if (sp->type != SRB_SCSI_CMD || GET_CMD_SP(sp) != cmd) {
/* there's a chance an interrupt could clear
the ptr as part of done & free */
spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
@@ -1381,58 +1378,31 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd)
"Aborting from RISC nexus=%ld:%d:%u sp=%p cmd=%p handle=%x\n",
vha->host_no, id, lun, sp, cmd, sp->handle);
- /* Get a reference to the sp and drop the lock.*/
-
rval = ha->isp_ops->abort_command(sp);
- if (rval) {
- if (rval == QLA_FUNCTION_PARAMETER_ERROR)
- ret = SUCCESS;
- else
- ret = FAILED;
-
- ql_dbg(ql_dbg_taskm, vha, 0x8003,
- "Abort command mbx failed cmd=%p, rval=%x.\n", cmd, rval);
- } else {
- ql_dbg(ql_dbg_taskm, vha, 0x8004,
- "Abort command mbx success cmd=%p.\n", cmd);
- wait = 1;
- }
-
- spin_lock_irqsave(qpair->qp_lock_ptr, flags);
- /*
- * Clear the slot in the oustanding_cmds array if we can't find the
- * command to reclaim the resources.
- */
- if (rval == QLA_FUNCTION_PARAMETER_ERROR)
- vha->req->outstanding_cmds[sp->handle] = NULL;
-
- /*
- * sp->done will do ref_count--
- * sp_get() took an extra count above
- */
- sp->done(sp, DID_RESET << 16);
+ ql_dbg(ql_dbg_taskm, vha, 0x8003,
+ "Abort command mbx cmd=%p, rval=%x.\n", cmd, rval);
- /* Did the command return during mailbox execution? */
- if (ret == FAILED && !CMD_SP(cmd))
+ 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;
-
- if (!CMD_SP(cmd))
- wait = 0;
-
- spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
-
- /* Wait for the command to be returned. */
- if (wait) {
- if (qla2x00_eh_wait_on_command(cmd) != QLA_SUCCESS) {
- ql_log(ql_log_warn, vha, 0x8006,
- "Abort handler timed out cmd=%p.\n", cmd);
- ret = FAILED;
- }
+ break;
+ default:
+ /*
+ * Either abort failed or abort and completion raced. Let
+ * the SCSI core retry the abort in the former case.
+ */
+ ret = FAILED;
+ break;
}
ql_log(ql_log_info, vha, 0x801c,
- "Abort command issued nexus=%ld:%d:%d -- %d %x.\n",
- vha->host_no, id, lun, wait, ret);
+ "Abort command issued nexus=%ld:%d:%d -- %x.\n",
+ vha->host_no, id, lun, ret);
return ret;
}
@@ -1806,34 +1776,34 @@ static void qla2x00_abort_srb(struct qla_qpair *qp, srb_t *sp, const int res,
__releases(qp->qp_lock_ptr)
__acquires(qp->qp_lock_ptr)
{
+ DECLARE_COMPLETION_ONSTACK(comp);
scsi_qla_host_t *vha = qp->vha;
struct qla_hw_data *ha = vha->hw;
+ int rval;
- if (sp->type == SRB_NVME_CMD || sp->type == SRB_NVME_LS) {
- if (!sp_get(sp)) {
- /* got sp */
- spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
- qla_nvme_abort(ha, sp, res);
- spin_lock_irqsave(qp->qp_lock_ptr, *flags);
- }
- } else if (GET_CMD_SP(sp) && !ha->flags.eeh_busy &&
- !test_bit(ABORT_ISP_ACTIVE, &vha->dpc_flags) &&
- !qla2x00_isp_reg_stat(ha) && sp->type == SRB_SCSI_CMD) {
- /*
- * Don't abort commands in adapter during EEH recovery as it's
- * not accessible/responding.
- *
- * Get a reference to the sp and drop the lock. The reference
- * ensures this sp->done() call and not the call in
- * qla2xxx_eh_abort() ends the SCSI cmd (with result 'res').
- */
- if (!sp_get(sp)) {
- spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
- qla2xxx_eh_abort(GET_CMD_SP(sp));
- spin_lock_irqsave(qp->qp_lock_ptr, *flags);
+ if (sp_get(sp))
+ 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))) {
+ sp->comp = ∁
+ rval = ha->isp_ops->abort_command(sp);
+ spin_unlock_irqrestore(qp->qp_lock_ptr, *flags);
+
+ switch (rval) {
+ case QLA_SUCCESS:
+ sp->done(sp, res);
+ break;
+ case QLA_FUNCTION_PARAMETER_ERROR:
+ wait_for_completion(&comp);
+ break;
}
+
+ spin_lock_irqsave(qp->qp_lock_ptr, *flags);
+ sp->comp = NULL;
}
- sp->done(sp, res);
}
static void
--
2.13.6