linux-fpga.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Xu Yilun <yilun.xu@intel.com>
To: Russ Weight <russell.h.weight@intel.com>
Cc: mdf@kernel.org, linux-fpga@vger.kernel.org,
	linux-kernel@vger.kernel.org, trix@redhat.com,
	lgoncalv@redhat.com, hao.wu@intel.com, matthew.gerlach@intel.com
Subject: Re: [PATCH v16 3/5] fpga: image-load: signal eventfd when complete
Date: Sun, 26 Sep 2021 09:06:10 +0800	[thread overview]
Message-ID: <20210926010610.GA1339205@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <2c63282d-504c-e428-5048-d0743d8523fa@intel.com>

On Fri, Sep 24, 2021 at 02:45:57PM -0700, Russ Weight wrote:
> 
> 
> On 9/24/21 1:49 AM, Xu Yilun wrote:
> > On Wed, Sep 22, 2021 at 05:10:54PM -0700, Russ Weight wrote:
> >> Amend the FPGA_IMAGE_LOAD_WRITE IOCTL implementation to include an
> >> eventfd file descriptor as a parameter. The eventfd will be triggered
> >> when the image upload completes.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> >> ---
> >> v16:
> >>  - Some cleanup of documentation for the FPGA_IMAGE_LOAD_WRITE IOCTL.
> >> v15:
> >>  - This patch is new to the patch-set, and adds an eventfd to the
> >>    FPGA_IMAGE_LOAD_WRITE IOCTL. The eventfd is signalled upon completion
> >>    of an update.
> >> ---
> >>  Documentation/fpga/fpga-image-load.rst | 12 +++++++-----
> >>  drivers/fpga/fpga-image-load.c         | 22 ++++++++++++++++++++--
> >>  include/linux/fpga/fpga-image-load.h   |  2 ++
> >>  include/uapi/linux/fpga-image-load.h   |  3 ++-
> >>  4 files changed, 31 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> >> index ba371c7c0ca0..22a455421bb4 100644
> >> --- a/Documentation/fpga/fpga-image-load.rst
> >> +++ b/Documentation/fpga/fpga-image-load.rst
> >> @@ -27,8 +27,10 @@ ioctl
> >>  
> >>  FPGA_IMAGE_LOAD_WRITE:
> >>  
> >> -Start an image upload with the provided image buffer. This IOCTL returns
> >> -immediately after starting a kernel worker thread to process the image
> >> -upload which could take as long a 40 minutes depending on the actual device
> >> -being updated. This is an exclusive operation; an attempt to start
> >> -concurrent image uploads for the same device will fail with EBUSY.
> >> +Start an image upload with the provided image buffer. This IOCTL returns
> >> +immediately after starting a kernel worker thread to process the image
> >> +upload which could take as long a 40 minutes depending on the actual device
> >> +being updated. This is an exclusive operation; an attempt to start
> > Just curious, there are marks here but seems no change.
> Hmmm - sometimes when fixing up patches, I find it easier to edit
> the patch files (e.g. changing file and symbol names). The patches
> applied cleanly, but I must have forgotten to regenerate them
> before sending them for review.
> 
> >
> >> +concurrent image loads for the same device will fail with EBUSY. An eventfd
> > You want to fix "uploads" to "loads"? But there are many other "upload(s)".
> My preference is upload, because load implies more than transferring
> the data but the data transfer is all that this driver really does.
> I preferred fpga_image_load to fpga_image_upload for the device name,
> just because it was a couple of characters less. I'll change "loads"
> to "uploads" here.

That's fine. This word is initiated in Patch #1 and changed in Patch #2,
which makes me wonder why.

> 
> Do you think I should rename the driver to fpga_image_upload? Or do
> you think the name is OK if it is probably documented?

The name is OK.

Thanks,
Yilun

