linux-kernel - RE: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BE8371DA886269458E0220A16DC1F8277D619E1C@SHSMSX101.ccr.corp.intel.com>
Date:   Wed, 5 Apr 2017 13:14:46 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     Moritz Fischer <mdf@...nel.org>
CC:     "atull@...nel.org" <atull@...nel.org>,
        "moritz.fischer@...us.com" <moritz.fischer@...us.com>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...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>,
        Xiao Guangrong <guangrong.xiao@...ux.intel.com>,
        "Wu, Hao" <hao.wu@...el.com>
Subject: RE: [PATCH 03/16] fpga: intel: add FPGA PCIe device driver

> > +#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	"EXPERIMENTAL VERSION"
> 
> Is that a leftover? :)

Sorry, will fix this.

> > +#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);
> > +		goto exit;
> Why not 'return ret' here ?

Yes, you are right, will fix this.

> > +	}
> > +
> > +	ret = pci_enable_pcie_error_reporting(pcidev);
> > +	if (ret && ret != -EINVAL)
> > +		dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);
> 
> What if it is EINVAL?

pci_enable_pcie_error_reporting is always return -EINVAL when CONFIG_PCIEAER is not selected.
Then we don't need this boring message. : )

> 
> > +
> > +	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);
> > +
> > +	if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(64))) {
> > +		dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(64));
> > +	} else if (!dma_set_mask(&pcidev->dev, DMA_BIT_MASK(32))) {
> > +		dma_set_coherent_mask(&pcidev->dev, DMA_BIT_MASK(32));
> > +	} 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);
> > +exit:
> > +	return ret;
> If you return as suggested above, this can go away.

Yes, you are right. Will fix this in next version.
Thanks a lot for your review and comments. : )

Hao