2020-03-24 08:36:02

by Xu Yilun

Subject: [PATCH v3 2/7] fpga: dfl: pci: add irq info for feature devices enumeration

Some DFL FPGA PCIe cards (e.g. Intel FPGA Programmable Acceleration
Card) support MSI-X based interrupts. This patch allows PCIe driver
to prepare and pass interrupt resources to DFL via enumeration API.
These interrupt resources could then be assigned to actual features
which use them.

Signed-off-by: Luwei Kang <[email protected]>
Signed-off-by: Wu Hao <[email protected]>
Signed-off-by: Xu Yilun <[email protected]>
----
v2: put irq resources init code inside cce_enumerate_feature_dev()
Some minor changes for Hao's comments.
v3: Some minor fix for Hao's comments for v2.
---
drivers/fpga/dfl-pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 67 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 5387550..66027aa 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -39,6 +39,28 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
return pcim_iomap_table(pcidev)[bar];
}

+static int cci_pci_alloc_irq(struct pci_dev *pcidev)
+{
+ int nvec = pci_msix_vec_count(pcidev);
+ int ret;
+
+ if (nvec <= 0) {
+ dev_dbg(&pcidev->dev, "fpga interrupt not supported\n");
+ return 0;
+ }
+
+ ret = pci_alloc_irq_vectors(pcidev, nvec, nvec, PCI_IRQ_MSIX);
+ if (ret < 0)
+ return ret;
+
+ return nvec;
+}
+
+static void cci_pci_free_irq(struct pci_dev *pcidev)
+{
+ pci_free_irq_vectors(pcidev);
+}
+
/* PCI Device ID */
#define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
#define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
@@ -78,17 +100,33 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)

/* remove all children feature devices */
dfl_fpga_feature_devs_remove(drvdata->cdev);
+ cci_pci_free_irq(pcidev);
+}
+
+static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
+{
+ unsigned int i;
+ int *table;
+
+ table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
+ if (table) {
+ for (i = 0; i < nvec; i++)
+ table[i] = pci_irq_vector(pcidev, i);
+ }
+
+ return table;
}

/* enumerate feature devices under pci device */
static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
{
struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
+ int port_num, bar, i, nvec, ret = 0;
struct dfl_fpga_enum_info *info;
struct dfl_fpga_cdev *cdev;
resource_size_t start, len;
- int port_num, bar, i, ret = 0;
void __iomem *base;
+ int *irq_table;
u32 offset;
u64 v;

@@ -97,11 +135,30 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
if (!info)
return -ENOMEM;

+ /* add irq info for enumeration if the device support irq */
+ nvec = cci_pci_alloc_irq(pcidev);
+ if (nvec < 0) {
+ dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
+ ret = nvec;
+ goto enum_info_free_exit;
+ } else if (nvec) {
+ irq_table = cci_pci_create_irq_table(pcidev, nvec);
+ if (!irq_table) {
+ ret = -ENOMEM;
+ goto irq_free_exit;
+ }
+
+ ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
+ kfree(irq_table);
+ if (ret)
+ goto irq_free_exit;
+ }
+
/* start to find Device Feature List from Bar 0 */
base = cci_pci_ioremap_bar(pcidev, 0);
if (!base) {
ret = -ENOMEM;
- goto enum_info_free_exit;
+ goto irq_free_exit;
}

/*
@@ -154,7 +211,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
dfl_fpga_enum_info_add_dfl(info, start, len, base);
} else {
ret = -ENODEV;
- goto enum_info_free_exit;
+ goto irq_free_exit;
}

/* start enumeration with prepared enumeration information */
@@ -162,11 +219,14 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
if (IS_ERR(cdev)) {
dev_err(&pcidev->dev, "Enumeration failure\n");
ret = PTR_ERR(cdev);
- goto enum_info_free_exit;
+ goto irq_free_exit;
}

drvdata->cdev = cdev;

+irq_free_exit:
+ if (ret)
+ cci_pci_free_irq(pcidev);
enum_info_free_exit:
dfl_fpga_enum_info_free(info);

@@ -211,12 +271,10 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
}

