From 1143883593512f29e30a7b3df73d26f7fedcd7b6 Mon Sep 17 00:00:00 2001 From: Quinn Tran Date: Tue, 11 Jun 2019 11:33:49 -0700 Subject: [PATCH] qla2xxx: migrate srb->ref_count to cmd_kref Refactor srb cleanup process. Current code uses ref_count to track srb usage. When ref_count reaches 0, then the srb is freed. This patch make use of struct kref to lessen qla's code in tracking the buffer. Instead, it uses standardize kernel service to manage the buffer. In addition, when srb buffer is freed, the code will unbind srb from scsi_cmnd. This prevents stale memory access. Signed-off-by: Quinn Tran --- drivers/scsi/qla2xxx/qla_def.h | 7 ++- drivers/scsi/qla2xxx/qla_inline.h | 4 ++ drivers/scsi/qla2xxx/qla_os.c | 109 +++++++++++++++++++------------------- 3 files changed, 64 insertions(+), 56 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h index f9f27da5..8bac85a0 100644 --- a/drivers/scsi/qla2xxx/qla_def.h +++ b/drivers/scsi/qla2xxx/qla_def.h @@ -547,7 +547,7 @@ typedef struct srb { */ uint8_t cmd_type; uint8_t pad[3]; - atomic_t ref_count; + u64 cmd_id; struct kref cmd_kref; /* need to migrate ref_count over to this */ void *priv; wait_queue_head_t nvme_ls_waitq; @@ -577,6 +577,10 @@ typedef struct srb { void (*free)(void *); } srb_t; +struct srb_private { + u64 cmd_id; + spinlock_t cmd_lock; +}; #define GET_CMD_SP(sp) (sp->u.scmd.cmd) #define SET_CMD_SP(sp, cmd) (sp->u.scmd.cmd = cmd) #define GET_CMD_CTX_SP(sp) (sp->u.scmd.ctx) @@ -3540,6 +3544,7 @@ struct qla_qpair { spinlock_t *qp_lock_ptr; struct scsi_qla_host *vha; u32 chip_reset; + u64 cmd_id; /* distill these fields down to 'online=0/1' * ha->flags.eeh_busy diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h index c0b98c5b..2d219256 100644 --- a/drivers/scsi/qla2xxx/qla_inline.h +++ b/drivers/scsi/qla2xxx/qla_inline.h @@ -239,6 +239,7 @@ qla2xxx_get_qpair_sp(scsi_qla_host_t *vha, struct qla_qpair *qpair, fc_port_t *f { srb_t *sp = NULL; uint8_t bail; + unsigned long flags; QLA_QPAIR_MARK_BUSY(qpair, bail); if (unlikely(bail)) @@ -255,6 +256,9 @@ qla2xxx_get_qpair_sp(scsi_qla_host_t *vha, struct qla_qpair *qpair, fc_port_t *f sp->qpair = qpair; sp->cmd_type = TYPE_SRB; INIT_LIST_HEAD(&sp->elem); + spin_lock_irqsave(qpair->qp_lock_ptr, flags); + sp->cmd_id = qpair->cmd_id++; + spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); done: if (!sp) diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c index 7d4d6e47..0d4fea90 100644 --- a/drivers/scsi/qla2xxx/qla_os.c +++ b/drivers/scsi/qla2xxx/qla_os.c @@ -321,6 +321,7 @@ static int qla2x00_change_queue_type(struct scsi_device *, int); static void qla2x00_clear_drv_active(struct qla_hw_data *); static void qla2x00_free_device(scsi_qla_host_t *); static void qla2x00_destroy_deferred_work(struct qla_hw_data *); +void qla_release_fcp_cmd_kref(struct kref *); int ql2xdifbundlinginternalbuffers; module_param(ql2xdifbundlinginternalbuffers, int, S_IRUGO|S_IWUSR); @@ -358,6 +359,7 @@ struct scsi_host_template qla2xxx_driver_template = { .supported_mode = MODE_INITIATOR, .use_host_wide_tags = 1, + .cmd_size = sizeof(struct srb_private), }; static struct scsi_transport_template *qla2xxx_transport_template = NULL; @@ -764,11 +766,9 @@ qla2x00_sp_free_dma(void *ptr) } end: - if (sp->type != SRB_NVME_CMD && sp->type != SRB_NVME_LS) { - CMD_SP(cmd) = NULL; - qla2x00_rel_sp(sp); - track_rls |= BIT_6; - } + QLA_VHA_MARK_NOT_BUSY(sp->vha); + track_rls |= BIT_6; + return; } void @@ -777,7 +777,7 @@ qla2x00_sp_compl(void *ptr, int res) srb_t *sp = ptr; struct scsi_cmnd *cmd = GET_CMD_SP(sp); - if (atomic_read(&sp->ref_count) == 0) { + if (atomic_read(&sp->cmd_kref.refcount) == 0) { ql_dbg(ql_dbg_io, sp->vha, 0x3015, "SP reference-count to ZERO -- sp=%p cmd=%p.\n", sp, GET_CMD_SP(sp)); @@ -785,12 +785,9 @@ qla2x00_sp_compl(void *ptr, int res) BUG(); return; } - if (!atomic_dec_and_test(&sp->ref_count)) - return; - sp->free(sp); cmd->result = res; - cmd->scsi_done(cmd); + kref_put(&sp->cmd_kref, qla_release_fcp_cmd_kref); } void @@ -875,7 +872,26 @@ qla2xxx_qpair_sp_free_dma(void *ptr) } end: + return; +} + +void +qla_release_fcp_cmd_kref(struct kref *kref) +{ + struct srb *sp = container_of(kref, struct srb, cmd_kref); + struct scsi_cmnd *cmd = GET_CMD_SP(sp); + unsigned long flags; + struct srb_private *priv = (struct srb_private*)(cmd + 1); + + sp->free(sp); + + spin_lock_irqsave(&priv->cmd_lock, flags); CMD_SP(cmd) = NULL; + sp->u.scmd.cmd = NULL; + spin_unlock_irqrestore(&priv->cmd_lock, flags); + + cmd->scsi_done(cmd); + qla2xxx_rel_qpair_sp(sp->qpair, sp); } @@ -887,19 +903,15 @@ qla2xxx_qpair_sp_compl(void *ptr, int res) cmd->result = res; - if (atomic_read(&sp->ref_count) == 0) { + if (atomic_read(&sp->cmd_kref.refcount) == 0) { ql_dbg(ql_dbg_io, sp->fcport->vha, 0x3079, "SP reference-count to ZERO -- sp=%p cmd=%p.\n", sp, GET_CMD_SP(sp)); - if (ql2xextended_error_logging & ql_dbg_io) - WARN_ON(atomic_read(&sp->ref_count) == 0); + WARN_ON(ql2xextended_error_logging & ql_dbg_io); return; } - if (!atomic_dec_and_test(&sp->ref_count)) - return; - sp->free(sp); - cmd->scsi_done(cmd); + kref_put(&sp->cmd_kref, qla_release_fcp_cmd_kref); } /* If we are SP1 here, we need to still take and release the host_lock as SP1 @@ -918,6 +930,7 @@ qla2xxx_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) struct qla_qpair *qpair = NULL; uint32_t tag; uint16_t hwq; + struct srb_private *priv = (struct srb_private*)(cmd + 1); if (unlikely(test_bit(UNLOADING, &base_vha->dpc_flags))) { cmd->result = DID_NO_CONNECT << 16; @@ -1004,7 +1017,9 @@ 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); + kref_init(&sp->cmd_kref); + priv->cmd_id = sp->cmd_id; + spin_lock_init(&priv->cmd_lock); CMD_SP(cmd) = (void *)sp; sp->free = qla2x00_sp_free_dma; sp->done = qla2x00_sp_compl; @@ -1020,6 +1035,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_host_busy: return SCSI_MLQUEUE_HOST_BUSY; @@ -1045,6 +1062,7 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd, struct scsi_qla_host *base_vha = pci_get_drvdata(ha->pdev); srb_t *sp; int rval; + struct srb_private *priv = (struct srb_private*)(cmd + 1); rval = fc_remote_port_chkready(rport); if (rval) { @@ -1090,7 +1108,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); + kref_init(&sp->cmd_kref); + priv->cmd_id = sp->cmd_id; + spin_lock_init(&priv->cmd_lock); CMD_SP(cmd) = (void *)sp; sp->free = qla2xxx_qpair_sp_free_dma; sp->done = qla2xxx_qpair_sp_compl; @@ -1109,6 +1129,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_host_busy: return SCSI_MLQUEUE_HOST_BUSY; @@ -1290,16 +1312,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 @@ -1355,6 +1367,7 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd) int rval, wait = 0; struct qla_hw_data *ha = vha->hw; struct qla_qpair *qpair; + struct srb_private *priv = (struct srb_private*)(cmd + 1); if (qla2x00_isp_reg_stat(ha)) { ql_log(ql_log_info, vha, 0x8042, @@ -1367,31 +1380,21 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd) return ret; ret = SUCCESS; + spin_lock_irqsave(&priv->cmd_lock, flags); sp = (srb_t *) CMD_SP(cmd); - if (!sp) - return SUCCESS; - - qpair = sp->qpair; - if (!qpair) - return SUCCESS; - - if (sp->fcport && sp->fcport->deleted) - return SUCCESS; - - spin_lock_irqsave(qpair->qp_lock_ptr, flags); - if (!CMD_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); + if (!sp || !sp->qpair || + (priv->cmd_id != sp->cmd_id) || + (sp->fcport && sp->fcport->deleted)) { + spin_unlock_irqrestore(&priv->cmd_lock, flags); return SUCCESS; } - if (sp_get(sp)){ - /* ref_count is already 0 */ - spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); + if (!kref_get_unless_zero(&sp->cmd_kref)) { + spin_unlock_irqrestore(&priv->cmd_lock, flags); return SUCCESS; } - spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); + spin_unlock_irqrestore(&priv->cmd_lock, flags); + qpair = sp->qpair; id = cmd->device->id; lun = cmd->device->lun; @@ -1405,11 +1408,6 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd) rval = ha->isp_ops->abort_command(sp); if (rval) { if (rval == QLA_FUNCTION_PARAMETER_ERROR) { - /* - * Decrement the ref_count since we can't find the - * command - */ - atomic_dec(&sp->ref_count); ret = SUCCESS; } else ret = FAILED; @@ -1429,8 +1427,9 @@ qla2xxx_eh_abort(struct scsi_cmnd *cmd) */ 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); + + cmd->result = DID_RESET << 16; + kref_put(&sp->cmd_kref, qla_release_fcp_cmd_kref); /* Did the command return during mailbox execution? */ if (ret == FAILED && !CMD_SP(cmd)) -- 2.12.2