linux-fpga.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Russ Weight <russell.h.weight@intel.com>
To: Xu Yilun <yilun.xu@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: Fri, 24 Sep 2021 14:45:57 -0700	[thread overview]
Message-ID: <2c63282d-504c-e428-5048-d0743d8523fa@intel.com> (raw)
In-Reply-To: <20210924084947.GC806603@yilunxu-OptiPlex-7050>



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.

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?

- 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-24 21:46 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 [this message]
2021-09-26  1:06       ` Xu Yilun
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=2c63282d-504c-e428-5048-d0743d8523fa@intel.com \
    --to=russell.h.weight@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=trix@redhat.com \
    --cc=yilun.xu@intel.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).