[PATCH 0/7] Add interrupt support to FPGA DFL drivers

[PATCH 0/7] Add interrupt support to FPGA DFL drivers

[Xu Yilun]

This patchset add interrupt support to FPGA DFL drivers.

With these patches, DFL driver will parse and assign interrupt resources
for enumerated feature devices and their sub features.

This patchset also introduces a set of APIs for user to monitor DFL
interrupts. Three sub features (DFL FME error, DFL AFU error and user
interrupt) drivers now support these APIs.

Patch #1: DFL framework change. Accept interrupt info input from DFL bus
	  driver, and add interrupt parsing and assignment for feature
	  sub devices.
Patch #2: DFL pci driver change, add interrupt info on DFL enumeration.
Patch #3: DFL framework change. Add helper functions for feature sub
	  device drivers to handle interrupt and notify users.
Patch #4: Add interrupt support for AFU error reporting sub feature.
Patch #5: Add interrupt support for FME global error reporting sub
	  feature.
Patch #6: Add interrupt support for a new sub feature, to handle user
	  interrupts implemented in AFU.
Patch #7: Documentation for DFL interrupt handling.

Xu Yilun (7):
  fpga: dfl: parse interrupt info for feature devices on enumeration
  fpga: dfl: pci: add irq info for feature devices enumeration
  fpga: dfl: introduce interrupt trigger setting API
  fpga: dfl: afu: add interrupt support for error reporting
  fpga: dfl: fme: add interrupt support for global error reporting
  fpga: dfl: afu: add user interrupt support
  Documentation: fpga: dfl: add descriptions for interrupt related
    interfaces.

 Documentation/fpga/dfl.rst    |  17 +++
 drivers/fpga/dfl-afu-error.c  |  69 +++++++++++++
 drivers/fpga/dfl-afu-main.c   |  83 +++++++++++++++
 drivers/fpga/dfl-fme-error.c  |  71 +++++++++++++
 drivers/fpga/dfl-fme-main.c   |   6 ++
 drivers/fpga/dfl-pci.c        |  66 +++++++++++-
 drivers/fpga/dfl.c            | 233 +++++++++++++++++++++++++++++++++++++++++-
 drivers/fpga/dfl.h            |  51 +++++++++
 include/uapi/linux/fpga-dfl.h |  89 ++++++++++++++++
 9 files changed, 676 insertions(+), 9 deletions(-)

-- 
2.7.4

[PATCH 1/7] fpga: dfl: parse interrupt info for feature devices on enumeration

[Xu Yilun]

DFL based FPGA devices could support interrupts for different purposes,
but current DFL framework only supports feature device enumeration with
given MMIO resources information via common DFL headers. This patch
introduces one new API dfl_fpga_enum_info_add_irq for low level bus
drivers (e.g. PCIe device driver) to pass its interrupt resources
information to DFL framework for enumeration, and also adds interrupt
enumeration code in framework to parse and assign interrupt resources
for enumerated feature devices and their own sub features.

With this patch, DFL framework enumerates interrupt resources for core
features, including PORT Error Reporting, FME (FPGA Management Engine)
Error Reporting and also AFU User Interrupts.

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.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/fpga/dfl.h |  40 +++++++++++++++
 2 files changed, 176 insertions(+), 4 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 9909948..493822d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -11,6 +11,7 @@
  *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
  */
 #include <linux/module.h>
+#include <asm/irq.h>
 
 #include "dfl.h"
 
@@ -421,6 +422,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
  *
  * @dev: device to enumerate.
  * @cdev: the container device for all feature devices.
+ * @nr_irqs: number of irqs for all feature devices.
+ * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
+ *	       this device.
  * @feature_dev: current feature device.
  * @ioaddr: header register region address of feature device in enumeration.
  * @sub_features: a sub features linked list for feature device in enumeration.
@@ -429,6 +433,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
 struct build_feature_devs_info {
 	struct device *dev;
 	struct dfl_fpga_cdev *cdev;
+	unsigned int nr_irqs;
+	int *irq_table;
+
 	struct platform_device *feature_dev;
 	void __iomem *ioaddr;
 	struct list_head sub_features;
@@ -442,12 +449,16 @@ struct build_feature_devs_info {
  * @mmio_res: mmio resource of this sub feature.
  * @ioaddr: mapped base address of mmio resource.
  * @node: node in sub_features linked list.
+ * @irq_base: start of irq index in this sub feature.
+ * @nr_irqs: number of irqs of this sub feature.
  */
 struct dfl_feature_info {
 	u64 fid;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
+	unsigned int irq_base;
+	unsigned int nr_irqs;
 };
 
 static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -520,6 +531,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 	/* fill features and resource information for feature dev */
 	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
 		struct dfl_feature *feature = &pdata->features[index];
+		struct dfl_feature_irq_ctx *ctx;
+		int i, virq;
 
 		/* save resource information for each feature */
 		feature->id = finfo->fid;
@@ -527,6 +540,24 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		feature->ioaddr = finfo->ioaddr;
 		fdev->resource[index++] = finfo->mmio_res;
 
+		if (finfo->nr_irqs) {
+			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
+					   sizeof(*ctx), GFP_KERNEL);
+			if (!ctx)
+				return -ENOMEM;
+
+			for (i = 0; i < finfo->nr_irqs; i++) {
+				virq = binfo->irq_table[finfo->irq_base + i];
+				if (virq < 0 || virq > NR_IRQS)
+					return -EINVAL;
+
+				ctx[i].irq = virq;
+			}
+
+			feature->irq_ctx = ctx;
+			feature->nr_irqs = finfo->nr_irqs;
+		}
+
 		list_del(&finfo->node);
 		kfree(finfo);
 	}
@@ -648,7 +679,8 @@ static u64 feature_id(void __iomem *start)
 static int
 create_feature_instance(struct build_feature_devs_info *binfo,
 			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
-			resource_size_t size, u64 fid)
+			resource_size_t size, u64 fid, unsigned int irq_base,
+			unsigned int nr_irqs)
 {
 	struct dfl_feature_info *finfo;
 
@@ -667,6 +699,8 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 	finfo->mmio_res.start = dfl->start + ofst;
 	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
+	finfo->irq_base = irq_base;
+	finfo->nr_irqs = nr_irqs;
 	finfo->ioaddr = dfl->ioaddr + ofst;
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
@@ -684,7 +718,8 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
 
 	WARN_ON(!size);
 
-	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
+	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU,
+				       0, 0);
 }
 
 static int parse_feature_afu(struct build_feature_devs_info *binfo,
@@ -724,7 +759,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
 	if (ret)
 		return ret;
 
-	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
+	ret = create_feature_instance(binfo, dfl, ofst, 0, 0, 0, 0);
 	if (ret)
 		return ret;
 	/*
@@ -742,17 +777,72 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
 	return ret;
 }
 
+static void parse_feature_irqs(struct build_feature_devs_info *binfo,
+			       struct dfl_fpga_enum_dfl *dfl,
+			       resource_size_t ofst,
+			       unsigned int *irq_base, unsigned int *nr_irqs)
+{
+	u64 id, v;
+
+	/*
+	 * Ideally DFL framework should only read info from DFL header, but
+	 * current version DFL only provides mmio resources information for
+	 * each feature in DFL Header, no field for interrupt resources.
+	 * Some interrupt resources information are provided by specific
+	 * mmio registers of each components(e.g. different private features)
+	 * which supports interrupt. So in order to parse and assign irq
+	 * resources to different components, DFL framework has to look into
+	 * specific capability registers of these core private features.
+	 *
+	 * Once future DFL version supports generic interrupt resources
+	 * information in common DFL headers, below code could be replaced by
+	 * some generic interrupt parsing code.
+	 */
+
+	id = feature_id((dfl->ioaddr + ofst));
+
+	if (id == PORT_FEATURE_ID_UINT) {
+		v = readq(dfl->ioaddr + ofst + PORT_UINT_CAP);
+		*irq_base = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+		*nr_irqs = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+	} else if (id == PORT_FEATURE_ID_ERROR) {
+		v = readq(dfl->ioaddr + ofst + PORT_ERROR_CAP);
+		*irq_base = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+		*nr_irqs = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+	} else if (id == FME_FEATURE_ID_GLOBAL_ERR) {
+		v = readq(dfl->ioaddr + ofst + FME_ERROR_CAP);
+		*irq_base = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+		*nr_irqs = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+	} else {
+		return;
+	}
+
+	dev_dbg(binfo->dev, "feature: 0x%llx, nr_irqs: %u, irq_base: %u\n",
+		(unsigned long long)id, *nr_irqs, *irq_base);
+
+	if (*irq_base + *nr_irqs > binfo->nr_irqs) {
+		*irq_base = 0;
+		*nr_irqs = 0;
+		dev_err(binfo->dev, "Inconsistent interrupt number on HW\n");
+	}
+}
+
 static int parse_feature_private(struct build_feature_devs_info *binfo,
 				 struct dfl_fpga_enum_dfl *dfl,
 				 resource_size_t ofst)
 {
+	unsigned int irq_base = 0, nr_irqs = 0;
+
 	if (!binfo->feature_dev) {
 		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
 			(unsigned long long)feature_id(dfl->ioaddr + ofst));
 		return -EINVAL;
 	}
 
-	return create_feature_instance(binfo, dfl, ofst, 0, 0);
+	parse_feature_irqs(binfo, dfl, ofst, &irq_base, &nr_irqs);
+
+	return create_feature_instance(binfo, dfl, ofst, 0, 0, irq_base,
+				       nr_irqs);
 }
 
 /**
@@ -853,6 +943,10 @@ void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info)
 		devm_kfree(dev, dfl);
 	}
 
+	/* remove irq table */
+	if (info->irq_table)
+		devm_kfree(dev, info->irq_table);
+
 	devm_kfree(dev, info);
 	put_device(dev);
 }
@@ -892,6 +986,40 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_dfl);
 
+/**
+ * dfl_fpga_enum_info_add_irq - add irq table to enum info
+ *
+ * @info: ptr to dfl_fpga_enum_info
+ * @nr_irqs: number of irqs for all feature devices.
+ * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
+ *	       this device.
+ *
+ * One FPGA device may have several interrupts. This function adds irq
+ * information of all feature devices to enumeration info for next step
+ * enumeration.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
+			       unsigned int nr_irqs, int *irq_table)
+{
+	if (!nr_irqs)
+		return -EINVAL;
+
+	if (info->irq_table)
+		return -EEXIST;
+
+	info->irq_table = devm_kmemdup(info->dev, irq_table,
+				       sizeof(int) * nr_irqs, GFP_KERNEL);
+	if (!info->irq_table)
+		return -ENOMEM;
+
+	info->nr_irqs = nr_irqs;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_irq);
+
 static int remove_feature_dev(struct device *dev, void *data)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -959,6 +1087,10 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
 	binfo->dev = info->dev;
 	binfo->cdev = cdev;
 
+	binfo->nr_irqs = info->nr_irqs;
+	if (info->nr_irqs)
+		binfo->irq_table = info->irq_table;
+
 	/*
 	 * start enumeration for all feature devices based on Device Feature
 	 * Lists.
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 4a9a33c..6a498cd 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -112,6 +112,13 @@
 #define FME_PORT_OFST_ACC_VF	1
 #define FME_PORT_OFST_IMP	BIT_ULL(60)
 
+/* FME Error Capability Register */
+#define FME_ERROR_CAP		0x70
+
+/* FME Error Capability Register Bitfield */
+#define FME_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
+#define FME_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
+
 /* PORT Header Register Set */
 #define PORT_HDR_DFH		DFH
 #define PORT_HDR_GUID_L		GUID_L
@@ -145,6 +152,20 @@
 #define PORT_STS_PWR_STATE_AP2	2			/* 90% throttling */
 #define PORT_STS_PWR_STATE_AP6	6			/* 100% throttling */
 
+/* Port Error Capability Register */
+#define PORT_ERROR_CAP		0x38
+
+/* Port Error Capability Register Bitfield */
+#define PORT_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
+#define PORT_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
+
+/* Port Uint Capability Register */
+#define PORT_UINT_CAP		0x8
+
+/* Port Uint Capability Register Bitfield */
+#define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts num */
+#define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector */
+
 /**
  * struct dfl_fpga_port_ops - port ops
  *
@@ -189,6 +210,15 @@ struct dfl_feature_driver {
 };
 
 /**
+ * struct dfl_feature_irq_ctx - dfl private feature interrupt context
+ *
+ * @irq: Linux IRQ number of this interrupt.
+ */
+struct dfl_feature_irq_ctx {
+	int irq;
+};
+
+/**
  * struct dfl_feature - sub feature of the feature devices
  *
  * @id: sub feature id.
@@ -196,12 +226,16 @@ struct dfl_feature_driver {
  *		    this index is used to find its mmio resource from the
  *		    feature dev (platform device)'s reources.
  * @ioaddr: mapped mmio resource address.
+ * @irq_ctx: interrupt context list.
+ * @nr_irqs: number of interrupt contexts.
  * @ops: ops of this sub feature.
  */
 struct dfl_feature {
 	u64 id;
 	int resource_index;
 	void __iomem *ioaddr;
+	struct dfl_feature_irq_ctx *irq_ctx;
+	unsigned int nr_irqs;
 	const struct dfl_feature_ops *ops;
 };
 
@@ -388,10 +422,14 @@ static inline u8 dfl_feature_revision(void __iomem *base)
  *
  * @dev: parent device.
  * @dfls: list of device feature lists.
+ * @nr_irqs: number of irqs for all feature devices.
+ * @irq_table: Linux IRQ numbers for all irqs, indexed by hw irq numbers.
  */
 struct dfl_fpga_enum_info {
 	struct device *dev;
 	struct list_head dfls;
+	unsigned int nr_irqs;
+	int *irq_table;
 };
 
 /**
@@ -415,6 +453,8 @@ struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
 int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
 			       resource_size_t start, resource_size_t len,
 			       void __iomem *ioaddr);
+int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
+			       unsigned int nr_irqs, int *irq_table);
 void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
 
 /**
-- 
2.7.4

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

[Xu Yilun]

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);
+	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);
 }
 
-- 
2.7.4

[PATCH 3/7] fpga: dfl: introduce interrupt trigger setting API

[Xu Yilun]

FPGA user applications may be interested in interrupts generated by
DFL features. For example, users can implement their own FPGA
logics with interrupts enabled in AFU (Accelerated Function Unit,
dynamic region of DFL based FPGA). So user applications need to be
notified to handle these interrupts.

In order to allow userspace applications to monitor interrupts,
driver requires userspace to provide eventfds as interrupt
notification channels. Applications then poll/select on the eventfds
to get notified.

This patch introduces a generic helper function for sub features to
do eventfds binding with given interrupts.

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.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl.h | 11 +++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 493822d..ae6baca 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -535,6 +535,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		int i, virq;
 
 		/* save resource information for each feature */
+		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->resource_index = index;
 		feature->ioaddr = finfo->ioaddr;