ret = cci_enumerate_feature_devs(pcidev);
- if (ret) {
- dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
- goto disable_error_report_exit;
- }
+ if (!ret)
+ return ret;

- return ret;
+ dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);

disable_error_report_exit:
pci_disable_pcie_error_reporting(pcidev);
--
2.7.4

2020-03-31 05:05:01

by Wu Hao

Subject: Re: [PATCH v3 2/7] fpga: dfl: pci: add irq info for feature devices enumeration

On Tue, Mar 24, 2020 at 04:32:38PM +0800, Xu Yilun wrote:
> Some DFL FPGA PCIe cards (e.g. Intel FPGA Programmable Acceleration
> Card) support MSI-X based interrupts. This patch allows PCIe driver
> to prepare and pass interrupt resources to DFL via enumeration API.
> These interrupt resources could then be assigned to actual features
> which use them.
>
> Signed-off-by: Luwei Kang <[email protected]>
> Signed-off-by: Wu Hao <[email protected]>
> Signed-off-by: Xu Yilun <[email protected]>
> ----
> v2: put irq resources init code inside cce_enumerate_feature_dev()
> Some minor changes for Hao's comments.
> v3: Some minor fix for Hao's comments for v2.
> ---
> drivers/fpga/dfl-pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 5387550..66027aa 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -39,6 +39,28 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> return pcim_iomap_table(pcidev)[bar];
> }
>
> +static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> +{
> + int nvec = pci_msix_vec_count(pcidev);
> + int ret;

maybe int ret, nvec = pci_msix..

> +
> + if (nvec <= 0) {
> + dev_dbg(&pcidev->dev, "fpga interrupt not supported\n");
> + return 0;
> + }
> +
> + ret = pci_alloc_irq_vectors(pcidev, nvec, nvec, PCI_IRQ_MSIX);
> + if (ret < 0)
> + return ret;
> +
> + return nvec;
> +}
> +
> +static void cci_pci_free_irq(struct pci_dev *pcidev)
> +{
> + pci_free_irq_vectors(pcidev);
> +}
> +
> /* PCI Device ID */
> #define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
> #define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
> @@ -78,17 +100,33 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
>
> /* remove all children feature devices */
> dfl_fpga_feature_devs_remove(drvdata->cdev);
> + cci_pci_free_irq(pcidev);
> +}
> +
> +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> +{
> + unsigned int i;
> + int *table;
> +
> + table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> + if (table) {
> + for (i = 0; i < nvec; i++)
> + table[i] = pci_irq_vector(pcidev, i);
> + }
> +
> + return table;
> }
>
> /* enumerate feature devices under pci device */
> static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> {
> struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> + int port_num, bar, i, nvec, ret = 0;
> struct dfl_fpga_enum_info *info;
> struct dfl_fpga_cdev *cdev;
> resource_size_t start, len;
> - int port_num, bar, i, ret = 0;
> void __iomem *base;
> + int *irq_table;
> u32 offset;
> u64 v;
>
> @@ -97,11 +135,30 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> if (!info)
> return -ENOMEM;
>
> + /* add irq info for enumeration if the device support irq */
> + nvec = cci_pci_alloc_irq(pcidev);
> + if (nvec < 0) {
> + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
> + ret = nvec;
> + goto enum_info_free_exit;
> + } else if (nvec) {
> + irq_table = cci_pci_create_irq_table(pcidev, nvec);
> + if (!irq_table) {
> + ret = -ENOMEM;
> + goto irq_free_exit;
> + }
> +
> + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> + kfree(irq_table);

i see you create a function for cci_pci_free_irq instead of using kernel api
directly, to make it more readable, so why not have a remove irq table function
here too.

Actually patch looks good to me, with above minor fixes.

Acked-by: Wu Hao <[email protected]>

Hao

> + if (ret)
> + goto irq_free_exit;
> + }
> +
> /* start to find Device Feature List from Bar 0 */
> base = cci_pci_ioremap_bar(pcidev, 0);
> if (!base) {
> ret = -ENOMEM;
> - goto enum_info_free_exit;
> + goto irq_free_exit;
> }
>
> /*
> @@ -154,7 +211,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> dfl_fpga_enum_info_add_dfl(info, start, len, base);
> } else {
> ret = -ENODEV;
> - goto enum_info_free_exit;
> + goto irq_free_exit;
> }
>
> /* start enumeration with prepared enumeration information */
> @@ -162,11 +219,14 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> if (IS_ERR(cdev)) {
> dev_err(&pcidev->dev, "Enumeration failure\n");
> ret = PTR_ERR(cdev);
> - goto enum_info_free_exit;
> + goto irq_free_exit;
> }
>
> drvdata->cdev = cdev;
>
> +irq_free_exit:
> + if (ret)
> + cci_pci_free_irq(pcidev);
> enum_info_free_exit:
> dfl_fpga_enum_info_free(info);
>
> @@ -211,12 +271,10 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> }
>
> ret = cci_enumerate_feature_devs(pcidev);
> - if (ret) {
> - dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> - goto disable_error_report_exit;
> - }
> + if (!ret)
> + return ret;
>
> - return ret;
> + dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
>
> disable_error_report_exit:
> pci_disable_pcie_error_reporting(pcidev);
> --
> 2.7.4

