[PATCH 0/10] qla2xxx: Reduce the number of Coverity warnings

[PATCH 0/10] qla2xxx: Reduce the number of Coverity warnings

[Bart Van Assche]

The current implementation of the qla2xxx Fibre Channel initiator driver 
triggers multiple warnings when analyzed with the Coverity static 
analyzer. This patch series addresses a significant number of these 
warnings.

Note: patch 10/10 of this series is a reworked version of a patch that 
had been posted a few weeks ago on the linux-scsi mailing list.

[PATCH 01/10] qla2xxx: Clean up qla24xx_iidma()

[Bart Van Assche]

Remove dead code and simplify a pointer computation.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 39719f8..7221521 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1282,14 +1282,7 @@ qla24xx_iidma(struct fc_bsg_job *bsg_job)
 		return -EINVAL;
 	}
 
-	port_param = (struct qla_port_param *)((char *)bsg_job->request +
-		sizeof(struct fc_bsg_request));
-	if (!port_param) {
-		ql_log(ql_log_warn, vha, 0x7047,
-		    "port_param header not provided.\n");
-		return -EINVAL;
-	}
-
+	port_param = (void *)bsg_job->request + sizeof(struct fc_bsg_request);
 	if (port_param->fc_scsi_addr.dest_type != EXT_DEF_TYPE_WWPN) {
 		ql_log(ql_log_warn, vha, 0x7048,
 		    "Invalid destination type.\n");
-- 
1.7.10.4

[PATCH 02/10] qla2xxx: Clean up qla84xx_mgmt_cmd()

[Bart Van Assche]

Remove dead code, simplify a pointer computation and move the
ql84_mgmt assignment to just before its first use.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 7221521..cf07491 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1084,14 +1084,6 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
 		return -EINVAL;
 	}
 
-	ql84_mgmt = (struct qla_bsg_a84_mgmt *)((char *)bsg_job->request +
-		sizeof(struct fc_bsg_request));
-	if (!ql84_mgmt) {
-		ql_log(ql_log_warn, vha, 0x703b,
-		    "MGMT header not provided, exiting.\n");
-		return -EINVAL;
-	}
-
 	mn = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &mn_dma);
 	if (!mn) {
 		ql_log(ql_log_warn, vha, 0x703c,
@@ -1103,6 +1095,7 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
 	mn->entry_type = ACCESS_CHIP_IOCB_TYPE;
 	mn->entry_count = 1;
 
+	ql84_mgmt = (void *)bsg_job->request + sizeof(struct fc_bsg_request);
 	switch (ql84_mgmt->mgmt.cmd) {
 	case QLA84_MGMT_READ_MEM:
 	case QLA84_MGMT_GET_INFO:
-- 
1.7.10.4

[PATCH 03/10] qla2xxx: Remove dead code in qla2x00_configure_hba()

[Bart Van Assche]

At the end of qla2x00_configure_hba() we know that rval == QLA_SUCCESS.
Hence remove the code that depends on rval != QLA_SUCCESS.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_init.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 3565dfd..c68bb01 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2309,13 +2309,7 @@ qla2x00_configure_hba(scsi_qla_host_t *vha)
 		    "Topology - %s, Host Loop address 0x%x.\n",
 		    connect_type, vha->loop_id);
 
-	if (rval) {
-		ql_log(ql_log_warn, vha, 0x2011,
-		    "%s FAILED\n", __func__);
-	} else {
-		ql_dbg(ql_dbg_disc, vha, 0x2012,
-		    "%s success\n", __func__);
-	}
+	ql_dbg(ql_dbg_disc, vha, 0x2012, "%s success\n", __func__);
 
 	return(rval);
 }
-- 
1.7.10.4

[PATCH 04/10] qla2xxx: Remove two superfluous tests

[Bart Van Assche]

Since ha->model_desc is an array comparing it against NULL is
superfluous. Hence remove these tests.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_gs.c |    3 +--
 drivers/scsi/qla2xxx/qla_os.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index d0ea8b9..f26442a 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -1370,8 +1370,7 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha)
 	/* Model description. */
 	eiter = (struct ct_fdmi_hba_attr *) (entries + size);
 	eiter->type = __constant_cpu_to_be16(FDMI_HBA_MODEL_DESCRIPTION);
