[PATCH 0/9] qla2xxx patches for kernel v5.9

[PATCH 0/9] qla2xxx patches for kernel v5.9

[Bart Van Assche]

Hi Martin,

Please consider these patches for kernel v5.9.

Thanks,

Bart.

Bart Van Assche (9):
  qla2xxx: Check the size of struct fcp_hdr at compile time
  qla2xxx: Remove the __packed annotation from struct fcp_hdr and
    fcp_hdr_le
  qla2xxx: Make qla82xx_flash_wait_write_finish() easier to read
  qla2xxx: Initialize 'n' before using it
  qla2xxx: Remove several superfluous casts
  qla2xxx: Make __qla2x00_alloc_iocbs() initialize 32 bits of
    request_t.handle
  qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()
  qla2xxx: Make qla2x00_restart_isp() easier to read
  qla2xxx: Introduce a function for computing the debug message prefix

 drivers/scsi/qla2xxx/qla_bsg.c     |  3 +-
 drivers/scsi/qla2xxx/qla_dbg.c     | 98 +++++++++++-------------------
 drivers/scsi/qla2xxx/qla_init.c    | 39 ++++++------
 drivers/scsi/qla2xxx/qla_iocb.c    |  4 +-
 drivers/scsi/qla2xxx/qla_mr.c      |  3 +-
 drivers/scsi/qla2xxx/qla_nx.c      | 20 +++---
 drivers/scsi/qla2xxx/qla_target.h  |  4 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  1 +
 8 files changed, 71 insertions(+), 101 deletions(-)

[PATCH 1/9] qla2xxx: Check the size of struct fcp_hdr at compile time

[Bart Van Assche]

Since struct fcp_hdr is used to exchange data with the firmware, check its
size at compile time.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 188aa5f02c01..f7e9b5bc0b26 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1971,6 +1971,7 @@ static int __init tcm_qla2xxx_init(void)
 	BUILD_BUG_ON(sizeof(struct ctio_crc2_to_fw) != 64);
 	BUILD_BUG_ON(sizeof(struct ctio_crc_from_fw) != 64);
 	BUILD_BUG_ON(sizeof(struct ctio_to_2xxx) != 64);
+	BUILD_BUG_ON(sizeof(struct fcp_hdr) != 24);
 	BUILD_BUG_ON(sizeof(struct fcp_hdr_le) != 24);
 	BUILD_BUG_ON(sizeof(struct nack_to_isp) != 64);
 

[PATCH 2/9] qla2xxx: Remove the __packed annotation from struct fcp_hdr and fcp_hdr_le

[Bart Van Assche]

Remove the __packed annotation from struct fcp_hdr* because that annotation
is not necessary for these data structures.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_target.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index 010f12523b2a..1cff7c69d448 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -258,7 +258,7 @@ struct fcp_hdr {
 	__be16   ox_id;
 	uint16_t rx_id;
 	__le32	parameter;
-} __packed;
+};
 
 struct fcp_hdr_le {
 	le_id_t  d_id;
@@ -273,7 +273,7 @@ struct fcp_hdr_le {
 	__le16	rx_id;
 	__le16	ox_id;
 	__le32	parameter;
-} __packed;
+};
 
 #define F_CTL_EXCH_CONTEXT_RESP	BIT_23
 #define F_CTL_SEQ_CONTEXT_RESIP	BIT_22

[PATCH 3/9] qla2xxx: Make qla82xx_flash_wait_write_finish() easier to read

[Bart Van Assche]

Return early instead of having a single return statement at the end of this
function. This patch fixes the following sparse warning:

qla_nx.c:1018: qla82xx_flash_wait_write_finish() error: uninitialized symbol 'val'.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_nx.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index 0baf55b7e88f..ff365b434a02 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -966,26 +966,21 @@ qla82xx_read_status_reg(struct qla_hw_data *ha, uint32_t *val)
 static int
 qla82xx_flash_wait_write_finish(struct qla_hw_data *ha)
 {
-	long timeout = 0;
-	uint32_t done = 1 ;
 	uint32_t val;
-	int ret = 0;
+	int i, ret;
 	scsi_qla_host_t *vha = pci_get_drvdata(ha->pdev);
 
 	qla82xx_wr_32(ha, QLA82XX_ROMUSB_ROM_ABYTE_CNT, 0);
-	while ((done != 0) && (ret == 0)) {
+	for (i = 0; i < 50000; i++) {
 		ret = qla82xx_read_status_reg(ha, &val);
-		done = val & 1;
-		timeout++;
+		if (ret < 0 || (val & 1) == 0)
+			return ret;
 		udelay(10);
 		cond_resched();
-		if (timeout >= 50000) {
-			ql_log(ql_log_warn, vha, 0xb00d,
-			    "Timeout reached waiting for write finish.\n");
-			return -1;
-		}
 	}
-	return ret;
+	ql_log(ql_log_warn, vha, 0xb00d,
+	       "Timeout reached waiting for write finish.\n");
+	return -1;
 }
 
 static int

[PATCH 4/9] qla2xxx: Initialize 'n' before using it

[Bart Van Assche]

The following code:
	qla82xx_rom_fast_read(ha, 0, &n)
only initializes 'n' if it succeeds. Since 'n' may be reported in a debug
message even if no ROM reads succeeded, initialize 'n' to zero.

This patch fixes the following sparse warning:

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

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_nx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
index ff365b434a02..71273eb634d3 100644
--- a/drivers/scsi/qla2xxx/qla_nx.c
+++ b/drivers/scsi/qla2xxx/qla_nx.c
@@ -1167,6 +1167,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
 	 * 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,

[PATCH 5/9] qla2xxx: Remove a superfluous cast

[Bart Van Assche]

Remove an unnecessary cast because it prevents the compiler to perform
type checking.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_bsg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
index 88c0338a2ec7..67efde1d4b8e 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -223,8 +223,7 @@ qla24xx_proc_fcp_prio_cfg_cmd(struct bsg_job *bsg_job)
 
 		/* validate fcp priority data */
 
-		if (!qla24xx_fcp_prio_cfg_valid(vha,
-		    (struct qla_fcp_prio_cfg *) ha->fcp_prio_cfg, 1)) {
+		if (!qla24xx_fcp_prio_cfg_valid(vha, ha->fcp_prio_cfg, 1)) {
 			bsg_reply->result = (DID_ERROR << 16);
 			ret = -EINVAL;
 			/* If buffer was invalidatic int

[PATCH 6/9] qla2xxx: Make __qla2x00_alloc_iocbs() initialize 32 bits of request_t.handle

[Bart Van Assche]

The request_t 'handle' member is 32-bits wide, hence use wrt_reg_dword().
Change the cast in the wrt_reg_byte() call to make it clear that a
regular pointer is casted to an __iomem pointer.

Note: 'pkt' points to I/O memory for the qlafx00 adapter family and to
coherent memory for all other adapter families.

This patch fixes the following Coverity complaint:

CID 358864 (#1 of 1): Reliance on integer endianness (INCOMPATIBLE_CAST)
incompatible_cast: Pointer &pkt->handle points to an object whose effective
type is unsigned int (32 bits, unsigned) but is dereferenced as a narrower
unsigned short (16 bits, unsigned). This may lead to unexpected results
depending on machine endianness.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Fixes: 8ae6d9c7eb10 ("[SCSI] qla2xxx: Enhancements to support ISPFx00.")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_iocb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index 8865c35d3421..7c2ad8c18398 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2305,8 +2305,8 @@ __qla2x00_alloc_iocbs(struct qla_qpair *qpair, srb_t *sp)
 	pkt = req->ring_ptr;
 	memset(pkt, 0, REQUEST_ENTRY_SIZE);
 	if (IS_QLAFX00(ha)) {
-		wrt_reg_byte((void __iomem *)&pkt->entry_count, req_cnt);
-		wrt_reg_word((void __iomem *)&pkt->handle, handle);
+		wrt_reg_byte((u8 __force __iomem *)&pkt->entry_count, req_cnt);
+		wrt_reg_dword((__le32 __force __iomem *)&pkt->handle, handle);
 	} else {
 		pkt->entry_count = req_cnt;
 		pkt->handle = handle;

[PATCH 7/9] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()

[Bart Van Assche]

'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity
complains by rewriting an address calculation expression. This patch fixes
the following Coverity complaint:

CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN)
109. overrun-local: Overrunning array of 122880 bytes at byte offset 122880
by dereferencing pointer &fw->risc_ram[cnt].

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_dbg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 2ed0b849fbfe..5e873b70e843 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha)
 	}
 
 	if (rval == QLA_SUCCESS)
-		qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]);
+		qla2xxx_copy_queues(ha, (char *)fw +
+				    offsetof(typeof(*fw), risc_ram) + cnt);
 
 	qla2xxx_dump_post_process(base_vha, rval);
 }

[PATCH 8/9] qla2xxx: Make qla2x00_restart_isp() easier to read

[Bart Van Assche]

Instead of using complicated control flow to only have one return statement
at the end of qla2x00_restart_isp(), return an error status as soon as it is
known that this function will fail.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_init.c | 39 +++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 9e12018e0105..f39a1a40d4c3 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -6998,36 +6998,41 @@ qla2x00_abort_isp(scsi_qla_host_t *vha)
 static int
 qla2x00_restart_isp(scsi_qla_host_t *vha)
 {
-	int status = 0;
+	int status;
 	struct qla_hw_data *ha = vha->hw;
 
 	/* If firmware needs to be loaded */
 	if (qla2x00_isp_firmware(vha)) {
 		vha->flags.online = 0;
 		status = ha->isp_ops->chip_diag(vha);
-		if (!status)
-			status = qla2x00_setup_chip(vha);
+		if (status)
+			return status;
+		status = qla2x00_setup_chip(vha);
+		if (status)
+			return status;
 	}
 
-	if (!status && !(status = qla2x00_init_rings(vha))) {
-		clear_bit(RESET_MARKER_NEEDED, &vha->dpc_flags);
-		ha->flags.chip_reset_done = 1;
+	status = qla2x00_init_rings(vha);
+	if (status)
+		return status;
 
-		/* Initialize the queues in use */
-		qla25xx_init_queues(ha);
+	clear_bit(RESET_MARKER_NEEDED, &vha->dpc_flags);
+	ha->flags.chip_reset_done = 1;
 
-		status = qla2x00_fw_ready(vha);
-		if (!status) {
-			/* Issue a marker after FW becomes ready. */
-			qla2x00_marker(vha, ha->base_qpair, 0, 0, MK_SYNC_ALL);
-			set_bit(LOOP_RESYNC_NEEDED, &vha->dpc_flags);
-		}
+	/* Initialize the queues in use */
+	qla25xx_init_queues(ha);
 
+	status = qla2x00_fw_ready(vha);
+	if (status) {
 		/* if no cable then assume it's good */
-		if ((vha->device_flags & DFLG_NO_CABLE))
-			status = 0;
+		return vha->device_flags & DFLG_NO_CABLE ? 0 : status;
 	}
-	return (status);
+
+	/* Issue a marker after FW becomes ready. */
+	qla2x00_marker(vha, ha->base_qpair, 0, 0, MK_SYNC_ALL);
+	set_bit(LOOP_RESYNC_NEEDED, &vha->dpc_flags);
+
+	return 0;
 }
 
 static int

[PATCH 9/9] qla2xxx: Introduce a function for computing the debug message prefix

[Bart Van Assche]

Instead of repeating the code for generating a debug message prefix
six times, introduce a function for computing the debug message prefix.

Cc: Nilesh Javali <njavali@marvell.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
Cc: Daniel Wagner <dwagner@suse.de>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_dbg.c | 95 ++++++++++++----------------------
 1 file changed, 32 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 5e873b70e843..44f4d3f23a8e 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -2448,6 +2448,23 @@ qla83xx_fw_dump(scsi_qla_host_t *vha)
 /*                         Driver Debug Functions.                          */
 /****************************************************************************/
 
+/* Write the debug message prefix into @pbuf. */
+static void ql_dbg_prefix(char *pbuf, int pbuf_size,
+			  const scsi_qla_host_t *vha, uint msg_id)
+{
+	if (vha) {
+		const struct pci_dev *pdev = vha->hw->pdev;
+
+		/* <module-name> [<dev-name>]-<msg-id>:<host>: */
+		snprintf(pbuf, pbuf_size, "%s [%s]-%04x:%ld: ", QL_MSGHDR,
+			 dev_name(&(pdev->dev)), msg_id, vha->host_no);
+	} else {
+		/* <module-name> [<dev-name>]-<msg-id>: : */
+		snprintf(pbuf, pbuf_size, "%s [%s]-%04x: : ", QL_MSGHDR,
+			 "0000:00:00.0", msg_id);
+	}
+}
+
 /*
  * This function is for formatting and logging debug information.
  * It is to be used when vha is available. It formats the message
@@ -2473,33 +2490,12 @@ ql_dbg(uint level, scsi_qla_host_t *vha, uint id, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &va;
 
-	if (!ql_mask_match(level)) {
-		if (vha != NULL) {
-			const struct pci_dev *pdev = vha->hw->pdev;
-			/* <module-name> <msg-id>:<host> Message */
-			snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x:%ld: ",
-			    QL_MSGHDR, dev_name(&(pdev->dev)), id,
-			    vha->host_no);
-		} else {
-			snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x: : ",
-			    QL_MSGHDR, "0000:00:00.0", id);
-		}
-		pbuf[sizeof(pbuf) - 1] = 0;
-		trace_ql_dbg_log(pbuf, &vaf);
-		va_end(va);
-		return;
-	}
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), vha, id);
 
