diff mbox series

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

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

Commit Message

Xu Yilun March 16, 2020, 4:16 a.m. UTC 
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>
----
v2: use unsigned int instead of int for irq array indexes in
    dfl_fpga_set_irq_triggers()
    Improves comments for NULL fds param in dfl_fpga_set_irq_triggers()
---
 drivers/fpga/dfl.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/dfl.h | 11 +++++++
 2 files changed, 106 insertions(+)

Comments

Wu, Hao March 18, 2020, 8:27 a.m. UTC | #1 
On Mon, Mar 16, 2020 at 12:16:58PM +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>
> ----
> v2: use unsigned int instead of int for irq array indexes in
>     dfl_fpga_set_irq_triggers()
>     Improves comments for NULL fds param in dfl_fpga_set_irq_triggers()
> ---
>  drivers/fpga/dfl.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.h | 11 +++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 28e2cd8..8dcc4e2 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;
>  
>  		/* save resource information for each feature */
> +		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->resource_index = index;
>  		feature->ioaddr = finfo->ioaddr;
> @@ -1385,6 +1386,100 @@ 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, unsigned int idx,
> +			      int fd)
> +{
> +	struct platform_device *pdev = feature->dev;
> +	struct eventfd_ctx *trigger;
> +	int irq, ret;
> +
> +	if (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[%u](%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 NULL or negative
> + * fds as parameter to unbind irqs.

It seems that it accepts valid fds and invalid fds (negative) in the same table
pointed by fds ptr, righ? Could we also add this into description 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)
> +{
> +	unsigned int i, j;
> +	int 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);
> +	}

could we just replace j with start + i?

other places look good to me.

Hao

> +
> +	if (ret) {
> +		for (--j; j >= 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
Xu Yilun March 19, 2020, 12:47 p.m. UTC | #2 
On Wed, Mar 18, 2020 at 04:27:23PM +0800, Wu Hao wrote:
> On Mon, Mar 16, 2020 at 12:16:58PM +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>
> > ----
> > v2: use unsigned int instead of int for irq array indexes in
> >     dfl_fpga_set_irq_triggers()
> >     Improves comments for NULL fds param in dfl_fpga_set_irq_triggers()
> > ---
> >  drivers/fpga/dfl.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/dfl.h | 11 +++++++
> >  2 files changed, 106 insertions(+)
> > 
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 28e2cd8..8dcc4e2 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;
> >  
> >  		/* save resource information for each feature */
> > +		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> >  		feature->resource_index = index;
> >  		feature->ioaddr = finfo->ioaddr;
> > @@ -1385,6 +1386,100 @@ 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, unsigned int idx,
> > +			      int fd)
> > +{
> > +	struct platform_device *pdev = feature->dev;
> > +	struct eventfd_ctx *trigger;
> > +	int irq, ret;
> > +
> > +	if (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[%u](%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 NULL or negative
> > + * fds as parameter to unbind irqs.
> 
> It seems that it accepts valid fds and invalid fds (negative) in the same table
> pointed by fds ptr, righ? Could we also add this into description as well?

Yes, I'll add the description.

> 
> > + *
> > + * 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)
> > +{
> > +	unsigned int i, j;
> > +	int 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);
> > +	}
> 
> could we just replace j with start + i?

The j will be used if error happens in following code. I tried to remove
j but feel the code becomes harder to understand.

How about we keep it?

> 
> other places look good to me.
> 
> Hao
> 
> > +
> > +	if (ret) {
> > +		for (--j; j >= 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
diff mbox series

Patch

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 28e2cd8..8dcc4e2 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;
 
 		/* save resource information for each feature */
+		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->resource_index = index;
 		feature->ioaddr = finfo->ioaddr;
@@ -1385,6 +1386,100 @@  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, unsigned int idx,
+			      int fd)
+{
+	struct platform_device *pdev = feature->dev;
+	struct eventfd_ctx *trigger;
+	int irq, ret;
+
+	if (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[%u](%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 NULL or 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)
+{
+	unsigned int i, j;
+	int 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 >= 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 */