@@ -1373,6 +1374,98 @@ int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_vf);
 
+static irqreturn_t dfl_irq_handler(int irq, void *arg)
+{
+	struct eventfd_ctx *trigger = arg;
+
+	eventfd_signal(trigger, 1);
+	return IRQ_HANDLED;
+}
+
+static int do_set_irq_trigger(struct dfl_feature *feature, int idx, int fd)
+{
+	struct platform_device *pdev = feature->dev;
+	struct eventfd_ctx *trigger;
+	int irq, ret;
+
+	if (idx < 0 || idx >= feature->nr_irqs)
+		return -EINVAL;
+
+	irq = feature->irq_ctx[idx].irq;
+
+	if (feature->irq_ctx[idx].trigger) {
+		free_irq(irq, feature->irq_ctx[idx].trigger);
+		kfree(feature->irq_ctx[idx].name);
+		eventfd_ctx_put(feature->irq_ctx[idx].trigger);
+		feature->irq_ctx[idx].trigger = NULL;
+	}
+
+	if (fd < 0)
+		return 0;
+
+	feature->irq_ctx[idx].name =
+		kasprintf(GFP_KERNEL, "fpga-irq[%d](%s-%llx)", idx,
+			  dev_name(&pdev->dev),
+			  (unsigned long long)feature->id);
+	if (!feature->irq_ctx[idx].name)
+		return -ENOMEM;
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		ret = PTR_ERR(trigger);
+		goto free_name;
+	}
+
+	ret = request_irq(irq, dfl_irq_handler, 0,
+			  feature->irq_ctx[idx].name, trigger);
+	if (!ret) {
+		feature->irq_ctx[idx].trigger = trigger;
+		return ret;
+	}
+
+	eventfd_ctx_put(trigger);
+free_name:
+	kfree(feature->irq_ctx[idx].name);
+
+	return ret;
+}
+
+/**
+ * dfl_fpga_set_irq_triggers - set eventfd triggers for dfl feature interrupts
+ *
+ * @feature: dfl sub feature.
+ * @start: start of irq index in this dfl sub feature.
+ * @count: number of irqs.
+ * @fds: eventfds to bind with irqs.
+ *
+ * Bind given eventfds with irqs in this dfl sub feature. Use negative fds as
+ * parameter to unbind irqs.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
+			      unsigned int count, int32_t *fds)
+{
+	int i, j, ret = 0;
+
+	if (start + count < start || start + count > feature->nr_irqs)
+		return -EINVAL;
+
+	for (i = 0, j = start; i < count && !ret; i++, j++) {
+		int fd = fds ? fds[i] : -1;
+
+		ret = do_set_irq_trigger(feature, j, fd);
+	}
+
+	if (ret) {
+		for (--j; j >= (int)start; j--)
+			do_set_irq_trigger(feature, j, -1);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dfl_fpga_set_irq_triggers);
+
 static void __exit dfl_fpga_exit(void)
 {
 	dfl_chardev_uinit();
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 6a498cd..6b60077 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -24,6 +24,8 @@
 #include <linux/slab.h>
 #include <linux/uuid.h>
 #include <linux/fpga/fpga-region.h>
+#include <linux/interrupt.h>
+#include <linux/eventfd.h>
 
 /* maximum supported number of ports */
 #define MAX_DFL_FPGA_PORT_NUM 4
@@ -213,14 +215,19 @@ struct dfl_feature_driver {
  * struct dfl_feature_irq_ctx - dfl private feature interrupt context
  *
  * @irq: Linux IRQ number of this interrupt.
+ * @trigger: eventfd context to signal when interrupt happens.
+ * @name: irq name needed when requesting irq.
  */
 struct dfl_feature_irq_ctx {
 	int irq;
+	struct eventfd_ctx *trigger;
+	char *name;
 };
 
 /**
  * struct dfl_feature - sub feature of the feature devices
  *
+ * @dev: ptr to pdev of the feature device which has the sub feature.
  * @id: sub feature id.
  * @resource_index: each sub feature has one mmio resource for its registers.
  *		    this index is used to find its mmio resource from the
@@ -231,6 +238,7 @@ struct dfl_feature_irq_ctx {
  * @ops: ops of this sub feature.
  */
 struct dfl_feature {
+	struct platform_device *dev;
 	u64 id;
 	int resource_index;
 	void __iomem *ioaddr;
@@ -506,4 +514,7 @@ int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id);
 int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id);
 void dfl_fpga_cdev_config_ports_pf(struct dfl_fpga_cdev *cdev);
 int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vf);
+
+int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
+			      unsigned int count, int32_t *fds);
 #endif /* __FPGA_DFL_H */
-- 
2.7.4

[PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting

[Xu Yilun]

Error reporting interrupt is very useful to notify users that some
errors are detected by the hardware. Once users are notified, they
could query hardware logged error states, no need to continuously
poll on these states.

This patch follows the common DFL interrupt notification and handling
mechanism, implements two ioctl commands below for user to query
hardware capability, and set/unset interrupt triggers.

 Ioctls:
 * DFL_FPGA_PORT_ERR_GET_INFO
   get error reporting feature info, including num_irqs which is used to
   determine whether/how many interrupts it supports.

 * DFL_FPGA_PORT_ERR_SET_IRQ
   set/unset given eventfds as error interrupt triggers.

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-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl-afu-main.c   |  4 +++
 include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
 3 files changed, 107 insertions(+)

diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
index c1467ae..a2c5454 100644
--- a/drivers/fpga/dfl-afu-error.c
+++ b/drivers/fpga/dfl-afu-error.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/uaccess.h>
+#include <linux/fpga-dfl.h>
 
 #include "dfl-afu.h"
 
@@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
 	afu_port_err_mask(&pdev->dev, true);
 }
 