-	if (ha->model_desc)
-		strncpy(eiter->a.model_desc, ha->model_desc, 80);
+	strncpy(eiter->a.model_desc, ha->model_desc, 80);
 	alen = strlen(eiter->a.model_desc);
 	alen += (alen & 3) ? (4 - (alen & 3)) : 4;
 	eiter->len = cpu_to_be16(4 + alen);
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index ad72c1d..1d97626 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -2840,8 +2840,7 @@ skip_dpc:
 	qla2x00_dfs_setup(base_vha);
 
 	ql_log(ql_log_info, base_vha, 0x00fb,
-	    "QLogic %s - %s.\n",
-	    ha->model_number, ha->model_desc ? ha->model_desc : "");
+	    "QLogic %s - %s.\n", ha->model_number, ha->model_desc);
 	ql_log(ql_log_info, base_vha, 0x00fc,
 	    "ISP%04X: %s @ %s hdma%c host#=%ld fw=%s.\n",
 	    pdev->device, ha->isp_ops->pci_info_str(base_vha, pci_info),
-- 
1.7.10.4

[PATCH 05/10] qla2xxx: Remove a dead assignment in qla24xx_build_scsi_crc_2_iocbs()

[Bart Van Assche]

Since the value of cur_seg is not used and since scsi_prot_sglist()
has no side effects it is safe to remove the statement
"cur_seg = scsi_prot_sglist(cmd)". Detected by Coverity.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_iocb.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 15e4080..b589d24 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -1189,7 +1189,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct cmd_type_crc_2 *cmd_pkt,
 	uint32_t		*cur_dsd, *fcp_dl;
 	scsi_qla_host_t		*vha;
 	struct scsi_cmnd	*cmd;
-	struct scatterlist	*cur_seg;
 	int			sgc;
 	uint32_t		total_bytes = 0;
 	uint32_t		data_bytes;
@@ -1396,7 +1395,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct cmd_type_crc_2 *cmd_pkt,
 
 	if (bundling && tot_prot_dsds) {
 		/* Walks dif segments */
-		cur_seg = scsi_prot_sglist(cmd);
 		cmd_pkt->control_flags |=
 			__constant_cpu_to_le16(CF_DIF_SEG_DESCR_ENABLE);
 		cur_dsd = (uint32_t *) &crc_ctx_pkt->u.bundling.dif_address;
-- 
1.7.10.4

[PATCH 06/10] qla2xxx: Remove redundant assignments

[Bart Van Assche]

The value of the pointer called "nxt" is not used after the
"nxt = qla24xx_copy_eft(ha, nxt)" statement. Hence keep the function
call but remove the assignment.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_dbg.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index cfa2a20..c612785 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1468,7 +1468,7 @@ qla25xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 
 	nxt = qla2xxx_copy_queues(ha, nxt);
 
-	nxt = qla24xx_copy_eft(ha, nxt);
+	qla24xx_copy_eft(ha, nxt);
 
 	/* Chain entries -- started with MQ. */
 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
@@ -1787,7 +1787,7 @@ qla81xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 
 	nxt = qla2xxx_copy_queues(ha, nxt);
 
-	nxt = qla24xx_copy_eft(ha, nxt);
+	qla24xx_copy_eft(ha, nxt);
 
 	/* Chain entries -- started with MQ. */
 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
@@ -2289,7 +2289,7 @@ qla83xx_fw_dump(scsi_qla_host_t *vha, int hardware_locked)
 copy_queue:
 	nxt = qla2xxx_copy_queues(ha, nxt);
 
-	nxt = qla24xx_copy_eft(ha, nxt);
+	qla24xx_copy_eft(ha, nxt);
 
 	/* Chain entries -- started with MQ. */
 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
-- 
1.7.10.4

[PATCH 07/10] qla2xxx: Help Coverity with analyzing ct_sns_pkt initialization

[Bart Van Assche]

Coverity reports "Overrunning struct type ct_sns_req of 1228 bytes
by passing it to a function which accesses it at byte offset 8207"
for each qla2x00_prep_ct_req(), qla2x00_prep_ct_fdmi_req() and
qla24xx_prep_ct_fm_req() call. Help Coverity to recognize that
these calls do not trigger a buffer overflow by making it explicit
that these three functions initializes both the request and reply
structures. This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_gs.c |   86 ++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
index f26442a..1ad361b 100644
--- a/drivers/scsi/qla2xxx/qla_gs.c
+++ b/drivers/scsi/qla2xxx/qla_gs.c
@@ -99,17 +99,17 @@ qla24xx_prep_ms_iocb(scsi_qla_host_t *vha, uint32_t req_size, uint32_t rsp_size)
  * Returns a pointer to the intitialized @ct_req.
  */
 static inline struct ct_sns_req *
-qla2x00_prep_ct_req(struct ct_sns_req *ct_req, uint16_t cmd, uint16_t rsp_size)
+qla2x00_prep_ct_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t rsp_size)
 {
-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
+	memset(p, 0, sizeof(struct ct_sns_pkt));
 
-	ct_req->header.revision = 0x01;
-	ct_req->header.gs_type = 0xFC;
-	ct_req->header.gs_subtype = 0x02;
-	ct_req->command = cpu_to_be16(cmd);
-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
+	p->p.req.header.revision = 0x01;
+	p->p.req.header.gs_type = 0xFC;
+	p->p.req.header.gs_subtype = 0x02;
+	p->p.req.command = cpu_to_be16(cmd);
+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
 
-	return (ct_req);
+	return &p->p.req;
 }
 
 static int
@@ -188,8 +188,7 @@ qla2x00_ga_nxt(scsi_qla_host_t *vha, fc_port_t *fcport)
 	    GA_NXT_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GA_NXT_CMD,
-	    GA_NXT_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, GA_NXT_CMD, GA_NXT_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare CT arguments -- port_id */
@@ -284,8 +283,7 @@ qla2x00_gid_pt(scsi_qla_host_t *vha, sw_info_t *list)
 	    gid_pt_rsp_size);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GID_PT_CMD,
-	    gid_pt_rsp_size);
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, GID_PT_CMD, gid_pt_rsp_size);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare CT arguments -- port_type */
@@ -359,7 +357,7 @@ qla2x00_gpn_id(scsi_qla_host_t *vha, sw_info_t *list)
 		    GPN_ID_RSP_SIZE);
 
 		/* Prepare CT request */
-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GPN_ID_CMD,
+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GPN_ID_CMD,
 		    GPN_ID_RSP_SIZE);
 		ct_rsp = &ha->ct_sns->p.rsp;
 
@@ -421,7 +419,7 @@ qla2x00_gnn_id(scsi_qla_host_t *vha, sw_info_t *list)
 		    GNN_ID_RSP_SIZE);
 
 		/* Prepare CT request */
-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GNN_ID_CMD,
+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GNN_ID_CMD,
 		    GNN_ID_RSP_SIZE);
 		ct_rsp = &ha->ct_sns->p.rsp;
 
@@ -495,7 +493,7 @@ qla2x00_rft_id(scsi_qla_host_t *vha)
 	    RFT_ID_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RFT_ID_CMD,
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RFT_ID_CMD,
 	    RFT_ID_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
@@ -551,8 +549,7 @@ qla2x00_rff_id(scsi_qla_host_t *vha)
 	    RFF_ID_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RFF_ID_CMD,
-	    RFF_ID_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RFF_ID_CMD, RFF_ID_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare CT arguments -- port_id, FC-4 feature, FC-4 type */
@@ -606,8 +603,7 @@ qla2x00_rnn_id(scsi_qla_host_t *vha)
 	    RNN_ID_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RNN_ID_CMD,
-	    RNN_ID_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RNN_ID_CMD, RNN_ID_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare CT arguments -- port_id, node_name */
@@ -676,8 +672,7 @@ qla2x00_rsnn_nn(scsi_qla_host_t *vha)
 	ms_pkt = ha->isp_ops->prep_ms_iocb(vha, 0, RSNN_NN_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RSNN_NN_CMD,
-	    RSNN_NN_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RSNN_NN_CMD, RSNN_NN_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare CT arguments -- node_name, symbolic node_name, size */
@@ -1262,18 +1257,17 @@ qla2x00_update_ms_fdmi_iocb(scsi_qla_host_t *vha, uint32_t req_size)
  * Returns a pointer to the intitialized @ct_req.
  */
 static inline struct ct_sns_req *
-qla2x00_prep_ct_fdmi_req(struct ct_sns_req *ct_req, uint16_t cmd,
-    uint16_t rsp_size)
+qla2x00_prep_ct_fdmi_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t rsp_size)
 {
-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
+	memset(p, 0, sizeof(struct ct_sns_pkt));
 
-	ct_req->header.revision = 0x01;
-	ct_req->header.gs_type = 0xFA;
-	ct_req->header.gs_subtype = 0x10;
-	ct_req->command = cpu_to_be16(cmd);
-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
+	p->p.req.header.revision = 0x01;
+	p->p.req.header.gs_type = 0xFA;
+	p->p.req.header.gs_subtype = 0x10;
+	p->p.req.command = cpu_to_be16(cmd);
+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
 
-	return ct_req;
+	return &p->p.req;
 }
 
 /**
@@ -1301,8 +1295,7 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha)
 	ms_pkt = ha->isp_ops->prep_ms_fdmi_iocb(vha, 0, RHBA_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, RHBA_CMD,
-	    RHBA_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, RHBA_CMD, RHBA_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare FDMI command arguments -- attribute block, attributes. */
@@ -1490,8 +1483,7 @@ qla2x00_fdmi_dhba(scsi_qla_host_t *vha)
 	    DHBA_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, DHBA_CMD,
