Re: [PATCH v15 6/6] fpga: image-load: enable cancel of image upload
From: Xu Yilun
Date: Fri Sep 10 2021 - 11:02:33 EST
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@xxxxxxxxx>
> ---
> 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?
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