[PATCH v2 0/6] qla2xxx patches for kernel v5.12 and v5.13

[PATCH v2 0/6] qla2xxx patches for kernel v5.12 and v5.13

[Bart Van Assche]

Hi Martin,

Please consider the first patch in this series for kernel v5.12 and the
remaining patches for kernel v5.13.

Thanks,

Bart.

Changes compared to v1:
- Improved the description of patch 1/6.
- Dropped one patch that is already upstream.

Bart Van Assche (6):
  Revert "qla2xxx: Make sure that aborted commands are freed"
  qla2xxx: Constify struct qla_tgt_func_tmpl
  qla2xxx: Fix endianness annotations
  qla2xxx: Suppress Coverity complaints about dseg_r*
  qla2xxx: Simplify qla8044_minidump_process_control()
  qla2xxx: Always check the return value of qla24xx_get_isp_stats()

 drivers/scsi/qla2xxx/qla_attr.c    |  6 +++++-
 drivers/scsi/qla2xxx/qla_def.h     |  4 ++--
 drivers/scsi/qla2xxx/qla_iocb.c    |  3 ++-
 drivers/scsi/qla2xxx/qla_isr.c     |  2 +-
 drivers/scsi/qla2xxx/qla_mr.c      | 12 ++++++------
 drivers/scsi/qla2xxx/qla_mr.h      |  4 ++--
 drivers/scsi/qla2xxx/qla_nx2.c     |  8 --------
 drivers/scsi/qla2xxx/qla_sup.c     |  9 +++++----
 drivers/scsi/qla2xxx/qla_target.c  | 13 +++++--------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  6 +-----
 10 files changed, 29 insertions(+), 38 deletions(-)

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

[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(). 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.

Fix this crash by reverting the patch that introduced it.

Fixes: 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed")
Cc: Quinn Tran <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_target.c  | 13 +++++--------
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 ----
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index d6366a46283e..5e8b2653e134 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -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);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 30959f8da065..15650a0bde09 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -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;

[PATCH v2 2/6] qla2xxx: Constify struct qla_tgt_func_tmpl

[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 <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_def.h     | 2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 49b42b430df4..3bdf55bb0833 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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;
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 15650a0bde09..46111f031be9 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -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,

Re: [PATCH v2 2/6] qla2xxx: Constify struct qla_tgt_func_tmpl

[Lee Duncan]

On 3/17/21 8:28 PM, Bart Van Assche wrote:
> Since the target function pointers are not modified at runtime, declare
> the data structure with the target function pointers const.
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_def.h     | 2 +-
>  drivers/scsi/qla2xxx/tcm_qla2xxx.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 49b42b430df4..3bdf55bb0833 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -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;
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 15650a0bde09..46111f031be9 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -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,
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>

[PATCH v2 3/6] qla2xxx: Fix endianness annotations

[Bart Van Assche]

Fix all recently introduced endianness annotation issues.

Cc: Quinn Tran <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 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(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 3bdf55bb0833..52ba75591f9a 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -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;
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 8b41cbaf8535..eb2376b138c1 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -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);
diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index 27165abda96d..0fa7082f3cc8 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -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)
diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
index f771fabcba59..060c89237777 100644
--- a/drivers/scsi/qla2xxx/qla_sup.c
+++ b/drivers/scsi/qla2xxx/qla_sup.c
@@ -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;

Re: [PATCH v2 3/6] qla2xxx: Fix endianness annotations

[Lee Duncan]

On 3/17/21 8:28 PM, Bart Van Assche wrote:
> Fix all recently introduced endianness annotation issues.
> 
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  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(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 3bdf55bb0833..52ba75591f9a 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -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;
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 8b41cbaf8535..eb2376b138c1 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -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);
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 27165abda96d..0fa7082f3cc8 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -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)
> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index f771fabcba59..060c89237777 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
> @@ -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;
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>

[PATCH v2 4/6] qla2xxx: Suppress Coverity complaints about dseg_r*