-	if (vha != NULL) {
-		const struct pci_dev *pdev = vha->hw->pdev;
-		/* <module-name> <pci-name> <msg-id>:<host> Message */
-		pr_warn("%s [%s]-%04x:%ld: %pV",
-			QL_MSGHDR, dev_name(&(pdev->dev)), id + ql_dbg_offset,
-			vha->host_no, &vaf);
-	} else {
-		pr_warn("%s [%s]-%04x: : %pV",
-			QL_MSGHDR, "0000:00:00.0", id + ql_dbg_offset, &vaf);
-	}
+	if (!ql_mask_match(level))
+		trace_ql_dbg_log(pbuf, &vaf);
+	else
+		pr_warn("%s%pV", pbuf, &vaf);
 
 	va_end(va);
 
@@ -2524,6 +2520,7 @@ ql_dbg_pci(uint level, struct pci_dev *pdev, uint id, const char *fmt, ...)
 {
 	va_list va;
 	struct va_format vaf;
+	char pbuf[128];
 
 	if (pdev == NULL)
 		return;
@@ -2535,9 +2532,8 @@ ql_dbg_pci(uint level, struct pci_dev *pdev, uint id, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &va;
 
-	/* <module-name> <dev-name>:<msg-id> Message */
-	pr_warn("%s [%s]-%04x: : %pV",
-		QL_MSGHDR, dev_name(&(pdev->dev)), id + ql_dbg_offset, &vaf);
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), NULL, id + ql_dbg_offset);
+	pr_warn("%s%pV", pbuf, &vaf);
 
 	va_end(va);
 }