-	    DHBA_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, DHBA_CMD, DHBA_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare FDMI command arguments -- portname. */
@@ -1547,8 +1539,7 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha)
 	ms_pkt = ha->isp_ops->prep_ms_fdmi_iocb(vha, 0, RPA_RSP_SIZE);
 
 	/* Prepare CT request */
-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, RPA_CMD,
-	    RPA_RSP_SIZE);
+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, RPA_CMD, RPA_RSP_SIZE);
 	ct_rsp = &ha->ct_sns->p.rsp;
 
 	/* Prepare FDMI command arguments -- attribute block, attributes. */
@@ -1775,7 +1766,7 @@ qla2x00_gfpn_id(scsi_qla_host_t *vha, sw_info_t *list)
 		    GFPN_ID_RSP_SIZE);
 
 		/* Prepare CT request */
-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GFPN_ID_CMD,
+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GFPN_ID_CMD,
 		    GFPN_ID_RSP_SIZE);
 		ct_rsp = &ha->ct_sns->p.rsp;
 
@@ -1842,18 +1833,17 @@ qla24xx_prep_ms_fm_iocb(scsi_qla_host_t *vha, uint32_t req_size,
 
 
 static inline struct ct_sns_req *
-qla24xx_prep_ct_fm_req(struct ct_sns_req *ct_req, uint16_t cmd,
-    uint16_t rsp_size)
+qla24xx_prep_ct_fm_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t rsp_size)
 {
-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
+	memset(p, 0, sizeof(struct ct_sns_pkt));
 
-	ct_req->header.revision = 0x01;
-	ct_req->header.gs_type = 0xFA;
-	ct_req->header.gs_subtype = 0x01;
-	ct_req->command = cpu_to_be16(cmd);
-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
+	p->p.req.header.revision = 0x01;
+	p->p.req.header.gs_type = 0xFA;
+	p->p.req.header.gs_subtype = 0x01;
+	p->p.req.command = cpu_to_be16(cmd);
+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
 
-	return ct_req;
+	return &p->p.req;
 }
 
 /**
@@ -1889,7 +1879,7 @@ qla2x00_gpsc(scsi_qla_host_t *vha, sw_info_t *list)
 		    GPSC_RSP_SIZE);
 
 		/* Prepare CT request */
-		ct_req = qla24xx_prep_ct_fm_req(&ha->ct_sns->p.req,
+		ct_req = qla24xx_prep_ct_fm_req(ha->ct_sns,
 		    GPSC_CMD, GPSC_RSP_SIZE);
 		ct_rsp = &ha->ct_sns->p.rsp;
 
@@ -2000,7 +1990,7 @@ qla2x00_gff_id(scsi_qla_host_t *vha, sw_info_t *list)
 		    GFF_ID_RSP_SIZE);
 
 		/* Prepare CT request */
-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GFF_ID_CMD,
+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GFF_ID_CMD,
 		    GFF_ID_RSP_SIZE);
 		ct_rsp = &ha->ct_sns->p.rsp;
 
-- 
1.7.10.4

[PATCH 08/10] qla2xxx: Fix qla2xxx_check_risc_status()

[Bart Van Assche]

Change the 'rval' variable from QLA_FUNCTION_TIMEOUT into QLA_SUCCESS
before starting a loop that is only executed if rval is initialized
to QLA_SUCCESS. Coverity reported that loop as "dead code".

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_isr.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 259d920..bd0e2fa 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2495,6 +2495,7 @@ qla2xxx_check_risc_status(scsi_qla_host_t *vha)
 	if (rval == QLA_SUCCESS)
 		goto next_test;
 
+	rval = QLA_SUCCESS;
 	WRT_REG_DWORD(&reg->iobase_window, 0x0003);
 	for (cnt = 100; (RD_REG_DWORD(&reg->iobase_window) & BIT_0) == 0 &&
 	    rval == QLA_SUCCESS; cnt--) {
-- 
1.7.10.4

[PATCH 09/10] qla2xxx: Remove an unused variable from qla2x00_remove_one()

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_os.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 1d97626..2176cc9 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -2980,14 +2980,12 @@ qla2x00_remove_one(struct pci_dev *pdev)
 	set_bit(UNLOADING, &base_vha->dpc_flags);
 	mutex_lock(&ha->vport_lock);
 	while (ha->cur_vport_count) {
-		struct Scsi_Host *scsi_host;
-
 		spin_lock_irqsave(&ha->vport_slock, flags);
 
 		BUG_ON(base_vha->list.next == &ha->vp_list);
 		/* This assumes first entry in ha->vp_list is always base vha */
 		vha = list_first_entry(&base_vha->list, scsi_qla_host_t, list);
-		scsi_host = scsi_host_get(vha->host);
+		scsi_host_get(vha->host);
 
 		spin_unlock_irqrestore(&ha->vport_slock, flags);
 		mutex_unlock(&ha->vport_lock);
-- 
1.7.10.4

[PATCH 10/10] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

[Bart Van Assche]

Avoid that the fcport structure gets leaked if
bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN, the fcport
allocation succeeds and the !vha->flags.online branch is taken.
This was detected by Coverity. However, Coverity does not recognize
that all qla2x00_process_els() callers specify either
FC_BSG_RPT_ELS or FC_BSG_HST_ELS_NOLOGIN in the field
bsg_job->request->msgcode and that the value of that field is not
modified inside that function. This results in a false positive
report about a possible memory leak in an error path for
bsg_job->request->msgcode values other than the two mentioned
values.  Make it easy for Coverity (and for humans) to recognize
that there is no fcport leak in the error path by changing the
bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN test into
bsg_job->request->msgcode != FC_BSG_RPT_ELS.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
---
 drivers/scsi/qla2xxx/qla_bsg.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index cf07491..f8a2634 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -255,6 +255,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 	int rval =  (DRIVER_ERROR << 16);
 	uint16_t nextlid = 0;
 
+	if (!vha->flags.online) {
+		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
+		rval = -EIO;
+		goto done;
+	}
+
 	if (bsg_job->request->msgcode == FC_BSG_RPT_ELS) {
 		rport = bsg_job->rport;
 		fcport = *(fc_port_t **) rport->dd_data;
@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 			NPH_FABRIC_CONTROLLER : NPH_F_PORT;
 	}
 
-	if (!vha->flags.online) {
-		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
-		rval = -EIO;
-		goto done;
-	}
-
 	req_sg_cnt =
 		dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
 		bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
@@ -399,7 +399,7 @@ done_unmap_sg:
 	goto done_free_fcport;
 
 done_free_fcport:
-	if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
+	if (bsg_job->request->msgcode != FC_BSG_RPT_ELS)
 		kfree(fcport);
 done:
 	return rval;
-- 
1.7.10.4

Re: [PATCH 0/10] qla2xxx: Reduce the number of Coverity warnings

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

Hi Bart,
Thanks for the patches. Please share the warnings reported by Coverity, so
that its easy for us to review the patches.

Thanks,
~Saurav


>The current implementation of the qla2xxx Fibre Channel initiator driver
>triggers multiple warnings when analyzed with the Coverity static
>analyzer. This patch series addresses a significant number of these
>warnings.
>
>Note: patch 10/10 of this series is a reworked version of a patch that
>had been posted a few weeks ago on the linux-scsi mailing list.
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 3885 bytes --]

Re: [PATCH 0/10] qla2xxx: Reduce the number of Coverity warnings

[Bart Van Assche]

On 06/07/13 21:06, Saurav Kashyap wrote:
> Thanks for the patches. Please share the warnings reported by Coverity, so
> that its easy for us to review the patches.

Hello Saurav,

The Coverity warnings that led me to developing this patch series are as 
follows:
- For patches 1, 2, 3, 8: Logically dead code - execution cannot reach 
this statement.
- For patch 4: Array compared against 0 (NO_EFFECT) array_null: 
Comparing an array to null is not useful: "ha->model_desc".
- For patch 5, 6 and 9: Unused pointer value.
- For patch 7: Overrunning struct type ct_sns_req of 1228 bytes by 
passing it to a function which accesses it at byte offset 8207.
- For patch 10: Resource leak (RESOURCE_LEAK) leaked_storage: Variable 
"fcport" going out of scope leaks the storage it points to.

Hope this helps,

Bart.

Re: [PATCH 0/10] qla2xxx: Reduce the number of Coverity warnings

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hi Bart,
Thanks for patchset. There are minor issue with some patches like updating
the missing msg_ids in qla_dbg.c file. I am replying to each of the patch.

Thanks,
~Saurav


>On 06/07/13 21:06, Saurav Kashyap wrote:
>> Thanks for the patches. Please share the warnings reported by Coverity,
>>so
>> that its easy for us to review the patches.
>
>Hello Saurav,
>
>The Coverity warnings that led me to developing this patch series are as
>follows:
>- For patches 1, 2, 3, 8: Logically dead code - execution cannot reach
>this statement.
>- For patch 4: Array compared against 0 (NO_EFFECT) array_null:
>Comparing an array to null is not useful: "ha->model_desc".
>- For patch 5, 6 and 9: Unused pointer value.
>- For patch 7: Overrunning struct type ct_sns_req of 1228 bytes by
>passing it to a function which accesses it at byte offset 8207.
>- For patch 10: Resource leak (RESOURCE_LEAK) leaked_storage: Variable
>"fcport" going out of scope leaks the storage it points to.
>
>Hope this helps,
>
>Bart.
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4277 bytes --]