> 
> - Russ
> >
> > Others look good to me.
> >
> >> +file descriptor parameter is provided to this IOCTL. It will be signalled
> >> +at the completion of the image upload.
> >> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> >> index 65f553b59011..09164a0258a5 100644
> >> --- a/drivers/fpga/fpga-image-load.c
> >> +++ b/drivers/fpga/fpga-image-load.c
> >> @@ -34,6 +34,7 @@ static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> >>  {
> >>  	mutex_lock(&imgld->lock);
> >>  	imgld->progress = FPGA_IMAGE_PROG_IDLE;
> >> +	eventfd_signal(imgld->finished, 1);
> >>  	mutex_unlock(&imgld->lock);
> >>  }
> >>  
> >> @@ -112,6 +113,8 @@ static void fpga_image_do_load(struct work_struct *work)
> >>  	vfree(imgld->data);
> >>  	imgld->data = NULL;
> >>  	fpga_image_prog_complete(imgld);
> >> +	eventfd_ctx_put(imgld->finished);
> >> +	imgld->finished = NULL;
> >>  }
> >>  
> >>  static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> @@ -119,6 +122,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >>  {
> >>  	struct fpga_image_write wb;
> >>  	unsigned long minsz;
> >> +	int ret;
> >>  	u8 *buf;
> >>  
> >>  	if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> >> @@ -135,13 +139,23 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >>  	if (wb.size & 0x3)
> >>  		return -EINVAL;
> >>  
> >> +	if (wb.evtfd < 0)
> >> +		return -EINVAL;
> >> +
> >>  	buf = vzalloc(wb.size);
> >>  	if (!buf)
> >>  		return -ENOMEM;
> >>  
> >>  	if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> >> -		vfree(buf);
> >> -		return -EFAULT;
> >> +		ret = -EFAULT;
> >> +		goto exit_free;
> >> +	}
> >> +
> >> +	imgld->finished = eventfd_ctx_fdget(wb.evtfd);
> >> +	if (IS_ERR(imgld->finished)) {
> >> +		ret = PTR_ERR(imgld->finished);
> >> +		imgld->finished = NULL;
> >> +		goto exit_free;
> >>  	}
> >>  
> >>  	imgld->data = buf;
> >> @@ -151,6 +165,10 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >>  	queue_work(system_unbound_wq, &imgld->work);
> >>  
> >>  	return 0;
> >> +
> >> +exit_free:
> >> +	vfree(buf);
> >> +	return ret;
> >>  }
> >>  
> >>  static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
> >> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> >> index 41ab63cf7b20..7d39daa4d921 100644
> >> --- a/include/linux/fpga/fpga-image-load.h
> >> +++ b/include/linux/fpga/fpga-image-load.h
> >> @@ -9,6 +9,7 @@
> >>  
> >>  #include <linux/cdev.h>
> >>  #include <linux/device.h>
> >> +#include <linux/eventfd.h>
> >>  #include <linux/mutex.h>
> >>  #include <linux/types.h>
> >>  #include <uapi/linux/fpga-image-load.h>
> >> @@ -52,6 +53,7 @@ struct fpga_image_load {
> >>  	u32 progress;
> >>  	u32 err_code;			/* image load error code */
> >>  	bool driver_unload;
> >> +	struct eventfd_ctx *finished;
> >>  	void *priv;
> >>  };
> >>  
> >> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
> >> index 0382078c5a6c..152a8e1c031f 100644
> >> --- a/include/uapi/linux/fpga-image-load.h
> >> +++ b/include/uapi/linux/fpga-image-load.h
> >> @@ -38,7 +38,7 @@
> >>   *				struct fpga_image_write)
> >>   *
> >>   * Upload a data buffer to the target device. The user must provide the
> >> - * data buffer and size.
> >> + * data buffer, size, and an eventfd file descriptor.
> >>   *
> >>   * Return: 0 on success, -errno on failure.
> >>   */
> >> @@ -46,6 +46,7 @@ struct fpga_image_write {
> >>  	/* Input */
> >>  	__u32 flags;		/* Zero for now */
> >>  	__u32 size;		/* Data size (in bytes) to be written */
> >> +	__s32 evtfd;		/* File descriptor for completion signal */
> >>  	__u64 buf;		/* User space address of source data */
> >>  };
> >>  
> >> -- 
> >> 2.25.1

  reply	other threads:[~2021-09-26  1:13 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-23  0:10 [PATCH v16 0/5] FPGA Image Load (previously Security Manager) Russ Weight
2021-09-23  0:10 ` [PATCH v16 1/5] fpga: image-load: fpga image load framework Russ Weight
2021-09-23  0:10 ` [PATCH v16 2/5] fpga: image-load: enable image uploads Russ Weight
2021-09-23  8:54   ` Xu Yilun
2021-09-24  0:21     ` Russ Weight
2021-09-24  8:12   ` Xu Yilun
2021-09-24 21:32     ` Russ Weight
2021-09-26  1:10       ` Xu Yilun
2021-09-27 16:33         ` Russ Weight
2021-09-23  0:10 ` [PATCH v16 3/5] fpga: image-load: signal eventfd when complete Russ Weight
2021-09-24  8:49   ` Xu Yilun
2021-09-24 21:45     ` Russ Weight
2021-09-26  1:06       ` Xu Yilun [this message]
2021-09-23  0:10 ` [PATCH v16 4/5] fpga: image-load: add status ioctl Russ Weight
2021-09-23  0:10 ` [PATCH v16 5/5] fpga: image-load: enable cancel of image upload Russ Weight
2021-09-24 14:53   ` Xu Yilun
2021-09-25  0:29     ` Russ Weight
2021-09-26  1:16       ` Xu Yilun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210926010610.GA1339205@yilunxu-OptiPlex-7050 \
    --to=yilun.xu@intel.com \
    --cc=hao.wu@intel.com \
    --cc=lgoncalv@redhat.com \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.gerlach@intel.com \
    --cc=mdf@kernel.org \
    --cc=russell.h.weight@intel.com \
    --cc=trix@redhat.com \
    --subject='Re: [PATCH v16 3/5] fpga: image-load: signal eventfd when complete' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).