Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.

Thanks Hannes for the review, please see my comments below,

On 19/10/16 1:15 PM, "Hannes Reinecke" [off-list ref] wrote:

On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:

quoted

From: Manish Rangankar <redacted>

The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
for 41000 Series Converged Network Adapters by QLogic.

This patch consists of following changes:
  - MAINTAINERS Makefile and Kconfig changes for qedi,
  - PCI driver registration,
  - iSCSI host level initialization,
  - Debugfs and log level infrastructure.

Signed-off-by: Nilesh Javali <redacted>
Signed-off-by: Adheer Chandravanshi <redacted>
Signed-off-by: Chad Dupuis <redacted>
Signed-off-by: Saurav Kashyap <redacted>
Signed-off-by: Arun Easi <redacted>
Signed-off-by: Manish Rangankar <redacted>
---
 MAINTAINERS                         |    6 +
 drivers/net/ethernet/qlogic/Kconfig |   12 -
 drivers/scsi/Kconfig                |    1 +
 drivers/scsi/Makefile               |    1 +
 drivers/scsi/qedi/Kconfig           |   10 +
 drivers/scsi/qedi/Makefile          |    5 +
 drivers/scsi/qedi/qedi.h            |  286 +++++++
 drivers/scsi/qedi/qedi_dbg.c        |  143 ++++
 drivers/scsi/qedi/qedi_dbg.h        |  144 ++++
 drivers/scsi/qedi/qedi_debugfs.c    |  244 ++++++
 drivers/scsi/qedi/qedi_hsi.h        |   52 ++
 drivers/scsi/qedi/qedi_main.c       | 1550
+++++++++++++++++++++++++++++++++++
 drivers/scsi/qedi/qedi_sysfs.c      |   52 ++
 drivers/scsi/qedi/qedi_version.h    |   14 +
 14 files changed, 2508 insertions(+), 12 deletions(-)
 create mode 100644 drivers/scsi/qedi/Kconfig
 create mode 100644 drivers/scsi/qedi/Makefile
 create mode 100644 drivers/scsi/qedi/qedi.h
 create mode 100644 drivers/scsi/qedi/qedi_dbg.c
 create mode 100644 drivers/scsi/qedi/qedi_dbg.h
 create mode 100644 drivers/scsi/qedi/qedi_debugfs.c
 create mode 100644 drivers/scsi/qedi/qedi_hsi.h
 create mode 100644 drivers/scsi/qedi/qedi_main.c
 create mode 100644 drivers/scsi/qedi/qedi_sysfs.c
 create mode 100644 drivers/scsi/qedi/qedi_version.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e925a2..906d05f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -9909,6 +9909,12 @@ F:	drivers/net/ethernet/qlogic/qed/
 F:	include/linux/qed/
 F:	drivers/net/ethernet/qlogic/qede/
 
+QLOGIC QL41xxx ISCSI DRIVER
+M:	QLogic-Storage-Upstream@cavium.com
+L:	linux-scsi@vger.kernel.org
+S:	Supported
+F:	drivers/scsi/qedi/
+
 QNX4 FILESYSTEM
 M:	Anders Larsen <al@alarsen.net>
 W:	http://www.alarsen.net/linux/qnx4fs/

diff --git a/drivers/net/ethernet/qlogic/Kconfig

b/drivers/net/ethernet/qlogic/Kconfig
index bad4fae..28b4366 100644

--- a/drivers/net/ethernet/qlogic/Kconfig
+++ b/drivers/net/ethernet/qlogic/Kconfig

@@ -121,16 +121,4 @@ config INFINIBAND_QEDR
 config QED_ISCSI
 	bool
 
-config QEDI
-	tristate "QLogic QED 25/40/100Gb iSCSI driver"
-	depends on QED
-	select QED_LL2
-	select QED_ISCSI
-	default n
-	---help---
-	  This provides a temporary node that allows the compilation
-	  and logical testing of the hardware offload iSCSI support
-	  for QLogic QED. This would be replaced by the 'real' option
-	  once the QEDI driver is added [+relocated].
-
 endif # NET_VENDOR_QLOGIC

Huh? You just introduce this one in patch 1/6.
Please fold them together so that this can be omitted.

Yes, we will remove this in the next revision.

-- snipped --

quoted

@@ -0,0 +1,52 @@
+/*
+ * QLogic iSCSI Offload Driver
+ * Copyright (c) 2016 Cavium Inc.
+ *
+ * This software is available under the terms of the GNU General

Public License
+ * (GPL) Version 2, available from the file COPYING in the main
directory of
+ * this source tree.
+ */
+#ifndef __QEDI_HSI__
+#define __QEDI_HSI__
+/********************************/
+/* Add include to common target */
+/********************************/
+#include <linux/qed/common_hsi.h>
+

Please use kernel-doc style for comments

Will do.

--snipped--

quoted

+static void qedi_int_fp(struct qedi_ctx *qedi)
+{
+	struct qedi_fastpath *fp;
+	int id;
+
+	memset((void *)qedi->fp_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
+	       sizeof(*qedi->fp_array));
+	memset((void *)qedi->sb_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
+	       sizeof(*qedi->sb_array));
+
+	for (id = 0; id < MIN_NUM_CPUS_MSIX(qedi); id++) {
+		fp = &qedi->fp_array[id];
+		fp->sb_info = &qedi->sb_array[id];
+		fp->sb_id = id;
+		fp->qedi = qedi;
+		snprintf(fp->name, sizeof(fp->name), "%s-fp-%d",
+			 "qedi", id);
+
+		/* fp_array[i] ---- irq cookie
+		 * So init data which is needed in int ctx
+		 */
+	}
+}
+