Re: [PATCH 01/10] qla2xxx: Clean up qla24xx_iidma()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

Hi Bart,
Missing msg ids needs to be updated in qla_dbg.c file. Here is an updated
patch

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
b/drivers/scsi/qla2xxx/qla_bsg.c
index 89dd050..25ded57 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1282,14 +1282,7 @@ qla24xx_iidma(struct fc_bsg_job *bsg_job)
                return -EINVAL;
        }
 
-       port_param = (struct qla_port_param *)((char *)bsg_job->request +
-               sizeof(struct fc_bsg_request));
-       if (!port_param) {
-               ql_log(ql_log_warn, vha, 0x7047,
-                   "port_param header not provided.\n");
-               return -EINVAL;
-       }
-
+       port_param = (void *)bsg_job->request + sizeof(struct
fc_bsg_request);
        if (port_param->fc_scsi_addr.dest_type != EXT_DEF_TYPE_WWPN) {
                ql_log(ql_log_warn, vha, 0x7048,
                    "Invalid destination type.\n");
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c
b/drivers/scsi/qla2xxx/qla_dbg.c
index 94f3a25..980dcbc 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -35,7 +35,8 @@
  * |                              |                    | 0x70a5,0x70a6, |
  * |                              |                    | 0x70a8,0x70ab, |
  * |                              |                    | 0x70ad-0x70ae, |
- * |                              |                    | 0x70d1-0x70da  |
+ * |                              |                    | 0x70d1-0x70da, |
+ * |                              |                    | 0x7047
     |
  * | Task Management              |       0x803c       | 0x8025-0x8026  |
  * |                              |                    | 0x800b,0x8039  |
  * | AER/EEH                      |       0x9011       |               |


Thanks,
~Saurav



>Remove dead code and simplify a pointer computation.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
>b/drivers/scsi/qla2xxx/qla_bsg.c
>index 39719f8..7221521 100644
>--- a/drivers/scsi/qla2xxx/qla_bsg.c
>+++ b/drivers/scsi/qla2xxx/qla_bsg.c
>@@ -1282,14 +1282,7 @@ qla24xx_iidma(struct fc_bsg_job *bsg_job)
> 		return -EINVAL;
> 	}
> 
>-	port_param = (struct qla_port_param *)((char *)bsg_job->request +
>-		sizeof(struct fc_bsg_request));
>-	if (!port_param) {
>-		ql_log(ql_log_warn, vha, 0x7047,
>-		    "port_param header not provided.\n");
>-		return -EINVAL;
>-	}
>-
>+	port_param = (void *)bsg_job->request + sizeof(struct fc_bsg_request);
> 	if (port_param->fc_scsi_addr.dest_type != EXT_DEF_TYPE_WWPN) {
> 		ql_log(ql_log_warn, vha, 0x7048,
> 		    "Invalid destination type.\n");
>-- 
>1.7.10.4
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4636 bytes --]

Re: [PATCH 02/10] qla2xxx: Clean up qla84xx_mgmt_cmd()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 3645 bytes --]

HI Bart,
Missing msg ids need to be updated on qla_dbg.c file. Here is an updated
patch.

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
b/drivers/scsi/qla2xxx/qla_bsg.c
index 25ded57..9520b1f 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -1084,14 +1084,6 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
                return -EINVAL;
        }
 
