[PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[Himanshu Madhani]

Hi Christoph, Bart,

Here's updated series of bug fixes for target code in the driver.
Please consider this for target-pending.

Changes from v1 --> v2

o Updated patches to remove braces. 
o Added description for the patch reqeusted.

Thanks,
Himanshu

Himanshu Madhani (3):
  qla2xxx: Include ATIO queue in firmware dump when in target mode
  qla2xxx: Set tcm_qla2xxx version to automatically track qla2xxx
    version.
  qla2xxx: Reset reserved field in firmware options to 0.

Quinn Tran (7):
  qla2xxx: Fix wrong IOCB type assumption.
  qla2xxx: Collect additional information to debug fw dump.
  qla2xxx: Fix crash due to null pointer access.
  qla2xxx: Terminate exchange if corrputed.
  qla2xxx: Reduce exess wait during chip reset
  qla2xxx: Fix invalid handle erroneous message.
  qla2xxx: Disable Out-of-order processing by default in Firmware

 drivers/scsi/qla2xxx/qla_def.h     |  3 ++-
 drivers/scsi/qla2xxx/qla_init.c    |  4 +--
 drivers/scsi/qla2xxx/qla_isr.c     |  4 +++
 drivers/scsi/qla2xxx/qla_mbx.c     | 27 ++++++++++++++-----
 drivers/scsi/qla2xxx/qla_os.c      | 23 +++++++++++++---
 drivers/scsi/qla2xxx/qla_target.c  | 55 +++++++++++++++++++++++++-------------
 drivers/scsi/qla2xxx/qla_target.h  | 22 ++++++++++++++-
 drivers/scsi/qla2xxx/qla_tmpl.c    | 24 +++++++++++++++++
 drivers/scsi/qla2xxx/tcm_qla2xxx.c |  4 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.h |  1 -
 10 files changed, 131 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH v2 01/10] qla2xxx: Fix wrong IOCB type assumption.

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

qlt_reset is called with Immedidate Notify IOCB only.
Current code wrongly cast it as ATIO IOCB.

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_target.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index bff9689..b9c559c 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -668,11 +668,9 @@ static int qlt_reset(struct scsi_qla_host *vha, void *iocb, int mcmd)
 {
 	struct qla_hw_data *ha = vha->hw;
 	struct qla_tgt_sess *sess = NULL;
-	uint32_t unpacked_lun, lun = 0;
 	uint16_t loop_id;
 	int res = 0;
 	struct imm_ntfy_from_isp *n = (struct imm_ntfy_from_isp *)iocb;
-	struct atio_from_isp *a = (struct atio_from_isp *)iocb;
 	unsigned long flags;
 
 	loop_id = le16_to_cpu(n->u.isp24.nport_handle);
@@ -725,11 +723,7 @@ static int qlt_reset(struct scsi_qla_host *vha, void *iocb, int mcmd)
 	    "loop_id %d)\n", vha->host_no, sess, sess->port_name,
 	    mcmd, loop_id);
 
-	lun = a->u.isp24.fcp_cmnd.lun;
-	unpacked_lun = scsilun_to_int((struct scsi_lun *)&lun);
-
-	return qlt_issue_task_mgmt(sess, unpacked_lun, mcmd,
-	    iocb, QLA24XX_MGMT_SEND_NACK);
+	return qlt_issue_task_mgmt(sess, 0, mcmd, iocb, QLA24XX_MGMT_SEND_NACK);
 }
 
 /* ha->tgt.sess_lock supposed to be held on entry */
-- 
1.8.3.1

[PATCH v2 02/10] qla2xxx: Include ATIO queue in firmware dump when in target mode

[Himanshu Madhani]

Include ATIO queue for ISP27XX when firmware dump is collected
for target mode.

Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Signed-off-by: Giridhar Malavali <giridhar.malavali@cavium.com>
---
 drivers/scsi/qla2xxx/qla_tmpl.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_tmpl.c b/drivers/scsi/qla2xxx/qla_tmpl.c
index 36935c9..d356ed0 100644
--- a/drivers/scsi/qla2xxx/qla_tmpl.c
+++ b/drivers/scsi/qla2xxx/qla_tmpl.c
@@ -433,6 +433,18 @@ static inline void (*qla27xx_read_vector(uint width))(void __iomem*, void *, ulo
 				count++;
 			}
 		}
+	} else if (QLA_TGT_MODE_ENABLED() &&
+	    ent->t263.queue_type == T263_QUEUE_TYPE_ATIO) {
+		struct qla_hw_data *ha = vha->hw;
+		struct atio *atr = ha->tgt.atio_ring;
+
+		if (atr || !buf) {
+			length = ha->tgt.atio_q_length;
+			qla27xx_insert16(0, buf, len);
+			qla27xx_insert16(length, buf, len);
+			qla27xx_insertbuf(atr, length * sizeof(*atr), buf, len);
+			count++;
+		}
 	} else {
 		ql_dbg(ql_dbg_misc, vha, 0xd026,
 		    "%s: unknown queue %x\n", __func__, ent->t263.queue_type);
@@ -676,6 +688,18 @@ static inline void (*qla27xx_read_vector(uint width))(void __iomem*, void *, ulo
 				count++;
 			}
 		}
