diff mbox series

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

Message ID 1583749790-10837-3-git-send-email-yilun.xu@intel.com (mailing list archive)
State Superseded, archived
Headers show
Series Add interrupt support to FPGA DFL drivers | expand

Commit Message

Xu Yilun March 9, 2020, 10:29 a.m. UTC 
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 <luwei.kang@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 drivers/fpga/dfl-pci.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 5 deletions(-)

Comments

Wu, Hao March 10, 2020, 2:46 a.m. UTC | #1 
Hi Yilun

Some comments inline. : )

On Mon, Mar 09, 2020 at 06:29:45PM +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 <luwei.kang@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  drivers/fpga/dfl-pci.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 5387550..a3370e5 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -80,8 +80,23 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
>  	dfl_fpga_feature_devs_remove(drvdata->cdev);
>  }
>  
> +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> +{
> +	int *table, i;
> +
> +	table = kcalloc(nvec, sizeof(int), GFP_KERNEL);

Maybe devm_ version is better?

> +	if (!table)
> +		return NULL;
> +
> +	for (i = 0; i < nvec; i++)

i should be unsigned int as well?

> +		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)
> +static int cci_enumerate_feature_devs(struct pci_dev *pcidev,
> +				      unsigned int nvec)
>  {
>  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
>  	struct dfl_fpga_enum_info *info;
> @@ -89,6 +104,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  	resource_size_t start, len;
>  	int port_num, bar, i, ret = 0;
>  	void __iomem *base;
> +	int *irq_table;
>  	u32 offset;
>  	u64 v;
>  
> @@ -97,6 +113,18 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  	if (!info)
>  		return -ENOMEM;
>  
> +	/* add irq info for enumeration if really needed */
> +	if (nvec) {
> +		irq_table = cci_pci_create_irq_table(pcidev, nvec);
> +		if (irq_table) {
> +			dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> +			kfree(irq_table);
> +		} else {
> +			ret = -ENOMEM;
> +			goto enum_info_free_exit;
> +		}
> +	}
> +
>  	/* start to find Device Feature List from Bar 0 */
>  	base = cci_pci_ioremap_bar(pcidev, 0);
>  	if (!base) {
> @@ -173,6 +201,28 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  	return ret;
>  }
>  
> +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);
> +}
> +
>  static
>  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
>  {
> @@ -210,14 +260,19 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
>  		goto disable_error_report_exit;
>  	}
>  
> -	ret = cci_enumerate_feature_devs(pcidev);
> -	if (ret) {
> -		dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> +	ret = cci_pci_alloc_irq(pcidev);
> +	if (ret < 0) {
> +		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", ret);

we prepare mmio resources in side cci_enumerate_feature_devs.

maybe we could put irq resources code in side cce_enumerate_feature_devs too?


Thanks
Hao

>  		goto disable_error_report_exit;
>  	}
>  
> -	return ret;
> +	ret = cci_enumerate_feature_devs(pcidev, (unsigned int)ret);
> +	if (!ret)
> +		return ret;
> +
> +	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
>  
> +	cci_pci_free_irq(pcidev);
>  disable_error_report_exit:
>  	pci_disable_pcie_error_reporting(pcidev);
>  	return ret;
> @@ -263,6 +318,7 @@ static void cci_pci_remove(struct pci_dev *pcidev)
>  		cci_pci_sriov_configure(pcidev, 0);
>  
>  	cci_remove_feature_devs(pcidev);
> +	cci_pci_free_irq(pcidev);
>  	pci_disable_pcie_error_reporting(pcidev);
>  }
>  
> -- 
> 2.7.4
Xu Yilun March 10, 2020, 9:41 a.m. UTC | #2 
On Tue, Mar 10, 2020 at 10:46:26AM +0800, Wu Hao wrote:
> Hi Yilun
> 
> Some comments inline. : )
> 
> On Mon, Mar 09, 2020 at 06:29:45PM +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 <luwei.kang@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> >  drivers/fpga/dfl-pci.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 61 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index 5387550..a3370e5 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -80,8 +80,23 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
> >  	dfl_fpga_feature_devs_remove(drvdata->cdev);
> >  }
> >  
> > +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > +{
> > +	int *table, i;
> > +
> > +	table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> 
> Maybe devm_ version is better?

This table will be freed right after dfl_fpga_enum_info_add_irq(). Maybe
don't need devm_ version.

> 
> > +	if (!table)
> > +		return NULL;
> > +
> > +	for (i = 0; i < nvec; i++)
> 
> i should be unsigned int as well?

Yes I should change it.

> 
> > +		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)
> > +static int cci_enumerate_feature_devs(struct pci_dev *pcidev,
> > +				      unsigned int nvec)
> >  {
> >  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> >  	struct dfl_fpga_enum_info *info;
> > @@ -89,6 +104,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  	resource_size_t start, len;
> >  	int port_num, bar, i, ret = 0;
> >  	void __iomem *base;
> > +	int *irq_table;
> >  	u32 offset;
> >  	u64 v;
> >  
> > @@ -97,6 +113,18 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  	if (!info)
> >  		return -ENOMEM;
> >  
> > +	/* add irq info for enumeration if really needed */
> > +	if (nvec) {
> > +		irq_table = cci_pci_create_irq_table(pcidev, nvec);
> > +		if (irq_table) {
> > +			dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> > +			kfree(irq_table);
> > +		} else {
> > +			ret = -ENOMEM;
> > +			goto enum_info_free_exit;
> > +		}
> > +	}
> > +
> >  	/* start to find Device Feature List from Bar 0 */
> >  	base = cci_pci_ioremap_bar(pcidev, 0);
> >  	if (!base) {
> > @@ -173,6 +201,28 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  	return ret;
> >  }
> >  
> > +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);
> > +}
> > +
> >  static
> >  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> >  {
> > @@ -210,14 +260,19 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> >  		goto disable_error_report_exit;
> >  	}
> >  
> > -	ret = cci_enumerate_feature_devs(pcidev);
> > -	if (ret) {
> > -		dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > +	ret = cci_pci_alloc_irq(pcidev);
> > +	if (ret < 0) {
> > +		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", ret);
> 
> we prepare mmio resources in side cci_enumerate_feature_devs.
> 
> maybe we could put irq resources code in side cce_enumerate_feature_devs too?

Yes I will try to change it in patch v2.

Thanks for your quick response.

> 
> 
> Thanks
> Hao
> 
> >  		goto disable_error_report_exit;
> >  	}
> >  
> > -	return ret;
> > +	ret = cci_enumerate_feature_devs(pcidev, (unsigned int)ret);
> > +	if (!ret)
> > +		return ret;
> > +
> > +	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> >  
> > +	cci_pci_free_irq(pcidev);
> >  disable_error_report_exit:
> >  	pci_disable_pcie_error_reporting(pcidev);
> >  	return ret;
> > @@ -263,6 +318,7 @@ static void cci_pci_remove(struct pci_dev *pcidev)
> >  		cci_pci_sriov_configure(pcidev, 0);
> >  
> >  	cci_remove_feature_devs(pcidev);
> > +	cci_pci_free_irq(pcidev);
> >  	pci_disable_pcie_error_reporting(pcidev);
> >  }
> >  
> > -- 
> > 2.7.4
Wu, Hao March 10, 2020, 10:26 a.m. UTC | #3 
On Tue, Mar 10, 2020 at 05:41:43PM +0800, Xu Yilun wrote:
> On Tue, Mar 10, 2020 at 10:46:26AM +0800, Wu Hao wrote:
> > Hi Yilun
> > 
> > Some comments inline. : )
> > 
> > On Mon, Mar 09, 2020 at 06:29:45PM +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 <luwei.kang@intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > ---
> > >  drivers/fpga/dfl-pci.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 61 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > index 5387550..a3370e5 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -80,8 +80,23 @@ static void cci_remove_feature_devs(struct pci_dev *pcidev)
> > >  	dfl_fpga_feature_devs_remove(drvdata->cdev);
> > >  }
> > >  
> > > +static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> > > +{
> > > +	int *table, i;
> > > +
> > > +	table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
> > 
> > Maybe devm_ version is better?
> 
> This table will be freed right after dfl_fpga_enum_info_add_irq(). Maybe
> don't need devm_ version.
> 
> > 
> > > +	if (!table)
> > > +		return NULL;
> > > +
> > > +	for (i = 0; i < nvec; i++)
> > 
> > i should be unsigned int as well?
> 
> Yes I should change it.
> 
> > 
> > > +		table[i] = pci_irq_vector(pcidev, i);

Hi Yilun,

one more place here.


table = kcalloc(nevc, sizeof(int), GFP_KERNEL);
if (table) {
	...
}	

return table;

Thanks
Hao

> > > +
> > > +	return table;
> > > +}
> > > +
> > >  /* enumerate feature devices under pci device */
> > > -static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > > +static int cci_enumerate_feature_devs(struct pci_dev *pcidev,
> > > +				      unsigned int nvec)
> > >  {
> > >  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > >  	struct dfl_fpga_enum_info *info;
> > > @@ -89,6 +104,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > >  	resource_size_t start, len;
> > >  	int port_num, bar, i, ret = 0;
> > >  	void __iomem *base;
> > > +	int *irq_table;
> > >  	u32 offset;
> > >  	u64 v;
> > >  
> > > @@ -97,6 +113,18 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > >  	if (!info)
> > >  		return -ENOMEM;
> > >  
> > > +	/* add irq info for enumeration if really needed */
> > > +	if (nvec) {
> > > +		irq_table = cci_pci_create_irq_table(pcidev, nvec);
> > > +		if (irq_table) {
> > > +			dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
> > > +			kfree(irq_table);
> > > +		} else {
> > > +			ret = -ENOMEM;
> > > +			goto enum_info_free_exit;
> > > +		}
> > > +	}
> > > +
> > >  	/* start to find Device Feature List from Bar 0 */
> > >  	base = cci_pci_ioremap_bar(pcidev, 0);
> > >  	if (!base) {
> > > @@ -173,6 +201,28 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> > >  	return ret;
> > >  }
> > >  
> > > +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);
> > > +}
> > > +
> > >  static
> > >  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > >  {
> > > @@ -210,14 +260,19 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
> > >  		goto disable_error_report_exit;
> > >  	}
> > >  
> > > -	ret = cci_enumerate_feature_devs(pcidev);
> > > -	if (ret) {
> > > -		dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > > +	ret = cci_pci_alloc_irq(pcidev);
> > > +	if (ret < 0) {
> > > +		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", ret);
> > 
> > we prepare mmio resources in side cci_enumerate_feature_devs.
> > 
> > maybe we could put irq resources code in side cce_enumerate_feature_devs too?
> 
> Yes I will try to change it in patch v2.
> 
> Thanks for your quick response.
> 
> > 
> > 
> > Thanks
> > Hao
> > 
> > >  		goto disable_error_report_exit;
> > >  	}
> > >  
> > > -	return ret;
> > > +	ret = cci_enumerate_feature_devs(pcidev, (unsigned int)ret);
> > > +	if (!ret)
> > > +		return ret;
> > > +
> > > +	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
> > >  
> > > +	cci_pci_free_irq(pcidev);
> > >  disable_error_report_exit:
> > >  	pci_disable_pcie_error_reporting(pcidev);
> > >  	return ret;
> > > @@ -263,6 +318,7 @@ static void cci_pci_remove(struct pci_dev *pcidev)
> > >  		cci_pci_sriov_configure(pcidev, 0);
> > >  
> > >  	cci_remove_feature_devs(pcidev);
> > > +	cci_pci_free_irq(pcidev);
> > >  	pci_disable_pcie_error_reporting(pcidev);
> > >  }
> > >  
> > > -- 
> > > 2.7.4
diff mbox series