-       ql84_mgmt = (struct qla_bsg_a84_mgmt *)((char *)bsg_job->request +
-               sizeof(struct fc_bsg_request));
-       if (!ql84_mgmt) {
-               ql_log(ql_log_warn, vha, 0x703b,
-                   "MGMT header not provided, exiting.\n");
-               return -EINVAL;
-       }
-
        mn = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &mn_dma);
        if (!mn) {
                ql_log(ql_log_warn, vha, 0x703c,
@@ -1102,7 +1094,7 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
        memset(mn, 0, sizeof(struct access_chip_84xx));
        mn->entry_type = ACCESS_CHIP_IOCB_TYPE;
        mn->entry_count = 1;
-
+       ql84_mgmt = (void *)bsg_job->request + sizeof(struct
fc_bsg_request);
        switch (ql84_mgmt->mgmt.cmd) {
        case QLA84_MGMT_READ_MEM:
        case QLA84_MGMT_GET_INFO:
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c
b/drivers/scsi/qla2xxx/qla_dbg.c
index 980dcbc..1c1972b 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -36,7 +36,7 @@
  * |                              |                    | 0x70a8,0x70ab, |
  * |                              |                    | 0x70ad-0x70ae, |
  * |                              |                    | 0x70d1-0x70da, |
- * |                              |                    | 0x7047
     |
+ * |                              |                    | 0x7047,0x703b |
  * | Task Management              |       0x803c       | 0x8025-0x8026  |
  * |                              |                    | 0x800b,0x8039  |
  * | AER/EEH                      |       0x9011       |               |


Thanks,
~Saurav



>Remove dead code, simplify a pointer computation and move the
>ql84_mgmt assignment to just before its first use.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
>b/drivers/scsi/qla2xxx/qla_bsg.c
>index 7221521..cf07491 100644
>--- a/drivers/scsi/qla2xxx/qla_bsg.c
>+++ b/drivers/scsi/qla2xxx/qla_bsg.c
>@@ -1084,14 +1084,6 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
> 		return -EINVAL;
> 	}
> 
>-	ql84_mgmt = (struct qla_bsg_a84_mgmt *)((char *)bsg_job->request +
>-		sizeof(struct fc_bsg_request));
>-	if (!ql84_mgmt) {
>-		ql_log(ql_log_warn, vha, 0x703b,
>-		    "MGMT header not provided, exiting.\n");
>-		return -EINVAL;
>-	}
>-
> 	mn = dma_pool_alloc(ha->s_dma_pool, GFP_KERNEL, &mn_dma);
> 	if (!mn) {
> 		ql_log(ql_log_warn, vha, 0x703c,
>@@ -1103,6 +1095,7 @@ qla84xx_mgmt_cmd(struct fc_bsg_job *bsg_job)
> 	mn->entry_type = ACCESS_CHIP_IOCB_TYPE;
> 	mn->entry_count = 1;
> 
>+	ql84_mgmt = (void *)bsg_job->request + sizeof(struct fc_bsg_request);
> 	switch (ql84_mgmt->mgmt.cmd) {
> 	case QLA84_MGMT_READ_MEM:
> 	case QLA84_MGMT_GET_INFO:
>-- 
>1.7.10.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 5047 bytes --]

Re: [PATCH 03/10] qla2xxx: Remove dead code in qla2x00_configure_hba()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

Hi Bart,
I removed the success message also and updated the qla_dbg.c file.


diff --git a/drivers/scsi/qla2xxx/qla_dbg.c
b/drivers/scsi/qla2xxx/qla_dbg.c
index 1c1972b..dc227fa 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -15,6 +15,7 @@
  * | Mailbox commands             |       0x117a       | 0x111a-0x111b  |
  * |                              |                    | 0x1155-0x1158  |
  * | Device Discovery             |       0x2095       | 0x2020-0x2022, |
+ * |                              |                    | 0x2011-0x2012, |
  * |                              |                    | 0x2016         |
  * | Queue Command and IO tracing |       0x3058       | 0x3006-0x300b  |
  * |                              |                    | 0x3027-0x3028  |
diff --git a/drivers/scsi/qla2xxx/qla_init.c
b/drivers/scsi/qla2xxx/qla_init.c
index 8c7a123..c4fc8a2 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2320,14 +2320,6 @@ qla2x00_configure_hba(scsi_qla_host_t *vha)
                    "Topology - %s, Host Loop address 0x%x.\n",
                    connect_type, vha->loop_id);
 
-       if (rval) {
-               ql_log(ql_log_warn, vha, 0x2011,
-                   "%s FAILED\n", __func__);
-       } else {
-               ql_dbg(ql_dbg_disc, vha, 0x2012,
-                   "%s success\n", __func__);
-       }
-
        return(rval);
 }

Thanks,
~Saurav


>At the end of qla2x00_configure_hba() we know that rval == QLA_SUCCESS.
>Hence remove the code that depends on rval != QLA_SUCCESS.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_init.c |    8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_init.c
>b/drivers/scsi/qla2xxx/qla_init.c
>index 3565dfd..c68bb01 100644
>--- a/drivers/scsi/qla2xxx/qla_init.c
>+++ b/drivers/scsi/qla2xxx/qla_init.c
>@@ -2309,13 +2309,7 @@ qla2x00_configure_hba(scsi_qla_host_t *vha)
> 		    "Topology - %s, Host Loop address 0x%x.\n",
> 		    connect_type, vha->loop_id);
> 
>-	if (rval) {
>-		ql_log(ql_log_warn, vha, 0x2011,
>-		    "%s FAILED\n", __func__);
>-	} else {
>-		ql_dbg(ql_dbg_disc, vha, 0x2012,
>-		    "%s success\n", __func__);
>-	}
>+	ql_dbg(ql_dbg_disc, vha, 0x2012, "%s success\n", __func__);
> 
> 	return(rval);
> }
>-- 
>1.7.10.4
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4683 bytes --]

Re: [PATCH 04/10] qla2xxx: Remove two superfluous tests

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>


>Since ha->model_desc is an array comparing it against NULL is
>superfluous. Hence remove these tests.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_gs.c |    3 +--
> drivers/scsi/qla2xxx/qla_os.c |    3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
>index d0ea8b9..f26442a 100644
>--- a/drivers/scsi/qla2xxx/qla_gs.c
>+++ b/drivers/scsi/qla2xxx/qla_gs.c
>@@ -1370,8 +1370,7 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha)
> 	/* Model description. */
> 	eiter = (struct ct_fdmi_hba_attr *) (entries + size);
> 	eiter->type = __constant_cpu_to_be16(FDMI_HBA_MODEL_DESCRIPTION);
>-	if (ha->model_desc)
>-		strncpy(eiter->a.model_desc, ha->model_desc, 80);
>+	strncpy(eiter->a.model_desc, ha->model_desc, 80);
> 	alen = strlen(eiter->a.model_desc);
> 	alen += (alen & 3) ? (4 - (alen & 3)) : 4;
> 	eiter->len = cpu_to_be16(4 + alen);
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index ad72c1d..1d97626 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -2840,8 +2840,7 @@ skip_dpc:
> 	qla2x00_dfs_setup(base_vha);
> 
> 	ql_log(ql_log_info, base_vha, 0x00fb,
>-	    "QLogic %s - %s.\n",
>-	    ha->model_number, ha->model_desc ? ha->model_desc : "");
>+	    "QLogic %s - %s.\n", ha->model_number, ha->model_desc);
> 	ql_log(ql_log_info, base_vha, 0x00fc,
> 	    "ISP%04X: %s @ %s hdma%c host#=%ld fw=%s.\n",
> 	    pdev->device, ha->isp_ops->pci_info_str(base_vha, pci_info),
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4428 bytes --]

Re: [PATCH 05/10] qla2xxx: Remove a dead assignment in qla24xx_build_scsi_crc_2_iocbs()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>




