* [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13
@ 2021-03-16 3:56 Bart Van Assche
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
` (6 more replies)
0 siblings, 7 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley; +Cc: linux-scsi, 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.
Bart Van Assche (7):
Revert "qla2xxx: Make sure that aborted commands are freed"
qla2xxx: Constify struct qla_tgt_func_tmpl
qla2xxx: Fix endianness annotations
qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it
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 | 5 ++++-
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_nx.c | 2 +-
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 +-----
11 files changed, 29 insertions(+), 39 deletions(-)
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
2021-03-16 16:25 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl Bart Van Assche
` (5 subsequent siblings)
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
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 <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_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;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
2021-03-16 16:23 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 3/7] qla2xxx: Fix endianness annotations Bart Van Assche
` (4 subsequent siblings)
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
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>
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_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,
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/7] qla2xxx: Fix endianness annotations
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
2021-03-16 3:56 ` [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:32 ` Daniel Wagner
2021-03-16 16:27 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it Bart Van Assche
` (3 subsequent siblings)
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
Fix all recently introduced endianness annotation issues.
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_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;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
` (2 preceding siblings ...)
2021-03-16 3:56 ` [PATCH 3/7] qla2xxx: Fix endianness annotations Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:36 ` Daniel Wagner
2021-03-16 16:27 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r* Bart Van Assche
` (2 subsequent siblings)
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
This patch suppresses the following sparse warning:
qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.
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_nx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index 0677295957bc..5683126e0cbc 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -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 {
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r*
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
` (3 preceding siblings ...)
2021-03-16 3:56 ` [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:37 ` Daniel Wagner
2021-03-16 16:28 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control() Bart Van Assche
2021-03-16 3:56 ` [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats() Bart Van Assche
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
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>
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_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;
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control()
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
` (4 preceding siblings ...)
2021-03-16 3:56 ` [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r* Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:45 ` Daniel Wagner
2021-03-16 16:29 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats() Bart Van Assche
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
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>
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_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;
}
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
` (5 preceding siblings ...)
2021-03-16 3:56 ` [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control() Bart Van Assche
@ 2021-03-16 3:56 ` Bart Van Assche
2021-03-16 8:48 ` Daniel Wagner
2021-03-16 16:30 ` Himanshu Madhani
6 siblings, 2 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 3:56 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Quinn Tran, Mike Christie,
Himanshu Madhani, Daniel Wagner
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 | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index 63391c9be05d..ad57111f8cb9 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,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);
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
@ 2021-03-16 8:25 ` Daniel Wagner
2021-03-16 20:36 ` Bart Van Assche
2021-03-16 16:25 ` Himanshu Madhani
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:25 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:49PM -0700, Bart Van Assche wrote:
> 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl
2021-03-16 3:56 ` [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl Bart Van Assche
@ 2021-03-16 8:25 ` Daniel Wagner
2021-03-16 16:23 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:25 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:50PM -0700, 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>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/7] qla2xxx: Fix endianness annotations
2021-03-16 3:56 ` [PATCH 3/7] qla2xxx: Fix endianness annotations Bart Van Assche
@ 2021-03-16 8:32 ` Daniel Wagner
2021-03-16 16:27 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:32 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:51PM -0700, Bart Van Assche wrote:
> Fix all recently introduced endianness annotation issues.
>
> 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>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it
2021-03-16 3:56 ` [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it Bart Van Assche
@ 2021-03-16 8:36 ` Daniel Wagner
2021-03-16 20:39 ` Bart Van Assche
2021-03-16 16:27 ` Himanshu Madhani
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:36 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:52PM -0700, Bart Van Assche wrote:
> This patch suppresses the following sparse warning:
[...]
> diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
> index 0677295957bc..5683126e0cbc 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.c
> +++ b/drivers/scsi/qla2xxx/qla_nx.c
> @@ -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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r*
2021-03-16 3:56 ` [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r* Bart Van Assche
@ 2021-03-16 8:37 ` Daniel Wagner
2021-03-16 16:28 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:37 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:53PM -0700, 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>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control()
2021-03-16 3:56 ` [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control() Bart Van Assche
@ 2021-03-16 8:45 ` Daniel Wagner
2021-03-16 16:29 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:45 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:54PM -0700, 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>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()
2021-03-16 3:56 ` [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats() Bart Van Assche
@ 2021-03-16 8:48 ` Daniel Wagner
2021-03-16 20:40 ` Bart Van Assche
2021-03-16 16:30 ` Himanshu Madhani
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Wagner @ 2021-03-16 8:48 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On Mon, Mar 15, 2021 at 08:56:55PM -0700, Bart Van Assche wrote:
> /* 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl
2021-03-16 3:56 ` [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
@ 2021-03-16 16:23 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:23 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> 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>
> 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_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,
Looks Good.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
@ 2021-03-16 16:25 ` Himanshu Madhani
2021-03-16 20:28 ` Bart Van Assche
1 sibling, 1 reply; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:25 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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 <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_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;
Curious …. What triggered this revert? Can you share your motivation for this revert.
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/7] qla2xxx: Fix endianness annotations
2021-03-16 3:56 ` [PATCH 3/7] qla2xxx: Fix endianness annotations Bart Van Assche
2021-03-16 8:32 ` Daniel Wagner
@ 2021-03-16 16:27 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:27 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>
> Fix all recently introduced endianness annotation issues.
>
> 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_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;
Looks Good.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it
2021-03-16 3:56 ` [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it Bart Van Assche
2021-03-16 8:36 ` Daniel Wagner
@ 2021-03-16 16:27 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:27 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch suppresses the following sparse warning:
>
> qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.
>
> 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_nx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
> index 0677295957bc..5683126e0cbc 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.c
> +++ b/drivers/scsi/qla2xxx/qla_nx.c
> @@ -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.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r*
2021-03-16 3:56 ` [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r* Bart Van Assche
2021-03-16 8:37 ` Daniel Wagner
@ 2021-03-16 16:28 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:28 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> 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>
> 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_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;
Looks Good.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control()
2021-03-16 3:56 ` [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control() Bart Van Assche
2021-03-16 8:45 ` Daniel Wagner
@ 2021-03-16 16:29 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:29 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> 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>
> 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_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;
> }
Looks Okay.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()
2021-03-16 3:56 ` [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats() Bart Van Assche
2021-03-16 8:48 ` Daniel Wagner
@ 2021-03-16 16:30 ` Himanshu Madhani
1 sibling, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-16 16:30 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 15, 2021, at 10:56 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>
> 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 | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 63391c9be05d..ad57111f8cb9 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,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.
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
--
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 16:25 ` Himanshu Madhani
@ 2021-03-16 20:28 ` Bart Van Assche
2021-03-17 17:37 ` Himanshu Madhani
0 siblings, 1 reply; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 20:28 UTC (permalink / raw)
To: Himanshu Madhani
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
On 3/16/21 9:25 AM, Himanshu Madhani wrote:
> 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 8:25 ` Daniel Wagner
@ 2021-03-16 20:36 ` Bart Van Assche
2021-03-17 8:56 ` Daniel Wagner
0 siblings, 1 reply; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 20:36 UTC (permalink / raw)
To: Daniel Wagner
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On 3/16/21 1:25 AM, Daniel Wagner wrote:
> On Mon, Mar 15, 2021 at 08:56:49PM -0700, Bart Van Assche wrote:
>> 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it
2021-03-16 8:36 ` Daniel Wagner
@ 2021-03-16 20:39 ` Bart Van Assche
0 siblings, 0 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 20:39 UTC (permalink / raw)
To: Daniel Wagner
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On 3/16/21 1:36 AM, Daniel Wagner wrote:
> On Mon, Mar 15, 2021 at 08:56:52PM -0700, Bart Van Assche wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
>> index 0677295957bc..5683126e0cbc 100644
>> --- a/drivers/scsi/qla2xxx/qla_nx.c
>> +++ b/drivers/scsi/qla2xxx/qla_nx.c
>> @@ -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.
> .
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats()
2021-03-16 8:48 ` Daniel Wagner
@ 2021-03-16 20:40 ` Bart Van Assche
0 siblings, 0 replies; 28+ messages in thread
From: Bart Van Assche @ 2021-03-16 20:40 UTC (permalink / raw)
To: Daniel Wagner
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
On 3/16/21 1:48 AM, Daniel Wagner wrote:
> On Mon, Mar 15, 2021 at 08:56:55PM -0700, Bart Van Assche wrote:
>> /* 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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 20:36 ` Bart Van Assche
@ 2021-03-17 8:56 ` Daniel Wagner
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Wagner @ 2021-03-17 8:56 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Quinn Tran, Mike Christie, Himanshu Madhani
Hi Bart,
On Tue, Mar 16, 2021 at 01:36:34PM -0700, Bart Van Assche wrote:
> 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 :)
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed"
2021-03-16 20:28 ` Bart Van Assche
@ 2021-03-17 17:37 ` Himanshu Madhani
0 siblings, 0 replies; 28+ messages in thread
From: Himanshu Madhani @ 2021-03-17 17:37 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin Petersen, James E . J . Bottomley, linux-scsi, Quinn Tran,
Michael Christie, Daniel Wagner
> On Mar 16, 2021, at 3:28 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 3/16/21 9:25 AM, Himanshu Madhani wrote:
>> 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.
Thanks, once you add more information to the patch, you can add my R-B
—
Himanshu Madhani Oracle Linux Engineering
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-03-17 17:38 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-16 3:56 [PATCH 0/7] qla2xxx patches for kernel v5.12 and v5.13 Bart Van Assche
2021-03-16 3:56 ` [PATCH 1/7] Revert "qla2xxx: Make sure that aborted commands are freed" Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
2021-03-16 20:36 ` Bart Van Assche
2021-03-17 8:56 ` Daniel Wagner
2021-03-16 16:25 ` Himanshu Madhani
2021-03-16 20:28 ` Bart Van Assche
2021-03-17 17:37 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 2/7] qla2xxx: Constify struct qla_tgt_func_tmpl Bart Van Assche
2021-03-16 8:25 ` Daniel Wagner
2021-03-16 16:23 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 3/7] qla2xxx: Fix endianness annotations Bart Van Assche
2021-03-16 8:32 ` Daniel Wagner
2021-03-16 16:27 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 4/7] qla2xxx: qla82xx_pinit_from_rom(): Initialize 'n' before using it Bart Van Assche
2021-03-16 8:36 ` Daniel Wagner
2021-03-16 20:39 ` Bart Van Assche
2021-03-16 16:27 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 5/7] qla2xxx: Suppress Coverity complaints about dseg_r* Bart Van Assche
2021-03-16 8:37 ` Daniel Wagner
2021-03-16 16:28 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 6/7] qla2xxx: Simplify qla8044_minidump_process_control() Bart Van Assche
2021-03-16 8:45 ` Daniel Wagner
2021-03-16 16:29 ` Himanshu Madhani
2021-03-16 3:56 ` [PATCH 7/7] qla2xxx: Always check the return value of qla24xx_get_isp_stats() Bart Van Assche
2021-03-16 8:48 ` Daniel Wagner
2021-03-16 20:40 ` Bart Van Assche
2021-03-16 16:30 ` Himanshu Madhani
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.