Patch

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 5387550..a3370e5 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -80,8 +80,23 @@  static void cci_remove_feature_devs(struct pci_dev *pcidev)
 	dfl_fpga_feature_devs_remove(drvdata->cdev);
 }
 
+static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
+{
+	int *table, i;
+
+	table = kcalloc(nvec, sizeof(int), GFP_KERNEL);
+	if (!table)
+		return NULL;
+
+	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)
+static int cci_enumerate_feature_devs(struct pci_dev *pcidev,
+				      unsigned int nvec)
 {
 	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
 	struct dfl_fpga_enum_info *info;
@@ -89,6 +104,7 @@  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 	resource_size_t start, len;
 	int port_num, bar, i, ret = 0;
 	void __iomem *base;
+	int *irq_table;
 	u32 offset;
 	u64 v;
 
@@ -97,6 +113,18 @@  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 	if (!info)
 		return -ENOMEM;
 
+	/* add irq info for enumeration if really needed */
+	if (nvec) {
+		irq_table = cci_pci_create_irq_table(pcidev, nvec);
+		if (irq_table) {
+			dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
+			kfree(irq_table);
+		} else {
+			ret = -ENOMEM;
+			goto enum_info_free_exit;
+		}
+	}
+
 	/* start to find Device Feature List from Bar 0 */
 	base = cci_pci_ioremap_bar(pcidev, 0);
 	if (!base) {
@@ -173,6 +201,28 @@  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 	return ret;
 }
 
+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);
+}
+
 static
 int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
 {
@@ -210,14 +260,19 @@  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
 		goto disable_error_report_exit;
 	}
 
-	ret = cci_enumerate_feature_devs(pcidev);
-	if (ret) {
-		dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
+	ret = cci_pci_alloc_irq(pcidev);
+	if (ret < 0) {
+		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", ret);
 		goto disable_error_report_exit;
 	}
 
-	return ret;
+	ret = cci_enumerate_feature_devs(pcidev, (unsigned int)ret);
+	if (!ret)
+		return ret;
+
+	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
 
+	cci_pci_free_irq(pcidev);
 disable_error_report_exit:
 	pci_disable_pcie_error_reporting(pcidev);
 	return ret;
@@ -263,6 +318,7 @@  static void cci_pci_remove(struct pci_dev *pcidev)
 		cci_pci_sriov_configure(pcidev, 0);
 
 	cci_remove_feature_devs(pcidev);
+	cci_pci_free_irq(pcidev);
 	pci_disable_pcie_error_reporting(pcidev);
 }