Please check if you cannot make use of Christophs irq rework.

Sure, we will explore this.

--snipped--

quoted

+static bool qedi_process_completions(struct qedi_fastpath *fp)
+{
+	struct qedi_work *qedi_work = NULL;
+	struct qedi_ctx *qedi = fp->qedi;
+	struct qed_sb_info *sb_info = fp->sb_info;
+	struct status_block *sb = sb_info->sb_virt;
+	struct qedi_percpu_s *p = NULL;
+	struct global_queue *que;
+	u16 prod_idx;
+	unsigned long flags;
+	union iscsi_cqe *cqe;
+	int cpu;
+
+	/* Get the current firmware producer index */
+	prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
+
+	if (prod_idx >= QEDI_CQ_SIZE)
+		prod_idx = prod_idx % QEDI_CQ_SIZE;
+
+	que = qedi->global_queues[fp->sb_id];
+	QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
+		  "Before: global queue=%p prod_idx=%d cons_idx=%d, sb_id=%d\n",
+		  que, prod_idx, que->cq_cons_idx, fp->sb_id);
+
+	qedi->intr_cpu = fp->sb_id;
+	cpu = smp_processor_id();
+	p = &per_cpu(qedi_percpu, cpu);
+
+	if (unlikely(!p->iothread))
+		WARN_ON(1);
+
+	spin_lock_irqsave(&p->p_work_lock, flags);
+	while (que->cq_cons_idx != prod_idx) {
+		cqe = &que->cq[que->cq_cons_idx];
+
+		QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
+			  "cqe=%p prod_idx=%d cons_idx=%d.\n",
+			  cqe, prod_idx, que->cq_cons_idx);
+
+		/* Alloc and copy to the cqe */
+		qedi_work = kzalloc(sizeof(*qedi_work), GFP_ATOMIC);
+		if (qedi_work) {
+			INIT_LIST_HEAD(&qedi_work->list);
+			qedi_work->qedi = qedi;
+			memcpy(&qedi_work->cqe, cqe, sizeof(union iscsi_cqe));
+			qedi_work->que_idx = fp->sb_id;
+			list_add_tail(&qedi_work->list, &p->work_list);
+		} else {
+			WARN_ON(1);
+			continue;
+		}
+

Memory allocation in an interrupt routine?
You must be kidding ...

We will revisit this code.

quoted

+		que->cq_cons_idx++;
+		if (que->cq_cons_idx == QEDI_CQ_SIZE)
+			que->cq_cons_idx = 0;
+	}
+	wake_up_process(p->iothread);
+	spin_unlock_irqrestore(&p->p_work_lock, flags);
+
+	return true;
+}
+
+static bool qedi_fp_has_work(struct qedi_fastpath *fp)
+{
+	struct qedi_ctx *qedi = fp->qedi;
+	struct global_queue *que;
+	struct qed_sb_info *sb_info = fp->sb_info;
+	struct status_block *sb = sb_info->sb_virt;
+	u16 prod_idx;
+
+	barrier();
+
+	/* Get the current firmware producer index */
+	prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
+
+	/* Get the pointer to the global CQ this completion is on */
+	que = qedi->global_queues[fp->sb_id];
+
+	/* prod idx wrap around uint16 */
+	if (prod_idx >= QEDI_CQ_SIZE)
+		prod_idx = prod_idx % QEDI_CQ_SIZE;
+
+	return (que->cq_cons_idx != prod_idx);
+}
+
+/* MSI-X fastpath handler code */
+static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
+{
+	struct qedi_fastpath *fp = dev_id;
+	struct qedi_ctx *qedi = fp->qedi;
+	bool wake_io_thread = true;
+
+	qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
+
+process_again:
+	wake_io_thread = qedi_process_completions(fp);
+	if (wake_io_thread) {
+		QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
+			  "process already running\n");
+	}
+
+	if (qedi_fp_has_work(fp) == 0)
+		qed_sb_update_sb_idx(fp->sb_info);
+
+	/* Check for more work */
+	rmb();
+
+	if (qedi_fp_has_work(fp) == 0)
+		qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
+	else
+		goto process_again;
+
+	return IRQ_HANDLED;
+}
+
+/* simd handler for MSI/INTa */
+static void qedi_simd_int_handler(void *cookie)
+{
+	/* Cookie is qedi_ctx struct */
+	struct qedi_ctx *qedi = (struct qedi_ctx *)cookie;
+
+	QEDI_WARN(&qedi->dbg_ctx, "qedi=%p.\n", qedi);
+}
+
+#define QEDI_SIMD_HANDLER_NUM		0
+static void qedi_sync_free_irqs(struct qedi_ctx *qedi)
+{
+	int i;
+
+	if (qedi->int_info.msix_cnt) {
+		for (i = 0; i < qedi->int_info.used_cnt; i++) {
+			synchronize_irq(qedi->int_info.msix[i].vector);
+			irq_set_affinity_hint(qedi->int_info.msix[i].vector,
+					      NULL);
+			free_irq(qedi->int_info.msix[i].vector,
+				 &qedi->fp_array[i]);
+		}
+	} else {
+		qedi_ops->common->simd_handler_clean(qedi->cdev,
+						     QEDI_SIMD_HANDLER_NUM);
+	}
+
+	qedi->int_info.used_cnt = 0;
+	qedi_ops->common->set_fp_int(qedi->cdev, 0);
+}
+

Again, consider using the interrupt affinity rework from Christoph Hellwig

Sure, we will explore this one also.