+static long
+port_err_get_info(struct platform_device *pdev,
+		  struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_fpga_port_err_info info;
+
+	info.flags = 0;
+	info.capability = 0;
+	info.num_irqs = feature->nr_irqs;
+
+	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long port_err_set_irq(struct platform_device *pdev,
+			     struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_fpga_irq_set hdr;
+	s32 *fds;
+	long ret;
+
+	if (!feature->nr_irqs)
+		return -ENOENT;
+
+	if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.flags || (hdr.start + hdr.count > feature->nr_irqs) ||
+	    (hdr.start + hdr.count < hdr.start) || !hdr.count)
+		return -EINVAL;
+
+	fds = memdup_user((void __user *)(arg + sizeof(hdr)),
+			  hdr.count * sizeof(s32));
+	if (IS_ERR(fds))
+		return PTR_ERR(fds);
+
+	mutex_lock(&pdata->lock);
+	ret = dfl_fpga_set_irq_triggers(feature, hdr.start, hdr.count, fds);
+	mutex_unlock(&pdata->lock);
+
+	kfree(fds);
+	return ret;
+}
+
+static long
+port_err_ioctl(struct platform_device *pdev, struct dfl_feature *feature,
+	       unsigned int cmd, unsigned long arg)
+{
+	long ret = -ENODEV;
+
+	switch (cmd) {
+	case DFL_FPGA_PORT_ERR_GET_INFO:
+		ret = port_err_get_info(pdev, feature, arg);
+		break;
+	case DFL_FPGA_PORT_ERR_SET_IRQ:
+		ret = port_err_set_irq(pdev, feature, arg);
+		break;
+	default:
+		dev_dbg(&pdev->dev, "%x cmd not handled", cmd);
+	}
+
+	return ret;
+}
+
 const struct dfl_feature_id port_err_id_table[] = {
 	{.id = PORT_FEATURE_ID_ERROR,},
 	{0,}
@@ -227,4 +295,5 @@ const struct dfl_feature_id port_err_id_table[] = {
 const struct dfl_feature_ops port_err_ops = {
 	.init = port_err_init,
 	.uinit = port_err_uinit,
+	.ioctl = port_err_ioctl,
 };
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index 435bde4..fc8b9cf 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -577,6 +577,7 @@ static int afu_release(struct inode *inode, struct file *filp)
 {
 	struct platform_device *pdev = filp->private_data;
 	struct dfl_feature_platform_data *pdata;
+	struct dfl_feature *feature;
 
 	dev_dbg(&pdev->dev, "Device File Release\n");
 
@@ -586,6 +587,9 @@ static int afu_release(struct inode *inode, struct file *filp)
 	dfl_feature_dev_use_end(pdata);
 
 	if (!dfl_feature_dev_use_count(pdata)) {
+		dfl_fpga_dev_for_each_feature(pdata, feature)
+			dfl_fpga_set_irq_triggers(feature, 0,
+						  feature->nr_irqs, NULL);
 		__port_reset(pdev);
 		afu_dma_region_destroy(pdata);
 	}
diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
index ec70a0746..846fc91 100644
--- a/include/uapi/linux/fpga-dfl.h
+++ b/include/uapi/linux/fpga-dfl.h
@@ -151,6 +151,40 @@ struct dfl_fpga_port_dma_unmap {
 
 #define DFL_FPGA_PORT_DMA_UNMAP		_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 4)
 
+/**
+ * DFL_FPGA_PORT_ERR_GET_INFO - _IOR(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5,
+ *						struct dfl_fpga_port_err_info)
+ *
+ * Retrieve information about the fpga port error reporting private feature.
+ * Driver fills the info in provided struct dfl_fpga_port_err_info.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_port_err_info {
+	/* Output */
+	__u32 flags;		/* Zero for now */
+	__u32 capability;	/* The capability of port error reporting */
+	__u32 num_irqs;		/* number of irqs it supports */
+};
+
+#define DFL_FPGA_PORT_ERR_GET_INFO	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5)
+
+/**
+ * DFL_FPGA_PORT_ERR_SET_IRQ - _IOW(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6,
+ *						struct dfl_fpga_irq_set)
+ *
+ * Set fpga port error reporting interrupt trigger if evtfds[n] is valid.
+ * Unset related interrupt trigger if evtfds[n] is a negative value.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_irq_set {
+	__u32 flags;		/* Zero for now */
+	__u32 start;		/* First irq number */
+	__u32 count;		/* The number of eventfd handler */
+	__s32 evtfds[];		/* Eventfd handler */
+};
+
+#define DFL_FPGA_PORT_ERR_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6)
+
 /* IOCTLs for FME file descriptor */
 
 /**
-- 
2.7.4

[PATCH 5/7] fpga: dfl: fme: add interrupt support for global error reporting

[Xu Yilun]

Error reporting interrupt is very useful to notify users that some
errors are detected by the hardware. Once users are notified, they
could query hardware logged error states, no need to continuously
poll on these states.

This patch follows the common DFL interrupt notification and handling
mechanism. And it implements two ioctls below for user to query
hardware capability and set/unset interrupt triggers.

 Ioctls:
 * DFL_FPGA_FME_ERR_GET_INFO
   get fme global error reporting feature info, including num_irqs which
   is used to determine whether/how many interrupts it supports.

 * DFL_FPGA_FME_ERR_SET_IRQ
   set/unset given eventfds as fme error reporting interrupt triggers.

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-fme-error.c  | 71 +++++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl-fme-main.c   |  6 ++++
 include/uapi/linux/fpga-dfl.h | 27 ++++++++++++++++
 3 files changed, 104 insertions(+)

diff --git a/drivers/fpga/dfl-fme-error.c b/drivers/fpga/dfl-fme-error.c
index f897d41..df5e7b8 100644
--- a/drivers/fpga/dfl-fme-error.c
+++ b/drivers/fpga/dfl-fme-error.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/uaccess.h>
+#include <linux/fpga-dfl.h>
 
 #include "dfl.h"
 #include "dfl-fme.h"
@@ -348,6 +349,75 @@ static void fme_global_err_uinit(struct platform_device *pdev,
 	fme_err_mask(&pdev->dev, true);
 }
 
+static long
+fme_global_err_get_info(struct platform_device *pdev,
+			struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_fpga_fme_err_info info;
+
+	info.flags = 0;
+	info.capability = 0;
+	info.num_irqs = feature->nr_irqs;
+
+	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long
+fme_global_err_set_irq(struct platform_device *pdev,
+		       struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_fpga_irq_set hdr;
+	s32 *fds;
+	long ret;
+
+	if (!feature->nr_irqs)
+		return -ENOENT;
+
+	if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.flags || (hdr.start + hdr.count > feature->nr_irqs) ||
+	    (hdr.start + hdr.count < hdr.start) || !hdr.count)
+		return -EINVAL;
+
+	fds = memdup_user((void __user *)(arg + sizeof(hdr)),
+			  hdr.count * sizeof(s32));
+	if (IS_ERR(fds))
+		return PTR_ERR(fds);
+
+	mutex_lock(&pdata->lock);
+	ret = dfl_fpga_set_irq_triggers(feature, hdr.start, hdr.count, fds);
+	mutex_unlock(&pdata->lock);
+
+	kfree(fds);
+	return ret;
+}
+
+static long
+fme_global_error_ioctl(struct platform_device *pdev,
+		       struct dfl_feature *feature,
+		       unsigned int cmd, unsigned long arg)
+{
+	long ret = -ENODEV;
+
+	switch (cmd) {
+	case DFL_FPGA_FME_ERR_GET_INFO:
+		ret = fme_global_err_get_info(pdev, feature, arg);
+		break;
+	case DFL_FPGA_FME_ERR_SET_IRQ:
+		ret = fme_global_err_set_irq(pdev, feature, arg);
+		break;
+	default:
+		dev_dbg(&pdev->dev, "%x cmd not handled", cmd);
+	}
+
+	return ret;
+}
+
 const struct dfl_feature_id fme_global_err_id_table[] = {
 	{.id = FME_FEATURE_ID_GLOBAL_ERR,},
 	{0,}
@@ -356,4 +426,5 @@ const struct dfl_feature_id fme_global_err_id_table[] = {
 const struct dfl_feature_ops fme_global_err_ops = {
 	.init = fme_global_err_init,
 	.uinit = fme_global_err_uinit,
+	.ioctl = fme_global_error_ioctl,
 };
diff --git a/drivers/fpga/dfl-fme-main.c b/drivers/fpga/dfl-fme-main.c
index 56d720c..ab3722d 100644
--- a/drivers/fpga/dfl-fme-main.c
+++ b/drivers/fpga/dfl-fme-main.c
@@ -616,11 +616,17 @@ static int fme_release(struct inode *inode, struct file *filp)
 {
 	struct dfl_feature_platform_data *pdata = filp->private_data;
 	struct platform_device *pdev = pdata->dev;
+	struct dfl_feature *feature;
 
 	dev_dbg(&pdev->dev, "Device File Release\n");
 
 	mutex_lock(&pdata->lock);
 	dfl_feature_dev_use_end(pdata);
+
+	if (!dfl_feature_dev_use_count(pdata))
+		dfl_fpga_dev_for_each_feature(pdata, feature)
+			dfl_fpga_set_irq_triggers(feature, 0,
+						  feature->nr_irqs, NULL);
 	mutex_unlock(&pdata->lock);
 
 	return 0;
diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
index 846fc91..c212fa9 100644
--- a/include/uapi/linux/fpga-dfl.h
+++ b/include/uapi/linux/fpga-dfl.h
@@ -228,4 +228,31 @@ struct dfl_fpga_fme_port_pr {
  */
 #define DFL_FPGA_FME_PORT_ASSIGN     _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 2, int)
 
+/**
+ * DFL_FPGA_FME_ERR_GET_INFO - _IOR(DFL_FPGA_MAGIC, DFL_FME_BASE + 3,
+ *						struct dfl_fpga_fme_err_info)
+ *
+ * Retrieve information about the fpga fme error reporting private feature.
+ * Driver fills the info in provided struct dfl_fpga_fme_err_info.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_fme_err_info {
+	/* Output */
+	__u32 flags;		/* Zero for now */
+	__u32 capability;	/* The capability of fme err */
+	__u32 num_irqs;		/* number of irqs fme err supports */
+};
+
+#define DFL_FPGA_FME_ERR_GET_INFO	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 3)
+
+/**
+ * DFL_FPGA_FME_ERR_SET_IRQ - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 4,
+ *						struct dfl_fpga_irq_set)
+ *
+ * Set fpga fme error reporting interrupt trigger if evtfds[n] is valid.
+ * Unset related interrupt trigger if evtfds[n] is a negative value.
+ * Return: 0 on success, -errno on failure.
+ */
+#define DFL_FPGA_FME_ERR_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 4)
+
 #endif /* _UAPI_LINUX_FPGA_DFL_H */
-- 
2.7.4

[PATCH 6/7] fpga: dfl: afu: add user interrupt support

[Xu Yilun]

AFU (Accelerated Function Unit) is dynamic region of the DFL based FPGA,
and always defined by users. Some DFL based FPGA cards allow users to
implement their own interrupts in AFU. In order to support this,
hardware implements a new UINT (User Interrupt) private feature with
related capability register which describes the number of supported
user interrupts as well as the local index of the interrupts for
software enumeration, and from software side, driver follows the common
DFL interrupt notification and handling mechanism, and it implements
two ioctls below for user to query capability and set/unset interrupt
triggers.

 Ioctls:
 * DFL_FPGA_PORT_UINT_GET_INFO
   Get UINT private feature info, including num_irqs which is used to
   determine how many interrupts it supports.

 * DFL_FPGA_PORT_UINT_SET_IRQ
   set/unset eventfds as AFU user interrupt triggers.

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-afu-main.c   | 79 +++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/fpga-dfl.h | 28 +++++++++++++++
 2 files changed, 107 insertions(+)

diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index fc8b9cf..7eec383 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -529,6 +529,81 @@ static const struct dfl_feature_ops port_stp_ops = {
 	.init = port_stp_init,
 };
 
+static long
+port_uint_get_info(struct platform_device *pdev,
+		   struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_fpga_port_uint_info info;
+
+	info.flags = 0;
+	info.capability = 0;
+	info.num_irqs = feature->nr_irqs;
+
+	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long port_uint_set_irq(struct platform_device *pdev,
+			      struct dfl_feature *feature, unsigned long arg)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_fpga_irq_set hdr;
+	s32 *fds;
+	long ret;
+
+	if (!feature->nr_irqs)
+		return -ENOENT;
+
+	if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
+		return -EFAULT;
+
+	if (hdr.flags || (hdr.start + hdr.count > feature->nr_irqs) ||
+	    (hdr.start + hdr.count < hdr.start) || !hdr.count)
+		return -EINVAL;
+
+	fds = memdup_user((void __user *)(arg + sizeof(hdr)),
+			  hdr.count * sizeof(s32));
+	if (IS_ERR(fds))
+		return PTR_ERR(fds);
+
+	mutex_lock(&pdata->lock);
+	ret = dfl_fpga_set_irq_triggers(feature, hdr.start, hdr.count, fds);
+	mutex_unlock(&pdata->lock);
+
+	kfree(fds);
+	return ret;
+}
+
+static long
+port_uint_ioctl(struct platform_device *pdev, struct dfl_feature *feature,
+		unsigned int cmd, unsigned long arg)
+{
+	long ret = -ENODEV;
+
+	switch (cmd) {
+	case DFL_FPGA_PORT_UINT_GET_INFO:
+		ret = port_uint_get_info(pdev, feature, arg);
+		break;
+	case DFL_FPGA_PORT_UINT_SET_IRQ:
+		ret = port_uint_set_irq(pdev, feature, arg);
+		break;
+	default:
+		dev_dbg(&pdev->dev, "%x cmd not handled", cmd);
+	}
+	return ret;
+}
+
+static const struct dfl_feature_id port_uint_id_table[] = {
+	{.id = PORT_FEATURE_ID_UINT,},
+	{0,}
+};
+
+static const struct dfl_feature_ops port_uint_ops = {
+	.ioctl = port_uint_ioctl,
+};
+
 static struct dfl_feature_driver port_feature_drvs[] = {
 	{
 		.id_table = port_hdr_id_table,
@@ -547,6 +622,10 @@ static struct dfl_feature_driver port_feature_drvs[] = {
 		.ops = &port_stp_ops,
 	},
 	{
+		.id_table = port_uint_id_table,
+		.ops = &port_uint_ops,
+	},
+	{
 		.ops = NULL,
 	}
 };
diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
index c212fa9..f392b93 100644
--- a/include/uapi/linux/fpga-dfl.h
+++ b/include/uapi/linux/fpga-dfl.h
@@ -185,6 +185,34 @@ struct dfl_fpga_irq_set {
 
 #define DFL_FPGA_PORT_ERR_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6)
 
+/**
+ * DFL_FPGA_PORT_UINT_GET_INFO - _IOR(DFL_FPGA_MAGIC, DFL_PORT_BASE + 7,
+ *						struct dfl_fpga_port_uint_info)
+ *
+ * Retrieve information about the fpga AFU user interrupt private feature.
+ * Driver fills the info in provided struct dfl_fpga_port_uint_info.
+ * Return: 0 on success, -errno on failure.
+ */
+struct dfl_fpga_port_uint_info {
+	/* Output */
+	__u32 flags;		/* Zero for now */
+	__u32 capability;	/* The capability of user interrupt */
+	__u32 num_irqs;		/* number of irqs user interrupt supports */
+};
+
+#define DFL_FPGA_PORT_UINT_GET_INFO	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 7)
+
+/**
+ * DFL_FPGA_PORT_UINT_SET_IRQ - _IOW(DFL_FPGA_MAGIC, DFL_PORT_BASE + 8,
+ *						struct dfl_fpga_irq_set)
+ *
+ * Set fpga afu user interrupt trigger if evtfds[n] is valid.
+ * Unset related interrupt trigger if evtfds[n] is a negative value.
+ * Return: 0 on success, -errno on failure.
+ */
+
+#define DFL_FPGA_PORT_UINT_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 8)
+
 /* IOCTLs for FME file descriptor */
 
 /**
-- 
2.7.4

[PATCH 7/7] Documentation: fpga: dfl: add descriptions for interrupt related interfaces.

[Xu Yilun]

This patch adds introductions of interrupt related interfaces for FME
error reporting, port error reporting and AFU user interrupts features.

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>
---
 Documentation/fpga/dfl.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 094fc8a..3c4e150 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -89,6 +89,8 @@ The following functions are exposed through ioctls:
 - Program bitstream (DFL_FPGA_FME_PORT_PR)
 - Assign port to PF (DFL_FPGA_FME_PORT_ASSIGN)
 - Release port from PF (DFL_FPGA_FME_PORT_RELEASE)
+- Get FME global error info (DFL_FPGA_FME_ERR_GET_INFO)
+- Set interrupt trigger for FME error (DFL_FPGA_FME_ERR_SET_IRQ)
 
 More functions are exposed through sysfs
 (/sys/class/fpga_region/regionX/dfl-fme.n/):
@@ -144,6 +146,10 @@ The following functions are exposed through ioctls:
 - Map DMA buffer (DFL_FPGA_PORT_DMA_MAP)
 - Unmap DMA buffer (DFL_FPGA_PORT_DMA_UNMAP)
 - Reset AFU (DFL_FPGA_PORT_RESET)
+- Get port error info (DFL_FPGA_PORT_ERR_GET_INFO)
+- Set interrupt trigger for port error(DFL_FPGA_PORT_ERR_SET_IRQ)
+- Get User AFU interrupt(UINT) info (DFL_FPGA_PORT_UINT_GET_INFO)
+- Set interrupt trigger for UINT (DFL_FPGA_PORT_UINT_SET_IRQ)
 
 DFL_FPGA_PORT_RESET:
   reset the FPGA Port and its AFU. Userspace can do Port
@@ -378,6 +384,17 @@ The device nodes used for ioctl() or mmap() can be referenced through::
 	/sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
 
 
+Interrupt support
+=================
+Some FME and AFU private features are able to generate interrupts. As mentioned
+above, users could call ioctl (DFL_FPGA_*_GET_INFO), query the nr_irqs field to
+know whether or how many interrupts are supported for this private feature.
+Drivers also implement an eventfd based interrupt handling mechanism for users
+to get notified when interrupt happens. Users could set eventfds to driver via
+ioctl (DFL_FPGA_*_SET_IRQ), and then poll/select on these eventfds waiting for
+notification.
+
+
 Add new FIUs support
 ====================
 It's possible that developers made some new function blocks (FIUs) under this
-- 
2.7.4

Re: [PATCH 1/7] fpga: dfl: parse interrupt info for feature devices on enumeration

[Wu Hao]

Hi Yilun

Some comments inline.

On Mon, Mar 09, 2020 at 06:29:44PM +0800, Xu Yilun wrote:
> DFL based FPGA devices could support interrupts for different purposes,
> but current DFL framework only supports feature device enumeration with
> given MMIO resources information via common DFL headers. This patch
> introduces one new API dfl_fpga_enum_info_add_irq for low level bus
> drivers (e.g. PCIe device driver) to pass its interrupt resources
> information to DFL framework for enumeration, and also adds interrupt
> enumeration code in framework to parse and assign interrupt resources
> for enumerated feature devices and their own sub features.
> 
> With this patch, DFL framework enumerates interrupt resources for core
> features, including PORT Error Reporting, FME (FPGA Management Engine)
> Error Reporting and also AFU User Interrupts.
> 
> 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.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/fpga/dfl.h |  40 +++++++++++++++
>  2 files changed, 176 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 9909948..493822d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -11,6 +11,7 @@
>   *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
>   */
>  #include <linux/module.h>
> +#include <asm/irq.h>
>  
>  #include "dfl.h"
>  
> @@ -421,6 +422,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
>   *
>   * @dev: device to enumerate.
>   * @cdev: the container device for all feature devices.
> + * @nr_irqs: number of irqs for all feature devices.
> + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> + *	       this device.
>   * @feature_dev: current feature device.
>   * @ioaddr: header register region address of feature device in enumeration.
>   * @sub_features: a sub features linked list for feature device in enumeration.
> @@ -429,6 +433,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
>  struct build_feature_devs_info {
>  	struct device *dev;
>  	struct dfl_fpga_cdev *cdev;
> +	unsigned int nr_irqs;
> +	int *irq_table;
> +
>  	struct platform_device *feature_dev;
>  	void __iomem *ioaddr;
>  	struct list_head sub_features;
> @@ -442,12 +449,16 @@ struct build_feature_devs_info {
>   * @mmio_res: mmio resource of this sub feature.
>   * @ioaddr: mapped base address of mmio resource.
>   * @node: node in sub_features linked list.
> + * @irq_base: start of irq index in this sub feature.
> + * @nr_irqs: number of irqs of this sub feature.
>   */
>  struct dfl_feature_info {
>  	u64 fid;
>  	struct resource mmio_res;
>  	void __iomem *ioaddr;
>  	struct list_head node;
> +	unsigned int irq_base;
> +	unsigned int nr_irqs;
>  };
>  
>  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> @@ -520,6 +531,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  	/* fill features and resource information for feature dev */
>  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
>  		struct dfl_feature *feature = &pdata->features[index];
> +		struct dfl_feature_irq_ctx *ctx;
> +		int i, virq;
>  
>  		/* save resource information for each feature */
>  		feature->id = finfo->fid;
> @@ -527,6 +540,24 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		feature->ioaddr = finfo->ioaddr;
>  		fdev->resource[index++] = finfo->mmio_res;
>  
> +		if (finfo->nr_irqs) {
> +			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> +					   sizeof(*ctx), GFP_KERNEL);
> +			if (!ctx)
> +				return -ENOMEM;
> +
> +			for (i = 0; i < finfo->nr_irqs; i++) {
> +				virq = binfo->irq_table[finfo->irq_base + i];

Looks like it checks the irq_table input in commit_dev which is the last step
of enumeration. I feel that maybe it's better to check the input earlier, e.g.
in dfl_fpga_enum_info_add_irq() function below.

BTW: why it's named as virq? any specific meaning?

> +				if (virq < 0 || virq > NR_IRQS)
> +					return -EINVAL;
> +
> +				ctx[i].irq = virq;
> +			}
> +
> +			feature->irq_ctx = ctx;
> +			feature->nr_irqs = finfo->nr_irqs;
> +		}
> +
>  		list_del(&finfo->node);
>  		kfree(finfo);
>  	}
> @@ -648,7 +679,8 @@ static u64 feature_id(void __iomem *start)
>  static int
>  create_feature_instance(struct build_feature_devs_info *binfo,
>  			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> -			resource_size_t size, u64 fid)
> +			resource_size_t size, u64 fid, unsigned int irq_base,
> +			unsigned int nr_irqs)
>  {
>  	struct dfl_feature_info *finfo;
>  
> @@ -667,6 +699,8 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  	finfo->mmio_res.start = dfl->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
> +	finfo->irq_base = irq_base;
> +	finfo->nr_irqs = nr_irqs;
>  	finfo->ioaddr = dfl->ioaddr + ofst;
>  
>  	list_add_tail(&finfo->node, &binfo->sub_features);
> @@ -684,7 +718,8 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
>  
>  	WARN_ON(!size);
>  
> -	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> +	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU,
> +				       0, 0);
>  }
>  
>  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> @@ -724,7 +759,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
>  	if (ret)
>  		return ret;
>  
> -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	ret = create_feature_instance(binfo, dfl, ofst, 0, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	/*
> @@ -742,17 +777,72 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
>  	return ret;
>  }
>  
> +static void parse_feature_irqs(struct build_feature_devs_info *binfo,
> +			       struct dfl_fpga_enum_dfl *dfl,
> +			       resource_size_t ofst,
> +			       unsigned int *irq_base, unsigned int *nr_irqs)
> +{
> +	u64 id, v;
> +
> +	/*
> +	 * Ideally DFL framework should only read info from DFL header, but
> +	 * current version DFL only provides mmio resources information for
> +	 * each feature in DFL Header, no field for interrupt resources.
> +	 * Some interrupt resources information are provided by specific
> +	 * mmio registers of each components(e.g. different private features)
> +	 * which supports interrupt. So in order to parse and assign irq
> +	 * resources to different components, DFL framework has to look into
> +	 * specific capability registers of these core private features.
> +	 *
> +	 * Once future DFL version supports generic interrupt resources
> +	 * information in common DFL headers, below code could be replaced by
> +	 * some generic interrupt parsing code.

Per my understanding, even later version hardware uses common DFL Header Field
for interrupt resources enumeration, but we still keep this quirk for old
version hardware, right?

> +	 */
> +
> +	id = feature_id((dfl->ioaddr + ofst));
> +
> +	if (id == PORT_FEATURE_ID_UINT) {
> +		v = readq(dfl->ioaddr + ofst + PORT_UINT_CAP);
> +		*irq_base = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +		*nr_irqs = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +	} else if (id == PORT_FEATURE_ID_ERROR) {
> +		v = readq(dfl->ioaddr + ofst + PORT_ERROR_CAP);
> +		*irq_base = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +		*nr_irqs = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +	} else if (id == FME_FEATURE_ID_GLOBAL_ERR) {
> +		v = readq(dfl->ioaddr + ofst + FME_ERROR_CAP);
> +		*irq_base = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +		*nr_irqs = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +	} else {
> +		return;
> +	}
> +
> +	dev_dbg(binfo->dev, "feature: 0x%llx, nr_irqs: %u, irq_base: %u\n",
> +		(unsigned long long)id, *nr_irqs, *irq_base);
> +
> +	if (*irq_base + *nr_irqs > binfo->nr_irqs) {
> +		*irq_base = 0;
> +		*nr_irqs = 0;
> +		dev_err(binfo->dev, "Inconsistent interrupt number on HW\n");

Maybe you want to print feature id as well in this error case.

> +	}
> +}
> +
>  static int parse_feature_private(struct build_feature_devs_info *binfo,
>  				 struct dfl_fpga_enum_dfl *dfl,
>  				 resource_size_t ofst)
>  {
> +	unsigned int irq_base = 0, nr_irqs = 0;
> +
>  	if (!binfo->feature_dev) {
>  		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
>  			(unsigned long long)feature_id(dfl->ioaddr + ofst));
>  		return -EINVAL;
>  	}
>  
> -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	parse_feature_irqs(binfo, dfl, ofst, &irq_base, &nr_irqs);
> +
> +	return create_feature_instance(binfo, dfl, ofst, 0, 0, irq_base,
> +				       nr_irqs);
>  }
>  
>  /**
> @@ -853,6 +943,10 @@ void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info)
>  		devm_kfree(dev, dfl);
>  	}
>  
> +	/* remove irq table */
> +	if (info->irq_table)
> +		devm_kfree(dev, info->irq_table);
> +
>  	devm_kfree(dev, info);
>  	put_device(dev);
>  }
> @@ -892,6 +986,40 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_dfl);
>  
> +/**
> + * dfl_fpga_enum_info_add_irq - add irq table to enum info
> + *
> + * @info: ptr to dfl_fpga_enum_info
> + * @nr_irqs: number of irqs for all feature devices.

It's better to say, number of irqs of the dfl fpga device to be enumerated.
This function needs to be used before enumerate API.

> + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> + *	       this device.
> + *
> + * One FPGA device may have several interrupts. This function adds irq
> + * information of all feature devices to enumeration info for next step

Same here.

> + * enumeration.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> +			       unsigned int nr_irqs, int *irq_table)
> +{
> +	if (!nr_irqs)
> +		return -EINVAL;
> +

Looks like currently it only allows add irq table once, so please document
this as it doesn't support users adding multiple irq tables.

Thanks
Hao

> +	if (info->irq_table)
> +		return -EEXIST;
> +
> +	info->irq_table = devm_kmemdup(info->dev, irq_table,
> +				       sizeof(int) * nr_irqs, GFP_KERNEL);
> +	if (!info->irq_table)
> +		return -ENOMEM;
> +
> +	info->nr_irqs = nr_irqs;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_irq);
> +
>  static int remove_feature_dev(struct device *dev, void *data)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -959,6 +1087,10 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
>  	binfo->dev = info->dev;
>  	binfo->cdev = cdev;
>  
> +	binfo->nr_irqs = info->nr_irqs;
> +	if (info->nr_irqs)
> +		binfo->irq_table = info->irq_table;
> +
>  	/*
>  	 * start enumeration for all feature devices based on Device Feature
>  	 * Lists.
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 4a9a33c..6a498cd 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -112,6 +112,13 @@
>  #define FME_PORT_OFST_ACC_VF	1
>  #define FME_PORT_OFST_IMP	BIT_ULL(60)
>  
> +/* FME Error Capability Register */
> +#define FME_ERROR_CAP		0x70
> +
> +/* FME Error Capability Register Bitfield */
> +#define FME_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> +#define FME_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> +
>  /* PORT Header Register Set */
>  #define PORT_HDR_DFH		DFH
>  #define PORT_HDR_GUID_L		GUID_L
> @@ -145,6 +152,20 @@
>  #define PORT_STS_PWR_STATE_AP2	2			/* 90% throttling */
>  #define PORT_STS_PWR_STATE_AP6	6			/* 100% throttling */
>  
> +/* Port Error Capability Register */
> +#define PORT_ERROR_CAP		0x38
> +
> +/* Port Error Capability Register Bitfield */
> +#define PORT_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> +#define PORT_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> +
> +/* Port Uint Capability Register */
> +#define PORT_UINT_CAP		0x8
> +
> +/* Port Uint Capability Register Bitfield */
> +#define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts num */
> +#define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector */
> +
>  /**
>   * struct dfl_fpga_port_ops - port ops
>   *
> @@ -189,6 +210,15 @@ struct dfl_feature_driver {
>  };
>  
>  /**
> + * struct dfl_feature_irq_ctx - dfl private feature interrupt context
> + *
> + * @irq: Linux IRQ number of this interrupt.
> + */
> +struct dfl_feature_irq_ctx {
> +	int irq;
> +};
> +
> +/**
>   * struct dfl_feature - sub feature of the feature devices
>   *
>   * @id: sub feature id.
> @@ -196,12 +226,16 @@ struct dfl_feature_driver {
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s reources.
>   * @ioaddr: mapped mmio resource address.
> + * @irq_ctx: interrupt context list.
> + * @nr_irqs: number of interrupt contexts.
>   * @ops: ops of this sub feature.
>   */
>  struct dfl_feature {
>  	u64 id;
>  	int resource_index;
>  	void __iomem *ioaddr;
> +	struct dfl_feature_irq_ctx *irq_ctx;
> +	unsigned int nr_irqs;
>  	const struct dfl_feature_ops *ops;
>  };
>  
> @@ -388,10 +422,14 @@ static inline u8 dfl_feature_revision(void __iomem *base)
>   *
>   * @dev: parent device.
>   * @dfls: list of device feature lists.
> + * @nr_irqs: number of irqs for all feature devices.
> + * @irq_table: Linux IRQ numbers for all irqs, indexed by hw irq numbers.
>   */
>  struct dfl_fpga_enum_info {
>  	struct device *dev;
>  	struct list_head dfls;
> +	unsigned int nr_irqs;
> +	int *irq_table;
>  };
>  
>  /**
> @@ -415,6 +453,8 @@ struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
>  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
>  			       resource_size_t start, resource_size_t len,
>  			       void __iomem *ioaddr);
> +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> +			       unsigned int nr_irqs, int *irq_table);
>  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
>  
>  /**
> -- 
> 2.7.4

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

[Wu Hao]

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

Re: [PATCH 1/7] fpga: dfl: parse interrupt info for feature devices on enumeration

[Xu Yilun]

On Tue, Mar 10, 2020 at 10:26:49AM +0800, Wu Hao wrote:
> Hi Yilun
> 
> Some comments inline.
> 
> On Mon, Mar 09, 2020 at 06:29:44PM +0800, Xu Yilun wrote:
> > DFL based FPGA devices could support interrupts for different purposes,
> > but current DFL framework only supports feature device enumeration with
> > given MMIO resources information via common DFL headers. This patch
> > introduces one new API dfl_fpga_enum_info_add_irq for low level bus
> > drivers (e.g. PCIe device driver) to pass its interrupt resources
> > information to DFL framework for enumeration, and also adds interrupt
> > enumeration code in framework to parse and assign interrupt resources
> > for enumerated feature devices and their own sub features.
> > 
> > With this patch, DFL framework enumerates interrupt resources for core
> > features, including PORT Error Reporting, FME (FPGA Management Engine)
> > Error Reporting and also AFU User Interrupts.
> > 
> > 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.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  drivers/fpga/dfl.h |  40 +++++++++++++++
> >  2 files changed, 176 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 9909948..493822d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -11,6 +11,7 @@
> >   *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >   */
> >  #include <linux/module.h>
> > +#include <asm/irq.h>
> >  
> >  #include "dfl.h"
> >  
> > @@ -421,6 +422,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> >   *
> >   * @dev: device to enumerate.
> >   * @cdev: the container device for all feature devices.
> > + * @nr_irqs: number of irqs for all feature devices.
> > + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> > + *	       this device.
> >   * @feature_dev: current feature device.
> >   * @ioaddr: header register region address of feature device in enumeration.
> >   * @sub_features: a sub features linked list for feature device in enumeration.
> > @@ -429,6 +433,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> >  struct build_feature_devs_info {
> >  	struct device *dev;
> >  	struct dfl_fpga_cdev *cdev;
> > +	unsigned int nr_irqs;
> > +	int *irq_table;
> > +
> >  	struct platform_device *feature_dev;
> >  	void __iomem *ioaddr;
> >  	struct list_head sub_features;
> > @@ -442,12 +449,16 @@ struct build_feature_devs_info {
> >   * @mmio_res: mmio resource of this sub feature.
> >   * @ioaddr: mapped base address of mmio resource.
> >   * @node: node in sub_features linked list.
> > + * @irq_base: start of irq index in this sub feature.
> > + * @nr_irqs: number of irqs of this sub feature.
> >   */
> >  struct dfl_feature_info {
> >  	u64 fid;
> >  	struct resource mmio_res;
> >  	void __iomem *ioaddr;
> >  	struct list_head node;
> > +	unsigned int irq_base;
> > +	unsigned int nr_irqs;
> >  };
> >  
> >  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> > @@ -520,6 +531,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  	/* fill features and resource information for feature dev */
> >  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> >  		struct dfl_feature *feature = &pdata->features[index];
> > +		struct dfl_feature_irq_ctx *ctx;
> > +		int i, virq;
> >  
> >  		/* save resource information for each feature */
> >  		feature->id = finfo->fid;
> > @@ -527,6 +540,24 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  		feature->ioaddr = finfo->ioaddr;
> >  		fdev->resource[index++] = finfo->mmio_res;
> >  
> > +		if (finfo->nr_irqs) {
> > +			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> > +					   sizeof(*ctx), GFP_KERNEL);
> > +			if (!ctx)
> > +				return -ENOMEM;
> > +
> > +			for (i = 0; i < finfo->nr_irqs; i++) {
> > +				virq = binfo->irq_table[finfo->irq_base + i];
> 
> Looks like it checks the irq_table input in commit_dev which is the last step
> of enumeration. I feel that maybe it's better to check the input earlier, e.g.
> in dfl_fpga_enum_info_add_irq() function below.

Yes the check could be done earlier. But maybe too early checking in
dfl_fpga_enum_info_add_irq(). I think dfl should allow holes in
irq_table. We make sure sub features get valid irq numbers for
each of its irq line, that should be enough.

So maybe do this check in parse_feature_irqs() is better?

> 
> BTW: why it's named as virq? any specific meaning?

I'm just trying to indicate it's a linux irq number, not a hardware
local irq number. I see the name also used as linux irq number in other
drivers.

> 
> > +				if (virq < 0 || virq > NR_IRQS)
> > +					return -EINVAL;
> > +
> > +				ctx[i].irq = virq;
> > +			}
> > +
> > +			feature->irq_ctx = ctx;
> > +			feature->nr_irqs = finfo->nr_irqs;
> > +		}
> > +
> >  		list_del(&finfo->node);
> >  		kfree(finfo);
> >  	}
> > @@ -648,7 +679,8 @@ static u64 feature_id(void __iomem *start)
> >  static int
> >  create_feature_instance(struct build_feature_devs_info *binfo,
> >  			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> > -			resource_size_t size, u64 fid)
> > +			resource_size_t size, u64 fid, unsigned int irq_base,
> > +			unsigned int nr_irqs)
> >  {
> >  	struct dfl_feature_info *finfo;
> >  
> > @@ -667,6 +699,8 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> >  	finfo->mmio_res.start = dfl->start + ofst;
> >  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > +	finfo->irq_base = irq_base;
> > +	finfo->nr_irqs = nr_irqs;
> >  	finfo->ioaddr = dfl->ioaddr + ofst;
> >  
> >  	list_add_tail(&finfo->node, &binfo->sub_features);
> > @@ -684,7 +718,8 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> >  
> >  	WARN_ON(!size);
> >  
> > -	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> > +	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU,
> > +				       0, 0);
> >  }
> >  
> >  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > @@ -724,7 +759,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +	ret = create_feature_instance(binfo, dfl, ofst, 0, 0, 0, 0);
> >  	if (ret)
> >  		return ret;
> >  	/*
> > @@ -742,17 +777,72 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> >  	return ret;
> >  }
> >  
> > +static void parse_feature_irqs(struct build_feature_devs_info *binfo,
> > +			       struct dfl_fpga_enum_dfl *dfl,
> > +			       resource_size_t ofst,
> > +			       unsigned int *irq_base, unsigned int *nr_irqs)
> > +{
> > +	u64 id, v;
> > +
> > +	/*
> > +	 * Ideally DFL framework should only read info from DFL header, but
> > +	 * current version DFL only provides mmio resources information for
> > +	 * each feature in DFL Header, no field for interrupt resources.
> > +	 * Some interrupt resources information are provided by specific
> > +	 * mmio registers of each components(e.g. different private features)
> > +	 * which supports interrupt. So in order to parse and assign irq
> > +	 * resources to different components, DFL framework has to look into
> > +	 * specific capability registers of these core private features.
> > +	 *
> > +	 * Once future DFL version supports generic interrupt resources
> > +	 * information in common DFL headers, below code could be replaced by
> > +	 * some generic interrupt parsing code.
> 
> Per my understanding, even later version hardware uses common DFL Header Field
> for interrupt resources enumeration, but we still keep this quirk for old
> version hardware, right?

Yes, later version hardware will introduce the common DFL description of
interrupt, which should override these quirks. But to be compatible to
old version hardware, driver should fallback to these quirks for these 3
sub feature if common interrupt description is not provided.

> 
> > +	 */
> > +
> > +	id = feature_id((dfl->ioaddr + ofst));
> > +
> > +	if (id == PORT_FEATURE_ID_UINT) {
> > +		v = readq(dfl->ioaddr + ofst + PORT_UINT_CAP);
> > +		*irq_base = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > +		*nr_irqs = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > +	} else if (id == PORT_FEATURE_ID_ERROR) {
> > +		v = readq(dfl->ioaddr + ofst + PORT_ERROR_CAP);
> > +		*irq_base = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > +		*nr_irqs = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > +	} else if (id == FME_FEATURE_ID_GLOBAL_ERR) {
> > +		v = readq(dfl->ioaddr + ofst + FME_ERROR_CAP);
> > +		*irq_base = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > +		*nr_irqs = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > +	} else {
> > +		return;
> > +	}
> > +
> > +	dev_dbg(binfo->dev, "feature: 0x%llx, nr_irqs: %u, irq_base: %u\n",
> > +		(unsigned long long)id, *nr_irqs, *irq_base);
> > +
> > +	if (*irq_base + *nr_irqs > binfo->nr_irqs) {
> > +		*irq_base = 0;
> > +		*nr_irqs = 0;
> > +		dev_err(binfo->dev, "Inconsistent interrupt number on HW\n");
> 
> Maybe you want to print feature id as well in this error case.

OK. Print feature_id should be helpful when we hit the error.

> 
> > +	}
> > +}
> > +
> >  static int parse_feature_private(struct build_feature_devs_info *binfo,
> >  				 struct dfl_fpga_enum_dfl *dfl,
> >  				 resource_size_t ofst)
> >  {
> > +	unsigned int irq_base = 0, nr_irqs = 0;
> > +
> >  	if (!binfo->feature_dev) {
> >  		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> >  			(unsigned long long)feature_id(dfl->ioaddr + ofst));
> >  		return -EINVAL;
> >  	}
> >  
> > -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +	parse_feature_irqs(binfo, dfl, ofst, &irq_base, &nr_irqs);
> > +
> > +	return create_feature_instance(binfo, dfl, ofst, 0, 0, irq_base,
> > +				       nr_irqs);
> >  }
> >  
> >  /**
> > @@ -853,6 +943,10 @@ void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info)
> >  		devm_kfree(dev, dfl);
> >  	}
> >  
> > +	/* remove irq table */
> > +	if (info->irq_table)
> > +		devm_kfree(dev, info->irq_table);
> > +
> >  	devm_kfree(dev, info);
> >  	put_device(dev);
> >  }
> > @@ -892,6 +986,40 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_dfl);
> >  
> > +/**
> > + * dfl_fpga_enum_info_add_irq - add irq table to enum info
> > + *
> > + * @info: ptr to dfl_fpga_enum_info
> > + * @nr_irqs: number of irqs for all feature devices.
> 
> It's better to say, number of irqs of the dfl fpga device to be enumerated.
> This function needs to be used before enumerate API.

Yes, I'll change it.

> 
> > + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> > + *	       this device.
> > + *
> > + * One FPGA device may have several interrupts. This function adds irq
> > + * information of all feature devices to enumeration info for next step
> 
> Same here.

Yes.

> 
> > + * enumeration.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> > +			       unsigned int nr_irqs, int *irq_table)
> > +{
> > +	if (!nr_irqs)
> > +		return -EINVAL;
> > +
> 
> Looks like currently it only allows add irq table once, so please document
> this as it doesn't support users adding multiple irq tables.

Sure.

> 
> Thanks
> Hao
> 
> > +	if (info->irq_table)
> > +		return -EEXIST;
> > +
> > +	info->irq_table = devm_kmemdup(info->dev, irq_table,
> > +				       sizeof(int) * nr_irqs, GFP_KERNEL);
> > +	if (!info->irq_table)
> > +		return -ENOMEM;
> > +
> > +	info->nr_irqs = nr_irqs;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_irq);
> > +
> >  static int remove_feature_dev(struct device *dev, void *data)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> > @@ -959,6 +1087,10 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
> >  	binfo->dev = info->dev;
> >  	binfo->cdev = cdev;
> >  
> > +	binfo->nr_irqs = info->nr_irqs;
> > +	if (info->nr_irqs)
> > +		binfo->irq_table = info->irq_table;
> > +
> >  	/*
> >  	 * start enumeration for all feature devices based on Device Feature
> >  	 * Lists.
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 4a9a33c..6a498cd 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -112,6 +112,13 @@
> >  #define FME_PORT_OFST_ACC_VF	1
> >  #define FME_PORT_OFST_IMP	BIT_ULL(60)
> >  
> > +/* FME Error Capability Register */
> > +#define FME_ERROR_CAP		0x70
> > +
> > +/* FME Error Capability Register Bitfield */
> > +#define FME_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> > +#define FME_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> > +
> >  /* PORT Header Register Set */
> >  #define PORT_HDR_DFH		DFH
> >  #define PORT_HDR_GUID_L		GUID_L
> > @@ -145,6 +152,20 @@
> >  #define PORT_STS_PWR_STATE_AP2	2			/* 90% throttling */
> >  #define PORT_STS_PWR_STATE_AP6	6			/* 100% throttling */
> >  
> > +/* Port Error Capability Register */
> > +#define PORT_ERROR_CAP		0x38
> > +
> > +/* Port Error Capability Register Bitfield */
> > +#define PORT_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> > +#define PORT_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> > +
> > +/* Port Uint Capability Register */
> > +#define PORT_UINT_CAP		0x8
> > +
> > +/* Port Uint Capability Register Bitfield */
> > +#define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts num */
> > +#define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector */
> > +
> >  /**
> >   * struct dfl_fpga_port_ops - port ops
> >   *
> > @@ -189,6 +210,15 @@ struct dfl_feature_driver {
> >  };
> >  
> >  /**
> > + * struct dfl_feature_irq_ctx - dfl private feature interrupt context
> > + *
> > + * @irq: Linux IRQ number of this interrupt.
> > + */
> > +struct dfl_feature_irq_ctx {
> > +	int irq;
> > +};
> > +
> > +/**
> >   * struct dfl_feature - sub feature of the feature devices
> >   *
> >   * @id: sub feature id.
> > @@ -196,12 +226,16 @@ struct dfl_feature_driver {
> >   *		    this index is used to find its mmio resource from the
> >   *		    feature dev (platform device)'s reources.
> >   * @ioaddr: mapped mmio resource address.
> > + * @irq_ctx: interrupt context list.
> > + * @nr_irqs: number of interrupt contexts.
> >   * @ops: ops of this sub feature.
> >   */
> >  struct dfl_feature {
> >  	u64 id;
> >  	int resource_index;
> >  	void __iomem *ioaddr;
> > +	struct dfl_feature_irq_ctx *irq_ctx;
> > +	unsigned int nr_irqs;
> >  	const struct dfl_feature_ops *ops;
> >  };
> >  
> > @@ -388,10 +422,14 @@ static inline u8 dfl_feature_revision(void __iomem *base)
> >   *
> >   * @dev: parent device.
> >   * @dfls: list of device feature lists.
> > + * @nr_irqs: number of irqs for all feature devices.
> > + * @irq_table: Linux IRQ numbers for all irqs, indexed by hw irq numbers.
> >   */
> >  struct dfl_fpga_enum_info {
> >  	struct device *dev;
> >  	struct list_head dfls;
> > +	unsigned int nr_irqs;
> > +	int *irq_table;
> >  };
> >  
> >  /**
> > @@ -415,6 +453,8 @@ struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> >  			       resource_size_t start, resource_size_t len,
> >  			       void __iomem *ioaddr);
> > +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> > +			       unsigned int nr_irqs, int *irq_table);
> >  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
> >  
> >  /**
> > -- 
> > 2.7.4

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

[Xu Yilun]

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

Re: [PATCH 1/7] fpga: dfl: parse interrupt info for feature devices on enumeration

[Wu Hao]

On Tue, Mar 10, 2020 at 05:25:31PM +0800, Xu Yilun wrote:
> On Tue, Mar 10, 2020 at 10:26:49AM +0800, Wu Hao wrote:
> > Hi Yilun
> > 
> > Some comments inline.
> > 
> > On Mon, Mar 09, 2020 at 06:29:44PM +0800, Xu Yilun wrote:
> > > DFL based FPGA devices could support interrupts for different purposes,
> > > but current DFL framework only supports feature device enumeration with
> > > given MMIO resources information via common DFL headers. This patch
> > > introduces one new API dfl_fpga_enum_info_add_irq for low level bus
> > > drivers (e.g. PCIe device driver) to pass its interrupt resources
> > > information to DFL framework for enumeration, and also adds interrupt
> > > enumeration code in framework to parse and assign interrupt resources
> > > for enumerated feature devices and their own sub features.
> > > 
> > > With this patch, DFL framework enumerates interrupt resources for core
> > > features, including PORT Error Reporting, FME (FPGA Management Engine)
> > > Error Reporting and also AFU User Interrupts.
> > > 
> > > 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.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  drivers/fpga/dfl.h |  40 +++++++++++++++
> > >  2 files changed, 176 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > > index 9909948..493822d 100644
> > > --- a/drivers/fpga/dfl.c
> > > +++ b/drivers/fpga/dfl.c
> > > @@ -11,6 +11,7 @@
> > >   *   Xiao Guangrong <guangrong.xiao@linux.intel.com>
> > >   */
> > >  #include <linux/module.h>
> > > +#include <asm/irq.h>
> > >  
> > >  #include "dfl.h"
> > >  
> > > @@ -421,6 +422,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> > >   *
> > >   * @dev: device to enumerate.
> > >   * @cdev: the container device for all feature devices.
> > > + * @nr_irqs: number of irqs for all feature devices.
> > > + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> > > + *	       this device.
> > >   * @feature_dev: current feature device.
> > >   * @ioaddr: header register region address of feature device in enumeration.
> > >   * @sub_features: a sub features linked list for feature device in enumeration.
> > > @@ -429,6 +433,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> > >  struct build_feature_devs_info {
> > >  	struct device *dev;
> > >  	struct dfl_fpga_cdev *cdev;
> > > +	unsigned int nr_irqs;
> > > +	int *irq_table;
> > > +
> > >  	struct platform_device *feature_dev;
> > >  	void __iomem *ioaddr;
> > >  	struct list_head sub_features;
> > > @@ -442,12 +449,16 @@ struct build_feature_devs_info {
> > >   * @mmio_res: mmio resource of this sub feature.
> > >   * @ioaddr: mapped base address of mmio resource.
> > >   * @node: node in sub_features linked list.
> > > + * @irq_base: start of irq index in this sub feature.
> > > + * @nr_irqs: number of irqs of this sub feature.
> > >   */
> > >  struct dfl_feature_info {
> > >  	u64 fid;
> > >  	struct resource mmio_res;
> > >  	void __iomem *ioaddr;
> > >  	struct list_head node;
> > > +	unsigned int irq_base;
> > > +	unsigned int nr_irqs;
> > >  };
> > >  
> > >  static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> > > @@ -520,6 +531,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > >  	/* fill features and resource information for feature dev */
> > >  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > >  		struct dfl_feature *feature = &pdata->features[index];
> > > +		struct dfl_feature_irq_ctx *ctx;
> > > +		int i, virq;
> > >  
> > >  		/* save resource information for each feature */
> > >  		feature->id = finfo->fid;
> > > @@ -527,6 +540,24 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> > >  		feature->ioaddr = finfo->ioaddr;
> > >  		fdev->resource[index++] = finfo->mmio_res;
> > >  
> > > +		if (finfo->nr_irqs) {
> > > +			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> > > +					   sizeof(*ctx), GFP_KERNEL);
> > > +			if (!ctx)
> > > +				return -ENOMEM;
> > > +
> > > +			for (i = 0; i < finfo->nr_irqs; i++) {
> > > +				virq = binfo->irq_table[finfo->irq_base + i];
> > 
> > Looks like it checks the irq_table input in commit_dev which is the last step
> > of enumeration. I feel that maybe it's better to check the input earlier, e.g.
> > in dfl_fpga_enum_info_add_irq() function below.
> 
> Yes the check could be done earlier. But maybe too early checking in
> dfl_fpga_enum_info_add_irq(). I think dfl should allow holes in
> irq_table. We make sure sub features get valid irq numbers for
> each of its irq line, that should be enough.

Ok. Understand that it's possible that table may have holes. A multiple function
device may only want to hand part of its irq resources to DFL.

> 
> So maybe do this check in parse_feature_irqs() is better?

Yes. Sounds better.

> 
> > 
> > BTW: why it's named as virq? any specific meaning?
> 
> I'm just trying to indicate it's a linux irq number, not a hardware
> local irq number. I see the name also used as linux irq number in other
> drivers.
> 
> > 
> > > +				if (virq < 0 || virq > NR_IRQS)
> > > +					return -EINVAL;
> > > +
> > > +				ctx[i].irq = virq;
> > > +			}
> > > +
> > > +			feature->irq_ctx = ctx;
> > > +			feature->nr_irqs = finfo->nr_irqs;
> > > +		}
> > > +
> > >  		list_del(&finfo->node);
> > >  		kfree(finfo);
> > >  	}
> > > @@ -648,7 +679,8 @@ static u64 feature_id(void __iomem *start)
> > >  static int
> > >  create_feature_instance(struct build_feature_devs_info *binfo,
> > >  			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> > > -			resource_size_t size, u64 fid)
> > > +			resource_size_t size, u64 fid, unsigned int irq_base,
> > > +			unsigned int nr_irqs)
> > >  {
> > >  	struct dfl_feature_info *finfo;
> > >  
> > > @@ -667,6 +699,8 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> > >  	finfo->mmio_res.start = dfl->start + ofst;
> > >  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> > >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> > > +	finfo->irq_base = irq_base;
> > > +	finfo->nr_irqs = nr_irqs;
> > >  	finfo->ioaddr = dfl->ioaddr + ofst;
> > >  
> > >  	list_add_tail(&finfo->node, &binfo->sub_features);
> > > @@ -684,7 +718,8 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> > >  
> > >  	WARN_ON(!size);
> > >  
> > > -	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> > > +	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU,
> > > +				       0, 0);
> > >  }
> > >  
> > >  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > > @@ -724,7 +759,7 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> > > +	ret = create_feature_instance(binfo, dfl, ofst, 0, 0, 0, 0);
> > >  	if (ret)
> > >  		return ret;
> > >  	/*
> > > @@ -742,17 +777,72 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > >  	return ret;
> > >  }
> > >  
> > > +static void parse_feature_irqs(struct build_feature_devs_info *binfo,
> > > +			       struct dfl_fpga_enum_dfl *dfl,
> > > +			       resource_size_t ofst,
> > > +			       unsigned int *irq_base, unsigned int *nr_irqs)
> > > +{
> > > +	u64 id, v;
> > > +
> > > +	/*
> > > +	 * Ideally DFL framework should only read info from DFL header, but
> > > +	 * current version DFL only provides mmio resources information for
> > > +	 * each feature in DFL Header, no field for interrupt resources.
> > > +	 * Some interrupt resources information are provided by specific
> > > +	 * mmio registers of each components(e.g. different private features)
> > > +	 * which supports interrupt. So in order to parse and assign irq
> > > +	 * resources to different components, DFL framework has to look into
> > > +	 * specific capability registers of these core private features.
> > > +	 *
> > > +	 * Once future DFL version supports generic interrupt resources
> > > +	 * information in common DFL headers, below code could be replaced by
> > > +	 * some generic interrupt parsing code.
> > 
> > Per my understanding, even later version hardware uses common DFL Header Field
> > for interrupt resources enumeration, but we still keep this quirk for old
> > version hardware, right?
> 
> Yes, later version hardware will introduce the common DFL description of
> interrupt, which should override these quirks. But to be compatible to
> old version hardware, driver should fallback to these quirks for these 3
> sub feature if common interrupt description is not provided.

So please update above comments, below code is still needed later, and
won't be replaced. Thanks!

Hao

> 
> > 
> > > +	 */
> > > +
> > > +	id = feature_id((dfl->ioaddr + ofst));
> > > +
> > > +	if (id == PORT_FEATURE_ID_UINT) {
> > > +		v = readq(dfl->ioaddr + ofst + PORT_UINT_CAP);
> > > +		*irq_base = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> > > +		*nr_irqs = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> > > +	} else if (id == PORT_FEATURE_ID_ERROR) {
> > > +		v = readq(dfl->ioaddr + ofst + PORT_ERROR_CAP);
> > > +		*irq_base = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> > > +		*nr_irqs = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> > > +	} else if (id == FME_FEATURE_ID_GLOBAL_ERR) {
> > > +		v = readq(dfl->ioaddr + ofst + FME_ERROR_CAP);
> > > +		*irq_base = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> > > +		*nr_irqs = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> > > +	} else {
> > > +		return;
> > > +	}
> > > +
> > > +	dev_dbg(binfo->dev, "feature: 0x%llx, nr_irqs: %u, irq_base: %u\n",
> > > +		(unsigned long long)id, *nr_irqs, *irq_base);
> > > +
> > > +	if (*irq_base + *nr_irqs > binfo->nr_irqs) {
> > > +		*irq_base = 0;
> > > +		*nr_irqs = 0;
> > > +		dev_err(binfo->dev, "Inconsistent interrupt number on HW\n");
> > 
> > Maybe you want to print feature id as well in this error case.
> 
> OK. Print feature_id should be helpful when we hit the error.
> 
> > 
> > > +	}
> > > +}
> > > +
> > >  static int parse_feature_private(struct build_feature_devs_info *binfo,
> > >  				 struct dfl_fpga_enum_dfl *dfl,
> > >  				 resource_size_t ofst)
> > >  {
> > > +	unsigned int irq_base = 0, nr_irqs = 0;
> > > +
> > >  	if (!binfo->feature_dev) {
> > >  		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> > >  			(unsigned long long)feature_id(dfl->ioaddr + ofst));
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > > +	parse_feature_irqs(binfo, dfl, ofst, &irq_base, &nr_irqs);
> > > +
> > > +	return create_feature_instance(binfo, dfl, ofst, 0, 0, irq_base,
> > > +				       nr_irqs);
> > >  }
> > >  
> > >  /**
> > > @@ -853,6 +943,10 @@ void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info)
> > >  		devm_kfree(dev, dfl);
> > >  	}
> > >  
> > > +	/* remove irq table */
> > > +	if (info->irq_table)
> > > +		devm_kfree(dev, info->irq_table);
> > > +
> > >  	devm_kfree(dev, info);
> > >  	put_device(dev);
> > >  }
> > > @@ -892,6 +986,40 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > >  }
> > >  EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_dfl);
> > >  
> > > +/**
> > > + * dfl_fpga_enum_info_add_irq - add irq table to enum info
> > > + *
> > > + * @info: ptr to dfl_fpga_enum_info
> > > + * @nr_irqs: number of irqs for all feature devices.
> > 
> > It's better to say, number of irqs of the dfl fpga device to be enumerated.
> > This function needs to be used before enumerate API.
> 
> Yes, I'll change it.
> 
> > 
> > > + * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> > > + *	       this device.
> > > + *
> > > + * One FPGA device may have several interrupts. This function adds irq
> > > + * information of all feature devices to enumeration info for next step
> > 
> > Same here.
> 
> Yes.
> 
> > 
> > > + * enumeration.
> > > + *
> > > + * Return: 0 on success, negative error code otherwise.
> > > + */
> > > +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> > > +			       unsigned int nr_irqs, int *irq_table)
> > > +{
> > > +	if (!nr_irqs)
> > > +		return -EINVAL;
> > > +
> > 
> > Looks like currently it only allows add irq table once, so please document
> > this as it doesn't support users adding multiple irq tables.
> 
> Sure.
> 
> > 
> > Thanks
> > Hao
> > 
> > > +	if (info->irq_table)
> > > +		return -EEXIST;
> > > +
> > > +	info->irq_table = devm_kmemdup(info->dev, irq_table,
> > > +				       sizeof(int) * nr_irqs, GFP_KERNEL);
> > > +	if (!info->irq_table)
> > > +		return -ENOMEM;
> > > +
> > > +	info->nr_irqs = nr_irqs;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_add_irq);
> > > +
> > >  static int remove_feature_dev(struct device *dev, void *data)
> > >  {
> > >  	struct platform_device *pdev = to_platform_device(dev);
> > > @@ -959,6 +1087,10 @@ dfl_fpga_feature_devs_enumerate(struct dfl_fpga_enum_info *info)
> > >  	binfo->dev = info->dev;
> > >  	binfo->cdev = cdev;
> > >  
> > > +	binfo->nr_irqs = info->nr_irqs;
> > > +	if (info->nr_irqs)
> > > +		binfo->irq_table = info->irq_table;
> > > +
> > >  	/*
> > >  	 * start enumeration for all feature devices based on Device Feature
> > >  	 * Lists.
> > > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > > index 4a9a33c..6a498cd 100644
> > > --- a/drivers/fpga/dfl.h
> > > +++ b/drivers/fpga/dfl.h
> > > @@ -112,6 +112,13 @@
> > >  #define FME_PORT_OFST_ACC_VF	1
> > >  #define FME_PORT_OFST_IMP	BIT_ULL(60)
> > >  
> > > +/* FME Error Capability Register */
> > > +#define FME_ERROR_CAP		0x70
> > > +
> > > +/* FME Error Capability Register Bitfield */
> > > +#define FME_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> > > +#define FME_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> > > +
> > >  /* PORT Header Register Set */
> > >  #define PORT_HDR_DFH		DFH
> > >  #define PORT_HDR_GUID_L		GUID_L
> > > @@ -145,6 +152,20 @@
> > >  #define PORT_STS_PWR_STATE_AP2	2			/* 90% throttling */
> > >  #define PORT_STS_PWR_STATE_AP6	6			/* 100% throttling */
> > >  
> > > +/* Port Error Capability Register */
> > > +#define PORT_ERROR_CAP		0x38
> > > +
> > > +/* Port Error Capability Register Bitfield */
> > > +#define PORT_ERROR_CAP_SUPP_INT	BIT_ULL(0)		/* Interrupt Support */
> > > +#define PORT_ERROR_CAP_INT_VECT	GENMASK_ULL(12, 1)	/* Interrupt vector */
> > > +
> > > +/* Port Uint Capability Register */
> > > +#define PORT_UINT_CAP		0x8
> > > +
> > > +/* Port Uint Capability Register Bitfield */
> > > +#define PORT_UINT_CAP_INT_NUM	GENMASK_ULL(11, 0)	/* Interrupts num */
> > > +#define PORT_UINT_CAP_FST_VECT	GENMASK_ULL(23, 12)	/* First Vector */
> > > +
> > >  /**
> > >   * struct dfl_fpga_port_ops - port ops
> > >   *
> > > @@ -189,6 +210,15 @@ struct dfl_feature_driver {
> > >  };
> > >  
> > >  /**
> > > + * struct dfl_feature_irq_ctx - dfl private feature interrupt context
> > > + *
> > > + * @irq: Linux IRQ number of this interrupt.
> > > + */
> > > +struct dfl_feature_irq_ctx {
> > > +	int irq;
> > > +};
> > > +
> > > +/**
> > >   * struct dfl_feature - sub feature of the feature devices
> > >   *
> > >   * @id: sub feature id.
> > > @@ -196,12 +226,16 @@ struct dfl_feature_driver {
> > >   *		    this index is used to find its mmio resource from the
> > >   *		    feature dev (platform device)'s reources.
> > >   * @ioaddr: mapped mmio resource address.
> > > + * @irq_ctx: interrupt context list.
> > > + * @nr_irqs: number of interrupt contexts.
> > >   * @ops: ops of this sub feature.
> > >   */
> > >  struct dfl_feature {
> > >  	u64 id;
> > >  	int resource_index;
> > >  	void __iomem *ioaddr;
> > > +	struct dfl_feature_irq_ctx *irq_ctx;
> > > +	unsigned int nr_irqs;
> > >  	const struct dfl_feature_ops *ops;
> > >  };
> > >  
> > > @@ -388,10 +422,14 @@ static inline u8 dfl_feature_revision(void __iomem *base)
> > >   *
> > >   * @dev: parent device.
> > >   * @dfls: list of device feature lists.
> > > + * @nr_irqs: number of irqs for all feature devices.
> > > + * @irq_table: Linux IRQ numbers for all irqs, indexed by hw irq numbers.
> > >   */
> > >  struct dfl_fpga_enum_info {
> > >  	struct device *dev;
> > >  	struct list_head dfls;
> > > +	unsigned int nr_irqs;
> > > +	int *irq_table;
> > >  };
> > >  
> > >  /**
> > > @@ -415,6 +453,8 @@ struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> > >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > >  			       resource_size_t start, resource_size_t len,
> > >  			       void __iomem *ioaddr);
> > > +int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> > > +			       unsigned int nr_irqs, int *irq_table);
> > >  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
> > >  
> > >  /**
> > > -- 
> > > 2.7.4

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

[Wu Hao]

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

Re: [PATCH 3/7] fpga: dfl: introduce interrupt trigger setting API

[Wu Hao]

On Mon, Mar 09, 2020 at 06:29:46PM +0800, Xu Yilun wrote:
> FPGA user applications may be interested in interrupts generated by
> DFL features. For example, users can implement their own FPGA
> logics with interrupts enabled in AFU (Accelerated Function Unit,
> dynamic region of DFL based FPGA). So user applications need to be
> notified to handle these interrupts.
> 
> In order to allow userspace applications to monitor interrupts,
> driver requires userspace to provide eventfds as interrupt
> notification channels. Applications then poll/select on the eventfds
> to get notified.
> 
> This patch introduces a generic helper function for sub features to
> do eventfds binding with given interrupts.
> 
> 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.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.h | 11 +++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 493822d..ae6baca 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -535,6 +535,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		int i, virq;
>  
>  		/* save resource information for each feature */
> +		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->resource_index = index;
>  		feature->ioaddr = finfo->ioaddr;
> @@ -1373,6 +1374,98 @@ int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_vf);
>  
> +static irqreturn_t dfl_irq_handler(int irq, void *arg)
> +{
> +	struct eventfd_ctx *trigger = arg;
> +
> +	eventfd_signal(trigger, 1);
> +	return IRQ_HANDLED;
> +}
> +
> +static int do_set_irq_trigger(struct dfl_feature *feature, int idx, int fd)
> +{
> +	struct platform_device *pdev = feature->dev;
> +	struct eventfd_ctx *trigger;
> +	int irq, ret;
> +
> +	if (idx < 0 || idx >= feature->nr_irqs)
> +		return -EINVAL;
> +
> +	irq = feature->irq_ctx[idx].irq;
> +
> +	if (feature->irq_ctx[idx].trigger) {
> +		free_irq(irq, feature->irq_ctx[idx].trigger);
> +		kfree(feature->irq_ctx[idx].name);
> +		eventfd_ctx_put(feature->irq_ctx[idx].trigger);
> +		feature->irq_ctx[idx].trigger = NULL;
> +	}
> +
> +	if (fd < 0)
> +		return 0;
> +
> +	feature->irq_ctx[idx].name =
> +		kasprintf(GFP_KERNEL, "fpga-irq[%d](%s-%llx)", idx,
> +			  dev_name(&pdev->dev),
> +			  (unsigned long long)feature->id);
> +	if (!feature->irq_ctx[idx].name)
> +		return -ENOMEM;
> +
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		ret = PTR_ERR(trigger);
> +		goto free_name;
> +	}
> +
> +	ret = request_irq(irq, dfl_irq_handler, 0,
> +			  feature->irq_ctx[idx].name, trigger);
> +	if (!ret) {
> +		feature->irq_ctx[idx].trigger = trigger;
> +		return ret;
> +	}
> +
> +	eventfd_ctx_put(trigger);
> +free_name:
> +	kfree(feature->irq_ctx[idx].name);
> +
> +	return ret;
> +}
> +
> +/**
> + * dfl_fpga_set_irq_triggers - set eventfd triggers for dfl feature interrupts
> + *
> + * @feature: dfl sub feature.
> + * @start: start of irq index in this dfl sub feature.
> + * @count: number of irqs.
> + * @fds: eventfds to bind with irqs.
> + *
> + * Bind given eventfds with irqs in this dfl sub feature. Use negative fds as
> + * parameter to unbind irqs.

Looks like it accepts NULL for fds to unbind irqs, please add some description
here as well.

> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
> +			      unsigned int count, int32_t *fds)
> +{
> +	int i, j, ret = 0;
> +
> +	if (start + count < start || start + count > feature->nr_irqs)
> +		return -EINVAL;
> +
> +	for (i = 0, j = start; i < count && !ret; i++, j++) {
> +		int fd = fds ? fds[i] : -1;
> +
> +		ret = do_set_irq_trigger(feature, j, fd);
> +	}
> +
> +	if (ret) {
> +		for (--j; j >= (int)start; j--)

it converts unsigned int start to int, what about upper loop code?
should start and count be converted as well?

Thanks
Hao

> +			do_set_irq_trigger(feature, j, -1);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_set_irq_triggers);
> +
>  static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 6a498cd..6b60077 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -24,6 +24,8 @@
>  #include <linux/slab.h>
>  #include <linux/uuid.h>
>  #include <linux/fpga/fpga-region.h>
> +#include <linux/interrupt.h>
> +#include <linux/eventfd.h>
>  
>  /* maximum supported number of ports */
>  #define MAX_DFL_FPGA_PORT_NUM 4
> @@ -213,14 +215,19 @@ struct dfl_feature_driver {
>   * struct dfl_feature_irq_ctx - dfl private feature interrupt context
>   *
>   * @irq: Linux IRQ number of this interrupt.
> + * @trigger: eventfd context to signal when interrupt happens.
> + * @name: irq name needed when requesting irq.
>   */
>  struct dfl_feature_irq_ctx {
>  	int irq;
> +	struct eventfd_ctx *trigger;
> +	char *name;
>  };
>  
>  /**
>   * struct dfl_feature - sub feature of the feature devices
>   *
> + * @dev: ptr to pdev of the feature device which has the sub feature.
>   * @id: sub feature id.
>   * @resource_index: each sub feature has one mmio resource for its registers.
>   *		    this index is used to find its mmio resource from the
> @@ -231,6 +238,7 @@ struct dfl_feature_irq_ctx {
>   * @ops: ops of this sub feature.
>   */
>  struct dfl_feature {
> +	struct platform_device *dev;
>  	u64 id;
>  	int resource_index;
>  	void __iomem *ioaddr;
> @@ -506,4 +514,7 @@ int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id);
>  int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id);
>  void dfl_fpga_cdev_config_ports_pf(struct dfl_fpga_cdev *cdev);
>  int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vf);
> +
> +int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
> +			      unsigned int count, int32_t *fds);
>  #endif /* __FPGA_DFL_H */
> -- 
> 2.7.4

Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting

