qla2xxx: Reduce the number of Coverity warnings

PATCH In review Bart Van Assche 3 participants 26 messages Jun 5, 2013 – Jun 13, 2013

Subsystems (2)

SCSI QLogic QLA2XXX FC-SCSI driver

lore

Share & tools

lore

Bart Van Assche

author

2013-06-05 13:01 UTC #0 lore raw

cover qla2xxx: Reduce the number of Coverity warnings

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.

Bart Van Assche

author

2013-06-05 13:02 UTC #1 lore raw

PATCH 1/10 qla2xxx: Clean up qla24xx_iidma()

Remove dead code and simplify a pointer computation.

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

drivers/scsi/qla2xxx/qla_bsg.c +1 -8

index 39719f8..7221521 100644
@@ -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");

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:03 UTC #5 lore raw

PATCH 2/10 qla2xxx: Clean up qla84xx_mgmt_cmd()

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_bsg.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

drivers/scsi/qla2xxx/qla_bsg.c +1 -8

index 7221521..cf07491 100644
@@ -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:

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:04 UTC #7 lore raw

PATCH 3/10 qla2xxx: Remove dead code in qla2x00_configure_hba()

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_init.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

drivers/scsi/qla2xxx/qla_init.c +1 -7

index 3565dfd..c68bb01 100644
@@ -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);
 }

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:05 UTC #9 lore raw

PATCH 4/10 qla2xxx: Remove two superfluous tests

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_gs.c |    3 +--
 drivers/scsi/qla2xxx/qla_os.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

drivers/scsi/qla2xxx/qla_gs.c +1 -2

index d0ea8b9..f26442a 100644
@@ -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);

drivers/scsi/qla2xxx/qla_os.c +1 -2

index ad72c1d..1d97626 100644
@@ -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),

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:06 UTC #11 lore raw

PATCH 5/10 qla2xxx: Remove a dead assignment in qla24xx_build_scsi_crc_2_iocbs()

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_iocb.c |    2 --
 1 file changed, 2 deletions(-)

drivers/scsi/qla2xxx/qla_iocb.c +0 -2

index 15e4080..b589d24 100644
@@ -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;

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:06 UTC #13 lore raw

PATCH 6/10 qla2xxx: Remove redundant assignments

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_dbg.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/scsi/qla2xxx/qla_dbg.c +3 -3

index cfa2a20..c612785 100644
@@ -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);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:07 UTC #15 lore raw

PATCH 7/10 qla2xxx: Help Coverity with analyzing ct_sns_pkt initialization

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_gs.c |   86 ++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

drivers/scsi/qla2xxx/qla_gs.c +38 ~~-48~~

index f26442a..1ad361b 100644
@@ -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;

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:08 UTC #17 lore raw

PATCH 8/10 qla2xxx: Fix qla2xxx_check_risc_status()

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_isr.c |    1 +
 1 file changed, 1 insertion(+)

drivers/scsi/qla2xxx/qla_isr.c +1 -0

index 259d920..bd0e2fa 100644
@@ -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--) {

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:09 UTC #19 lore raw

PATCH 9/10 qla2xxx: Remove an unused variable from qla2x00_remove_one()

In reply to Bart Van Assche

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_os.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/scsi/qla2xxx/qla_os.c +1 -3

index 1d97626..2176cc9 100644
@@ -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);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2013-06-05 13:10 UTC #21 lore raw

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

In reply to 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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
 drivers/scsi/qla2xxx/qla_bsg.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/scsi/qla2xxx/qla_bsg.c +7 -7

index cf07491..f8a2634 100644
@@ -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;

Signature · 1 line

1.7.10.4

Saurav Kashyap

2013-06-07 19:06 UTC #23 lore raw

In reply to Bart Van Assche

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

Quoted 8 lines

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.

Bart Van Assche

author

2013-06-08 08:32 UTC #24 lore raw

Quoted 2 lines · Saurav Kashyap

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.

Saurav Kashyap

2013-06-12 08:04 UTC #25 lore raw

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

Quoted 23 lines · Saurav Kashyap