[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 <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_mr.c | 12 ++++++------
 drivers/scsi/qla2xxx/qla_mr.h |  4 ++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index ca7306685325..61eaf6c4ac47 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -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,
diff --git a/drivers/scsi/qla2xxx/qla_mr.h b/drivers/scsi/qla2xxx/qla_mr.h
index 73be8348402a..eefbae9d7547 100644
--- a/drivers/scsi/qla2xxx/qla_mr.h
+++ b/drivers/scsi/qla2xxx/qla_mr.h
@@ -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;

Re: [PATCH v2 4/6] qla2xxx: Suppress Coverity complaints about dseg_r*

[Lee Duncan]

On 3/17/21 8:28 PM, Bart Van Assche wrote:
> 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 <qutran@marvell.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_mr.c | 12 ++++++------
>  drivers/scsi/qla2xxx/qla_mr.h |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
> index ca7306685325..61eaf6c4ac47 100644
> --- a/drivers/scsi/qla2xxx/qla_mr.c
> +++ b/drivers/scsi/qla2xxx/qla_mr.c
> @@ -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,
> diff --git a/drivers/scsi/qla2xxx/qla_mr.h b/drivers/scsi/qla2xxx/qla_mr.h
> index 73be8348402a..eefbae9d7547 100644
> --- a/drivers/scsi/qla2xxx/qla_mr.h
> +++ b/drivers/scsi/qla2xxx/qla_mr.h
> @@ -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;
> 

I dislike such an uncommented change just to keep a tool happy. Could
you add a comment in qla_mr.h saying why these are one-element-long
arrays, just so nobody optimizes those out later?

If you do that, please add my:

Reviewed-by: Lee Duncan <lduncan@suse.com>
-- 
Lee Duncan

Re: [PATCH v2 4/6] qla2xxx: Suppress Coverity complaints about dseg_r*

[Bart Van Assche]

On 3/18/21 9:00 AM, Lee Duncan wrote:
> I dislike such an uncommented change just to keep a tool happy. Could
> you add a comment in qla_mr.h saying why these are one-element-long
> arrays, just so nobody optimizes those out later?
I will add a comment. Thanks for the feedback.

Bart.

[PATCH v2 5/6] qla2xxx: Simplify qla8044_minidump_process_control()

[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 <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_nx2.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c
index 68a16c95dcb7..792c55fcec8c 100644
--- a/drivers/scsi/qla2xxx/qla_nx2.c
+++ b/drivers/scsi/qla2xxx/qla_nx2.c
@@ -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;
 	}

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

[Lee Duncan]

On 3/17/21 8:28 PM, Bart Van Assche wrote:
> 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 <qutran@marvell.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_nx2.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nx2.c b/drivers/scsi/qla2xxx/qla_nx2.c
> index 68a16c95dcb7..792c55fcec8c 100644
> --- a/drivers/scsi/qla2xxx/qla_nx2.c
> +++ b/drivers/scsi/qla2xxx/qla_nx2.c
> @@ -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;
>  	}
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>

[PATCH v2 6/6] qla2xxx: Always check the return value of qla24xx_get_isp_stats()

[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 <qutran@marvell.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_attr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index 63391c9be05d..5813b17f1633 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -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,9 @@ 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);
+		ql_log(ql_log_warn, vha, 0x70d9,
+		       "Resetting ISP statistics failed: rval = %d\n", rval);
 
 		dma_free_coherent(&ha->pdev->dev, sizeof(*stats),
 		    stats, stats_dma);

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

[Daniel Wagner]

On Wed, Mar 17, 2021 at 08:28:40PM -0700, Bart Van Assche wrote:
> @@ -2873,7 +2875,9 @@ 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);
> +		ql_log(ql_log_warn, vha, 0x70d9,
> +		       "Resetting ISP statistics failed: rval = %d\n", rval);

Is there not an 'if' missing?

	if (rval)
		ql_log(...)

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

[Bart Van Assche]

On 3/18/21 1:08 AM, Daniel Wagner wrote:
> On Wed, Mar 17, 2021 at 08:28:40PM -0700, Bart Van Assche wrote:
>> @@ -2873,7 +2875,9 @@ 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);
>> +		ql_log(ql_log_warn, vha, 0x70d9,
>> +		       "Resetting ISP statistics failed: rval = %d\n", rval);
> 
> Is there not an 'if' missing?
> 
> 	if (rval)
> 		ql_log(...)

Right, I will add an if-statement.

Thanks,

Bart.