@@ -2565,16 +2561,7 @@ ql_log(uint level, scsi_qla_host_t *vha, uint id, const char *fmt, ...)
 	if (level > ql_errlev)
 		return;
 
-	if (vha != NULL) {
-		const struct pci_dev *pdev = vha->hw->pdev;
-		/* <module-name> <msg-id>:<host> Message */
-		snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x:%ld: ",
-			QL_MSGHDR, dev_name(&(pdev->dev)), id, vha->host_no);
-	} else {
-		snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x: : ",
-			QL_MSGHDR, "0000:00:00.0", id);
-	}
-	pbuf[sizeof(pbuf) - 1] = 0;
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), vha, id);
 
 	va_start(va, fmt);
 
@@ -2625,10 +2612,7 @@ ql_log_pci(uint level, struct pci_dev *pdev, uint id, const char *fmt, ...)
 	if (level > ql_errlev)
 		return;
 
-	/* <module-name> <dev-name>:<msg-id> Message */
-	snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x: : ",
-		 QL_MSGHDR, dev_name(&(pdev->dev)), id);
-	pbuf[sizeof(pbuf) - 1] = 0;
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), NULL, id);
 
 	va_start(va, fmt);
 
@@ -2724,16 +2708,7 @@ ql_log_qp(uint32_t level, struct qla_qpair *qpair, int32_t id,
 	if (level > ql_errlev)
 		return;
 
-	if (qpair != NULL) {
-		const struct pci_dev *pdev = qpair->pdev;
-		/* <module-name> <msg-id>:<host> Message */
-		snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x: ",
-			QL_MSGHDR, dev_name(&(pdev->dev)), id);
-	} else {
-		snprintf(pbuf, sizeof(pbuf), "%s [%s]-%04x: : ",
-			QL_MSGHDR, "0000:00:00.0", id);
-	}
-	pbuf[sizeof(pbuf) - 1] = 0;
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), qpair ? qpair->vha : NULL, id);
 
 	va_start(va, fmt);
 
