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

[Moritz Fischer]

On Mon, Apr 20, 2020 at 04:11:39PM +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 functions to do eventfds binding
> with given interrupts.
> 
> Sub feature drivers are expected to use XXX_GET_IRQ_NUM to query irq
> info, and XXX_SET_IRQ to set eventfds for interrupts. This patch also
> introduces helper functions for these 2 ioctls.
> 
> Signed-off-by: Luwei Kang <luwei.kang@xxxxxxxxx>
> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> Acked-by: Wu Hao <hao.wu@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()
> v3: Improve comments of dfl_fpga_set_irq_triggers()
>     refines code for dfl_fpga_set_irq_triggers, delete local variable j
> v4: Introduce 2 helper functions to help handle the XXX_GET_IRQ_NUM &
>     XXX_SET_IRQ ioctls for sub feature drivers.
> v5: Some minor fix for Hao's comments
> ---
>  drivers/fpga/dfl.c            | 156 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/dfl.h            |  17 +++++
>  include/uapi/linux/fpga-dfl.h |  13 ++++
>  3 files changed, 186 insertions(+)
> 
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b49fbed..208d8f0 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -10,7 +10,9 @@
>   *   Wu Hao <hao.wu@xxxxxxxxx>
>   *   Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>   */
> +#include <linux/fpga-dfl.h>
>  #include <linux/module.h>
> +#include <linux/uaccess.h>
>  
>  #include "dfl.h"
>  
> @@ -534,6 +536,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		unsigned int i;
>  
>  		/* save resource information for each feature */
> +		feature->dev = fdev;
>  		feature->id = finfo->fid;
>  		feature->resource_index = index;
>  		feature->ioaddr = finfo->ioaddr;
> @@ -1395,6 +1398,159 @@ 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. unbind related irq if fds[n] is negative.
> + *	 unbind "count" specified number of irqs if fds ptr is NULL.
> + *
> + * Bind given eventfds with irqs in this dfl sub feature. Unbind related irq if
> + * fds[n] is negative. Unbind "count" specified number of irqs if fds ptr is
> + * NULL.
> + *
> + * 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;
> +	int ret = 0;
> +
> +	if (start + count < start || start + count > feature->nr_irqs)
> +		return -EINVAL;
Up to you, but have you considered to break up the condition above into
two if () ?
> +
> +	for (i = 0; i < count; i++) {
> +		int fd = fds ? fds[i] : -1;
> +
> +		ret = do_set_irq_trigger(feature, start + i, fd);
> +		if (ret) {
> +			while (i--)
> +				do_set_irq_trigger(feature, start + i, -1);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfl_fpga_set_irq_triggers);
> +
> +/**
> + * dfl_feature_ioctl_get_num_irqs - dfl feature _GET_IRQ_NUM ioctl interface.
> + * @pdev: the feature device which has the sub feature
> + * @feature: the dfl sub feature
> + * @arg: ioctl argument
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +long dfl_feature_ioctl_get_num_irqs(struct platform_device *pdev,
> +				    struct dfl_feature *feature,
> +				    unsigned long arg)
> +{
> +	return put_user(feature->nr_irqs, (__u32 __user *)arg);
> +}
> +EXPORT_SYMBOL_GPL(dfl_feature_ioctl_get_num_irqs);
> +
> +/**
> + * dfl_feature_ioctl_set_irq - dfl feature _SET_IRQ ioctl interface.
> + * @pdev: the feature device which has the sub feature
> + * @feature: the dfl sub feature
> + * @arg: ioctl argument
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +long dfl_feature_ioctl_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.count || (hdr.start + hdr.count > feature->nr_irqs) ||
> +	    (hdr.start + hdr.count < hdr.start))
> +		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;
> +}
> +EXPORT_SYMBOL_GPL(dfl_feature_ioctl_set_irq);
> +
>  static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index 4bc165f..f7a8c59 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -17,7 +17,9 @@
>  #include <linux/bitfield.h>
>  #include <linux/cdev.h>
>  #include <linux/delay.h>
> +#include <linux/eventfd.h>
>  #include <linux/fs.h>
> +#include <linux/interrupt.h>
>  #include <linux/iopoll.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/platform_device.h>
> @@ -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,13 @@ 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);
> +long dfl_feature_ioctl_get_num_irqs(struct platform_device *pdev,
> +				    struct dfl_feature *feature,
> +				    unsigned long arg);
> +long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> +			       struct dfl_feature *feature,
> +			       unsigned long arg);
> +
>  #endif /* __FPGA_DFL_H */
> diff --git a/include/uapi/linux/fpga-dfl.h b/include/uapi/linux/fpga-dfl.h
> index ec70a0746..7331350 100644
> --- a/include/uapi/linux/fpga-dfl.h
> +++ b/include/uapi/linux/fpga-dfl.h
> @@ -151,6 +151,19 @@ struct dfl_fpga_port_dma_unmap {
>  
>  #define DFL_FPGA_PORT_DMA_UNMAP		_IO(DFL_FPGA_MAGIC, DFL_PORT_BASE + 4)
>  
> +/**
> + * struct dfl_fpga_irq_set - the argument for DFL_FPGA_XXX_SET_IRQ ioctl.
> + *
> + * @start: Index of the first irq.
> + * @count: The number of eventfd handler.
> + * @evtfds: Eventfd handlers.
> + */
> +struct dfl_fpga_irq_set {
> +	__u32 start;
> +	__u32 count;
> +	__s32 evtfds[];
> +};
> +
>  /* IOCTLs for FME file descriptor */
>  
>  /**
> -- 
> 2.7.4
> 
Thanks,
Moritz