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 5/5] fpga: image-load: enable cancel of image upload
Date: Sun, 26 Sep 2021 09:16:55 +0800 [thread overview]
Message-ID: <20210926011655.GC1339205@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <d03bc8f3-5afc-207c-62db-68bb4d533e36@intel.com>
On Fri, Sep 24, 2021 at 05:29:28PM -0700, Russ Weight wrote:
>
>
> On 9/24/21 7:53 AM, Xu Yilun wrote:
> > On Wed, Sep 22, 2021 at 05:10:56PM -0700, Russ Weight wrote:
> >> Extend the FPGA Image Load framework 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>
> >> ---
> >> v16:
> >> - This was previously patch 6/6
> >> - Amend fpga_image_load_release() to request cancellation of an ongoing
> >> update when possible.
> >> 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 | 49 +++++++++++++++++++++++---
> >> include/linux/fpga/fpga-image-load.h | 1 +
> >> include/uapi/linux/fpga-image-load.h | 2 ++
> >> 4 files changed, 53 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> >> index 572e18afebb9..21fa85f18680 100644
> >> --- a/Documentation/fpga/fpga-image-load.rst
> >> +++ b/Documentation/fpga/fpga-image-load.rst
> >> @@ -40,3 +40,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
> > an
> I'll fix it
> >> +EBUSY if it cannot be cancelled by software or ENODEV if there is no update
> > -EBUSY -ENODEV
>
> >From the userspace perspective, these are seen as positive errno numbers.
> I think this is OK as is? If not, I need to change it in other places as well.
I agree, just keep it.
> >
> >> +in progress.
> >> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> >> index 2e9a5a041535..a95d18077d58 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, u32 err_code)
> >> imgld->ops->cancel(imgld);
> >> }
> >>
> >> +static int fpga_image_prog_transition(struct fpga_image_load *imgld,
> >> + u32 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->ops->cancel(imgld);
> > We could only cancel in 2 conditions.
> > This is the first one: on progress transition.
> >
> >> + 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);
> >> @@ -79,8 +97,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) {
> > This is the second condition: when we finished a block write. But if the
> > low level driver accepts the whole block size, we cannot cancel in
> > between.
> >
> > Actually the framework doesn't know when to successfully cancel an
> > update. It depends on the hardware.
> >
> > So maybe the framework just calls cancel() immediately in IOCTL,
> > let the low level driver decides if it is feasible and how to cancel.
>
> Yes - I agree. This current implementation assumes too much about
> the low-level driver. I have some ideas to try in the next patchset.
>
> Thanks,
> - Russ
> >
> > Thanks,
> > Yilun
> >
> >> /*
> >> * The write_blk() op has the option to use the blk_size
> >> * value provided here, or to modify it to something more
> >> @@ -105,7 +125,9 @@ static void fpga_image_do_load(struct work_struct *work)
> >> cond_resched();
> >> }
> >>
> >> - fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
> >> + if (fpga_image_prog_transition(imgld, FPGA_IMAGE_PROG_PROGRAMMING))
> >> + goto done;
> >> +
> >> ret = imgld->ops->poll_complete(imgld);
> >> if (ret != FPGA_IMAGE_ERR_NONE)
> >> fpga_image_dev_error(imgld, ret);
> >> @@ -178,8 +200,8 @@ 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;
> >> queue_work(system_unbound_wq, &imgld->work);
> >> -
> >> return 0;
> >>
> >> exit_free:
> >> @@ -208,7 +230,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);
> >>
> >> @@ -219,6 +241,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);
> >> @@ -249,6 +282,9 @@ static int fpga_image_load_release(struct inode *inode, struct file *filp)
> >> goto close_exit;
> >> }
> >>
> >> + if (imgld->progress != FPGA_IMAGE_PROG_PROGRAMMING)
> >> + imgld->request_cancel = true;
> >> +
> >> mutex_unlock(&imgld->lock);
> >> flush_work(&imgld->work);
> >>
> >> @@ -363,6 +399,9 @@ void fpga_image_load_unregister(struct fpga_image_load *imgld)
> >> goto unregister;
> >> }
> >>
> >> + if (imgld->progress != FPGA_IMAGE_PROG_PROGRAMMING)
> >> + imgld->request_cancel = true;
> >> +
> >> mutex_unlock(&imgld->lock);
> >> flush_work(&imgld->work);
> >>
> >> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> >> index 8b58365893fc..8ba39d3299d9 100644
> >> --- a/include/linux/fpga/fpga-image-load.h
> >> +++ b/include/linux/fpga/fpga-image-load.h
> >> @@ -53,6 +53,7 @@ struct fpga_image_load {
> >> u32 progress;
> >> u32 err_progress; /* progress at time of error */
> >> u32 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 dc0c9f1d78b1..da8a7452c29a 100644
> >> --- a/include/uapi/linux/fpga-image-load.h
> >> +++ b/include/uapi/linux/fpga-image-load.h
> >> @@ -70,4 +70,6 @@ struct fpga_image_status {
> >>
> >> #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)
> >> +
> >> #endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
> >> --
> >> 2.25.1
prev parent reply other threads:[~2021-09-26 1:23 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
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 [this message]
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=20210926011655.GC1339205@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 \
/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).