2020-04-01 03:01:59

by Xu Yilun

Subject: Re: [PATCH v3 2/7] fpga: dfl: pci: add irq info for feature devices enumeration

On Tue, Mar 31, 2020 at 12:41:20PM +0800, Wu Hao wrote:
> On Tue, Mar 24, 2020 at 04:32:38PM +0800, Xu Yilun wrote:
> > Some DFL FPGA PCIe cards (e.g. Intel FPGA Programmable Acceleration
> > Card) support MSI-X based interrupts. This patch allows PCIe driver
> > to prepare and pass interrupt resources to DFL via enumeration API.
> > These interrupt resources could then be assigned to actual features
> > which use them.
> >
> > Signed-off-by: Luwei Kang <[email protected]>
> > Signed-off-by: Wu Hao <[email protected]>
> > Signed-off-by: Xu Yilun <[email protected]>
> > ----
> > v2: put irq resources init code inside cce_enumerate_feature_dev()
> > Some minor changes for Hao's comments.
> > v3: Some minor fix for Hao's comments for v2.
> > ---
> > drivers/fpga/dfl-pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 67 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index 5387550..66027aa 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -39,6 +39,28 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> > return pcim_iomap_table(pcidev)[bar];
> > }
> >
> > +static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> > +{
> > + int nvec = pci_msix_vec_count(pcidev);
> > + int ret;
>
> maybe int ret, nvec = pci_msix..
>
> > +
> > + if (nvec <= 0) {
> > + dev_dbg(&pcidev->dev, "fpga interrupt not supported\n");
> > + return 0;
> > + }
> > +
> > + ret = pci_alloc_irq_vectors(pcidev, nvec, nvec, PCI_IRQ_MSIX);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return nvec;
> > +}
> > +
> > +static void cci_pci_free_irq(struct pci_dev *pcidev)
> > +{
> > + pci_free_irq_vectors(pcidev);
> > +}
> > +
> > /* PCI Device ID */
> > #define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
> > #define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
> > @@ -78,17 +100,33 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
> >
> > /* remove all children feature devices */
> > dfl_fpga_feature_devs_remove(drvdata->cdev);
> > + cci_pci_free_irq(pcidev);
> > +}
> > +
> > +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > +{
> > + unsigned int i;
> > + int *table;
> > +
> > + table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> > + if (table) {
> > + for (i = 0; i < nvec; i++)
> > + table[i] = pci_irq_vector(pcidev, i);
> > + }
> > +
> > + return table;
> > }
> >
> > /* enumerate feature devices under pci device */
> > static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > {
> > struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > + int port_num, bar, i, nvec, ret = 0;
> > struct dfl_fpga_enum_info *info;
> > struct dfl_fpga_cdev *cdev;
> > resource_size_t start, len;
> > - int port_num, bar, i, ret = 0;
> > void __iomem *base;
> > + int *irq_table;
> > u32 offset;
> > u64 v;
> >
> > @@ -97,11 +135,30 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > if (!info)
> > return -ENOMEM;
> >
> > + /* add irq info for enumeration if the device support irq */
> > + nvec = cci_pci_alloc_irq(pcidev);
> > + if (nvec < 0) {
> > + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
> > + ret = nvec;
> > + goto enum_info_free_exit;
> > + } else if (nvec) {
> > + irq_table = cci_pci_create_irq_table(pcidev, nvec);
> > + if (!irq_table) {
> > + ret = -ENOMEM;
> > + goto irq_free_exit;
> > + }
> > +
> > + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> > + kfree(irq_table);
>
> i see you create a function for cci_pci_free_irq instead of using kernel api
> directly, to make it more readable, so why not have a remove irq table function
> here too.

The irq_table is not alloced in cci_pci_alloc_irq,
cci_pci_create_irq_table does. Actually cci_pci_alloc/free_irq are not
related to irq_table for DFL.

So maybe we don't have to change this?

>
> Actually patch looks good to me, with above minor fixes.
>
> Acked-by: Wu Hao <[email protected]>
>
> Hao
>
> > + if (ret)
> > + goto irq_free_exit;
> > + }
> > +
> > /* start to find Device Feature List from Bar 0 */
> > base = cci_pci_ioremap_bar(pcidev, 0);
> > if (!base) {
> > ret = -ENOMEM;
> > - goto enum_info_free_exit;
> > + goto irq_free_exit;
> > }
> >
> > /*
> > @@ -154,7 +211,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > } else {
> > ret = -ENODEV;
> > - goto enum_info_free_exit;
> > + goto irq_free_exit;
> > }
> >
> > /* start enumeration with prepared enumeration information */
> > @@ -162,11 +219,14 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > if (IS_ERR(cdev)) {
> > dev_err(&pcidev->dev, "Enumeration failure\n");
> > ret = PTR_ERR(cdev);
> > - goto enum_info_free_exit;
> > + goto irq_free_exit;
> > }
> >
> > drvdata->cdev = cdev;
> >
> > +irq_free_exit:
> > + if (ret)
> > + cci_pci_free_irq(pcidev);
> > enum_info_free_exit:
> > dfl_fpga_enum_info_free(info);
> >
> > @@ -211,12 +271,10 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > }
> >
> > ret = cci_enumerate_feature_devs(pcidev);
> > - if (ret) {
> > - dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > - goto disable_error_report_exit;
> > - }
> > + if (!ret)
> > + return ret;
> >
> > - return ret;
> > + dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> >
> > disable_error_report_exit:
> > pci_disable_pcie_error_reporting(pcidev);
> > --
> > 2.7.4

