| Message ID | 20210909021846.681121-7-russell.h.weight@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | FPGA Image Load (previously Security Manager) | expand |
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? 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
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
On 9/10/21 4:38 PM, Russ Weight wrote: > > 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? I do not think stalling a shutdown for 40 minutes is good. Is there a chance that cancelling would brick the card? After restarting, can the hw detect an interrupted update ? Tom > > - 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
On Sat, Sep 11, 2021 at 06:13:08AM -0700, Tom Rix wrote: > > On 9/10/21 4:38 PM, Russ Weight wrote: > > > > 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? > > I do not think stalling a shutdown for 40 minutes is good. > > Is there a chance that cancelling would brick the card? If the cancel cannot be possible, may be we could wait. Or the user image would be corrupted with a simple poweroff command. Thanks, Yilun > > After restarting, can the hw detect an interrupted update ? > > Tom > > > > > - 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
On 9/13/21 3:00 AM, Xu Yilun wrote: > On Sat, Sep 11, 2021 at 06:13:08AM -0700, Tom Rix wrote: >> On 9/10/21 4:38 PM, Russ Weight wrote: >>> 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? >> I do not think stalling a shutdown for 40 minutes is good. >> >> Is there a chance that cancelling would brick the card? > If the cancel cannot be possible, may be we could wait. Or the user > image would be corrupted with a simple poweroff command. OK - I'm leaving the above code in for cancelling. The check for FPGA_IMAGE_PROG_PROGRAMMING ensures that it can be safely canceled. Once programming begins, then we will block until it is complete. I'm still using the driver-unload flag to prevent new uploads from starting, but driver_unload will no longer be used to force an exit from the lower-level driver. The maximum wait for programming is less than 40 minutes - probably more like 20 minutes? - Russ > > Thanks, > Yilun > >> After restarting, can the hw detect an interrupted update ? >> >> Tom >> >>> - 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
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; + 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,
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(-)