@@ -2777,6 +2752,7 @@ ql_dbg_qp(uint32_t level, struct qla_qpair *qpair, int32_t id,
 {
 	va_list va;
 	struct va_format vaf;
+	char pbuf[128];
 
 	if (!ql_mask_match(level))
 		return;
@@ -2786,16 +2762,9 @@ ql_dbg_qp(uint32_t level, struct qla_qpair *qpair, int32_t id,
 	vaf.fmt = fmt;
 	vaf.va = &va;
 
-	if (qpair != NULL) {
-		const struct pci_dev *pdev = qpair->pdev;
-		/* <module-name> <pci-name> <msg-id>:<host> Message */
-		pr_warn("%s [%s]-%04x: %pV",
-		    QL_MSGHDR, dev_name(&(pdev->dev)), id + ql_dbg_offset,
-		    &vaf);
-	} else {
-		pr_warn("%s [%s]-%04x: : %pV",
-			QL_MSGHDR, "0000:00:00.0", id + ql_dbg_offset, &vaf);
-	}
+	ql_dbg_prefix(pbuf, ARRAY_SIZE(pbuf), qpair ? qpair->vha : NULL,
+		      id + ql_dbg_offset);
+	pr_warn("%s%pV", pbuf, &vaf);
 
 	va_end(va);
 

Re: [PATCH 0/9] qla2xxx patches for kernel v5.9

[Himanshu Madhani]

> On Jun 14, 2020, at 5:39 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> Hi Martin,
> 
> Please consider these patches for kernel v5.9.
> 
> Thanks,
> 
> Bart.
> 
> Bart Van Assche (9):
>  qla2xxx: Check the size of struct fcp_hdr at compile time
>  qla2xxx: Remove the __packed annotation from struct fcp_hdr and
>    fcp_hdr_le
>  qla2xxx: Make qla82xx_flash_wait_write_finish() easier to read
>  qla2xxx: Initialize 'n' before using it
>  qla2xxx: Remove several superfluous casts
>  qla2xxx: Make __qla2x00_alloc_iocbs() initialize 32 bits of
>    request_t.handle
>  qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()
>  qla2xxx: Make qla2x00_restart_isp() easier to read
>  qla2xxx: Introduce a function for computing the debug message prefix
> 
> drivers/scsi/qla2xxx/qla_bsg.c     |  3 +-
> drivers/scsi/qla2xxx/qla_dbg.c     | 98 +++++++++++-------------------
> drivers/scsi/qla2xxx/qla_init.c    | 39 ++++++------
> drivers/scsi/qla2xxx/qla_iocb.c    |  4 +-
> drivers/scsi/qla2xxx/qla_mr.c      |  3 +-
> drivers/scsi/qla2xxx/qla_nx.c      | 20 +++---
> drivers/scsi/qla2xxx/qla_target.h  |  4 +-
> drivers/scsi/qla2xxx/tcm_qla2xxx.c |  1 +
> 8 files changed, 71 insertions(+), 101 deletions(-)
> 

Looks Good.

For the series Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	Oracle Linux Engineering

Re: [PATCH 1/9] qla2xxx: Check the size of struct fcp_hdr at compile time

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:13PM -0700, Bart Van Assche wrote:
> Since struct fcp_hdr is used to exchange data with the firmware, check its
> size at compile time.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 2/9] qla2xxx: Remove the __packed annotation from struct fcp_hdr and fcp_hdr_le

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:14PM -0700, Bart Van Assche wrote:
> Remove the __packed annotation from struct fcp_hdr* because that annotation
> is not necessary for these data structures.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 3/9] qla2xxx: Make qla82xx_flash_wait_write_finish() easier to read

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:15PM -0700, Bart Van Assche wrote:
> Return early instead of having a single return statement at the end of this
> function. This patch fixes the following sparse warning:
> 
> qla_nx.c:1018: qla82xx_flash_wait_write_finish() error: uninitialized symbol 'val'.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 4/9] qla2xxx: Initialize 'n' before using it

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:16PM -0700, Bart Van Assche wrote:
> The following code:
> 	qla82xx_rom_fast_read(ha, 0, &n)
> only initializes 'n' if it succeeds. Since 'n' may be reported in a debug
> message even if no ROM reads succeeded, initialize 'n' to zero.
> 
> This patch fixes the following sparse warning:
> 
> qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 5/9] qla2xxx: Remove a superfluous cast

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:17PM -0700, Bart Van Assche wrote:
> Remove an unnecessary cast because it prevents the compiler to perform
> type checking.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 6/9] qla2xxx: Make __qla2x00_alloc_iocbs() initialize 32 bits of request_t.handle

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:18PM -0700, Bart Van Assche wrote:
> The request_t 'handle' member is 32-bits wide, hence use wrt_reg_dword().
> Change the cast in the wrt_reg_byte() call to make it clear that a
> regular pointer is casted to an __iomem pointer.
> 
> Note: 'pkt' points to I/O memory for the qlafx00 adapter family and to
> coherent memory for all other adapter families.
> 
> This patch fixes the following Coverity complaint:
> 
> CID 358864 (#1 of 1): Reliance on integer endianness (INCOMPATIBLE_CAST)
> incompatible_cast: Pointer &pkt->handle points to an object whose effective
> type is unsigned int (32 bits, unsigned) but is dereferenced as a narrower
> unsigned short (16 bits, unsigned). This may lead to unexpected results
> depending on machine endianness.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Fixes: 8ae6d9c7eb10 ("[SCSI] qla2xxx: Enhancements to support ISPFx00.")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

> ---
>  drivers/scsi/qla2xxx/qla_iocb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index 8865c35d3421..7c2ad8c18398 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -2305,8 +2305,8 @@ __qla2x00_alloc_iocbs(struct qla_qpair *qpair, srb_t *sp)
>  	pkt = req->ring_ptr;
>  	memset(pkt, 0, REQUEST_ENTRY_SIZE);
>  	if (IS_QLAFX00(ha)) {
> -		wrt_reg_byte((void __iomem *)&pkt->entry_count, req_cnt);
> -		wrt_reg_word((void __iomem *)&pkt->handle, handle);
> +		wrt_reg_byte((u8 __force __iomem *)&pkt->entry_count, req_cnt);
> +		wrt_reg_dword((__le32 __force __iomem *)&pkt->handle, handle);

Makes me wonder how this ever worked.

Re: [PATCH 7/9] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()

[Daniel Wagner]

Hi Bart,

On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote:
> @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha)
>  	}
>  
>  	if (rval == QLA_SUCCESS)
> -		qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]);
> +		qla2xxx_copy_queues(ha, (char *)fw +
> +				    offsetof(typeof(*fw), risc_ram) + cnt);