>Since the value of cur_seg is not used and since scsi_prot_sglist()
>has no side effects it is safe to remove the statement
>"cur_seg = scsi_prot_sglist(cmd)". Detected by Coverity.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_iocb.c |    2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
>b/drivers/scsi/qla2xxx/qla_iocb.c
>index 15e4080..b589d24 100644
>--- a/drivers/scsi/qla2xxx/qla_iocb.c
>+++ b/drivers/scsi/qla2xxx/qla_iocb.c
>@@ -1189,7 +1189,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct
>cmd_type_crc_2 *cmd_pkt,
> 	uint32_t		*cur_dsd, *fcp_dl;
> 	scsi_qla_host_t		*vha;
> 	struct scsi_cmnd	*cmd;
>-	struct scatterlist	*cur_seg;
> 	int			sgc;
> 	uint32_t		total_bytes = 0;
> 	uint32_t		data_bytes;
>@@ -1396,7 +1395,6 @@ qla24xx_build_scsi_crc_2_iocbs(srb_t *sp, struct
>cmd_type_crc_2 *cmd_pkt,
> 
> 	if (bundling && tot_prot_dsds) {
> 		/* Walks dif segments */
>-		cur_seg = scsi_prot_sglist(cmd);
> 		cmd_pkt->control_flags |=
> 			__constant_cpu_to_le16(CF_DIF_SEG_DESCR_ENABLE);
> 		cur_dsd = (uint32_t *) &crc_ctx_pkt->u.bundling.dif_address;
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4472 bytes --]

Re: [PATCH 06/10] qla2xxx: Remove redundant assignments

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>



>The value of the pointer called "nxt" is not used after the
>"nxt = qla24xx_copy_eft(ha, nxt)" statement. Hence keep the function
>call but remove the assignment.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_dbg.c |    6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_dbg.c
>b/drivers/scsi/qla2xxx/qla_dbg.c
>index cfa2a20..c612785 100644
>--- a/drivers/scsi/qla2xxx/qla_dbg.c
>+++ b/drivers/scsi/qla2xxx/qla_dbg.c
>@@ -1468,7 +1468,7 @@ qla25xx_fw_dump(scsi_qla_host_t *vha, int
>hardware_locked)
> 
> 	nxt = qla2xxx_copy_queues(ha, nxt);
> 
>-	nxt = qla24xx_copy_eft(ha, nxt);
>+	qla24xx_copy_eft(ha, nxt);
> 
> 	/* Chain entries -- started with MQ. */
> 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
>@@ -1787,7 +1787,7 @@ qla81xx_fw_dump(scsi_qla_host_t *vha, int
>hardware_locked)
> 
> 	nxt = qla2xxx_copy_queues(ha, nxt);
> 
>-	nxt = qla24xx_copy_eft(ha, nxt);
>+	qla24xx_copy_eft(ha, nxt);
> 
> 	/* Chain entries -- started with MQ. */
> 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
>@@ -2289,7 +2289,7 @@ qla83xx_fw_dump(scsi_qla_host_t *vha, int
>hardware_locked)
> copy_queue:
> 	nxt = qla2xxx_copy_queues(ha, nxt);
> 
>-	nxt = qla24xx_copy_eft(ha, nxt);
>+	qla24xx_copy_eft(ha, nxt);
> 
> 	/* Chain entries -- started with MQ. */
> 	nxt_chain = qla25xx_copy_fce(ha, nxt_chain, &last_chain);
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4196 bytes --]

Re: [PATCH 07/10] qla2xxx: Help Coverity with analyzing ct_sns_pkt initialization

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 9022 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>



