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 v15 6/6] fpga: image-load: enable cancel of image upload
Date: Fri, 10 Sep 2021 16:38:35 -0700	[thread overview]
Message-ID: <839aa95e-c3d1-1f60-1b36-20d29445e61f@intel.com> (raw)
In-Reply-To: <20210910145545.GA757507@yilunxu-OptiPlex-7050>



On 9/10/21 7:55 AM, Xu Yilun wrote:
> On Wed, Sep 08, 2021 at 07:18:46PM -0700, Russ Weight wrote:
>> Extend the FPGA Image Load class driver to include a cancel IOCTL that
>> can be used to request that an image upload be canceled. The IOCTL may
>> return EBUSY if it cannot be canceled by software or ENODEV if there
>> is no update in progress.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v15:
>>  - Compare to previous patch:
>>      [PATCH v14 6/6] fpga: sec-mgr: enable cancel of secure update
>>  - Changed file, symbol, and config names to reflect the new driver name
>>  - Cancel is now initiated by IOCT instead of sysfs
>>  - Removed signed-off/reviewed-by tags
>> v14:
>>  - Updated ABI documentation date and kernel version
>> v13:
>>   - No change
>> v12:
>>   - Updated Date and KernelVersion fields in ABI documentation
>> v11:
>>   - No change
>> v10:
>>   - Rebased to 5.12-rc2 next
>>   - Updated Date and KernelVersion in ABI documentation
>> v9:
>>   - Updated Date and KernelVersion in ABI documentation
>> v8:
>>   - No change
>> v7:
>>   - Changed Date in documentation file to December 2020
>> v6:
>>   - No change
>> v5:
>>   - No change
>> v4:
>>   - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
>>     and removed unnecessary references to "Intel".
>>   - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
>> v3:
>>   - No change
>> v2:
>>   - Bumped documentation date and version
>>   - Minor code cleanup per review comments
>> ---
>> ---
>>  Documentation/fpga/fpga-image-load.rst |  6 ++++
>>  drivers/fpga/fpga-image-load.c         | 45 +++++++++++++++++++++++---
>>  include/linux/fpga/fpga-image-load.h   |  1 +
>>  include/uapi/linux/fpga-image-load.h   |  1 +
>>  4 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index 3d5eb51223e3..763e7833a6ea 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -37,3 +37,9 @@ FPGA_IMAGE_LOAD_STATUS:
>>  Collect status for an on-going image upload. The status returned includes
>>  how much data remains to be transferred, the progress of the image load,
>>  and error information in the case of a failure.
>> +
>> +FPGA_IMAGE_LOAD_CANCEL:
>> +
>> +Request that a on-going image upload be cancelled. This IOCTL may return
>> +EBUSY if it cannot be cancelled by software or ENODEV if there is no update
>> +in progress.
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index 6ec0a39f07b3..c32e4b1ea35a 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -46,6 +46,24 @@ static void fpga_image_dev_error(struct fpga_image_load *imgld,
>>  	imgld->lops->cancel(imgld);
>>  }
>>  
>> +static int fpga_image_prog_transition(struct fpga_image_load *imgld,
>> +				      enum fpga_image_prog new_progress)
>> +{
>> +	int ret = 0;
>> +
>> +	mutex_lock(&imgld->lock);
>> +	if (imgld->request_cancel) {
>> +		imgld->err_progress = imgld->progress;
>> +		imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
>> +		imgld->lops->cancel(imgld);
>> +		ret = -ECANCELED;
>> +	} else {
>> +		imgld->progress = new_progress;
>> +	}
>> +	mutex_unlock(&imgld->lock);
>> +	return ret;
>> +}
>> +
>>  static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>>  {
>>  	mutex_lock(&imgld->lock);
>> @@ -77,8 +95,10 @@ static void fpga_image_do_load(struct work_struct *work)
>>  		goto modput_exit;
>>  	}
>>  
>> -	fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING);
>> -	while (imgld->remaining_size) {
>> +	if (fpga_image_prog_transition(imgld, FPGA_IMAGE_PROG_WRITING))
>> +		goto done;
>> +
>> +	while (imgld->remaining_size && !imgld->request_cancel) {
>>  		ret = imgld->lops->write_blk(imgld, offset);
>>  		if (ret != FPGA_IMAGE_ERR_NONE) {
>>  			fpga_image_dev_error(imgld, ret);
>> @@ -88,7 +108,9 @@ static void fpga_image_do_load(struct work_struct *work)
>>  		offset = size - imgld->remaining_size;
>>  	}
>>  
>> -	fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
>> +	if (fpga_image_prog_transition(imgld, FPGA_IMAGE_PROG_PROGRAMMING))
>> +		goto done;
>> +
>>  	ret = imgld->lops->poll_complete(imgld);
>>  	if (ret != FPGA_IMAGE_ERR_NONE)
>>  		fpga_image_dev_error(imgld, ret);
>> @@ -159,6 +181,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>>  	imgld->remaining_size = wb.size;
>>  	imgld->err_code = FPGA_IMAGE_ERR_NONE;
>>  	imgld->progress = FPGA_IMAGE_PROG_STARTING;
>> +	imgld->request_cancel = false;
>>  	reinit_completion(&imgld->update_done);
>>  	schedule_work(&imgld->work);
>>  	return 0;
>> @@ -189,7 +212,7 @@ static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>>  				  unsigned long arg)
>>  {
>>  	struct fpga_image_load *imgld = filp->private_data;
>> -	int ret = -ENOTTY;
>> +	int ret = 0;
>>  
>>  	mutex_lock(&imgld->lock);
>>  
>> @@ -200,6 +223,17 @@ static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>>  	case FPGA_IMAGE_LOAD_STATUS:
>>  		ret = fpga_image_load_ioctl_status(imgld, arg);
>>  		break;
>> +	case FPGA_IMAGE_LOAD_CANCEL:
>> +		if (imgld->progress == FPGA_IMAGE_PROG_PROGRAMMING)
>> +			ret = -EBUSY;
>> +		else if (imgld->progress == FPGA_IMAGE_PROG_IDLE)
>> +			ret = -ENODEV;
>> +		else
>> +			imgld->request_cancel = true;
>> +		break;
>> +	default:
>> +		ret = -ENOTTY;
>> +		break;
>>  	}
>>  
>>  	mutex_unlock(&imgld->lock);
>> @@ -374,6 +408,9 @@ void fpga_image_load_unregister(struct fpga_image_load *imgld)
>>  		goto unregister;
>>  	}
>>  
>> +	if (imgld->progress != FPGA_IMAGE_PROG_PROGRAMMING)
>> +		imgld->request_cancel = true;
>> +
> Why we cancel the programing rather than waiting for programing done?