+	} else if (QLA_TGT_MODE_ENABLED() &&
+	    ent->t274.queue_type == T274_QUEUE_TYPE_ATIO_SHAD) {
+		struct qla_hw_data *ha = vha->hw;
+		struct atio *atr = ha->tgt.atio_ring_ptr;
+
+		if (atr || !buf) {
+			qla27xx_insert16(0, buf, len);
+			qla27xx_insert16(1, buf, len);
+			qla27xx_insert32(ha->tgt.atio_q_in ?
+			    *ha->tgt.atio_q_in : 0, buf, len);
+			count++;
+		}
 	} else {
 		ql_dbg(ql_dbg_misc, vha, 0xd02f,
 		    "%s: unknown queue %x\n", __func__, ent->t274.queue_type);
-- 
1.8.3.1

[PATCH v2 03/10] qla2xxx: Set tcm_qla2xxx version to automatically track qla2xxx version.

[Himanshu Madhani]

Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Signed-off-by: Giridhar Malavali <giridhar.malavali@cavium.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 4 ++--
 drivers/scsi/qla2xxx/tcm_qla2xxx.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 6643f6f..d925910 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1800,7 +1800,7 @@ static ssize_t tcm_qla2xxx_wwn_version_show(struct config_item *item,
 {
 	return sprintf(page,
 	    "TCM QLOGIC QLA2XXX NPIV capable fabric module %s on %s/%s on "
-	    UTS_RELEASE"\n", TCM_QLA2XXX_VERSION, utsname()->sysname,
+	    UTS_RELEASE"\n", QLA2XXX_VERSION, utsname()->sysname,
 	    utsname()->machine);
 }
 
@@ -1906,7 +1906,7 @@ static int tcm_qla2xxx_register_configfs(void)
 	int ret;
 
 	pr_debug("TCM QLOGIC QLA2XXX fabric module %s on %s/%s on "
-	    UTS_RELEASE"\n", TCM_QLA2XXX_VERSION, utsname()->sysname,
+	    UTS_RELEASE"\n", QLA2XXX_VERSION, utsname()->sysname,
 	    utsname()->machine);
 
 	ret = target_register_template(&tcm_qla2xxx_ops);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.h b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
index 37e026a..cf8430b 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.h
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.h
@@ -1,7 +1,6 @@
 #include <target/target_core_base.h>
 #include <linux/btree.h>
 
-#define TCM_QLA2XXX_VERSION	"v0.1"
 /* length of ASCII WWPNs including pad */
 #define TCM_QLA2XXX_NAMELEN	32
 /*
-- 
1.8.3.1

[PATCH v2 04/10] qla2xxx: Reset reserved field in firmware options to 0.

[Himanshu Madhani]

During NVRAM initialization in target mode, reset reserved
fields in firmware options to Zero (BIT 15)

Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Signed-off-by: Giridhar Malavali <giridhar.malavali@cavium.com>
---
 drivers/scsi/qla2xxx/qla_target.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index b9c559c..5037b51 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -6539,6 +6539,14 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
 
 		/* Disable Full Login after LIP */
 		nv->host_p &= cpu_to_le32(~BIT_10);
+
+		/*
+		 * clear BIT 15 explicitly as we have seen at least
+		 * a couple of instances where this was set and this
+		 * was causing the firmware to not be initialized.
+		 */
+		nv->firmware_options_1 &=
+		    __constant_cpu_to_le32(~BIT_15);
 		/* Enable target PRLI control */
 		nv->firmware_options_2 |= cpu_to_le32(BIT_14);
 	} else {
@@ -6623,11 +6631,18 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
 		/* Disable ini mode, if requested */
 		if (!qla_ini_mode_enabled(vha))
 			nv->firmware_options_1 |= cpu_to_le32(BIT_5);
-
 		/* Disable Full Login after LIP */
 		nv->firmware_options_1 &= cpu_to_le32(~BIT_13);
 		/* Enable initial LIP */
 		nv->firmware_options_1 &= cpu_to_le32(~BIT_9);
+		/*
+		 * clear BIT 15 explicitly as we have seen at
+		 * least a couple of instances where this was set
+		 * and this was causing the firmware to not be
+		 * initialized.
+		 */
+		nv->firmware_options_1 &=
+		    __constant_cpu_to_le32(~BIT_15);
 		if (ql2xtgt_tape_enable)
 			/* Enable FC tape support */
 			nv->firmware_options_2 |= cpu_to_le32(BIT_12);
-- 
1.8.3.1

[PATCH v2 05/10] qla2xxx: Collect additional information to debug fw dump.

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_mbx.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
index 2819ceb..b4386fc 100644
--- a/drivers/scsi/qla2xxx/qla_mbx.c
+++ b/drivers/scsi/qla2xxx/qla_mbx.c
@@ -323,20 +323,33 @@ static int is_rom_cmd(uint16_t cmd)
 		}
 	} else {
 
-		uint16_t mb0;
-		uint32_t ictrl;
+		uint16_t mb[8];
+		uint32_t ictrl, host_status, hccr;
 		uint16_t        w;
 
 		if (IS_FWI2_CAPABLE(ha)) {
-			mb0 = RD_REG_WORD(&reg->isp24.mailbox0);
+			mb[0] = RD_REG_WORD(&reg->isp24.mailbox0);
+			mb[1] = RD_REG_WORD(&reg->isp24.mailbox1);
+			mb[2] = RD_REG_WORD(&reg->isp24.mailbox2);
+			mb[3] = RD_REG_WORD(&reg->isp24.mailbox3);
+			mb[7] = RD_REG_WORD(&reg->isp24.mailbox7);
 			ictrl = RD_REG_DWORD(&reg->isp24.ictrl);
+			host_status = RD_REG_DWORD(&reg->isp24.host_status);
+			hccr = RD_REG_DWORD(&reg->isp24.hccr);
+
+			ql_log(ql_log_warn, vha, 0x1119,
+			    "MBX Command timeout for cmd %x, iocontrol=%x jiffies=%lx "
+			    "mb[0-3]=[0x%x 0x%x 0x%x 0x%x] mb7 0x%x host_status 0x%x hccr 0x%x\n",
+			    command, ictrl, jiffies, mb[0], mb[1], mb[2], mb[3],
+			    mb[7], host_status, hccr);
+
 		} else {
-			mb0 = RD_MAILBOX_REG(ha, &reg->isp, 0);
+			mb[0] = RD_MAILBOX_REG(ha, &reg->isp, 0);
 			ictrl = RD_REG_WORD(&reg->isp.ictrl);
+			ql_dbg(ql_dbg_mbx + ql_dbg_buffer, vha, 0x1119,
+			    "MBX Command timeout for cmd %x, iocontrol=%x jiffies=%lx "
+			    "mb[0]=0x%x\n", command, ictrl, jiffies, mb[0]);
 		}
-		ql_dbg(ql_dbg_mbx + ql_dbg_buffer, vha, 0x1119,
-		    "MBX Command timeout for cmd %x, iocontrol=%x jiffies=%lx "
-		    "mb[0]=0x%x\n", command, ictrl, jiffies, mb0);
 		ql_dump_regs(ql_dbg_mbx + ql_dbg_buffer, vha, 0x1019);
 
 		/* Capture FW dump only, if PCI device active */
-- 
1.8.3.1

[PATCH v2 06/10] qla2xxx: Fix crash due to null pointer access.

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

This patch fixes crash due to NULL pointer access.

Following stack trace will be seen.

[1469877.797315] Call Trace:
[1469877.799940]  [<ffffffffa03ab6e9>] qla2x00_mem_alloc+0xb09/0x10c0 [qla2xxx]
[1469877.806980]  [<ffffffffa03ac50a>] qla2x00_probe_one+0x86a/0x1b50 [qla2xxx]
[1469877.814013]  [<ffffffff813b6d01>] ? __pm_runtime_resume+0x51/0xa0
[1469877.820265]  [<ffffffff8157c1f5>] ? _raw_spin_lock_irqsave+0x25/0x90
[1469877.826776]  [<ffffffff8157cd2d>] ? _raw_spin_unlock_irqrestore+0x6d/0x80
[1469877.833720]  [<ffffffff810741d1>] ? preempt_count_sub+0xb1/0x100
[1469877.839885]  [<ffffffff8157cd0c>] ? _raw_spin_unlock_irqrestore+0x4c/0x80
[1469877.846830]  [<ffffffff81319b9c>] local_pci_probe+0x4c/0xb0
[1469877.852562]  [<ffffffff810741d1>] ? preempt_count_sub+0xb1/0x100
[1469877.858727]  [<ffffffff81319c89>] pci_call_probe+0x89/0xb0

Cc: <stable@vger.kernel.org>
Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_os.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 8521cfe..df57fb3 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -3662,7 +3662,7 @@ void qla2x00_mark_device_lost(scsi_qla_host_t *vha, fc_port_t *fcport,
 				sizeof(struct ct6_dsd), 0,
 				SLAB_HWCACHE_ALIGN, NULL);
 			if (!ctx_cachep)
-				goto fail_free_gid_list;
+				goto fail_free_srb_mempool;
 		}
 		ha->ctx_mempool = mempool_create_slab_pool(SRB_MIN_REQ,
 			ctx_cachep);