>Coverity reports "Overrunning struct type ct_sns_req of 1228 bytes
>by passing it to a function which accesses it at byte offset 8207"
>for each qla2x00_prep_ct_req(), qla2x00_prep_ct_fdmi_req() and
>qla24xx_prep_ct_fm_req() call. Help Coverity to recognize that
>these calls do not trigger a buffer overflow by making it explicit
>that these three functions initializes both the request and reply
>structures. This patch does not change any functionality.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_gs.c |   86
>++++++++++++++++++-----------------------
> 1 file changed, 38 insertions(+), 48 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c
>index f26442a..1ad361b 100644
>--- a/drivers/scsi/qla2xxx/qla_gs.c
>+++ b/drivers/scsi/qla2xxx/qla_gs.c
>@@ -99,17 +99,17 @@ qla24xx_prep_ms_iocb(scsi_qla_host_t *vha, uint32_t
>req_size, uint32_t rsp_size)
>  * Returns a pointer to the intitialized @ct_req.
>  */
> static inline struct ct_sns_req *
>-qla2x00_prep_ct_req(struct ct_sns_req *ct_req, uint16_t cmd, uint16_t
>rsp_size)
>+qla2x00_prep_ct_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t
>rsp_size)
> {
>-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
>+	memset(p, 0, sizeof(struct ct_sns_pkt));
> 
>-	ct_req->header.revision = 0x01;
>-	ct_req->header.gs_type = 0xFC;
>-	ct_req->header.gs_subtype = 0x02;
>-	ct_req->command = cpu_to_be16(cmd);
>-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
>+	p->p.req.header.revision = 0x01;
>+	p->p.req.header.gs_type = 0xFC;
>+	p->p.req.header.gs_subtype = 0x02;
>+	p->p.req.command = cpu_to_be16(cmd);
>+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
> 
>-	return (ct_req);
>+	return &p->p.req;
> }
> 
> static int
>@@ -188,8 +188,7 @@ qla2x00_ga_nxt(scsi_qla_host_t *vha, fc_port_t
>*fcport)
> 	    GA_NXT_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GA_NXT_CMD,
>-	    GA_NXT_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, GA_NXT_CMD, GA_NXT_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare CT arguments -- port_id */
>@@ -284,8 +283,7 @@ qla2x00_gid_pt(scsi_qla_host_t *vha, sw_info_t *list)
> 	    gid_pt_rsp_size);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GID_PT_CMD,
>-	    gid_pt_rsp_size);
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, GID_PT_CMD, gid_pt_rsp_size);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare CT arguments -- port_type */
>@@ -359,7 +357,7 @@ qla2x00_gpn_id(scsi_qla_host_t *vha, sw_info_t *list)
> 		    GPN_ID_RSP_SIZE);
> 
> 		/* Prepare CT request */
>-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GPN_ID_CMD,
>+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GPN_ID_CMD,
> 		    GPN_ID_RSP_SIZE);
> 		ct_rsp = &ha->ct_sns->p.rsp;
> 
>@@ -421,7 +419,7 @@ qla2x00_gnn_id(scsi_qla_host_t *vha, sw_info_t *list)
> 		    GNN_ID_RSP_SIZE);
> 
> 		/* Prepare CT request */
>-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GNN_ID_CMD,
>+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GNN_ID_CMD,
> 		    GNN_ID_RSP_SIZE);
> 		ct_rsp = &ha->ct_sns->p.rsp;
> 
>@@ -495,7 +493,7 @@ qla2x00_rft_id(scsi_qla_host_t *vha)
> 	    RFT_ID_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RFT_ID_CMD,
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RFT_ID_CMD,
> 	    RFT_ID_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
>@@ -551,8 +549,7 @@ qla2x00_rff_id(scsi_qla_host_t *vha)
> 	    RFF_ID_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RFF_ID_CMD,
>-	    RFF_ID_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RFF_ID_CMD, RFF_ID_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare CT arguments -- port_id, FC-4 feature, FC-4 type */
>@@ -606,8 +603,7 @@ qla2x00_rnn_id(scsi_qla_host_t *vha)
> 	    RNN_ID_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RNN_ID_CMD,
>-	    RNN_ID_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RNN_ID_CMD, RNN_ID_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare CT arguments -- port_id, node_name */
>@@ -676,8 +672,7 @@ qla2x00_rsnn_nn(scsi_qla_host_t *vha)
> 	ms_pkt = ha->isp_ops->prep_ms_iocb(vha, 0, RSNN_NN_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, RSNN_NN_CMD,
>-	    RSNN_NN_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_req(ha->ct_sns, RSNN_NN_CMD, RSNN_NN_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare CT arguments -- node_name, symbolic node_name, size */
>@@ -1262,18 +1257,17 @@ qla2x00_update_ms_fdmi_iocb(scsi_qla_host_t *vha,
>uint32_t req_size)
>  * Returns a pointer to the intitialized @ct_req.
>  */
> static inline struct ct_sns_req *
>-qla2x00_prep_ct_fdmi_req(struct ct_sns_req *ct_req, uint16_t cmd,
>-    uint16_t rsp_size)
>+qla2x00_prep_ct_fdmi_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t
>rsp_size)
> {
>-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
>+	memset(p, 0, sizeof(struct ct_sns_pkt));
> 
>-	ct_req->header.revision = 0x01;
>-	ct_req->header.gs_type = 0xFA;
>-	ct_req->header.gs_subtype = 0x10;
>-	ct_req->command = cpu_to_be16(cmd);
>-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
>+	p->p.req.header.revision = 0x01;
>+	p->p.req.header.gs_type = 0xFA;
>+	p->p.req.header.gs_subtype = 0x10;
>+	p->p.req.command = cpu_to_be16(cmd);
>+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
> 
>-	return ct_req;
>+	return &p->p.req;
> }
> 
> /**
>@@ -1301,8 +1295,7 @@ qla2x00_fdmi_rhba(scsi_qla_host_t *vha)
> 	ms_pkt = ha->isp_ops->prep_ms_fdmi_iocb(vha, 0, RHBA_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, RHBA_CMD,
>-	    RHBA_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, RHBA_CMD, RHBA_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare FDMI command arguments -- attribute block, attributes. */
>@@ -1490,8 +1483,7 @@ qla2x00_fdmi_dhba(scsi_qla_host_t *vha)
> 	    DHBA_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, DHBA_CMD,
>-	    DHBA_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, DHBA_CMD, DHBA_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare FDMI command arguments -- portname. */
>@@ -1547,8 +1539,7 @@ qla2x00_fdmi_rpa(scsi_qla_host_t *vha)
> 	ms_pkt = ha->isp_ops->prep_ms_fdmi_iocb(vha, 0, RPA_RSP_SIZE);
> 
> 	/* Prepare CT request */
>-	ct_req = qla2x00_prep_ct_fdmi_req(&ha->ct_sns->p.req, RPA_CMD,
>-	    RPA_RSP_SIZE);
>+	ct_req = qla2x00_prep_ct_fdmi_req(ha->ct_sns, RPA_CMD, RPA_RSP_SIZE);
> 	ct_rsp = &ha->ct_sns->p.rsp;
> 
> 	/* Prepare FDMI command arguments -- attribute block, attributes. */
>@@ -1775,7 +1766,7 @@ qla2x00_gfpn_id(scsi_qla_host_t *vha, sw_info_t
>*list)
> 		    GFPN_ID_RSP_SIZE);
> 
> 		/* Prepare CT request */
>-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GFPN_ID_CMD,
>+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GFPN_ID_CMD,
> 		    GFPN_ID_RSP_SIZE);
> 		ct_rsp = &ha->ct_sns->p.rsp;
> 
>@@ -1842,18 +1833,17 @@ qla24xx_prep_ms_fm_iocb(scsi_qla_host_t *vha,
>uint32_t req_size,
> 
> 
> static inline struct ct_sns_req *
>-qla24xx_prep_ct_fm_req(struct ct_sns_req *ct_req, uint16_t cmd,
>-    uint16_t rsp_size)
>+qla24xx_prep_ct_fm_req(struct ct_sns_pkt *p, uint16_t cmd, uint16_t
>rsp_size)
> {
>-	memset(ct_req, 0, sizeof(struct ct_sns_pkt));
>+	memset(p, 0, sizeof(struct ct_sns_pkt));
> 
>-	ct_req->header.revision = 0x01;
>-	ct_req->header.gs_type = 0xFA;
>-	ct_req->header.gs_subtype = 0x01;
>-	ct_req->command = cpu_to_be16(cmd);
>-	ct_req->max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
>+	p->p.req.header.revision = 0x01;
>+	p->p.req.header.gs_type = 0xFA;
>+	p->p.req.header.gs_subtype = 0x01;
>+	p->p.req.command = cpu_to_be16(cmd);
>+	p->p.req.max_rsp_size = cpu_to_be16((rsp_size - 16) / 4);
> 
>-	return ct_req;
>+	return &p->p.req;
> }
> 
> /**
>@@ -1889,7 +1879,7 @@ qla2x00_gpsc(scsi_qla_host_t *vha, sw_info_t *list)
> 		    GPSC_RSP_SIZE);
> 
> 		/* Prepare CT request */
>-		ct_req = qla24xx_prep_ct_fm_req(&ha->ct_sns->p.req,
>+		ct_req = qla24xx_prep_ct_fm_req(ha->ct_sns,
> 		    GPSC_CMD, GPSC_RSP_SIZE);
> 		ct_rsp = &ha->ct_sns->p.rsp;
> 
>@@ -2000,7 +1990,7 @@ qla2x00_gff_id(scsi_qla_host_t *vha, sw_info_t
>*list)
> 		    GFF_ID_RSP_SIZE);
> 
> 		/* Prepare CT request */
>-		ct_req = qla2x00_prep_ct_req(&ha->ct_sns->p.req, GFF_ID_CMD,
>+		ct_req = qla2x00_prep_ct_req(ha->ct_sns, GFF_ID_CMD,
> 		    GFF_ID_RSP_SIZE);
> 		ct_rsp = &ha->ct_sns->p.rsp;
> 
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6494 bytes --]

Re: [PATCH 08/10] qla2xxx: Fix qla2xxx_check_risc_status()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>



>Change the 'rval' variable from QLA_FUNCTION_TIMEOUT into QLA_SUCCESS
>before starting a loop that is only executed if rval is initialized
>to QLA_SUCCESS. Coverity reported that loop as "dead code".
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_isr.c |    1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c
>b/drivers/scsi/qla2xxx/qla_isr.c
>index 259d920..bd0e2fa 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -2495,6 +2495,7 @@ qla2xxx_check_risc_status(scsi_qla_host_t *vha)
> 	if (rval == QLA_SUCCESS)
> 		goto next_test;
> 
>+	rval = QLA_SUCCESS;
> 	WRT_REG_DWORD(&reg->iobase_window, 0x0003);
> 	for (cnt = 100; (RD_REG_DWORD(&reg->iobase_window) & BIT_0) == 0 &&
> 	    rval == QLA_SUCCESS; cnt--) {
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4135 bytes --]

Re: [PATCH 09/10] qla2xxx: Remove an unused variable from qla2x00_remove_one()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

Acked-by: Saurav Kashyap <saurav.kashyap@qlogic.com>



>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_os.c |    4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>index 1d97626..2176cc9 100644
>--- a/drivers/scsi/qla2xxx/qla_os.c
>+++ b/drivers/scsi/qla2xxx/qla_os.c
>@@ -2980,14 +2980,12 @@ qla2x00_remove_one(struct pci_dev *pdev)
> 	set_bit(UNLOADING, &base_vha->dpc_flags);
> 	mutex_lock(&ha->vport_lock);
> 	while (ha->cur_vport_count) {
>-		struct Scsi_Host *scsi_host;
>-
> 		spin_lock_irqsave(&ha->vport_slock, flags);
> 
> 		BUG_ON(base_vha->list.next == &ha->vp_list);
> 		/* This assumes first entry in ha->vp_list is always base vha */
> 		vha = list_first_entry(&base_vha->list, scsi_qla_host_t, list);
>-		scsi_host = scsi_host_get(vha->host);
>+		scsi_host_get(vha->host);
> 
> 		spin_unlock_irqrestore(&ha->vport_slock, flags);
> 		mutex_unlock(&ha->vport_lock);
>-- 
>1.7.10.4
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 4255 bytes --]

Re: [PATCH 10/10] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 4332 bytes --]

