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

[Xu Yilun]

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@xxxxxxxxx>
> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > ----
> > 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