This looks pretty ugly to me. Any chance to write this in a way it's
understandable by humans and coverity is not annoyed?

Do I understand it correctly, it's valid to read after the end of risc_ram?

Thanks,
Daniel

Re: [PATCH 8/9] qla2xxx: Make qla2x00_restart_isp() easier to read

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:20PM -0700, Bart Van Assche wrote:
> Instead of using complicated control flow to only have one return statement
> at the end of qla2x00_restart_isp(), return an error status as soon as it is
> known that this function will fail.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 9/9] qla2xxx: Introduce a function for computing the debug message prefix

[Daniel Wagner]

On Sun, Jun 14, 2020 at 03:39:21PM -0700, Bart Van Assche wrote:
> Instead of repeating the code for generating a debug message prefix
> six times, introduce a function for computing the debug message prefix.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH 4/9] qla2xxx: Initialize 'n' before using it

[Shyam Sundar]

Looks good. 

Reviewed-by: Shyam Sundar <ssundar@marvell.com>

> On Jun 14, 2020, at 3:39 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> The following code:
> 	qla82xx_rom_fast_read(ha, 0, &n)
> only initializes 'n' if it succeeds. Since 'n' may be reported in a debug
> message even if no ROM reads succeeded, initialize 'n' to zero.
> 
> This patch fixes the following sparse warning:
> 
> qla_nx.c:1218: qla82xx_pinit_from_rom() error: uninitialized symbol 'n'.
> 
> Cc: Nilesh Javali <njavali@marvell.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <himanshu.madhani@oracle.com>
> Cc: Daniel Wagner <dwagner@suse.de>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/scsi/qla2xxx/qla_nx.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
> index ff365b434a02..71273eb634d3 100644
> --- a/drivers/scsi/qla2xxx/qla_nx.c
> +++ b/drivers/scsi/qla2xxx/qla_nx.c
> @@ -1167,6 +1167,7 @@ qla82xx_pinit_from_rom(scsi_qla_host_t *vha)
> 	 * 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,