2020-04-01 04:06:21

by Wu Hao

Subject: Re: [PATCH v3 2/7] fpga: dfl: pci: add irq info for feature devices enumeration

On Wed, Apr 01, 2020 at 10:59:02AM +0800, Xu Yilun wrote:
> On Tue, Mar 31, 2020 at 12:41:20PM +0800, Wu Hao wrote:
> > On Tue, Mar 24, 2020 at 04:32:38PM +0800, Xu Yilun wrote:
> > > Some DFL FPGA PCIe cards (e.g. Intel FPGA Programmable Acceleration
> > > Card) support MSI-X based interrupts. This patch allows PCIe driver
> > > to prepare and pass interrupt resources to DFL via enumeration API.
> > > These interrupt resources could then be assigned to actual features
> > > which use them.
> > >
> > > Signed-off-by: Luwei Kang <[email protected]>
> > > Signed-off-by: Wu Hao <[email protected]>
> > > Signed-off-by: Xu Yilun <[email protected]>
> > > ----
> > > v2: put irq resources init code inside cce_enumerate_feature_dev()
> > > Some minor changes for Hao's comments.
> > > v3: Some minor fix for Hao's comments for v2.
> > > ---
> > > drivers/fpga/dfl-pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 67 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > index 5387550..66027aa 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -39,6 +39,28 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> > > return pcim_iomap_table(pcidev)[bar];
> > > }
> > >
> > > +static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> > > +{
> > > + int nvec = pci_msix_vec_count(pcidev);
> > > + int ret;
> >
> > maybe int ret, nvec = pci_msix..
> >
> > > +
> > > + if (nvec <= 0) {
> > > + dev_dbg(&pcidev->dev, "fpga interrupt not supported\n");
> > > + return 0;
> > > + }
> > > +
> > > + ret = pci_alloc_irq_vectors(pcidev, nvec, nvec, PCI_IRQ_MSIX);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return nvec;
> > > +}
> > > +
> > > +static void cci_pci_free_irq(struct pci_dev *pcidev)
> > > +{
> > > + pci_free_irq_vectors(pcidev);
> > > +}
> > > +
> > > /* PCI Device ID */
> > > #define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
> > > #define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
> > > @@ -78,17 +100,33 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
> > >
> > > /* remove all children feature devices */
> > > dfl_fpga_feature_devs_remove(drvdata->cdev);
> > > + cci_pci_free_irq(pcidev);
> > > +}
> > > +
> > > +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > > +{
> > > + unsigned int i;
> > > + int *table;
> > > +
> > > + table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> > > + if (table) {
> > > + for (i = 0; i < nvec; i++)
> > > + table[i] = pci_irq_vector(pcidev, i);
> > > + }
> > > +
> > > + return table;
> > > }
> > >
> > > /* enumerate feature devices under pci device */
> > > static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > {
> > > struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > > + int port_num, bar, i, nvec, ret = 0;
> > > struct dfl_fpga_enum_info *info;
> > > struct dfl_fpga_cdev *cdev;
> > > resource_size_t start, len;
> > > - int port_num, bar, i, ret = 0;
> > > void __iomem *base;
> > > + int *irq_table;
> > > u32 offset;
> > > u64 v;
> > >
> > > @@ -97,11 +135,30 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > if (!info)
> > > return -ENOMEM;
> > >
> > > + /* add irq info for enumeration if the device support irq */
> > > + nvec = cci_pci_alloc_irq(pcidev);
> > > + if (nvec < 0) {
> > > + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
> > > + ret = nvec;
> > > + goto enum_info_free_exit;
> > > + } else if (nvec) {
> > > + irq_table = cci_pci_create_irq_table(pcidev, nvec);
> > > + if (!irq_table) {
> > > + ret = -ENOMEM;
> > > + goto irq_free_exit;
> > > + }
> > > +
> > > + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> > > + kfree(irq_table);
> >
> > i see you create a function for cci_pci_free_irq instead of using kernel api
> > directly, to make it more readable, so why not have a remove irq table function
> > here too.
>
> The irq_table is not alloced in cci_pci_alloc_irq,
> cci_pci_create_irq_table does. Actually cci_pci_alloc/free_irq are not
> related to irq_table for DFL.
>
> So maybe we don't have to change this?

I mean, you have cci_pci_alloc/free_irq but cci_pci_create_irq_table/kfree.
why don't you use cci_pci_remove_irq_table or something instead of kfree?

Hao

>
> >
> > Actually patch looks good to me, with above minor fixes.
> >
> > Acked-by: Wu Hao <[email protected]>
> >
> > Hao
> >
> > > + if (ret)
> > > + goto irq_free_exit;
> > > + }
> > > +
> > > /* start to find Device Feature List from Bar 0 */
> > > base = cci_pci_ioremap_bar(pcidev, 0);
> > > if (!base) {
> > > ret = -ENOMEM;
> > > - goto enum_info_free_exit;
> > > + goto irq_free_exit;
> > > }
> > >
> > > /*
> > > @@ -154,7 +211,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > > } else {
> > > ret = -ENODEV;
> > > - goto enum_info_free_exit;
> > > + goto irq_free_exit;
> > > }
> > >
> > > /* start enumeration with prepared enumeration information */
> > > @@ -162,11 +219,14 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > if (IS_ERR(cdev)) {
> > > dev_err(&pcidev->dev, "Enumeration failure\n");
> > > ret = PTR_ERR(cdev);
> > > - goto enum_info_free_exit;
> > > + goto irq_free_exit;
> > > }
> > >
> > > drvdata->cdev = cdev;
> > >
> > > +irq_free_exit:
> > > + if (ret)
> > > + cci_pci_free_irq(pcidev);
> > > enum_info_free_exit:
> > > dfl_fpga_enum_info_free(info);
> > >
> > > @@ -211,12 +271,10 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > > }
> > >
> > > ret = cci_enumerate_feature_devs(pcidev);
> > > - if (ret) {
> > > - dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > > - goto disable_error_report_exit;
> > > - }
> > > + if (!ret)
> > > + return ret;
> > >
> > > - return ret;
> > > + dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > >
> > > disable_error_report_exit:
> > > pci_disable_pcie_error_reporting(pcidev);
> > > --
> > > 2.7.4