Hi Bart,
In this case online check is move to far, the vha is still not
dereferenced. The right patch is moving online flag just after getting vha.

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
b/drivers/scsi/qla2xxx/qla_bsg.c
index 9520b1f..11f84dc 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -269,6 +269,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
                type = "FC_BSG_HST_ELS_NOLOGIN";
        }
 
+       if (!vha->flags.online) {
+               ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
+               rval = -EIO;
+               goto done;
+       }
+
        /* pass through is supported only for ISP 4Gb or higher */
        if (!IS_FWI2_CAPABLE(ha)) {
                ql_dbg(ql_dbg_user, vha, 0x7001,
@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
                        NPH_FABRIC_CONTROLLER : NPH_F_PORT;
        }
 
-       if (!vha->flags.online) {
-               ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
-               rval = -EIO;
-               goto done;
-       }
-
        req_sg_cnt =
                dma_map_sg(&ha->pdev->dev,
bsg_job->request_payload.sg_list,
                bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
@@ -399,7 +399,7 @@ done_unmap_sg:
        goto done_free_fcport;
 
 done_free_fcport:
-       if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
+       if (bsg_job->request->msgcode == FC_BSG_RPT_ELS)
                kfree(fcport);
 done:
        return rval;


Thanks,
~Saurav



-----Original Message-----
From: Bart Van Assche <bvanassche@acm.org>
Date: Wed, 5 Jun 2013 15:09:59 +0200
To: linux-scsi <linux-scsi@vger.kernel.org>
Cc: Chad Dupuis <chad.dupuis@qlogic.com>, Saurav Kashyap
<saurav.kashyap@qlogic.com>
Subject: [PATCH 10/10] qla2xxx: Fix a memory leak in an error path of
qla2x00_process_els()

>Avoid that the fcport structure gets leaked if
>bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN, the fcport
>allocation succeeds and the !vha->flags.online branch is taken.
>This was detected by Coverity. However, Coverity does not recognize
>that all qla2x00_process_els() callers specify either
>FC_BSG_RPT_ELS or FC_BSG_HST_ELS_NOLOGIN in the field
>bsg_job->request->msgcode and that the value of that field is not
>modified inside that function. This results in a false positive
>report about a possible memory leak in an error path for
>bsg_job->request->msgcode values other than the two mentioned
>values.  Make it easy for Coverity (and for humans) to recognize
>that there is no fcport leak in the error path by changing the
>bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN test into
>bsg_job->request->msgcode != FC_BSG_RPT_ELS.
>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>Cc: Chad Dupuis <chad.dupuis@qlogic.com>
>Cc: Saurav Kashyap <saurav.kashyap@qlogic.com>
>---
> drivers/scsi/qla2xxx/qla_bsg.c |   14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
>b/drivers/scsi/qla2xxx/qla_bsg.c
>index cf07491..f8a2634 100644
>--- a/drivers/scsi/qla2xxx/qla_bsg.c
>+++ b/drivers/scsi/qla2xxx/qla_bsg.c
>@@ -255,6 +255,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
> 	int rval =  (DRIVER_ERROR << 16);
> 	uint16_t nextlid = 0;
> 
>+	if (!vha->flags.online) {
>+		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
>+		rval = -EIO;
>+		goto done;
>+	}
>+
> 	if (bsg_job->request->msgcode == FC_BSG_RPT_ELS) {
> 		rport = bsg_job->rport;
> 		fcport = *(fc_port_t **) rport->dd_data;
>@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
> 			NPH_FABRIC_CONTROLLER : NPH_F_PORT;
> 	}
> 
>-	if (!vha->flags.online) {
>-		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
>-		rval = -EIO;
>-		goto done;
>-	}
>-
> 	req_sg_cnt =
> 		dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
> 		bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
>@@ -399,7 +399,7 @@ done_unmap_sg:
> 	goto done_free_fcport;
> 
> done_free_fcport:
>-	if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
>+	if (bsg_job->request->msgcode != FC_BSG_RPT_ELS)
> 		kfree(fcport);
> done:
> 	return rval;
>-- 
>1.7.10.4
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 5635 bytes --]

Re: [PATCH 01/10] qla2xxx: Clean up qla24xx_iidma()

[James Bottomley]

On Wed, 2013-06-12 at 08:05 +0000, Saurav Kashyap wrote:
> Hi Bart,
> Missing msg ids needs to be updated in qla_dbg.c file. Here is an updated
> patch

OK, so since you modified the first patch in the series, you need to be
the one who sends it all to me (with proper signoffs).

Thanks,

James

Re: [PATCH 01/10] qla2xxx: Clean up qla24xx_iidma()

[Saurav Kashyap]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]



>On Wed, 2013-06-12 at 08:05 +0000, Saurav Kashyap wrote:
>> Hi Bart,
>> Missing msg ids needs to be updated in qla_dbg.c file. Here is an
>>updated
>> patch
>
>OK, so since you modified the first patch in the series, you need to be
>the one who sends it all to me (with proper signoffs).

<SK> Sure, I will send all the patches in next scsi-misc submission.

Thanks,
~Saurav

>
>Thanks,
>
>James
>
>
>


[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 3800 bytes --]