[Wu Hao]

On Mon, Mar 09, 2020 at 06:29:47PM +0800, Xu Yilun wrote:
> Error reporting interrupt is very useful to notify users that some
> errors are detected by the hardware. Once users are notified, they
> could query hardware logged error states, no need to continuously
> poll on these states.
> 
> This patch follows the common DFL interrupt notification and handling
> mechanism, implements two ioctl commands below for user to query
> hardware capability, and set/unset interrupt triggers.
> 
>  Ioctls:
>  * DFL_FPGA_PORT_ERR_GET_INFO
>    get error reporting feature info, including num_irqs which is used to
>    determine whether/how many interrupts it supports.
> 
>  * DFL_FPGA_PORT_ERR_SET_IRQ
>    set/unset given eventfds as error interrupt triggers.
> 
> 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-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl-afu-main.c   |  4 +++
>  include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
>  3 files changed, 107 insertions(+)
> 
> diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> index c1467ae..a2c5454 100644
> --- a/drivers/fpga/dfl-afu-error.c
> +++ b/drivers/fpga/dfl-afu-error.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/uaccess.h>
> +#include <linux/fpga-dfl.h>
>  
>  #include "dfl-afu.h"
>  
> @@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
>  	afu_port_err_mask(&pdev->dev, true);
>  }
>  
> +static long
> +port_err_get_info(struct platform_device *pdev,
> +		  struct dfl_feature *feature, unsigned long arg)
> +{
> +	struct dfl_fpga_port_err_info info;
> +
> +	info.flags = 0;
> +	info.capability = 0;

as flags and capability are not used at this moment, so actually it only exposes
irq information to user. I understand flags and capability are used for
future extension, but it may not work without argsz, as we never know what
comes next, e.g. a capability requires > 32bit can't fit into this ioctl.
So maybe just a ioctl for IRQ_INFO is enough for now.

How do you think?

> +	info.num_irqs = feature->nr_irqs;
> +
> +	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static long port_err_set_irq(struct platform_device *pdev,
> +			     struct dfl_feature *feature, unsigned long arg)
> +{
> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct dfl_fpga_irq_set hdr;
> +	s32 *fds;
> +	long ret;
> +
> +	if (!feature->nr_irqs)
> +		return -ENOENT;
> +
> +	if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> +		return -EFAULT;
> +
> +	if (hdr.flags || (hdr.start + hdr.count > feature->nr_irqs) ||
> +	    (hdr.start + hdr.count < hdr.start) || !hdr.count)
> +		return -EINVAL;
> +
> +	fds = memdup_user((void __user *)(arg + sizeof(hdr)),
> +			  hdr.count * sizeof(s32));
> +	if (IS_ERR(fds))
> +		return PTR_ERR(fds);
> +
> +	mutex_lock(&pdata->lock);
> +	ret = dfl_fpga_set_irq_triggers(feature, hdr.start, hdr.count, fds);
> +	mutex_unlock(&pdata->lock);
> +
> +	kfree(fds);
> +	return ret;
> +}
> +
> +static long
> +port_err_ioctl(struct platform_device *pdev, struct dfl_feature *feature,
> +	       unsigned int cmd, unsigned long arg)
> +{
> +	long ret = -ENODEV;
> +
> +	switch (cmd) {
> +	case DFL_FPGA_PORT_ERR_GET_INFO:
> +		ret = port_err_get_info(pdev, feature, arg);
> +		break;
> +	case DFL_FPGA_PORT_ERR_SET_IRQ:
> +		ret = port_err_set_irq(pdev, feature, arg);
> +		break;
> +	default:
> +		dev_dbg(&pdev->dev, "%x cmd not handled", cmd);
> +	}
> +
> +	return ret;
> +}
> +
>  const struct dfl_feature_id port_err_id_table[] = {
>  	{.id = PORT_FEATURE_ID_ERROR,},
>  	{0,}
> @@ -227,4 +295,5 @@ const struct dfl_feature_id port_err_id_table[] = {
>  const struct dfl_feature_ops port_err_ops = {
>  	.init = port_err_init,
>  	.uinit = port_err_uinit,
> +	.ioctl = port_err_ioctl,
>  };
> diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> index 435bde4..fc8b9cf 100644
> --- a/drivers/fpga/dfl-afu-main.c
> +++ b/drivers/fpga/dfl-afu-main.c
> @@ -577,6 +577,7 @@ static int afu_release(struct inode *inode, struct file *filp)
>  {
>  	struct platform_device *pdev = filp->private_data;
>  	struct dfl_feature_platform_data *pdata;
> +	struct dfl_feature *feature;
>  
>  	dev_dbg(&pdev->dev, "Device File Release\n");
>  
> @@ -586,6 +587,9 @@ static int afu_release(struct inode *inode, struct file *filp)
>  	dfl_feature_dev_use_end(pdata);
>  
>  	if (!dfl_feature_dev_use_count(pdata)) {
> +		dfl_fpga_dev_for_each_feature(pdata, feature)
> +			dfl_fpga_set_irq_triggers(feature, 0,
> +						  feature->nr_irqs, NULL);
>  		__port_reset(pdev);
>  		afu_dma_region_destroy(pdata);
>  	}
> diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
> index ec70a0746..846fc91 100644
> --- a/include/uapi/linux/fpga-dfl.h
> +++ b/include/uapi/linux/fpga-dfl.h
> @@ -151,6 +151,40 @@ struct dfl_fpga_port_dma_unmap {
>  
>  #define DFL_FPGA_PORT_DMA_UNMAP		_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 4)
>  
> +/**
> + * DFL_FPGA_PORT_ERR_GET_INFO - _IOR(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5,
> + *						struct dfl_fpga_port_err_info)
> + *
> + * Retrieve information about the fpga port error reporting private feature.
> + * Driver fills the info in provided struct dfl_fpga_port_err_info.
> + * Return: 0 on success, -errno on failure.
> + */
> +struct dfl_fpga_port_err_info {
> +	/* Output */
> +	__u32 flags;		/* Zero for now */
> +	__u32 capability;	/* The capability of port error reporting */
> +	__u32 num_irqs;		/* number of irqs it supports */
> +};
> +
> +#define DFL_FPGA_PORT_ERR_GET_INFO	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5)
> +
> +/**
> + * DFL_FPGA_PORT_ERR_SET_IRQ - _IOW(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6,
> + *						struct dfl_fpga_irq_set)
> + *
> + * Set fpga port error reporting interrupt trigger if evtfds[n] is valid.
> + * Unset related interrupt trigger if evtfds[n] is a negative value.
> + * Return: 0 on success, -errno on failure.
> + */
> +struct dfl_fpga_irq_set {
> +	__u32 flags;		/* Zero for now */
> +	__u32 start;		/* First irq number */
> +	__u32 count;		/* The number of eventfd handler */
> +	__s32 evtfds[];		/* Eventfd handler */
> +};
> +
> +#define DFL_FPGA_PORT_ERR_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6)
> +
>  /* IOCTLs for FME file descriptor */
>  
>  /**
> -- 
> 2.7.4

Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting

[Xu Yilun]

On Tue, Mar 10, 2020 at 06:39:21PM +0800, Wu Hao wrote:
> On Mon, Mar 09, 2020 at 06:29:47PM +0800, Xu Yilun wrote:
> > Error reporting interrupt is very useful to notify users that some
> > errors are detected by the hardware. Once users are notified, they
> > could query hardware logged error states, no need to continuously
> > poll on these states.
> > 
> > This patch follows the common DFL interrupt notification and handling
> > mechanism, implements two ioctl commands below for user to query
> > hardware capability, and set/unset interrupt triggers.
> > 
> >  Ioctls:
> >  * DFL_FPGA_PORT_ERR_GET_INFO
> >    get error reporting feature info, including num_irqs which is used to
> >    determine whether/how many interrupts it supports.
> > 
> >  * DFL_FPGA_PORT_ERR_SET_IRQ
> >    set/unset given eventfds as error interrupt triggers.
> > 
> > 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-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl-afu-main.c   |  4 +++
> >  include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
> >  3 files changed, 107 insertions(+)
> > 
> > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > index c1467ae..a2c5454 100644
> > --- a/drivers/fpga/dfl-afu-error.c
> > +++ b/drivers/fpga/dfl-afu-error.c
> > @@ -15,6 +15,7 @@
> >   */
> >  
> >  #include <linux/uaccess.h>
> > +#include <linux/fpga-dfl.h>
> >  
> >  #include "dfl-afu.h"
> >  
> > @@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
> >  	afu_port_err_mask(&pdev->dev, true);
> >  }
> >  
> > +static long
> > +port_err_get_info(struct platform_device *pdev,
> > +		  struct dfl_feature *feature, unsigned long arg)
> > +{
> > +	struct dfl_fpga_port_err_info info;
> > +
> > +	info.flags = 0;
> > +	info.capability = 0;
> 
> as flags and capability are not used at this moment, so actually it only exposes
> irq information to user. I understand flags and capability are used for
> future extension, but it may not work without argsz, as we never know what
> comes next, e.g. a capability requires > 32bit can't fit into this ioctl.
> So maybe just a ioctl for IRQ_INFO is enough for now.
> 
> How do you think?

Yes the flags & capability are for future extension.

The capability field is planned to a bitmask, each bit could indicate the feature
has some capability or not. So it could describe up to 32 capabilities.
I think it would be enough for one sub feature.

With this field, users could get a general description of capabilities with one
ioctl. If some capability has more detailed info, we may add addtional ioctl to
fetch it. This is how it works without argsz. Does it make sense?

And same definition for flag field. The flag fields could contain some
bool running state represented by each bit.

> 
> > +	info.num_irqs = feature->nr_irqs;
> > +
> > +	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static long port_err_set_irq(struct platform_device *pdev,
> > +			     struct dfl_feature *feature, unsigned long arg)
> > +{
> > +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +	struct dfl_fpga_irq_set hdr;
> > +	s32 *fds;
> > +	long ret;
> > +
> > +	if (!feature->nr_irqs)
> > +		return -ENOENT;
> > +
> > +	if (copy_from_user(&hdr, (void __user *)arg, sizeof(hdr)))
> > +		return -EFAULT;
> > +
> > +	if (hdr.flags || (hdr.start + hdr.count > feature->nr_irqs) ||
> > +	    (hdr.start + hdr.count < hdr.start) || !hdr.count)
> > +		return -EINVAL;
> > +
> > +	fds = memdup_user((void __user *)(arg + sizeof(hdr)),
> > +			  hdr.count * sizeof(s32));
> > +	if (IS_ERR(fds))
> > +		return PTR_ERR(fds);
> > +
> > +	mutex_lock(&pdata->lock);
> > +	ret = dfl_fpga_set_irq_triggers(feature, hdr.start, hdr.count, fds);
> > +	mutex_unlock(&pdata->lock);
> > +
> > +	kfree(fds);
> > +	return ret;
> > +}
> > +
> > +static long
> > +port_err_ioctl(struct platform_device *pdev, struct dfl_feature *feature,
> > +	       unsigned int cmd, unsigned long arg)
> > +{
> > +	long ret = -ENODEV;
> > +
> > +	switch (cmd) {
> > +	case DFL_FPGA_PORT_ERR_GET_INFO:
> > +		ret = port_err_get_info(pdev, feature, arg);
> > +		break;
> > +	case DFL_FPGA_PORT_ERR_SET_IRQ:
> > +		ret = port_err_set_irq(pdev, feature, arg);
> > +		break;
> > +	default:
> > +		dev_dbg(&pdev->dev, "%x cmd not handled", cmd);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  const struct dfl_feature_id port_err_id_table[] = {
> >  	{.id = PORT_FEATURE_ID_ERROR,},
> >  	{0,}
> > @@ -227,4 +295,5 @@ const struct dfl_feature_id port_err_id_table[] = {
> >  const struct dfl_feature_ops port_err_ops = {
> >  	.init = port_err_init,
> >  	.uinit = port_err_uinit,
> > +	.ioctl = port_err_ioctl,
> >  };
> > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
> > index 435bde4..fc8b9cf 100644
> > --- a/drivers/fpga/dfl-afu-main.c
> > +++ b/drivers/fpga/dfl-afu-main.c
> > @@ -577,6 +577,7 @@ static int afu_release(struct inode *inode, struct file *filp)
> >  {
> >  	struct platform_device *pdev = filp->private_data;
> >  	struct dfl_feature_platform_data *pdata;
> > +	struct dfl_feature *feature;
> >  
> >  	dev_dbg(&pdev->dev, "Device File Release\n");
> >  
> > @@ -586,6 +587,9 @@ static int afu_release(struct inode *inode, struct file *filp)
> >  	dfl_feature_dev_use_end(pdata);
> >  
> >  	if (!dfl_feature_dev_use_count(pdata)) {
> > +		dfl_fpga_dev_for_each_feature(pdata, feature)
> > +			dfl_fpga_set_irq_triggers(feature, 0,
> > +						  feature->nr_irqs, NULL);
> >  		__port_reset(pdev);
> >  		afu_dma_region_destroy(pdata);
> >  	}
> > diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
> > index ec70a0746..846fc91 100644
> > --- a/include/uapi/linux/fpga-dfl.h
> > +++ b/include/uapi/linux/fpga-dfl.h
> > @@ -151,6 +151,40 @@ struct dfl_fpga_port_dma_unmap {
> >  
> >  #define DFL_FPGA_PORT_DMA_UNMAP		_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 4)
> >  
> > +/**
> > + * DFL_FPGA_PORT_ERR_GET_INFO - _IOR(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5,
> > + *						struct dfl_fpga_port_err_info)
> > + *
> > + * Retrieve information about the fpga port error reporting private feature.
> > + * Driver fills the info in provided struct dfl_fpga_port_err_info.
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct dfl_fpga_port_err_info {
> > +	/* Output */
> > +	__u32 flags;		/* Zero for now */
> > +	__u32 capability;	/* The capability of port error reporting */
> > +	__u32 num_irqs;		/* number of irqs it supports */
> > +};
> > +
> > +#define DFL_FPGA_PORT_ERR_GET_INFO	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 5)
> > +
> > +/**
> > + * DFL_FPGA_PORT_ERR_SET_IRQ - _IOW(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6,
> > + *						struct dfl_fpga_irq_set)
> > + *
> > + * Set fpga port error reporting interrupt trigger if evtfds[n] is valid.
> > + * Unset related interrupt trigger if evtfds[n] is a negative value.
> > + * Return: 0 on success, -errno on failure.
> > + */
> > +struct dfl_fpga_irq_set {
> > +	__u32 flags;		/* Zero for now */
> > +	__u32 start;		/* First irq number */
> > +	__u32 count;		/* The number of eventfd handler */
> > +	__s32 evtfds[];		/* Eventfd handler */
> > +};
> > +
> > +#define DFL_FPGA_PORT_ERR_SET_IRQ	_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 6)
> > +
> >  /* IOCTLs for FME file descriptor */
> >  
> >  /**
> > -- 
> > 2.7.4

Re: [PATCH 3/7] fpga: dfl: introduce interrupt trigger setting API

[Xu Yilun]

On Tue, Mar 10, 2020 at 06:30:24PM +0800, Wu Hao wrote:
> On Mon, Mar 09, 2020 at 06:29:46PM +0800, Xu Yilun wrote:
> > FPGA user applications may be interested in interrupts generated by
> > DFL features. For example, users can implement their own FPGA
> > logics with interrupts enabled in AFU (Accelerated Function Unit,
> > dynamic region of DFL based FPGA). So user applications need to be
> > notified to handle these interrupts.
> > 
> > In order to allow userspace applications to monitor interrupts,
> > driver requires userspace to provide eventfds as interrupt
> > notification channels. Applications then poll/select on the eventfds
> > to get notified.
> > 
> > This patch introduces a generic helper function for sub features to
> > do eventfds binding with given interrupts.
> > 
> > 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.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.h | 11 +++++++
> >  2 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 493822d..ae6baca 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -535,6 +535,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  		int i, virq;
> >  
> >  		/* save resource information for each feature */
> > +		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> >  		feature->resource_index = index;
> >  		feature->ioaddr = finfo->ioaddr;
> > @@ -1373,6 +1374,98 @@ int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_vf);
> >  
> > +static irqreturn_t dfl_irq_handler(int irq, void *arg)
> > +{
> > +	struct eventfd_ctx *trigger = arg;
> > +
> > +	eventfd_signal(trigger, 1);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int do_set_irq_trigger(struct dfl_feature *feature, int idx, int fd)
> > +{
> > +	struct platform_device *pdev = feature->dev;
> > +	struct eventfd_ctx *trigger;
> > +	int irq, ret;
> > +
> > +	if (idx < 0 || idx >= feature->nr_irqs)
> > +		return -EINVAL;
> > +
> > +	irq = feature->irq_ctx[idx].irq;
> > +
> > +	if (feature->irq_ctx[idx].trigger) {
> > +		free_irq(irq, feature->irq_ctx[idx].trigger);
> > +		kfree(feature->irq_ctx[idx].name);
> > +		eventfd_ctx_put(feature->irq_ctx[idx].trigger);
> > +		feature->irq_ctx[idx].trigger = NULL;
> > +	}
> > +
> > +	if (fd < 0)
> > +		return 0;
> > +
> > +	feature->irq_ctx[idx].name =
> > +		kasprintf(GFP_KERNEL, "fpga-irq[%d](%s-%llx)", idx,
> > +			  dev_name(&pdev->dev),
> > +			  (unsigned long long)feature->id);
> > +	if (!feature->irq_ctx[idx].name)
> > +		return -ENOMEM;
> > +
> > +	trigger = eventfd_ctx_fdget(fd);
> > +	if (IS_ERR(trigger)) {
> > +		ret = PTR_ERR(trigger);
> > +		goto free_name;
> > +	}
> > +
> > +	ret = request_irq(irq, dfl_irq_handler, 0,
> > +			  feature->irq_ctx[idx].name, trigger);
> > +	if (!ret) {
> > +		feature->irq_ctx[idx].trigger = trigger;
> > +		return ret;
> > +	}
> > +
> > +	eventfd_ctx_put(trigger);
> > +free_name:
> > +	kfree(feature->irq_ctx[idx].name);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * dfl_fpga_set_irq_triggers - set eventfd triggers for dfl feature interrupts
> > + *
> > + * @feature: dfl sub feature.
> > + * @start: start of irq index in this dfl sub feature.
> > + * @count: number of irqs.
> > + * @fds: eventfds to bind with irqs.
> > + *
> > + * Bind given eventfds with irqs in this dfl sub feature. Use negative fds as
> > + * parameter to unbind irqs.
> 
> Looks like it accepts NULL for fds to unbind irqs, please add some description
> here as well.

Sure.

> 
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
> > +			      unsigned int count, int32_t *fds)
> > +{
> > +	int i, j, ret = 0;
> > +
> > +	if (start + count < start || start + count > feature->nr_irqs)
> > +		return -EINVAL;
> > +
> > +	for (i = 0, j = start; i < count && !ret; i++, j++) {
> > +		int fd = fds ? fds[i] : -1;
> > +
> > +		ret = do_set_irq_trigger(feature, j, fd);
> > +	}
> > +
> > +	if (ret) {
> > +		for (--j; j >= (int)start; j--)
> 
> it converts unsigned int start to int, what about upper loop code?
> should start and count be converted as well?

Using int for i, j seems not suitable. Let me change them to unsigned
int and fix other parts accordingly.

Thanks
Yilun

> 
> Thanks
> Hao
> 
> > +			do_set_irq_trigger(feature, j, -1);
> > +	}
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dfl_fpga_set_irq_triggers);
> > +
> >  static void __exit dfl_fpga_exit(void)
> >  {
> >  	dfl_chardev_uinit();
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index 6a498cd..6b60077 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -24,6 +24,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/uuid.h>
> >  #include <linux/fpga/fpga-region.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/eventfd.h>
> >  
> >  /* maximum supported number of ports */
> >  #define MAX_DFL_FPGA_PORT_NUM 4
> > @@ -213,14 +215,19 @@ struct dfl_feature_driver {
> >   * struct dfl_feature_irq_ctx - dfl private feature interrupt context
> >   *
> >   * @irq: Linux IRQ number of this interrupt.
> > + * @trigger: eventfd context to signal when interrupt happens.
> > + * @name: irq name needed when requesting irq.
> >   */
> >  struct dfl_feature_irq_ctx {
> >  	int irq;
> > +	struct eventfd_ctx *trigger;
> > +	char *name;
> >  };
> >  
> >  /**
> >   * struct dfl_feature - sub feature of the feature devices
> >   *
> > + * @dev: ptr to pdev of the feature device which has the sub feature.
> >   * @id: sub feature id.
> >   * @resource_index: each sub feature has one mmio resource for its registers.
> >   *		    this index is used to find its mmio resource from the
> > @@ -231,6 +238,7 @@ struct dfl_feature_irq_ctx {
> >   * @ops: ops of this sub feature.
> >   */
> >  struct dfl_feature {
> > +	struct platform_device *dev;
> >  	u64 id;
> >  	int resource_index;
> >  	void __iomem *ioaddr;
> > @@ -506,4 +514,7 @@ int dfl_fpga_cdev_release_port(struct dfl_fpga_cdev *cdev, int port_id);
> >  int dfl_fpga_cdev_assign_port(struct dfl_fpga_cdev *cdev, int port_id);
> >  void dfl_fpga_cdev_config_ports_pf(struct dfl_fpga_cdev *cdev);
> >  int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vf);
> > +
> > +int dfl_fpga_set_irq_triggers(struct dfl_feature *feature, unsigned int start,
> > +			      unsigned int count, int32_t *fds);
> >  #endif /* __FPGA_DFL_H */
> > -- 
> > 2.7.4

Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting

[Wu Hao]

On Wed, Mar 11, 2020 at 12:47:38AM +0800, Xu Yilun wrote:
> On Tue, Mar 10, 2020 at 06:39:21PM +0800, Wu Hao wrote:
> > On Mon, Mar 09, 2020 at 06:29:47PM +0800, Xu Yilun wrote:
> > > Error reporting interrupt is very useful to notify users that some
> > > errors are detected by the hardware. Once users are notified, they
> > > could query hardware logged error states, no need to continuously
> > > poll on these states.
> > > 
> > > This patch follows the common DFL interrupt notification and handling
> > > mechanism, implements two ioctl commands below for user to query
> > > hardware capability, and set/unset interrupt triggers.
> > > 
> > >  Ioctls:
> > >  * DFL_FPGA_PORT_ERR_GET_INFO
> > >    get error reporting feature info, including num_irqs which is used to
> > >    determine whether/how many interrupts it supports.
> > > 
> > >  * DFL_FPGA_PORT_ERR_SET_IRQ
> > >    set/unset given eventfds as error interrupt triggers.
> > > 
> > > 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-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/fpga/dfl-afu-main.c   |  4 +++
> > >  include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
> > >  3 files changed, 107 insertions(+)
> > > 
> > > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > > index c1467ae..a2c5454 100644
> > > --- a/drivers/fpga/dfl-afu-error.c
> > > +++ b/drivers/fpga/dfl-afu-error.c
> > > @@ -15,6 +15,7 @@
> > >   */
> > >  
> > >  #include <linux/uaccess.h>
> > > +#include <linux/fpga-dfl.h>
> > >  
> > >  #include "dfl-afu.h"
> > >  
> > > @@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
> > >  	afu_port_err_mask(&pdev->dev, true);
> > >  }
> > >  
> > > +static long
> > > +port_err_get_info(struct platform_device *pdev,
> > > +		  struct dfl_feature *feature, unsigned long arg)
> > > +{
> > > +	struct dfl_fpga_port_err_info info;
> > > +
> > > +	info.flags = 0;
> > > +	info.capability = 0;
> > 
> > as flags and capability are not used at this moment, so actually it only exposes
> > irq information to user. I understand flags and capability are used for
> > future extension, but it may not work without argsz, as we never know what
> > comes next, e.g. a capability requires > 32bit can't fit into this ioctl.
> > So maybe just a ioctl for IRQ_INFO is enough for now.
> > 
> > How do you think?
> 
> Yes the flags & capability are for future extension.
> 
> The capability field is planned to a bitmask, each bit could indicate the feature
> has some capability or not. So it could describe up to 32 capabilities.
> I think it would be enough for one sub feature.
> 
> With this field, users could get a general description of capabilities with one
> ioctl. If some capability has more detailed info, we may add addtional ioctl to
> fetch it. This is how it works without argsz. Does it make sense?
> 
> And same definition for flag field. The flag fields could contain some
> bool running state represented by each bit.

This should work for some cases, but kernel doc (core-api/ioctl.rst) says it's
better to avoid bitfield completely. I understand it's possible to extend this
ioctl with flags and capability, even we can define if flags = A, then given 
capability = definition B, if flags = C, then capbaility definition is D, to
maximum the usage for extension, but it may make this interface very very
complicated to users. This should be the same reason why you didn't put irq
info into capability directly. Another reason is, in my understanding, it
choices ioctl to expose irq info becasue user must use ioctl to set irq, for
other capabilities which doesn't use device file, then some sysfs may be enough
for their own functions.

Thanks
Hao

Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting

[Xu Yilun]

On Wed, Mar 11, 2020 at 10:43:56AM +0800, Wu Hao wrote:
> On Wed, Mar 11, 2020 at 12:47:38AM +0800, Xu Yilun wrote:
> > On Tue, Mar 10, 2020 at 06:39:21PM +0800, Wu Hao wrote:
> > > On Mon, Mar 09, 2020 at 06:29:47PM +0800, Xu Yilun wrote:
> > > > Error reporting interrupt is very useful to notify users that some
> > > > errors are detected by the hardware. Once users are notified, they
> > > > could query hardware logged error states, no need to continuously
> > > > poll on these states.
> > > > 
> > > > This patch follows the common DFL interrupt notification and handling
> > > > mechanism, implements two ioctl commands below for user to query
> > > > hardware capability, and set/unset interrupt triggers.
> > > > 
> > > >  Ioctls:
> > > >  * DFL_FPGA_PORT_ERR_GET_INFO
> > > >    get error reporting feature info, including num_irqs which is used to
> > > >    determine whether/how many interrupts it supports.
> > > > 
> > > >  * DFL_FPGA_PORT_ERR_SET_IRQ
> > > >    set/unset given eventfds as error interrupt triggers.
> > > > 
> > > > 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-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/fpga/dfl-afu-main.c   |  4 +++
> > > >  include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
> > > >  3 files changed, 107 insertions(+)
> > > > 
> > > > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > > > index c1467ae..a2c5454 100644
> > > > --- a/drivers/fpga/dfl-afu-error.c
> > > > +++ b/drivers/fpga/dfl-afu-error.c
> > > > @@ -15,6 +15,7 @@
> > > >   */
> > > >  
> > > >  #include <linux/uaccess.h>
> > > > +#include <linux/fpga-dfl.h>
> > > >  
> > > >  #include "dfl-afu.h"
> > > >  
> > > > @@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
> > > >  	afu_port_err_mask(&pdev->dev, true);
> > > >  }
> > > >  
> > > > +static long
> > > > +port_err_get_info(struct platform_device *pdev,
> > > > +		  struct dfl_feature *feature, unsigned long arg)
> > > > +{
> > > > +	struct dfl_fpga_port_err_info info;
> > > > +
> > > > +	info.flags = 0;
> > > > +	info.capability = 0;
> > > 
> > > as flags and capability are not used at this moment, so actually it only exposes
> > > irq information to user. I understand flags and capability are used for
> > > future extension, but it may not work without argsz, as we never know what
> > > comes next, e.g. a capability requires > 32bit can't fit into this ioctl.
> > > So maybe just a ioctl for IRQ_INFO is enough for now.
> > > 
> > > How do you think?
> > 
> > Yes the flags & capability are for future extension.
> > 
> > The capability field is planned to a bitmask, each bit could indicate the feature
> > has some capability or not. So it could describe up to 32 capabilities.
> > I think it would be enough for one sub feature.
> > 
> > With this field, users could get a general description of capabilities with one
> > ioctl. If some capability has more detailed info, we may add addtional ioctl to
> > fetch it. This is how it works without argsz. Does it make sense?
> > 
> > And same definition for flag field. The flag fields could contain some
> > bool running state represented by each bit.
> 
> This should work for some cases, but kernel doc (core-api/ioctl.rst) says it's
> better to avoid bitfield completely. I understand it's possible to extend this
> ioctl with flags and capability, even we can define if flags = A, then given 
> capability = definition B, if flags = C, then capbaility definition is D, to
> maximum the usage for extension, but it may make this interface very very
> complicated to users. This should be the same reason why you didn't put irq
> info into capability directly. Another reason is, in my understanding, it
> choices ioctl to expose irq info becasue user must use ioctl to set irq, for
> other capabilities which doesn't use device file, then some sysfs may be enough
> for their own functions.

Thanks for clarify this, I'll remove the flags & capability fields

Yilun

> 
> Thanks
> Hao