2020-04-01 05:45:25

by Xu Yilun

Subject: Re: [PATCH v3 2/7] fpga: dfl: pci: add irq info for feature devices enumeration

On Wed, Apr 01, 2020 at 11:44:30AM +0800, Wu Hao wrote:
> On Wed, Apr 01, 2020 at 10:59:02AM +0800, Xu Yilun wrote:
> > On Tue, Mar 31, 2020 at 12:41:20PM +0800, Wu Hao wrote:
> > > On Tue, Mar 24, 2020 at 04:32:38PM +0800, Xu Yilun wrote:
> > > > Some DFL FPGA PCIe cards (e.g. Intel FPGA Programmable Acceleration
> > > > Card) support MSI-X based interrupts. This patch allows PCIe driver
> > > > to prepare and pass interrupt resources to DFL via enumeration API.
> > > > These interrupt resources could then be assigned to actual features
> > > > which use them.
> > > >
> > > > Signed-off-by: Luwei Kang <[email protected]>
> > > > Signed-off-by: Wu Hao <[email protected]>
> > > > Signed-off-by: Xu Yilun <[email protected]>
> > > > ----
> > > > v2: put irq resources init code inside cce_enumerate_feature_dev()
> > > > Some minor changes for Hao's comments.
> > > > v3: Some minor fix for Hao's comments for v2.
> > > > ---
> > > > drivers/fpga/dfl-pci.c | 76 ++++++++++++++++++++++++++++++++++++++++++++------
> > > > 1 file changed, 67 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > > index 5387550..66027aa 100644
> > > > --- a/drivers/fpga/dfl-pci.c
> > > > +++ b/drivers/fpga/dfl-pci.c
> > > > @@ -39,6 +39,28 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> > > > return pcim_iomap_table(pcidev)[bar];
> > > > }
> > > >
> > > > +static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> > > > +{
> > > > + int nvec = pci_msix_vec_count(pcidev);
> > > > + int ret;
> > >
> > > maybe int ret, nvec = pci_msix..
> > >
> > > > +
> > > > + if (nvec <= 0) {
> > > > + dev_dbg(&pcidev->dev, "fpga interrupt not supported\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + ret = pci_alloc_irq_vectors(pcidev, nvec, nvec, PCI_IRQ_MSIX);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return nvec;
> > > > +}
> > > > +
> > > > +static void cci_pci_free_irq(struct pci_dev *pcidev)
> > > > +{
> > > > + pci_free_irq_vectors(pcidev);
> > > > +}
> > > > +
> > > > /* PCI Device ID */
> > > > #define PCIE_DEVICE_ID_PF_INT_5_X 0xBCBD
> > > > #define PCIE_DEVICE_ID_PF_INT_6_X 0xBCC0
> > > > @@ -78,17 +100,33 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
> > > >
> > > > /* remove all children feature devices */
> > > > dfl_fpga_feature_devs_remove(drvdata->cdev);
> > > > + cci_pci_free_irq(pcidev);
> > > > +}
> > > > +
> > > > +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > > > +{
> > > > + unsigned int i;
> > > > + int *table;
> > > > +
> > > > + table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> > > > + if (table) {
> > > > + for (i = 0; i < nvec; i++)
> > > > + table[i] = pci_irq_vector(pcidev, i);
> > > > + }
> > > > +
> > > > + return table;
> > > > }
> > > >
> > > > /* enumerate feature devices under pci device */
> > > > static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > > {
> > > > struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > > > + int port_num, bar, i, nvec, ret = 0;
> > > > struct dfl_fpga_enum_info *info;
> > > > struct dfl_fpga_cdev *cdev;
> > > > resource_size_t start, len;
> > > > - int port_num, bar, i, ret = 0;
> > > > void __iomem *base;
> > > > + int *irq_table;
> > > > u32 offset;
> > > > u64 v;
> > > >
> > > > @@ -97,11 +135,30 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > > if (!info)
> > > > return -ENOMEM;
> > > >
> > > > + /* add irq info for enumeration if the device support irq */
> > > > + nvec = cci_pci_alloc_irq(pcidev);
> > > > + if (nvec < 0) {
> > > > + dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
> > > > + ret = nvec;
> > > > + goto enum_info_free_exit;
> > > > + } else if (nvec) {
> > > > + irq_table = cci_pci_create_irq_table(pcidev, nvec);
> > > > + if (!irq_table) {
> > > > + ret = -ENOMEM;
> > > > + goto irq_free_exit;
> > > > + }
> > > > +
> > > > + ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> > > > + kfree(irq_table);
> > >
> > > i see you create a function for cci_pci_free_irq instead of using kernel api
> > > directly, to make it more readable, so why not have a remove irq table function
> > > here too.
> >
> > The irq_table is not alloced in cci_pci_alloc_irq,
> > cci_pci_create_irq_table does. Actually cci_pci_alloc/free_irq are not
> > related to irq_table for DFL.
> >
> > So maybe we don't have to change this?
>
> I mean, you have cci_pci_alloc/free_irq but cci_pci_create_irq_table/kfree.
> why don't you use cci_pci_remove_irq_table or something instead of kfree?

Sorry for my misunderstanding. I can make this change. Thanks.

>
> Hao
>
> >
> > >
> > > Actually patch looks good to me, with above minor fixes.
> > >
> > > Acked-by: Wu Hao <[email protected]>
> > >
> > > Hao
> > >
> > > > + if (ret)
> > > > + goto irq_free_exit;
> > > > + }
> > > > +
> > > > /* start to find Device Feature List from Bar 0 */
> > > > base = cci_pci_ioremap_bar(pcidev, 0);
> > > > if (!base) {
> > > > ret = -ENOMEM;
> > > > - goto enum_info_free_exit;
> > > > + goto irq_free_exit;
> > > > }
> > > >
> > > > /*
> > > > @@ -154,7 +211,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > > dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > > > } else {
> > > > ret = -ENODEV;
> > > > - goto enum_info_free_exit;
> > > > + goto irq_free_exit;
> > > > }
> > > >
> > > > /* start enumeration with prepared enumeration information */
> > > > @@ -162,11 +219,14 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > > if (IS_ERR(cdev)) {
> > > > dev_err(&pcidev->dev, "Enumeration failure\n");
> > > > ret = PTR_ERR(cdev);
> > > > - goto enum_info_free_exit;
> > > > + goto irq_free_exit;
> > > > }
> > > >
> > > > drvdata->cdev = cdev;
> > > >
> > > > +irq_free_exit:
> > > > + if (ret)
> > > > + cci_pci_free_irq(pcidev);
> > > > enum_info_free_exit:
> > > > dfl_fpga_enum_info_free(info);
> > > >
> > > > @@ -211,12 +271,10 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > > > }
> > > >
> > > > ret = cci_enumerate_feature_devs(pcidev);
> > > > - if (ret) {
> > > > - dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > > > - goto disable_error_report_exit;
> > > > - }
> > > > + if (!ret)
> > > > + return ret;
> > > >
> > > > - return ret;
> > > > + dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > > >
> > > > disable_error_report_exit:
> > > > pci_disable_pcie_error_reporting(pcidev);
> > > > --
> > > > 2.7.4