@@ -3815,7 +3815,7 @@ void qla2x00_mark_device_lost(scsi_qla_host_t *vha, fc_port_t *fcport,
 	ha->loop_id_map = kzalloc(BITS_TO_LONGS(LOOPID_MAP_SIZE) * sizeof(long),
 	    GFP_KERNEL);
 	if (!ha->loop_id_map)
-		goto fail_async_pd;
+		goto fail_loop_id_map;
 	else {
 		qla2x00_set_reserved_loop_ids(ha);
 		ql_dbg_pci(ql_dbg_init, ha->pdev, 0x0123,
@@ -3824,10 +3824,15 @@ void qla2x00_mark_device_lost(scsi_qla_host_t *vha, fc_port_t *fcport,
 
 	return 0;
 
+fail_loop_id_map:
+	dma_pool_free(ha->s_dma_pool, ha->async_pd, ha->async_pd_dma);
+	ha->async_pd = NULL;
 fail_async_pd:
 	dma_pool_free(ha->s_dma_pool, ha->ex_init_cb, ha->ex_init_cb_dma);
+	ha->ex_init_cb = NULL;
 fail_ex_init_cb:
 	kfree(ha->npiv_info);
+	ha->npiv_info = NULL;
 fail_npiv_info:
 	dma_free_coherent(&ha->pdev->dev, ((*rsp)->length + 1) *
 		sizeof(response_t), (*rsp)->ring, (*rsp)->dma);
@@ -3851,6 +3856,14 @@ void qla2x00_mark_device_lost(scsi_qla_host_t *vha, fc_port_t *fcport,
 	dma_pool_free(ha->s_dma_pool, ha->ms_iocb, ha->ms_iocb_dma);
 	ha->ms_iocb = NULL;
 	ha->ms_iocb_dma = 0;
+
+	if (ha->sns_cmd) {
+		dma_free_coherent(&ha->pdev->dev, sizeof(struct sns_cmd_pkt),
+		    ha->sns_cmd, ha->sns_cmd_dma);
+		ha->sns_cmd_dma = 0;
+		ha->sns_cmd = NULL;
+	}
+
 fail_dma_pool:
 	if (IS_QLA82XX(ha) || ql2xenabledif) {
 		dma_pool_destroy(ha->fcp_cmnd_dma_pool);
@@ -3868,10 +3881,12 @@ void qla2x00_mark_device_lost(scsi_qla_host_t *vha, fc_port_t *fcport,
 	kfree(ha->nvram);
 	ha->nvram = NULL;
 fail_free_ctx_mempool:
-	mempool_destroy(ha->ctx_mempool);
+	if (ha->ctx_mempool)
+		mempool_destroy(ha->ctx_mempool);
 	ha->ctx_mempool = NULL;
 fail_free_srb_mempool:
-	mempool_destroy(ha->srb_mempool);
+	if (ha->srb_mempool)
+		mempool_destroy(ha->srb_mempool);
 	ha->srb_mempool = NULL;
 fail_free_gid_list:
 	dma_free_coherent(&ha->pdev->dev, qla2x00_gid_list_size(ha),
-- 
1.8.3.1

[PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

Corrupted ATIO is defined as length of fcp_header & fcp_cmd
payload is less than 0x38. It's the minimum size for a frame to
carry 8..16 bytes SCSI CDB. The exchange will be dropped or
terminated if corrupted.

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_def.h    |  3 ++-
 drivers/scsi/qla2xxx/qla_target.c | 22 +++++++++++++++++++---
 drivers/scsi/qla2xxx/qla_target.h | 22 +++++++++++++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index f7df01b..b14455e 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -1556,7 +1556,8 @@ struct link_statistics {
 struct atio {
 	uint8_t		entry_type;		/* Entry type. */
 	uint8_t		entry_count;		/* Entry count. */
-	uint8_t		data[58];
+	uint16_t	attr_n_length;
+	uint8_t		data[56];
 	uint32_t	signature;
 #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
 };
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 5037b51..9252694 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -6451,12 +6451,28 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
 	if (!vha->flags.online)
 		return;
 
-	while (ha->tgt.atio_ring_ptr->signature != ATIO_PROCESSED) {
+	while ((ha->tgt.atio_ring_ptr->signature != ATIO_PROCESSED) ||
+	    fcpcmd_is_corrupted(ha->tgt.atio_ring_ptr)) {
 		pkt = (struct atio_from_isp *)ha->tgt.atio_ring_ptr;
 		cnt = pkt->u.raw.entry_count;
 
-		qlt_24xx_atio_pkt_all_vps(vha, (struct atio_from_isp *)pkt,
-		    ha_locked);
+		if (unlikely(fcpcmd_is_corrupted(ha->tgt.atio_ring_ptr))) {
+			/* This packet is corrupted. The header + payload
+			 * can not be trusted. There is no point in passing
+			 * it further up.
+			 */
+			ql_log(ql_log_warn, vha, 0xffff,
+			    "corrupted fcp frame SID[%3phN] OXID[%04x] EXCG[%x] %64phN\n",
+			    pkt->u.isp24.fcp_hdr.s_id,
+			    be16_to_cpu(pkt->u.isp24.fcp_hdr.ox_id),
+			    le32_to_cpu(pkt->u.isp24.exchange_addr), pkt);
+
+			adjust_corrupted_atio(pkt);
+			qlt_send_term_exchange(vha, NULL, pkt, ha_locked, 0);
+		} else {
+			qlt_24xx_atio_pkt_all_vps(vha,
+			    (struct atio_from_isp *)pkt, ha_locked);
+		}
 
 		for (i = 0; i < cnt; i++) {
 			ha->tgt.atio_ring_index++;
diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
index f26c5f6..f31d343 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -427,13 +427,33 @@ struct atio_from_isp {
 		struct {
 			uint8_t  entry_type;	/* Entry type. */
 			uint8_t  entry_count;	/* Entry count. */
-			uint8_t  data[58];
+			uint16_t attr_n_length;
+#define FCP_CMD_LENGTH_MASK 0x0fff
+#define FCP_CMD_LENGTH_MIN  0x38
+			uint8_t  data[56];
 			uint32_t signature;
 #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
 		} raw;
 	} u;
 } __packed;
 
+static inline int fcpcmd_is_corrupted(struct atio *atio)
+{
+	if (atio->entry_type == ATIO_TYPE7 &&
+	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
+	    FCP_CMD_LENGTH_MIN))
+		return 1;
+	else
+		return 0;
+}
+
+/* adjust corrupted atio so we won't trip over the same entry again. */
+static inline void adjust_corrupted_atio(struct atio_from_isp *atio)
+{
+	atio->u.raw.attr_n_length = cpu_to_le16(FCP_CMD_LENGTH_MIN);
+	atio->u.isp24.fcp_cmnd.add_cdb_len = 0;
+}
+
 #define CTIO_TYPE7 0x12 /* Continue target I/O entry (for 24xx) */
 
 /*
-- 
1.8.3.1

[PATCH v2 08/10] qla2xxx: Reduce exess wait during chip reset

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

Soft reset and Risc reset should take 100uS to complete.
This change pad the timeout up to 400uS, which should be
plenty.

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 632d5f3..7b6317c 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -1191,7 +1191,7 @@ static int qla2x00_fabric_dev_login(scsi_qla_host_t *, fc_port_t *,
 
 	/* Wait for soft-reset to complete. */
 	RD_REG_DWORD(&reg->ctrl_status);
-	for (cnt = 0; cnt < 6000000; cnt++) {
+	for (cnt = 0; cnt < 60; cnt++) {
 		barrier();
 		if ((RD_REG_DWORD(&reg->ctrl_status) &
 		    CSRX_ISP_SOFT_RESET) == 0)
@@ -1234,7 +1234,7 @@ static int qla2x00_fabric_dev_login(scsi_qla_host_t *, fc_port_t *,
 	RD_REG_DWORD(&reg->hccr);
 
 	RD_REG_WORD(&reg->mailbox0);
-	for (cnt = 6000000; RD_REG_WORD(&reg->mailbox0) != 0 &&
+	for (cnt = 60; RD_REG_WORD(&reg->mailbox0) != 0 &&
 	    rval == QLA_SUCCESS; cnt--) {
 		barrier();
 		if (cnt)
-- 
1.8.3.1

[PATCH v2 09/10] qla2xxx: Fix invalid handle erroneous message.

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

Termination of Immediate Notify IOCB was using wrong
IOCB handle. IOCB completion code was unable to find
appropriate code path due to wrong handle.

Following message is seen in the logs.

"Error entry - invalid handle/queue (ffff)."

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_isr.c    | 4 ++++
 drivers/scsi/qla2xxx/qla_target.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index af840bf..eefcf2f 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -2495,6 +2495,10 @@ struct scsi_dif_tuple {
 	if (pkt->entry_status & RF_BUSY)
 		res = DID_BUS_BUSY << 16;
 
+	if (pkt->entry_type == NOTIFY_ACK_TYPE &&
+	    pkt->handle == QLA_TGT_SKIP_HANDLE)
+		return;
+
 	sp = qla2x00_get_sp_from_handle(vha, func, req, pkt);
 	if (sp) {
 		sp->done(ha, sp, res);
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 9252694..c02f31a 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -3061,7 +3061,7 @@ static int __qlt_send_term_imm_notif(struct scsi_qla_host *vha,
 
 	pkt->entry_type = NOTIFY_ACK_TYPE;
 	pkt->entry_count = 1;
-	pkt->handle = QLA_TGT_SKIP_HANDLE | CTIO_COMPLETION_HANDLE_MARK;
+	pkt->handle = QLA_TGT_SKIP_HANDLE;
 
 	nack = (struct nack_to_isp *)pkt;
 	nack->ox_id = ntfy->ox_id;
-- 
1.8.3.1

[PATCH v2 10/10] qla2xxx: Disable Out-of-order processing by default in Firmware

[Himanshu Madhani]

From: Quinn Tran <quinn.tran@cavium.com>

Out of order(OOO) processing requires initiator, switch
and target to support OOO. In today¹s environment, none
of the switches support OOO. OOO requires extra buffer
space which affect performance. By turning ON this feature
in QLogic's FW, it delays error recovery because droped
frame is treated as out of order frame. We¹re turning OFF
this option of speed up error recovery,

Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_target.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index c02f31a..6ab191a 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -6578,9 +6578,6 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
 		return;
 	}
 
-	/* out-of-order frames reassembly */
-	nv->firmware_options_3 |= BIT_6|BIT_9;
-
 	if (ha->tgt.enable_class_2) {
 		if (vha->flags.init_done)
 			fc_host_supported_classes(vha->host) =
@@ -6683,9 +6680,6 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
 		return;
 	}
 
-	/* out-of-order frames reassembly */
-	nv->firmware_options_3 |= BIT_6|BIT_9;
-
 	if (ha->tgt.enable_class_2) {
 		if (vha->flags.init_done)
 			fc_host_supported_classes(vha->host) =
-- 
1.8.3.1

Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[Christoph Hellwig]

The whole series looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

(not sure why you dropped my Reviewed-by: tags for all the previously
reviewed patches, though)

Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[Madhani, Himanshu]

On 12/22/16, 1:25 AM, "Christoph Hellwig" <hch@infradead.org> wrote:

>The whole series looks fine:
>
>Reviewed-by: Christoph Hellwig <hch@lst.de>
>
>(not sure why you dropped my Reviewed-by: tags for all the previously
>reviewed patches, though)

Thanks for the review. Looks like oversight while sending updated series.

Bart, 

Do you want me to send series updating Reviewed-by Tag or would you be able to update 
while applying to target-pending tree? 

Thanks,
Himanshu
>

Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[Bart Van Assche]

On Thu, 2016-12-22 at 16:44 +0000, Madhani, Himanshu wrote:
> Do you want me to send series updating Reviewed-by Tag or would you be able to update 
> while applying to target-pending tree? 

Hello Himanshu,

The past three days I have been busy with rewriting the blk-mq-sched code so I have
not yet had the time to review this patch series. I plan to do this tomorrow.

BTW, it is a good idea to wait for feedback from everyone who is likely to post
feedback before reposting a patch series such that all review comments can be
addressed at once. Additionally, since v2 of this patch series was posted less than
24 hours ago it is too early anyway to repost this patch series.

Bart.

Re: [PATCH v2 00/10] qla2xxx: Bug fixes for driver.

[Madhani, Himanshu]

Hi Bart, 



On 12/22/16, 9:02 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>On Thu, 2016-12-22 at 16:44 +0000, Madhani, Himanshu wrote:
>> Do you want me to send series updating Reviewed-by Tag or would you be able to update 
>> while applying to target-pending tree? 
>
>Hello Himanshu,
>
>The past three days I have been busy with rewriting the blk-mq-sched code so I have
>not yet had the time to review this patch series. I plan to do this tomorrow.
>
>BTW, it is a good idea to wait for feedback from everyone who is likely to post
>feedback before reposting a patch series such that all review comments can be
>addressed at once. Additionally, since v2 of this patch series was posted less than
>24 hours ago it is too early anyway to repost this patch series.
>
>Bart.

Understood. I’ll hold off on sending update to this series. 

I am going to post series where we have done additional improvements and added new features.
I wanted to get review comments before most of the folks take some time off next week.

Thanks,
Himanshu

Re: [PATCH v2 02/10] qla2xxx: Include ATIO queue in firmware dump when in target mode

[Bart Van Assche]

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> @@ -676,6 +688,18 @@ static inline void (*qla27xx_read_vector(uint width))(void __iomem*, void *, ulo
>  				count++;
>  			}
>  		}
> +	} else if (QLA_TGT_MODE_ENABLED() &&
> +	    ent->t274.queue_type == T274_QUEUE_TYPE_ATIO_SHAD) {
> +		struct qla_hw_data *ha = vha->hw;
> +		struct atio *atr = ha->tgt.atio_ring_ptr;
> +
> +		if (atr || !buf) {
> +			qla27xx_insert16(0, buf, len);
> +			qla27xx_insert16(1, buf, len);
> +			qla27xx_insert32(ha->tgt.atio_q_in ?
> +			    *ha->tgt.atio_q_in : 0, buf, len);
> +			count++;
> +		}
>  	} else {
>  		ql_dbg(ql_dbg_misc, vha, 0xd02f,
>  		    "%s: unknown queue %x\n", __func__, ent->t274.queue_type);

Hello Himanshu,

This patch introduces a new sparse warning for *ha->tgt.atio_q_in:

drivers/scsi/qla2xxx/qla_tmpl.c:700:37: warning: dereference of noderef expression

Sparse reports this because the atio_q_in pointer is declared as uint32_t __iomem*.
Does this perhaps mean that a readl() call is missing?

Bart.

Re: [PATCH v2 04/10] qla2xxx: Reset reserved field in firmware options to 0.

[Bart Van Assche]

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> During NVRAM initialization in target mode, reset reserved
> fields in firmware options to Zero (BIT 15)
> 
> Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
> Signed-off-by: Giridhar Malavali <giridhar.malavali@cavium.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index b9c559c..5037b51 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -6539,6 +6539,14 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
>  
>  		/* Disable Full Login after LIP */
>  		nv->host_p &= cpu_to_le32(~BIT_10);
> +
> +		/*
> +		 * clear BIT 15 explicitly as we have seen at least
> +		 * a couple of instances where this was set and this
> +		 * was causing the firmware to not be initialized.
> +		 */
> +		nv->firmware_options_1 &=
> +		    __constant_cpu_to_le32(~BIT_15);
>  		/* Enable target PRLI control */
>  		nv->firmware_options_2 |= cpu_to_le32(BIT_14);
>  	} else {
> @@ -6623,11 +6631,18 @@ static void qlt_disable_vha(struct scsi_qla_host *vha)
>  		/* Disable ini mode, if requested */
>  		if (!qla_ini_mode_enabled(vha))
>  			nv->firmware_options_1 |= cpu_to_le32(BIT_5);
> -
>  		/* Disable Full Login after LIP */
>  		nv->firmware_options_1 &= cpu_to_le32(~BIT_13);
>  		/* Enable initial LIP */
>  		nv->firmware_options_1 &= cpu_to_le32(~BIT_9);
> +		/*
> +		 * clear BIT 15 explicitly as we have seen at
> +		 * least a couple of instances where this was set
> +		 * and this was causing the firmware to not be
> +		 * initialized.
> +		 */
> +		nv->firmware_options_1 &=
> +		    __constant_cpu_to_le32(~BIT_15);
>  		if (ql2xtgt_tape_enable)
>  			/* Enable FC tape support */
>  			nv->firmware_options_2 |= cpu_to_le32(BIT_12);

Hello Himanshu,

Please use cpu_to_le32() in new code instead of __constant_cpu_to_le32().
gcc generates the same code for both conversion functions but the former
function makes source code easier to read.

Bart.

Re: [PATCH v2 06/10] qla2xxx: Fix crash due to null pointer access.

[Bart Van Assche]

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> From: Quinn Tran <quinn.tran@cavium.com>
> 
> This patch fixes crash due to NULL pointer access.
> 
> Following stack trace will be seen.
> 
> [1469877.797315] Call Trace:
> [1469877.799940]  [<ffffffffa03ab6e9>] qla2x00_mem_alloc+0xb09/0x10c0 [qla2xxx]
> [1469877.806980]  [<ffffffffa03ac50a>] qla2x00_probe_one+0x86a/0x1b50 [qla2xxx]
> [1469877.814013]  [<ffffffff813b6d01>] ? __pm_runtime_resume+0x51/0xa0
> [1469877.820265]  [<ffffffff8157c1f5>] ? _raw_spin_lock_irqsave+0x25/0x90
> [1469877.826776]  [<ffffffff8157cd2d>] ? _raw_spin_unlock_irqrestore+0x6d/0x80
> [1469877.833720]  [<ffffffff810741d1>] ? preempt_count_sub+0xb1/0x100
> [1469877.839885]  [<ffffffff8157cd0c>] ? _raw_spin_unlock_irqrestore+0x4c/0x80
> [1469877.846830]  [<ffffffff81319b9c>] local_pci_probe+0x4c/0xb0
> [1469877.852562]  [<ffffffff810741d1>] ? preempt_count_sub+0xb1/0x100
> [1469877.858727]  [<ffffffff81319c89>] pci_call_probe+0x89/0xb0

This patch needs a more detailed description. There are many changes in this
patch. What changes are the changes that prevent the NULL pointer dereference?
What changes (if any) were made as the result of code inspection?

Bart.

Re: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.

[Bart Van Assche]

On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index f7df01b..b14455e 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -1556,7 +1556,8 @@ struct link_statistics {
>  struct atio {
>  	uint8_t		entry_type;		/* Entry type. */
>  	uint8_t		entry_count;		/* Entry count. */
> -	uint8_t		data[58];
> +	uint16_t	attr_n_length;
> +	uint8_t		data[56];
>  	uint32_t	signature;
>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
> };

Are struct atio and/or struct atio_from_isp the description of a firmware
data structure? If so, do all firmware versions initialize the attr_n_length
field or is there a minimum firmware version required for this field to be
valid? Please mention any changed dependencies on the firmware version in the
patch description. Please also fix the typo in the patch title when reposting
this patch ("corrputed").

> +			/* This packet is corrupted. The header + payload
> +			 * can not be trusted. There is no point in passing
> +			 * it further up.
> +			 */

This is not the proper Linux kernel comment style (see also
Documentation/process/coding-style.rst).

> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> index f26c5f6..f31d343 100644
> --- a/drivers/scsi/qla2xxx/qla_target.h
> +++ b/drivers/scsi/qla2xxx/qla_target.h
> @@ -427,13 +427,33 @@ struct atio_from_isp {
>  		struct {
>  			uint8_t  entry_type;	/* Entry type. */
>  			uint8_t  entry_count;	/* Entry count. */
> -			uint8_t  data[58];
> +			uint16_t attr_n_length;
> +#define FCP_CMD_LENGTH_MASK 0x0fff
> +#define FCP_CMD_LENGTH_MIN  0x38
> +			uint8_t  data[56];
>  			uint32_t signature;
>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
>  		} raw;
>  	} u;
>  } __packed;
>  
> +static inline int fcpcmd_is_corrupted(struct atio *atio)
> +{
> +	if (atio->entry_type == ATIO_TYPE7 &&
> +	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
> +	    FCP_CMD_LENGTH_MIN))
> +		return 1;
> +	else
> +		return 0;
> +}

The above code shows that attr_n_length is a little endian field so please
declare it as little endian where appropriate (__le16 instead of uint16_t).

Bart.

Re: [PATCH v2 07/10] qla2xxx: Terminate exchange if corrputed.

[Madhani, Himanshu]

On 12/23/16, 1:01 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>On Wed, 2016-12-21 at 13:57 -0800, Himanshu Madhani wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
>> index f7df01b..b14455e 100644
>> --- a/drivers/scsi/qla2xxx/qla_def.h
>> +++ b/drivers/scsi/qla2xxx/qla_def.h
>> @@ -1556,7 +1556,8 @@ struct link_statistics {
>>  struct atio {
>>  	uint8_t		entry_type;		/* Entry type. */
>>  	uint8_t		entry_count;		/* Entry count. */
>> -	uint8_t		data[58];
>> +	uint16_t	attr_n_length;
>> +	uint8_t		data[56];
>>  	uint32_t	signature;
>>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
>> };
>
>Are struct atio and/or struct atio_from_isp the description of a firmware
>data structure? If so, do all firmware versions initialize the attr_n_length
>field or is there a minimum firmware version required for this field to be
>valid? Please mention any changed dependencies on the firmware version in the
>patch description. Please also fix the typo in the patch title when reposting
>this patch ("corrputed").

They represent same data structure from firmware. We will send follow up patch to
Clean up redundant structure.

>
>> +			/* This packet is corrupted. The header + payload
>> +			 * can not be trusted. There is no point in passing
>> +			 * it further up.
>> +			 */
>
>This is not the proper Linux kernel comment style (see also
>Documentation/process/coding-style.rst).

Ack. Will update patch with correct format.

>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
>> index f26c5f6..f31d343 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.h
>> +++ b/drivers/scsi/qla2xxx/qla_target.h
>> @@ -427,13 +427,33 @@ struct atio_from_isp {
>>  		struct {
>>  			uint8_t  entry_type;	/* Entry type. */
>>  			uint8_t  entry_count;	/* Entry count. */
>> -			uint8_t  data[58];
>> +			uint16_t attr_n_length;
>> +#define FCP_CMD_LENGTH_MASK 0x0fff
>> +#define FCP_CMD_LENGTH_MIN  0x38
>> +			uint8_t  data[56];
>>  			uint32_t signature;
>>  #define ATIO_PROCESSED 0xDEADDEAD		/* Signature */
>>  		} raw;
>>  	} u;
>>  } __packed;
>>  
>> +static inline int fcpcmd_is_corrupted(struct atio *atio)
>> +{
>> +	if (atio->entry_type == ATIO_TYPE7 &&
>> +	    (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
>> +	    FCP_CMD_LENGTH_MIN))
>> +		return 1;
>> +	else
>> +		return 0;
>> +}
>
>The above code shows that attr_n_length is a little endian field so please
>declare it as little endian where appropriate (__le16 instead of uint16_t).

Ack. Will update the patch

>
>Bart.

Re: [PATCH v2 06/10] qla2xxx: Fix crash due to null pointer access.

[Madhani, Himanshu]

On 12/23/16, 12:47 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>This patch needs a more detailed description. There are many changes in this
>patch. What changes are the changes that prevent the NULL pointer dereference?
>What changes (if any) were made as the result of code inspection?

We saw this stack trace on one test setup which never was reproduced.
This patch is result of code inspection only and we have not seen this being 
Reproduced or reported by customer. 

Re: [PATCH v2 04/10] qla2xxx: Reset reserved field in firmware options to 0.

[Madhani, Himanshu]

On 12/23/16, 12:37 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>Please use cpu_to_le32() in new code instead of __constant_cpu_to_le32().
>gcc generates the same code for both conversion functions but the former
>function makes source code easier to read.

Ack. Will update the patch.

>

Re: [PATCH v2 02/10] qla2xxx: Include ATIO queue in firmware dump when in target mode

[Madhani, Himanshu]

On 12/23/16, 12:32 AM, "Bart Van Assche" <Bart.VanAssche@sandisk.com> wrote:

>Sparse reports this because the atio_q_in pointer is declared as uint32_t __iomem*.
>Does this perhaps mean that a readl() call is missing?

Ack. Will fix in revised series.