qla2xxx patches for kernel v5.12 and v5.13

---
 drivers/scsi/qla2xxx/qla_target.c  | 13 +++++--------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 ----
 2 files changed, 5 insertions(+), 12 deletions(-)

drivers/scsi/qla2xxx/qla_target.c +5 -8

index d6366a46283e..5e8b2653e134 100644
@@ -3222,8 +3222,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 	if (!qpair->fw_started || (cmd->reset_count != qpair->chip_reset) ||
 	    (cmd->sess && cmd->sess->deleted)) {
 		cmd->state = QLA_TGT_STATE_PROCESSED;
-		res = 0;
-		goto free;
+		return 0;
 	}
 
 	ql_dbg_qp(ql_dbg_tgt, qpair, 0xe018,

@@ -3234,8 +3233,9 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 
 	res = qlt_pre_xmit_response(cmd, &prm, xmit_type, scsi_status,
 	    &full_req_cnt);
-	if (unlikely(res != 0))
-		goto free;
+	if (unlikely(res != 0)) {
+		return res;
+	}
 
 	spin_lock_irqsave(qpair->qp_lock_ptr, flags);

@@ -3255,8 +3255,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 			vha->flags.online, qla2x00_reset_active(vha),
 			cmd->reset_count, qpair->chip_reset);
 		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
-		res = 0;
-		goto free;
+		return 0;
 	}
 
 	/* Does F/W have an IOCBs for this request */

@@ -3359,8 +3358,6 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 	qlt_unmap_sg(vha, cmd);
 	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
 
-free:
-	vha->hw->tgt.tgt_ops->free_cmd(cmd);
 	return res;
 }
 EXPORT_SYMBOL(qlt_xmit_response);

drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -4

index 30959f8da065..15650a0bde09 100644
@@ -653,7 +653,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,
 				struct qla_tgt_cmd, se_cmd);
-	struct scsi_qla_host *vha = cmd->vha;
 
 	if (cmd->aborted) {
 		/* Cmd can loop during Q-full.  tcm_qla2xxx_aborted_task

@@ -666,7 +665,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 			cmd->se_cmd.transport_state,
 			cmd->se_cmd.t_state,
 			cmd->se_cmd.se_cmd_flags);
-		vha->hw->tgt.tgt_ops->free_cmd(cmd);
 		return 0;
 	}

@@ -694,7 +692,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,
 				struct qla_tgt_cmd, se_cmd);
-	struct scsi_qla_host *vha = cmd->vha;
 	int xmit_type = QLA_TGT_XMIT_STATUS;
 
 	if (cmd->aborted) {

@@ -708,7 +705,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 		    cmd, kref_read(&cmd->se_cmd.cmd_kref),
 		    cmd->se_cmd.transport_state, cmd->se_cmd.t_state,
 		    cmd->se_cmd.se_cmd_flags);
-		vha->hw->tgt.tgt_ops->free_cmd(cmd);
 		return 0;
 	}
 	cmd->bufflen = se_cmd->data_length;

Daniel Wagner

2021-03-16 08:26 UTC #2 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Quoted 3 lines · Bart Van Assche

Calling vha->hw->tgt.tgt_ops->free_cmd() from qlt_xmit_response() is wrong since the command for which a response is sent must remain valid until the SCSI target core calls .release_cmd().

The commit message from 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed") says 'avoids that the code for removing a session hangs due to commands that do not make progress'.

As this patch reverts the change, is the problem mentioned gone? Did some other change fix it? Just wondering.

Bart Van Assche

author

2021-03-16 20:37 UTC #3 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Quoted 11 lines · Daniel Wagner

Quoted 3 lines · Bart Van Assche
Calling vha->hw->tgt.tgt_ops->free_cmd() from qlt_xmit_response() is wrong since the command for which a response is sent must remain valid until the SCSI target core calls .release_cmd().
The commit message from 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed") says 'avoids that the code for removing a session hangs due to commands that do not make progress'.
As this patch reverts the change, is the problem mentioned gone? Did some other change fix it? Just wondering.

Hi Daniel,

Commit 0dcec41acb85 was the result of source reading. The changes made by this patch in qlt_xmit_response() are wrong and may lead to a kernel crash. Since triggering the other code paths that are modified by that patch, I'd like to revert that patch in its entirety.

Thanks,

Bart.

Daniel Wagner

Reviewed-by

2021-03-17 08:56 UTC #4 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Hi Bart,

Quoted 4 lines · Bart Van Assche

Commit 0dcec41acb85 was the result of source reading. The changes made by this patch in qlt_xmit_response() are wrong and may lead to a kernel crash. Since triggering the other code paths that are modified by that patch, I'd like to revert that patch in its entirety.

Thanks for the explanation. I was hoping that you have a fix for something I see in our customer bug reports :)

Himanshu Madhani

2021-03-16 16:26 UTC #5 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Quoted 93 lines · Bart Van Assche

Calling vha->hw->tgt.tgt_ops->free_cmd() from qlt_xmit_response() is wrong since the command for which a response is sent must remain valid until the SCSI target core calls .release_cmd().

Fixes 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed")

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
drivers/scsi/qla2xxx/qla_target.c | 13 +++++-------- drivers/scsi/qla2xxx/tcm_qla2xxx.c | 4 ----

2 files changed, 5 insertions(+), 12 deletions(-)

drivers/scsi/qla2xxx/qla_target.c +5 -8

index d6366a46283e..5e8b2653e134 100644
@@ -3222,8 +3222,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 	if (!qpair->fw_started || (cmd->reset_count != qpair->chip_reset) ||
 	    (cmd->sess && cmd->sess->deleted)) {
 		cmd->state = QLA_TGT_STATE_PROCESSED;
-		res = 0;
-		goto free;
+		return 0;
 	}
 
 	ql_dbg_qp(ql_dbg_tgt, qpair, 0xe018,

@@ -3234,8 +3233,9 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 
 	res = qlt_pre_xmit_response(cmd, &prm, xmit_type, scsi_status,
 	    &full_req_cnt);
-	if (unlikely(res != 0))
-		goto free;
+	if (unlikely(res != 0)) {
+		return res;
+	}
 
 	spin_lock_irqsave(qpair->qp_lock_ptr, flags);

@@ -3255,8 +3255,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 			vha->flags.online, qla2x00_reset_active(vha),
 			cmd->reset_count, qpair->chip_reset);
 		spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
-		res = 0;
-		goto free;
+		return 0;
 	}
 
 	/* Does F/W have an IOCBs for this request */

@@ -3359,8 +3358,6 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type,
 	qlt_unmap_sg(vha, cmd);
 	spin_unlock_irqrestore(qpair->qp_lock_ptr, flags);
 
-free:
-	vha->hw->tgt.tgt_ops->free_cmd(cmd);
 	return res;
 }
 EXPORT_SYMBOL(qlt_xmit_response);

drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -3

index 30959f8da065..15650a0bde09 100644
@@ -653,7 +653,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,
 				struct qla_tgt_cmd, se_cmd);
-	struct scsi_qla_host *vha = cmd->vha;
 
 	if (cmd->aborted) {
 		/* Cmd can loop during Q-full.  tcm_qla2xxx_aborted_task

@@ -666,7 +665,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd)
 			cmd->se_cmd.transport_state,
 			cmd->se_cmd.t_state,
 			cmd->se_cmd.se_cmd_flags);
-		vha->hw->tgt.tgt_ops->free_cmd(cmd);
 		return 0;
 	}

@@ -694,7 +692,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 {
 	struct qla_tgt_cmd *cmd = container_of(se_cmd,
 				struct qla_tgt_cmd, se_cmd);
-	struct scsi_qla_host *vha = cmd->vha;
 	int xmit_type = QLA_TGT_XMIT_STATUS;
 
 	if (cmd->aborted) {

drivers/scsi/qla2xxx/tcm_qla2xxx.c static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) · line 710

Quoted 7 lines

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

@@ -708,7 +705,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd)
 		    cmd, kref_read(&cmd->se_cmd.cmd_kref),
 		    cmd->se_cmd.transport_state, cmd->se_cmd.t_state,
 		    cmd->se_cmd.se_cmd_flags);
-		vha->hw->tgt.tgt_ops->free_cmd(cmd);
 		return 0;
 	}
 	cmd->bufflen = se_cmd->data_length;

Curious …. What triggered this revert? Can you share your motivation for this revert.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 20:29 UTC #6 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Quoted 1 line · Himanshu Madhani

Curious …. What triggered this revert? Can you share your motivation for this revert.

Hi Himanshu,

It has been observed that the following scenario triggers a kernel crash:
- qlt_xmit_response() calls qlt_check_reserve_free_req().
- qlt_check_reserve_free_req() returns -EAGAIN.
- qlt_xmit_response() calls vha->hw->tgt.tgt_ops->free_cmd(cmd).
- transport_handle_queue_full() tries to retransmit the response.

I will add this information to the patch description.

Bart.

Himanshu Madhani

2021-03-17 17:38 UTC #7 lore raw

Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"

Quoted 15 lines · Bart Van Assche

Quoted 1 line · Himanshu Madhani
Curious …. What triggered this revert? Can you share your motivation for this revert.
Hi Himanshu,
It has been observed that the following scenario triggers a kernel crash:
- qlt_xmit_response() calls qlt_check_reserve_free_req().
- qlt_check_reserve_free_req() returns -EAGAIN.
- qlt_xmit_response() calls vha->hw->tgt.tgt_ops->free_cmd(cmd).
- transport_handle_queue_full() tries to retransmit the response.

Quoted 3 lines

I will add this information to the patch description.
Bart.

Thanks, once you add more information to the patch, you can add my R-B

—
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #8 lore raw

PATCH 2/7 qla2xxx: Constify struct qla_tgt_func_tmpl

Since the target function pointers are not modified at runtime, declare the data structure with the target function pointers const.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
 drivers/scsi/qla2xxx/qla_def.h     | 2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/qla2xxx/qla_def.h +1 -1

index 49b42b430df4..3bdf55bb0833 100644
@@ -3815,7 +3815,7 @@ struct qlt_hw_data {
 	__le32 __iomem *atio_q_in;
 	__le32 __iomem *atio_q_out;
 
-	struct qla_tgt_func_tmpl *tgt_ops;
+	const struct qla_tgt_func_tmpl *tgt_ops;
 	struct qla_tgt_vp_map *tgt_vp_map;
 
 	int saved_set;

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

index 15650a0bde09..46111f031be9 100644
@@ -1578,7 +1578,7 @@ static void tcm_qla2xxx_update_sess(struct fc_port *sess, port_id_t s_id,
 /*
  * Calls into tcm_qla2xxx used by qla2xxx LLD I/O path.
  */
-static struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
+static const struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
 	.find_cmd_by_tag	= tcm_qla2xxx_find_cmd_by_tag,
 	.handle_cmd		= tcm_qla2xxx_handle_cmd,
 	.handle_data		= tcm_qla2xxx_handle_data,

Daniel Wagner

Reviewed-by on v1 2/7

2021-03-16 08:26 UTC #9 lore raw

Re: [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl

Quoted 8 lines · Bart Van Assche

Since the target function pointers are not modified at runtime, declare the data structure with the target function pointers const.
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Himanshu Madhani

Reviewed-by on v1 2/7

2021-03-16 16:24 UTC #10 lore raw

Re: [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl

Quoted 35 lines · Bart Van Assche

Since the target function pointers are not modified at runtime, declare the data structure with the target function pointers const.
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
drivers/scsi/qla2xxx/qla_def.h | 2 +-
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/qla2xxx/qla_def.h +1 -1
index 49b42b430df4..3bdf55bb0833 100644
@@ -3815,7 +3815,7 @@ struct qlt_hw_data {
 	__le32 __iomem *atio_q_in;
 	__le32 __iomem *atio_q_out;
 
-	struct qla_tgt_func_tmpl *tgt_ops;
+	const struct qla_tgt_func_tmpl *tgt_ops;
 	struct qla_tgt_vp_map *tgt_vp_map;
 
 	int saved_set;
drivers/scsi/qla2xxx/tcm_qla2xxx.c +0 -0
index 15650a0bde09..46111f031be9 100644

drivers/scsi/qla2xxx/tcm_qla2xxx.c static void tcm_qla2xxx_update_sess(struct fc_port *sess, port_id_t s_id, · line 1584

Quoted 8 lines

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

@@ -1578,7 +1578,7 @@ static void tcm_qla2xxx_update_sess(struct fc_port *sess, port_id_t s_id,
 /*
 * Calls into tcm_qla2xxx used by qla2xxx LLD I/O path.
 */
-static struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
+static const struct qla_tgt_func_tmpl tcm_qla2xxx_template = {
 	.find_cmd_by_tag	= tcm_qla2xxx_find_cmd_by_tag,
 	.handle_cmd		= tcm_qla2xxx_handle_cmd,
 	.handle_data		= tcm_qla2xxx_handle_data,

Looks Good.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #11 lore raw

PATCH 3/7 qla2xxx: Fix endianness annotations

Fix all recently introduced endianness annotation issues.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
 drivers/scsi/qla2xxx/qla_def.h  | 2 +-
 drivers/scsi/qla2xxx/qla_iocb.c | 3 ++-
 drivers/scsi/qla2xxx/qla_isr.c  | 2 +-
 drivers/scsi/qla2xxx/qla_sup.c  | 9 +++++----
 4 files changed, 9 insertions(+), 7 deletions(-)

drivers/scsi/qla2xxx/qla_def.h +1 -1

index 3bdf55bb0833..52ba75591f9a 100644
@@ -1527,7 +1527,7 @@ struct init_sf_cb {
 	 * BIT_12 = Remote Write Optimization (1 - Enabled, 0 - Disabled)
 	 * BIT 11-0 = Reserved
 	 */
-	uint16_t flags;
+	__le16	flags;
 	uint8_t	reserved1[32];
 	uint16_t discard_OHRB_timeout_value;
 	uint16_t remote_write_opt_queue_num;

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

index 8b41cbaf8535..eb2376b138c1 100644
@@ -2379,7 +2379,8 @@ qla24xx_prli_iocb(srb_t *sp, struct logio_entry_24xx *logio)
 				cpu_to_le32(NVME_PRLI_SP_FIRST_BURST);
 		if (sp->vha->flags.nvme2_enabled) {
 			/* Set service parameter BIT_7 for NVME CONF support */
-			logio->io_parameter[0] |= NVME_PRLI_SP_CONF;
+			logio->io_parameter[0] |=
+				cpu_to_le32(NVME_PRLI_SP_CONF);
 			/* Set service parameter BIT_8 for SLER support */
 			logio->io_parameter[0] |=
 				cpu_to_le32(NVME_PRLI_SP_SLER);

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

index 27165abda96d..0fa7082f3cc8 100644
@@ -3440,7 +3440,7 @@ qla24xx_abort_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
 		return;
 
 	abt = &sp->u.iocb_cmd;
-	abt->u.abt.comp_status = le16_to_cpu(pkt->comp_status);
+	abt->u.abt.comp_status = pkt->comp_status;
 	orig_sp = sp->cmd_sp;
 	/* Need to pass original sp */
 	if (orig_sp)

drivers/scsi/qla2xxx/qla_sup.c +5 -4

index f771fabcba59..060c89237777 100644
@@ -2621,10 +2621,11 @@ qla24xx_read_optrom_data(struct scsi_qla_host *vha, void *buf,
 }
 
 static int
-qla28xx_extract_sfub_and_verify(struct scsi_qla_host *vha, uint32_t *buf,
+qla28xx_extract_sfub_and_verify(struct scsi_qla_host *vha, __le32 *buf,
     uint32_t len, uint32_t buf_size_without_sfub, uint8_t *sfub_buf)
 {
-	uint32_t *p, check_sum = 0;
+	uint32_t check_sum = 0;
+	__le32 *p;
 	int i;
 
 	p = buf + buf_size_without_sfub;

@@ -2790,8 +2791,8 @@ qla28xx_write_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t faddr,
 			goto done;
 		}
 
-		rval = qla28xx_extract_sfub_and_verify(vha, dwptr, dwords,
-			buf_size_without_sfub, (uint8_t *)sfub);
+		rval = qla28xx_extract_sfub_and_verify(vha, (__le32 *)dwptr,
+			dwords, buf_size_without_sfub, (uint8_t *)sfub);
 
 		if (rval != QLA_SUCCESS)
 			goto done;

Daniel Wagner

Reviewed-by on v1 3/7

2021-03-16 08:33 UTC #12 lore raw

Re: [PATCH 3/7] qla2xxx: Fix endianness annotations

Quoted 7 lines · Bart Van Assche

Fix all recently introduced endianness annotation issues.
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Himanshu Madhani

Reviewed-by on v1 3/7

2021-03-16 16:28 UTC #13 lore raw

Re: [PATCH 3/7] qla2xxx: Fix endianness annotations

Quoted 77 lines · Bart Van Assche

Fix all recently introduced endianness annotation issues.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---

drivers/scsi/qla2xxx/qla_def.h  | 2 +-
drivers/scsi/qla2xxx/qla_iocb.c | 3 ++-
drivers/scsi/qla2xxx/qla_isr.c  | 2 +-
drivers/scsi/qla2xxx/qla_sup.c  | 9 +++++----
4 files changed, 9 insertions(+), 7 deletions(-)

drivers/scsi/qla2xxx/qla_def.h +1 -1

index 3bdf55bb0833..52ba75591f9a 100644
@@ -1527,7 +1527,7 @@ struct init_sf_cb {
 	 * BIT_12 = Remote Write Optimization (1 - Enabled, 0 - Disabled)
 	 * BIT 11-0 = Reserved
 	 */
-	uint16_t flags;
+	__le16	flags;
 	uint8_t	reserved1[32];
 	uint16_t discard_OHRB_timeout_value;
 	uint16_t remote_write_opt_queue_num;

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

index 8b41cbaf8535..eb2376b138c1 100644
@@ -2379,7 +2379,8 @@ qla24xx_prli_iocb(srb_t *sp, struct logio_entry_24xx *logio)
 				cpu_to_le32(NVME_PRLI_SP_FIRST_BURST);
 		if (sp->vha->flags.nvme2_enabled) {
 			/* Set service parameter BIT_7 for NVME CONF support */
-			logio->io_parameter[0] |= NVME_PRLI_SP_CONF;
+			logio->io_parameter[0] |=
+				cpu_to_le32(NVME_PRLI_SP_CONF);
 			/* Set service parameter BIT_8 for SLER support */
 			logio->io_parameter[0] |=
 				cpu_to_le32(NVME_PRLI_SP_SLER);

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

index 27165abda96d..0fa7082f3cc8 100644
@@ -3440,7 +3440,7 @@ qla24xx_abort_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
 		return;
 
 	abt = &sp->u.iocb_cmd;
-	abt->u.abt.comp_status = le16_to_cpu(pkt->comp_status);
+	abt->u.abt.comp_status = pkt->comp_status;
 	orig_sp = sp->cmd_sp;
 	/* Need to pass original sp */
 	if (orig_sp)

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

index f771fabcba59..060c89237777 100644
@@ -2621,10 +2621,11 @@ qla24xx_read_optrom_data(struct scsi_qla_host *vha, void *buf,
 }

static int

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

-qla28xx_extract_sfub_and_verify(struct scsi_qla_host *vha, uint32_t *buf,
+qla28xx_extract_sfub_and_verify(struct scsi_qla_host *vha, __le32 *buf,
    uint32_t len, uint32_t buf_size_without_sfub, uint8_t *sfub_buf)

{

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

-	uint32_t *p, check_sum = 0;
+	uint32_t check_sum = 0;
+	__le32 *p;

	int i;

	p = buf + buf_size_without_sfub;

drivers/scsi/qla2xxx/qla_sup.c qla28xx_write_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t faddr, · line 2798

Quoted 10 lines

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

@@ -2790,8 +2791,8 @@ qla28xx_write_flash_data(scsi_qla_host_t *vha, uint32_t *dwptr, uint32_t faddr,
 			goto done;
 		}
 
-		rval = qla28xx_extract_sfub_and_verify(vha, dwptr, dwords,
-			buf_size_without_sfub, (uint8_t *)sfub);
+		rval = qla28xx_extract_sfub_and_verify(vha, (__le32 *)dwptr,
+			dwords, buf_size_without_sfub, (uint8_t *)sfub);
 
 		if (rval != QLA_SUCCESS)
 			goto done;

Looks Good.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #14 lore raw

PATCH 4/7 qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it

This patch suppresses the following sparse warning:

qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

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

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

index 0677295957bc..5683126e0cbc 100644
@@ -1095,7 +1095,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	int i ;
 	struct crb_addr_pair *buf;
 	unsigned long off;
-	unsigned offset, n;
+	unsigned offset, n = 0;
 	struct qla_hw_data *ha = vha->hw;
 
 	struct crb_addr_pair {

Daniel Wagner

2021-03-16 08:37 UTC #15 lore raw

Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it

Quoted 1 line · Bart Van Assche

This patch suppresses the following sparse warning:

[...]

drivers/scsi/qla2xxx/qla_nx.c qla82xx_pinit_from_rom(scsi_qla_host_t *vha) · line 1101

Quoted 13 lines

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

index 0677295957bc..5683126e0cbc 100644
@@ -1095,7 +1095,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	int i ;
 	struct crb_addr_pair *buf;
 	unsigned long off;
-	unsigned offset, n;
+	unsigned offset, n = 0;
 	struct qla_hw_data *ha = vha->hw;
 
 	struct crb_addr_pair {

I think sparse is not able to see that n is initialized before it is used.

	/* Read the signature value from the flash.
	 * Offset 0: Contain signature (0xcafecafe)
	 * Offset 4: Offset and number of addr/value pairs
	 * that present in CRB initialize sequence
	 */
	n = 0;
	if (qla82xx_rom_fast_read(ha, 0, &n) != 0 || n != 0xcafecafeUL ||
	    qla82xx_rom_fast_read(ha, 4, &n) != 0) {
		ql_log(ql_log_fatal, vha, 0x006e,
		    "Error Reading crb_init area: n: %08x.\n", n);
		return -1;
	}

I suppose this n = 0 should be dropped if n is initialized at the beginning of the function.

Bart Van Assche

author

2021-03-16 20:40 UTC #16 lore raw

Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it

Quoted 31 lines · Daniel Wagner

drivers/scsi/qla2xxx/qla_nx.c qla82xx_pinit_from_rom(scsi_qla_host_t *vha) · line 1101

Quoted 13 lines · Bart Van Assche

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

index 0677295957bc..5683126e0cbc 100644
@@ -1095,7 +1095,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	int i ;
 	struct crb_addr_pair *buf;
 	unsigned long off;
-	unsigned offset, n;
+	unsigned offset, n = 0;
 	struct qla_hw_data *ha = vha->hw;
 
 	struct crb_addr_pair {

I think sparse is not able to see that n is initialized before it is used.

	/* Read the signature value from the flash.
	 * Offset 0: Contain signature (0xcafecafe)
	 * Offset 4: Offset and number of addr/value pairs
	 * that present in CRB initialize sequence
	 */
	n = 0;
	if (qla82xx_rom_fast_read(ha, 0, &n) != 0 || n != 0xcafecafeUL ||
	    qla82xx_rom_fast_read(ha, 4, &n) != 0) {
		ql_log(ql_log_fatal, vha, 0x006e,
		    "Error Reading crb_init area: n: %08x.\n", n);
		return -1;
	}

Quoted 3 lines

I suppose this n = 0 should be dropped if n is initialized at the beginning of the function.
.

Right, the variable 'n' is already being initialized. The patch that added that initialization code went in after I came up with the above patch. I will drop this patch.

Thanks,

Bart.

Himanshu Madhani

Reviewed-by on v1 4/7

2021-03-16 16:28 UTC #17 lore raw

Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it

Quoted 22 lines · Bart Van Assche

This patch suppresses the following sparse warning:
qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
drivers/scsi/qla2xxx/qla_nx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_nx.c +0 -0
index 0677295957bc..5683126e0cbc 100644

drivers/scsi/qla2xxx/qla_nx.c qla82xx_pinit_from_rom(scsi_qla_host_t *vha) · line 1101

Quoted 8 lines

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

@@ -1095,7 +1095,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	int i ;
 	struct crb_addr_pair *buf;
 	unsigned long off;
-	unsigned offset, n;
+	unsigned offset, n = 0;
 	struct qla_hw_data *ha = vha->hw;
 
 	struct crb_addr_pair {

Looks Good.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #18 lore raw

PATCH 5/7 qla2xxx: Suppress Coverity complaints about dseg_r*

Change dseq_rq and dseg_rsp from scalar structure members into single-element arrays such that Coverity does not complain about the (*cur_dsd)++ statement in append_dsd64().

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
 drivers/scsi/qla2xxx/qla_mr.c | 12 ++++++------
 drivers/scsi/qla2xxx/qla_mr.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

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

index ca7306685325..61eaf6c4ac47 100644
@@ -3266,8 +3266,8 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.req_xfrcnt =
 			    cpu_to_le16(fxio->u.fxiocb.req_len);
 			put_unaligned_le64(fxio->u.fxiocb.req_dma_handle,
-					   &fx_iocb.dseg_rq.address);
-			fx_iocb.dseg_rq.length =
+					   &fx_iocb.dseg_rq[0].address);
+			fx_iocb.dseg_rq[0].length =
 			    cpu_to_le32(fxio->u.fxiocb.req_len);
 		}

@@ -3276,8 +3276,8 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.rsp_xfrcnt =
 			    cpu_to_le16(fxio->u.fxiocb.rsp_len);
 			put_unaligned_le64(fxio->u.fxiocb.rsp_dma_handle,
-					   &fx_iocb.dseg_rsp.address);
-			fx_iocb.dseg_rsp.length =
+					   &fx_iocb.dseg_rsp[0].address);
+			fx_iocb.dseg_rsp[0].length =
 			    cpu_to_le32(fxio->u.fxiocb.rsp_len);
 		}

@@ -3314,7 +3314,7 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			    cpu_to_le16(bsg_job->request_payload.sg_cnt);
 			tot_dsds =
 			    bsg_job->request_payload.sg_cnt;
-			cur_dsd = &fx_iocb.dseg_rq;
+			cur_dsd = &fx_iocb.dseg_rq[0];
 			avail_dsds = 1;
 			for_each_sg(bsg_job->request_payload.sg_list, sg,
 			    tot_dsds, index) {

@@ -3369,7 +3369,7 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.rsp_dsdcnt =
 			   cpu_to_le16(bsg_job->reply_payload.sg_cnt);
 			tot_dsds = bsg_job->reply_payload.sg_cnt;
-			cur_dsd = &fx_iocb.dseg_rsp;
+			cur_dsd = &fx_iocb.dseg_rsp[0];
 			avail_dsds = 1;
 
 			for_each_sg(bsg_job->reply_payload.sg_list, sg,

drivers/scsi/qla2xxx/qla_mr.h +2 -2

index 73be8348402a..eefbae9d7547 100644
@@ -176,8 +176,8 @@ struct fxdisc_entry_fx00 {
 	uint8_t flags;
 	uint8_t reserved_1;
 
-	struct dsd64 dseg_rq;
-	struct dsd64 dseg_rsp;
+	struct dsd64 dseg_rq[1];
+	struct dsd64 dseg_rsp[1];
 
 	__le32 dataword;
 	__le32 adapid;

Daniel Wagner

Reviewed-by on v1 5/7

2021-03-16 08:38 UTC #19 lore raw

Re: [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r*

Quoted 9 lines · Bart Van Assche

Change dseq_rq and dseg_rsp from scalar structure members into single-element arrays such that Coverity does not complain about the (*cur_dsd)++ statement in append_dsd64().
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Himanshu Madhani

Reviewed-by on v1 5/7

2021-03-16 16:29 UTC #20 lore raw

Re: [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r*

Quoted 67 lines · Bart Van Assche

Change dseq_rq and dseg_rsp from scalar structure members into single-element arrays such that Coverity does not complain about the (*cur_dsd)++ statement in append_dsd64().

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
drivers/scsi/qla2xxx/qla_mr.c | 12 ++++++------
drivers/scsi/qla2xxx/qla_mr.h | 4 ++--

2 files changed, 8 insertions(+), 8 deletions(-)

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

index ca7306685325..61eaf6c4ac47 100644
@@ -3266,8 +3266,8 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.req_xfrcnt =
 			    cpu_to_le16(fxio->u.fxiocb.req_len);
 			put_unaligned_le64(fxio->u.fxiocb.req_dma_handle,
-					   &fx_iocb.dseg_rq.address);
-			fx_iocb.dseg_rq.length =
+					   &fx_iocb.dseg_rq[0].address);
+			fx_iocb.dseg_rq[0].length =
 			    cpu_to_le32(fxio->u.fxiocb.req_len);
 		}

@@ -3276,8 +3276,8 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.rsp_xfrcnt =
 			    cpu_to_le16(fxio->u.fxiocb.rsp_len);
 			put_unaligned_le64(fxio->u.fxiocb.rsp_dma_handle,
-					   &fx_iocb.dseg_rsp.address);
-			fx_iocb.dseg_rsp.length =
+					   &fx_iocb.dseg_rsp[0].address);
+			fx_iocb.dseg_rsp[0].length =
 			    cpu_to_le32(fxio->u.fxiocb.rsp_len);
 		}

@@ -3314,7 +3314,7 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			    cpu_to_le16(bsg_job->request_payload.sg_cnt);
 			tot_dsds =
 			    bsg_job->request_payload.sg_cnt;
-			cur_dsd = &fx_iocb.dseg_rq;
+			cur_dsd = &fx_iocb.dseg_rq[0];
 			avail_dsds = 1;
 			for_each_sg(bsg_job->request_payload.sg_list, sg,
 			    tot_dsds, index) {

@@ -3369,7 +3369,7 @@ qlafx00_fxdisc_iocb(srb_t *sp, struct fxdisc_entry_fx00 *pfxiocb)
 			fx_iocb.rsp_dsdcnt =
 			   cpu_to_le16(bsg_job->reply_payload.sg_cnt);
 			tot_dsds = bsg_job->reply_payload.sg_cnt;
-			cur_dsd = &fx_iocb.dseg_rsp;
+			cur_dsd = &fx_iocb.dseg_rsp[0];
 			avail_dsds = 1;
 
 			for_each_sg(bsg_job->reply_payload.sg_list, sg,

drivers/scsi/qla2xxx/qla_mr.h +0 -0

index 73be8348402a..eefbae9d7547 100644

drivers/scsi/qla2xxx/qla_mr.h struct fxdisc_entry_fx00 { · line 183

Quoted 10 lines

drivers/scsi/qla2xxx/qla_mr.h +2 -2

@@ -176,8 +176,8 @@ struct fxdisc_entry_fx00 {
 	uint8_t flags;
 	uint8_t reserved_1;
 
-	struct dsd64 dseg_rq;
-	struct dsd64 dseg_rsp;
+	struct dsd64 dseg_rq[1];
+	struct dsd64 dseg_rsp[1];
 
 	__le32 dataword;
 	__le32 adapid;

Looks Good.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #21 lore raw

PATCH 6/7 qla2xxx: Simplify qla8044_minidump_process_control()

This patch fixes the following Coverity complaint:

    CID 177490 (#1 of 1): Unused value (UNUSED_VALUE)
    assigned_value: Assigning value from opcode & 0xffffff7fU to opcode
    here, but that stored value is overwritten before it can be used.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
 drivers/scsi/qla2xxx/qla_nx2.c | 8 --------
 1 file changed, 8 deletions(-)

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

index 68a16c95dcb7..792c55fcec8c 100644
@@ -2226,19 +2226,16 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 		if (opcode & QLA82XX_DBG_OPCODE_WR) {
 			qla8044_wr_reg_indirect(vha, crb_addr,
 			    crb_entry->value_1);
-			opcode &= ~QLA82XX_DBG_OPCODE_WR;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_RW) {
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			qla8044_wr_reg_indirect(vha, crb_addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_RW;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_AND) {
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			read_value &= crb_entry->value_2;
-			opcode &= ~QLA82XX_DBG_OPCODE_AND;
 			if (opcode & QLA82XX_DBG_OPCODE_OR) {
 				read_value |= crb_entry->value_3;
 				opcode &= ~QLA82XX_DBG_OPCODE_OR;

@@ -2249,7 +2246,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			read_value |= crb_entry->value_3;
 			qla8044_wr_reg_indirect(vha, crb_addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_OR;
 		}
 		if (opcode & QLA82XX_DBG_OPCODE_POLL) {
 			poll_time = crb_entry->crb_strd.poll_timeout;

@@ -2269,7 +2265,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 					    crb_addr, &read_value);
 				}
 			} while (1);
-			opcode &= ~QLA82XX_DBG_OPCODE_POLL;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_RDSTATE) {

@@ -2283,7 +2278,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			qla8044_rd_reg_indirect(vha, addr, &read_value);
 			index = crb_entry->crb_ctrl.state_index_v;
 			tmplt_hdr->saved_state_array[index] = read_value;
-			opcode &= ~QLA82XX_DBG_OPCODE_RDSTATE;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_WRSTATE) {

@@ -2303,7 +2297,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			}
 
 			qla8044_wr_reg_indirect(vha, addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_WRSTATE;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_MDSTATE) {

@@ -2316,7 +2309,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			read_value |= crb_entry->value_3;
 			read_value += crb_entry->value_1;
 			tmplt_hdr->saved_state_array[index] = read_value;
-			opcode &= ~QLA82XX_DBG_OPCODE_MDSTATE;
 		}
 		crb_addr += crb_entry->crb_strd.addr_stride;
 	}

Daniel Wagner

Reviewed-by on v1 6/7

2021-03-16 08:46 UTC #22 lore raw

Re: [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control()

Quoted 11 lines · Bart Van Assche

This patch fixes the following Coverity complaint:
    CID 177490 (#1 of 1): Unused value (UNUSED_VALUE)
    assigned_value: Assigning value from opcode & 0xffffff7fU to opcode
    here, but that stored value is overwritten before it can be used.
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Himanshu Madhani

Reviewed-by on v1 6/7

2021-03-16 16:30 UTC #23 lore raw

Re: [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control()

Quoted 76 lines · Bart Van Assche

This patch fixes the following Coverity complaint:

CID 177490 (#1 of 1): Unused value (UNUSED_VALUE)
assigned_value: Assigning value from opcode & 0xffffff7fU to opcode here, but that stored value is overwritten before it can be used.

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
drivers/scsi/qla2xxx/qla_nx2.c | 8 --------

1 file changed, 8 deletions(-)

drivers/scsi/qla2xxx/qla_nx2.c +0 -7

index 68a16c95dcb7..792c55fcec8c 100644
@@ -2226,19 +2226,16 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 		if (opcode & QLA82XX_DBG_OPCODE_WR) {
 			qla8044_wr_reg_indirect(vha, crb_addr,
 			    crb_entry->value_1);
-			opcode &= ~QLA82XX_DBG_OPCODE_WR;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_RW) {
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			qla8044_wr_reg_indirect(vha, crb_addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_RW;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_AND) {
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			read_value &= crb_entry->value_2;
-			opcode &= ~QLA82XX_DBG_OPCODE_AND;
 			if (opcode & QLA82XX_DBG_OPCODE_OR) {
 				read_value |= crb_entry->value_3;
 				opcode &= ~QLA82XX_DBG_OPCODE_OR;

@@ -2249,7 +2246,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			qla8044_rd_reg_indirect(vha, crb_addr, &read_value);
 			read_value |= crb_entry->value_3;
 			qla8044_wr_reg_indirect(vha, crb_addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_OR;
 		}
 		if (opcode & QLA82XX_DBG_OPCODE_POLL) {
 			poll_time = crb_entry->crb_strd.poll_timeout;

@@ -2269,7 +2265,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 					    crb_addr, &read_value);
 				}
 			} while (1);
-			opcode &= ~QLA82XX_DBG_OPCODE_POLL;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_RDSTATE) {

@@ -2283,7 +2278,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			qla8044_rd_reg_indirect(vha, addr, &read_value);
 			index = crb_entry->crb_ctrl.state_index_v;
 			tmplt_hdr->saved_state_array[index] = read_value;
-			opcode &= ~QLA82XX_DBG_OPCODE_RDSTATE;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_WRSTATE) {

@@ -2303,7 +2297,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			}
 
 			qla8044_wr_reg_indirect(vha, addr, read_value);
-			opcode &= ~QLA82XX_DBG_OPCODE_WRSTATE;
 		}
 
 		if (opcode & QLA82XX_DBG_OPCODE_MDSTATE) {

drivers/scsi/qla2xxx/qla_nx2.c qla8044_minidump_process_control(struct scsi_qla_host *vha, · line 2314

Quoted 7 lines

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

@@ -2316,7 +2309,6 @@ qla8044_minidump_process_control(struct scsi_qla_host *vha,
 			read_value |= crb_entry->value_3;
 			read_value += crb_entry->value_1;
 			tmplt_hdr->saved_state_array[index] = read_value;
-			opcode &= ~QLA82XX_DBG_OPCODE_MDSTATE;
 		}
 		crb_addr += crb_entry->crb_strd.addr_stride;
 	}

Looks Okay.

--
Himanshu Madhani Oracle Linux Engineering

Bart Van Assche

author

2021-03-16 03:57 UTC #24 lore raw

PATCH 7/7 qla2xxx: Always check the return value of qla24xx_get_isp_stats()

This patch fixes the following Coverity warning:

CID 361199 (#1 of 1): Unchecked return value (CHECKED_RETURN)
3. check_return: Calling qla24xx_get_isp_stats without checking return value (as is done elsewhere 4 out of 5 times).

Cc Quinn Tran <[email protected]>

Cc Mike Christie <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Daniel Wagner <[email protected]>

---
 drivers/scsi/qla2xxx/qla_attr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/scsi/qla2xxx/qla_attr.c +4 -1

index 63391c9be05d..ad57111f8cb9 100644
@@ -2864,6 +2864,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost)
 	vha->qla_stats.jiffies_at_last_reset = get_jiffies_64();
 
 	if (IS_FWI2_CAPABLE(ha)) {
+		int rval;
+
 		stats = dma_alloc_coherent(&ha->pdev->dev,
 		    sizeof(*stats), &stats_dma, GFP_KERNEL);
 		if (!stats) {

@@ -2873,7 +2875,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost)
 		}
 
 		/* reset firmware statistics */
-		qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		WARN_ONCE(rval != QLA_SUCCESS, "rval = %d\n", rval);
 
 		dma_free_coherent(&ha->pdev->dev, sizeof(*stats),
 		    stats, stats_dma);

Daniel Wagner

2021-03-16 08:48 UTC #25 lore raw

Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()

drivers/scsi/qla2xxx/qla_attr.c qla2x00_reset_host_stats(struct Scsi_Host *shost) · line 2879

Quoted 4 lines · Bart Van Assche

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

 		/* reset firmware statistics */
-		qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		WARN_ONCE(rval != QLA_SUCCESS, "rval = %d\n", rval);

ql_log() instead of WARN_ONCE? The function uses ql_log() if dma_alloc_coherrent() fails a few lines above.

Bart Van Assche

author

2021-03-16 20:41 UTC #26 lore raw

Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()

Quoted 8 lines · Daniel Wagner

drivers/scsi/qla2xxx/qla_attr.c qla2x00_reset_host_stats(struct Scsi_Host *shost) · line 2879
Quoted 4 lines · Bart Van Assche
drivers/scsi/qla2xxx/qla_attr.c +2 -1
 		/* reset firmware statistics */
-		qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		WARN_ONCE(rval != QLA_SUCCESS, "rval = %d\n", rval);
ql_log() instead of WARN_ONCE? The function uses ql_log() if dma_alloc_coherrent() fails a few lines above.

Hi Daniel,

I will make that change.

Thanks,

Bart.

Himanshu Madhani

Reviewed-by on v1 7/7

2021-03-16 16:31 UTC #27 lore raw

Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()

Quoted 33 lines · Bart Van Assche

This patch fixes the following Coverity warning:
CID 361199 (#1 of 1): Unchecked return value (CHECKED_RETURN)
3. check_return: Calling qla24xx_get_isp_stats without checking return value (as is done elsewhere 4 out of 5 times).
Cc Quinn Tran <[email protected]>
Cc Mike Christie <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Daniel Wagner <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
drivers/scsi/qla2xxx/qla_attr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_attr.c +2 -0
index 63391c9be05d..ad57111f8cb9 100644
@@ -2864,6 +2864,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost)
 	vha->qla_stats.jiffies_at_last_reset = get_jiffies_64();
 
 	if (IS_FWI2_CAPABLE(ha)) {
+		int rval;
+
 		stats = dma_alloc_coherent(&ha->pdev->dev,
 		    sizeof(*stats), &stats_dma, GFP_KERNEL);
 		if (!stats) {

drivers/scsi/qla2xxx/qla_attr.c qla2x00_reset_host_stats(struct Scsi_Host *shost) · line 2882

Quoted 9 lines

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

@@ -2873,7 +2875,8 @@ qla2x00_reset_host_stats(struct Scsi_Host *shost)
 		}
 
 		/* reset firmware statistics */
-		qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		rval = qla24xx_get_isp_stats(base_vha, stats, stats_dma, BIT_0);
+		WARN_ONCE(rval != QLA_SUCCESS, "rval = %d\n", rval);
 
 		dma_free_coherent(&ha->pdev->dev, sizeof(*stats),
 		    stats, stats_dma);

Looks Good.