| 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
| ||
|
Message-ID: <BE8371DA886269458E0220A16DC1F8277E07D4EC@SHSMSX104.ccr.corp.intel.com>
Date: Mon, 14 Aug 2017 12:33:31 +0000
From: "Wu, Hao" <hao.wu@...el.com>
To: Alan Tull <atull@...nel.org>
CC: Moritz Fischer <mdf@...nel.org>,
"linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
"Kang, Luwei" <luwei.kang@...el.com>,
"Zhang, Yi Z" <yi.z.zhang@...el.com>,
"Whisonant, Tim" <tim.whisonant@...el.com>,
"Luebbers, Enno" <enno.luebbers@...el.com>,
"Rao, Shiva" <shiva.rao@...el.com>,
"Rauer, Christopher" <christopher.rauer@...el.com>
Subject: RE: [PATCH v2 06/22] fpga: intel: add FPGA PCIe device driver
> On Sun, Jun 25, 2017 at 8:52 PM, Wu Hao <hao.wu@...el.com> wrote:
>
> Hi Hao,
>
> Making my way through your patchset.
Hi Alan
Thanks a lot for your comments. :)
>
> > From: Zhang Yi <yi.z.zhang@...el.com>
> >
> > The Intel FPGA device appears as a PCIe device on the system. This patch
> > implements the basic framework of the driver for Intel PCIe device which
> > locates between CPU and Accelerated Function Units (AFUs).
>
> ...which is located between the CPU and...
Will fix this.
>
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@...el.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@...el.com>
> > Signed-off-by: Shiva Rao <shiva.rao@...el.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@...el.com>
> > Signed-off-by: Zhang Yi <yi.z.zhang@...el.com>
> > Signed-off-by: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> > Signed-off-by: Wu Hao <hao.wu@...el.com>
> > ---
> > v2: moved the code to drivers/fpga folder as suggested by Alan Tull.
> > switched to GPLv2 license.
> > fixed comments from Moritz Fischer.
> > ---
> > drivers/fpga/Kconfig | 28 +++++++++++
> > drivers/fpga/Makefile | 5 ++
> > drivers/fpga/intel-pcie.c | 126
> ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 159 insertions(+)
> > create mode 100644 drivers/fpga/intel-pcie.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index c1d8f41..3f3b7f4 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -117,6 +117,34 @@ config XILINX_PR_DECOUPLER
> > region of the FPGA from the busses while that region is
> > being reprogrammed during partial reconfig.
> >
> > +menuconfig INTEL_FPGA
> > + tristate "Intel(R) FPGA support"
> > + depends on FPGA_DEVICE
> > + help
> > + Select this option to enable driver support for Intel(R)
> > + Field-Programmable Gate Array (FPGA) solutions. This driver
>
> This is confusing since there is FPGA support for Intel FPGA devices
> in the kernel already such as any of the Altera support that already
> is in this Kconfig. This description will have to be more specific
> that this is one particular Intel solution.
Make sense, will add more specific description here.
>
> > + provides interfaces for userspace applications to configure,
> > + enumerate, open, and access FPGA accelerators on platforms
> > + equipped with Intel(R) FPGA solutions and enables system
> > + level management functions such as FPGA reconfiguration,
> > + power management, and virtualization.
> > +
> > + Say Y if your platform has this technology. Say N if unsure.
> > +
> > +if INTEL_FPGA
> > +
> > +config INTEL_FPGA_PCI
> > + tristate "Intel FPGA PCIe Driver"
> > + depends on PCI
> > + help
> > + This is the driver for the PCIe device which locates between
>
> ...which is located between...
Will fix this.
>
> > + CPU and Accelerated Function Units (AFUs) and allows them to
> > + communicate with each other.
> > +
> > + To compile this as a module, choose M here.
> > +
> > +endif
> > +
> > endif # FPGA
> >
> > endmenu
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 8950a8f..5613133 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -27,3 +27,8 @@ obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-
> decoupler.o
> > # High Level Interfaces
> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o
> > obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > +
> > +# Intel FPGA Support
> > +obj-$(CONFIG_INTEL_FPGA_PCI) += intel-fpga-pci.o
> > +
> > +intel-fpga-pci-objs := intel-pcie.o
> > diff --git a/drivers/fpga/intel-pcie.c b/drivers/fpga/intel-pcie.c
> > new file mode 100644
> > index 0000000..f697de4
> > --- /dev/null
> > +++ b/drivers/fpga/intel-pcie.c
> > @@ -0,0 +1,126 @@
> > +/*
> > + * Driver for Intel FPGA PCIe device
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Zhang Yi <Yi.Z.Zhang@...el.com>
> > + * Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> > + * Joseph Grecco <joe.grecco@...el.com>
> > + * Enno Luebbers <enno.luebbers@...el.com>
> > + * Tim Whisonant <tim.whisonant@...el.com>
> > + * Ananda Ravuri <ananda.ravuri@...el.com>
> > + * Henry Mitchel <henry.mitchel@...el.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL version 2. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/stddef.h>
> > +#include <linux/errno.h>
> > +#include <linux/aer.h>
> > +
> > +#define DRV_VERSION "0.8"
> > +#define DRV_NAME "intel-fpga-pci"
> > +
> > +/* PCI Device ID */
> > +#define PCIe_DEVICE_ID_PF_INT_5_X 0xBCBD
> > +#define PCIe_DEVICE_ID_PF_INT_6_X 0xBCC0
> > +#define PCIe_DEVICE_ID_PF_DSC_1_X 0x09C4
> > +/* VF Device */
> > +#define PCIe_DEVICE_ID_VF_INT_5_X 0xBCBF
> > +#define PCIe_DEVICE_ID_VF_INT_6_X 0xBCC1
> > +#define PCIe_DEVICE_ID_VF_DSC_1_X 0x09C5
> > +
> > +static struct pci_device_id cci_pcie_id_tbl[] = {
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_5_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_5_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_INT_6_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_INT_6_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_PF_DSC_1_X),},
> > + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIe_DEVICE_ID_VF_DSC_1_X),},
> > + {0,}
> > +};
> > +MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> > +
> > +static
> > +int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > +{
> > + int ret;
> > +
> > + ret = pci_enable_device(pcidev);
> > + if (ret < 0) {
> > + dev_err(&pcidev->dev, "Failed to enable device %d.\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = pci_enable_pcie_error_reporting(pcidev);
> > + if (ret && ret != -EINVAL)
> > + dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);
> > +
> > + ret = pci_request_regions(pcidev, DRV_NAME);
> > + if (ret) {
> > + dev_err(&pcidev->dev, "Failed to request regions.\n");
> > + goto disable_error_report_exit;
> > + }
> > +
> > + pci_set_master(pcidev);
> > + pci_save_state(pcidev);
>
> Is pci_save_state needed here? Thought it was for going into suspend.
>
I think no, will remove this in the next version.
> > +
> > + if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(64))) {
>
> Please use pci_dma_set_mask()
Will fix this.
>
> > + dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(64));
>
> pci_set_consistent_dma_mask() and check its return code.
>
Will fix this.
> > + } else if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(32))) {
> > + dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(32));
>
> Same as above.
>
Will fix this.
Thanks again for the review.
Hao
> > + } else {
> > + ret = -EIO;
> > + dev_err(&pcidev->dev, "No suitable DMA support available.\n");
> > + goto release_region_exit;
> > + }
> > +
> > + /* TODO: create and add the platform device per feature list */
> > + return 0;
> > +
> > +release_region_exit:
> > + pci_release_regions(pcidev);
> > +disable_error_report_exit:
> > + pci_disable_pcie_error_reporting(pcidev);
> > + pci_disable_device(pcidev);
> > + return ret;
> > +}
> > +
> > +static void cci_pci_remove(struct pci_dev *pcidev)
> > +{
> > + pci_release_regions(pcidev);
> > + pci_disable_pcie_error_reporting(pcidev);
> > + pci_disable_device(pcidev);
> > +}
> > +
> > +static struct pci_driver cci_pci_driver = {
> > + .name = DRV_NAME,
> > + .id_table = cci_pcie_id_tbl,
> > + .probe = cci_pci_probe,
> > + .remove = cci_pci_remove,
> > +};
> > +
> > +static int __init ccidrv_init(void)
> > +{
> > + pr_info("Intel(R) FPGA PCIe Driver: Version %s\n", DRV_VERSION);
> > +
> > + return pci_register_driver(&cci_pci_driver);
> > +}
> > +
> > +static void __exit ccidrv_exit(void)
> > +{
> > + pci_unregister_driver(&cci_pci_driver);
> > +}
> > +
> > +module_init(ccidrv_init);
> > +module_exit(ccidrv_exit);
> > +
> > +MODULE_DESCRIPTION("Intel FPGA PCIe Device Driver");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 1.8.3.1
> >
Powered by blists - more mailing lists