Quoted 3 lines
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.

Saurav Kashyap

2013-06-12 08:05 UTC #2 lore raw

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

In reply to Bart Van Assche

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

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_bsg.c
index 89dd050..25ded57 100644

drivers/scsi/qla2xxx/qla_bsg.c +1 -8

@@ -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");

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_dbg.c
index 94f3a25..980dcbc 100644

drivers/scsi/qla2xxx/qla_dbg.c +2 -1

@@ -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

Quoted 33 lines

Remove dead code and simplify a pointer computation.

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
drivers/scsi/qla2xxx/qla_bsg.c | 9 +--------

1 file changed, 1 insertion(+), 8 deletions(-)

drivers/scsi/qla2xxx/qla_bsg.c +1 -8

index 39719f8..7221521 100644
@@ -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");

Signature · 1 line

1.7.10.4

Saurav Kashyap

2013-06-12 08:06 UTC #6 lore raw

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

In reply to Bart Van Assche

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

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_bsg.c
index 25ded57..9520b1f 100644

drivers/scsi/qla2xxx/qla_bsg.c +1 -9

@@ -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:

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_dbg.c
index 980dcbc..1c1972b 100644

drivers/scsi/qla2xxx/qla_dbg.c +1 -1

@@ -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

Quoted 45 lines

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 <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
drivers/scsi/qla2xxx/qla_bsg.c | 9 +--------

1 file changed, 1 insertion(+), 8 deletions(-)

drivers/scsi/qla2xxx/qla_bsg.c +1 -8

index 7221521..cf07491 100644
@@ -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:

Signature · 6 lines

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Saurav Kashyap

2013-06-12 08:08 UTC #8 lore raw

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

In reply to Bart Van Assche

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

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_dbg.c
index 1c1972b..dc227fa 100644

drivers/scsi/qla2xxx/qla_dbg.c +1 -0

@@ -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  |

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_init.c
index 8c7a123..c4fc8a2 100644

drivers/scsi/qla2xxx/qla_init.c +0 -8

@@ -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

Quoted 33 lines

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 <[email protected]>
Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
drivers/scsi/qla2xxx/qla_init.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
drivers/scsi/qla2xxx/qla_init.c +1 -7
index 3565dfd..c68bb01 100644
@@ -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);
 }
Signature · 1 line
1.7.10.4

Saurav Kashyap

Acked-by on v1 4/10

2013-06-12 08:08 UTC #10 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 43 lines

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

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---

drivers/scsi/qla2xxx/qla_gs.c |    3 +--
drivers/scsi/qla2xxx/qla_os.c |    3 +--
2 files changed, 2 insertions(+), 4 deletions(-)

drivers/scsi/qla2xxx/qla_gs.c +1 -2

index d0ea8b9..f26442a 100644
@@ -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);

drivers/scsi/qla2xxx/qla_os.c +1 -2

index ad72c1d..1d97626 100644
@@ -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),

Signature · 1 line

1.7.10.4

Saurav Kashyap

Acked-by on v1 5/10

2013-06-12 08:08 UTC #12 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 38 lines

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 <[email protected]>
Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
drivers/scsi/qla2xxx/qla_iocb.c | 2 --
1 file changed, 2 deletions(-)
drivers/scsi/qla2xxx/qla_iocb.c +0 -1
index 15e4080..b589d24 100644
@@ -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;
drivers/scsi/qla2xxx/qla_iocb.c +0 -1
@@ -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;
Signature · 1 line
1.7.10.4

Saurav Kashyap

Acked-by on v1 6/10

2013-06-12 08:09 UTC #14 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 50 lines

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 <[email protected]>
Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
drivers/scsi/qla2xxx/qla_dbg.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/qla2xxx/qla_dbg.c +1 -1
index cfa2a20..c612785 100644
@@ -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);
drivers/scsi/qla2xxx/qla_dbg.c +1 -1
@@ -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);
drivers/scsi/qla2xxx/qla_dbg.c +0 -0
@@ -2289,7 +2289,7 @@ qla83xx_fw_dump(scsi_qla_host_t *vha, int
 hardware_locked)