This isn't new - it is the way the security manager was implemented.

Updates can take up to 40 minutes for the N3000. If a person tries
to unload the driver modules, should we hang for 40 minutes? Or should
we try to cancel the update and allow the module to be unloaded?
I think it is reasonable to cancel the update (if possible) under these
circumstances. What do you think?

- Russ

>
> Thanks,
> Yilun
>
>>  	mutex_unlock(&imgld->lock);
>>  	wait_for_completion(&imgld->update_done);
>>  
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 68f3105b51d2..4e51b9fd1724 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -52,6 +52,7 @@ struct fpga_image_load {
>>  	enum fpga_image_prog progress;
>>  	enum fpga_image_prog err_progress;	/* progress at time of failure */
>>  	enum fpga_image_err err_code;		/* image load error code */
>> +	bool request_cancel;
>>  	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 6a995bcc0fb7..8d0dfa1f9b77 100644
>> --- a/include/uapi/linux/fpga-image-load.h
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -39,6 +39,7 @@ enum fpga_image_err {
>>  
>>  #define FPGA_IMAGE_LOAD_WRITE	_IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>>  #define FPGA_IMAGE_LOAD_STATUS	_IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status)
>> +#define FPGA_IMAGE_LOAD_CANCEL	_IO(FPGA_IMAGE_LOAD_MAGIC, 2)
>>  
>>  /**
>>   * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
>> -- 
>> 2.25.1


  reply	other threads:[~2021-09-10 23:38 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-09  2:18 [PATCH v15 0/6] FPGA Image Load (previously Security Manager) Russ Weight
2021-09-09  2:18 ` [PATCH v15 1/6] fpga: image-load: fpga image load class driver Russ Weight
2021-09-10  6:46   ` Xu Yilun
2021-09-10 20:47     ` Russ Weight
2021-09-09  2:18 ` [PATCH v15 2/6] fpga: image-load: enable image loads Russ Weight
2021-09-10  8:22   ` Xu Yilun
2021-09-10 23:18     ` Russ Weight
2021-09-13  6:48       ` Xu Yilun
2021-09-13  9:36         ` Xu Yilun
2021-09-21 19:08           ` Russ Weight
2021-09-09  2:18 ` [PATCH v15 3/6] fpga: image-load: signal eventfd when complete Russ Weight
2021-09-09  2:18 ` [PATCH v15 4/6] fpga: image-load: add status ioctl Russ Weight
2021-09-10  8:50   ` Xu Yilun
2021-09-10 23:23     ` Russ Weight
2021-09-11 18:12       ` Tom Rix
2021-09-13  8:24         ` Xu Yilun
2021-09-09  2:18 ` [PATCH v15 5/6] fpga: image-load: create status sysfs node Russ Weight
2021-09-10  8:52   ` Xu Yilun
2021-09-10 23:30     ` Russ Weight
2021-09-11 17:58       ` Tom Rix
2021-09-13  8:27         ` Xu Yilun
2021-09-09  2:18 ` [PATCH v15 6/6] fpga: image-load: enable cancel of image upload Russ Weight
2021-09-10 14:55   ` Xu Yilun
2021-09-10 23:38     ` Russ Weight [this message]
2021-09-11 13:13       ` Tom Rix
2021-09-13 10:00         ` Xu Yilun
2021-09-21 20:43           ` Russ Weight
     [not found] ` <20210912023739.4078-1-hdanton@sina.com>
2021-09-21 18:36   ` [PATCH v15 2/6] fpga: image-load: enable image loads Russ Weight

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=839aa95e-c3d1-1f60-1b36-20d29445e61f@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 \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).