Re: [PATCH 7/9] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()

[Bart Van Assche]

On 2020-06-23 01:33, Daniel Wagner wrote:
> On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote:
>> @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha)
>>  	}
>>  
>>  	if (rval == QLA_SUCCESS)
>> -		qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]);
>> +		qla2xxx_copy_queues(ha, (char *)fw +
>> +				    offsetof(typeof(*fw), risc_ram) + cnt);
> 
> This looks pretty ugly to me. Any chance to write this in a way it's
> understandable by humans and coverity is not annoyed?
> 
> Do I understand it correctly, it's valid to read after the end of risc_ram?

Doesn't the function qla2xxx_copy_queues() write to the pointer passed
as the second argument instead of reading?

A possible alternative can be found below. The only reason I can think
of why this works is because the qla2100_fw_dump structure is occurs in
a union and because there is probably a larger structure present in the
same union. I would like to specify a size for the queue_dump[] array
but I am not sure where to get that information from.

Subject: [PATCH] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()

'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity
complains by rewriting an address calculation expression. This patch
fixes the following Coverity complaint:

CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN)
109. overrun-local: Overrunning array of 122880 bytes at byte offset
122880 by dereferencing pointer &fw->risc_ram[cnt].

---
 drivers/scsi/qla2xxx/qla_dbg.c | 2 +-
 drivers/scsi/qla2xxx/qla_dbg.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c
index 19005710f7f6..41493bd53fc0 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.c
+++ b/drivers/scsi/qla2xxx/qla_dbg.c
@@ -1063,7 +1063,7 @@ qla2100_fw_dump(scsi_qla_host_t *vha)
 	}

 	if (rval == QLA_SUCCESS)
-		qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]);
+		qla2xxx_copy_queues(ha, &fw->queue_dump[0]);

 	qla2xxx_dump_post_process(base_vha, rval);
 }
diff --git a/drivers/scsi/qla2xxx/qla_dbg.h b/drivers/scsi/qla2xxx/qla_dbg.h
index 54ed020e6f75..91eb6901815c 100644
--- a/drivers/scsi/qla2xxx/qla_dbg.h
+++ b/drivers/scsi/qla2xxx/qla_dbg.h
@@ -53,6 +53,7 @@ struct qla2100_fw_dump {
 	__be16 fpm_b0_reg[64];
 	__be16 fpm_b1_reg[64];
 	__be16 risc_ram[0xf000];
+	u8	queue_dump[];
 };

 struct qla24xx_fw_dump {

Re: [PATCH 7/9] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()

[Daniel Wagner]

Hi Bart,

On Sun, Jun 28, 2020 at 03:31:37PM -0700, Bart Van Assche wrote:
> On 2020-06-23 01:33, Daniel Wagner wrote:
> > On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote:
> >> @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha)
> >>  	}
> >>  
> >>  	if (rval == QLA_SUCCESS)
> >> -		qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]);
> >> +		qla2xxx_copy_queues(ha, (char *)fw +
> >> +				    offsetof(typeof(*fw), risc_ram) + cnt);
> > 
> > This looks pretty ugly to me. Any chance to write this in a way it's
> > understandable by humans and coverity is not annoyed?
> > 
> > Do I understand it correctly, it's valid to read after the end of risc_ram?
> 
> Doesn't the function qla2xxx_copy_queues() write to the pointer passed
> as the second argument instead of reading?

Yes, it seems to copy the request queue and the response queue
to the pointer. Funny, it returns a pointer offset which only uses
the size of the response queue which is used py qla2xxx_copy_eft.
Looking at I would swear qla2xxx_copy_eft overwrites data from
qla2xxx_copy_queues.

> A possible alternative can be found below. The only reason I can think
> of why this works is because the qla2100_fw_dump structure is occurs in
> a union and because there is probably a larger structure present in the
> same union. I would like to specify a size for the queue_dump[] array
> but I am not sure where to get that information from.

Yes, this would be indeed a good thing.

> Subject: [PATCH] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump()
> 
> 'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity
> complains by rewriting an address calculation expression. This patch
> fixes the following Coverity complaint:
> 
> CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN)
> 109. overrun-local: Overrunning array of 122880 bytes at byte offset
> 122880 by dereferencing pointer &fw->risc_ram[cnt].

Much better, now I get it :)

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Thanks,
Daniel