copy_queue:
	nxt = qla2xxx_copy_queues(ha, nxt);
drivers/scsi/qla2xxx/qla_dbg.c +1 -1
-	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);
Signature · 1 line
1.7.10.4

Saurav Kashyap

Acked-by on v1 7/10

2013-06-12 08:10 UTC #16 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 248 lines

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
drivers/scsi/qla2xxx/qla_gs.c | 86
++++++++++++++++++-----------------------

1 file changed, 38 insertions(+), 48 deletions(-)

drivers/scsi/qla2xxx/qla_gs.c +0 -0

index f26442a..1ad361b 100644
@@ -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)
{

drivers/scsi/qla2xxx/qla_gs.c +7 -7

-	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

drivers/scsi/qla2xxx/qla_gs.c +1 -2

@@ -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 */

drivers/scsi/qla2xxx/qla_gs.c +7 ~~-11~~

@@ -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 *

drivers/scsi/qla2xxx/qla_gs.c +1 -2

-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)
{

drivers/scsi/qla2xxx/qla_gs.c +7 -7

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

}

/**

drivers/scsi/qla2xxx/qla_gs.c +4 -7

@@ -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 *

drivers/scsi/qla2xxx/qla_gs.c +1 -2

-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)
{

drivers/scsi/qla2xxx/qla_gs.c +7 -7

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

}

/**

drivers/scsi/qla2xxx/qla_gs.c +2 -2

@@ -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;

Signature · 2 lines

-- 
1.7.10.4

Saurav Kashyap

Acked-by on v1 8/10

2013-06-12 08:10 UTC #18 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 28 lines

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 <[email protected]>
Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
drivers/scsi/qla2xxx/qla_isr.c | 1 +
1 file changed, 1 insertion(+)
drivers/scsi/qla2xxx/qla_isr.c +1 -0
index 259d920..bd0e2fa 100644
@@ -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--) {
Signature · 1 line
1.7.10.4

Saurav Kashyap

Acked-by on v1 9/10

2013-06-12 08:10 UTC #20 lore raw

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

In reply to Bart Van Assche

Acked-by Saurav Kashyap <[email protected]>

Quoted 31 lines

Signed-off-by Bart Van Assche <[email protected]>

Cc Chad Dupuis <[email protected]>

Cc Saurav Kashyap <[email protected]>

---
drivers/scsi/qla2xxx/qla_os.c | 4 +---

1 file changed, 1 insertion(+), 3 deletions(-)

drivers/scsi/qla2xxx/qla_os.c +1 -3

index 1d97626..2176cc9 100644
@@ -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);

Signature · 1 line

1.7.10.4

Saurav Kashyap

2013-06-12 08:12 UTC #22 lore raw

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

In reply to Bart Van Assche

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.

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_bsg.c
index 9520b1f..11f84dc 100644

drivers/scsi/qla2xxx/qla_bsg.c +6 -6

@@ -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 <[email protected]>
Date: Wed, 5 Jun 2013 15:09:59 +0200
To: linux-scsi <[email protected]>
Cc: Chad Dupuis <[email protected]>, Saurav Kashyap <[email protected]>
Subject: [PATCH 10/10] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

Quoted 65 lines

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 <[email protected]>
Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
drivers/scsi/qla2xxx/qla_bsg.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
drivers/scsi/qla2xxx/qla_bsg.c +6 -6
index cf07491..f8a2634 100644
@@ -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:
drivers/scsi/qla2xxx/qla_bsg.c +1 -1
-	if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
+	if (bsg_job->request->msgcode != FC_BSG_RPT_ELS)
		kfree(fcport);
done:
	return rval;
Signature · 1 line
1.7.10.4

James Bottomley

maintainer

2013-06-12 18:59 UTC #3 lore raw

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

In reply to Saurav Kashyap

Quoted 3 lines · Saurav Kashyap

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

Saurav Kashyap

2013-06-13 05:21 UTC #4 lore raw

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

Quoted 10 lines · Saurav Kashyap

Quoted 4 lines
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

Quoted 7 lines

Thanks,
James