| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 20 Oct 2016 08:27:50 +0000
From: "Rangankar, Manish" <Manish.Rangankar@...ium.com>
To: Hannes Reinecke <hare@...e.de>,
"lduncan@...e.com" <lduncan@...e.com>,
"cleech@...hat.com" <cleech@...hat.com>
CC: "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
"jejb@...ux.vnet.ibm.com" <jejb@...ux.vnet.ibm.com>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Mintz, Yuval" <Yuval.Mintz@...ium.com>,
Dept-Eng QLogic Storage Upstream
<QLogic-Storage-Upstream@...ium.com>,
"Javali, Nilesh" <Nilesh.Javali@...ium.com>,
Adheer Chandravanshi <adheer.chandravanshi@...gic.com>,
"Dupuis, Chad" <Chad.Dupuis@...ium.com>,
"Kashyap, Saurav" <Saurav.Kashyap@...ium.com>,
"Easi, Arun" <Arun.Easi@...ium.com>
Subject: 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" <hare@...e.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@...ium.com wrote:
>> From: Manish Rangankar <manish.rangankar@...ium.com>
>>
>> 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 <nilesh.javali@...ium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@...gic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@...ium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@...ium.com>
>> Signed-off-by: Arun Easi <arun.easi@...ium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@...ium.com>
>> ---
>> 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@...ium.com
>> +L: linux-scsi@...r.kernel.org
>> +S: Supported
>> +F: drivers/scsi/qedi/
>> +
>> QNX4 FILESYSTEM
>> M: Anders Larsen <al@...rsen.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 --
>> @@ -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--
>> +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--
>> +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.
>
>> + 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.
Powered by blists - more mailing lists