Messages in this thread |  | | Subject | Re: [PATCH v17 4/5] fpga: image-load: add status ioctl | From | Lizhi Hou <> | Date | Mon, 25 Oct 2021 17:07:02 -0700 |
| |
On 10/20/21 2:42 PM, Russ Weight wrote: > On 10/15/21 1:22 PM, Lizhi Hou wrote: >> On 9/29/21 4:00 PM, Russ Weight wrote: >>> Extend the FPGA Image Load framework to include an FPGA_IMAGE_LOAD_STATUS >>> IOCTL that can be used to monitor the progress of an ongoing image upload. >>> The status returned includes how much data remains to be transferred, the >>> progress of the image upload, and error information in the case of a >>> failure. >>> >>> Signed-off-by: Russ Weight <russell.h.weight@intel.com> >>> --- >>> v17: >>> - Rebased for changes to earlier patches. >>> v16: >>> - Minor changes to adapt in changes in prevoius patches. >>> v15: >>> - This patch is new to the patchset and provides an FPGA_IMAGE_LOAD_STATUS >>> IOCTL to return the current values for: remaining_size, progress, >>> err_progress, and err_code. >>> - This patch has elements of the following three patches from the previous >>> patch-set: >>> [PATCH v14 3/6] fpga: sec-mgr: expose sec-mgr update status >>> [PATCH v14 4/6] fpga: sec-mgr: expose sec-mgr update errors >>> [PATCH v14 5/6] fpga: sec-mgr: expose sec-mgr update size >>> - Changed file, symbol, and config names to reflect the new driver name >>> - There are some minor changes to locking to enable this ioctl to return >>> coherent data. >>> --- >>> Documentation/fpga/fpga-image-load.rst | 6 +++ >>> drivers/fpga/fpga-image-load.c | 58 +++++++++++++++++++++----- >>> include/linux/fpga/fpga-image-load.h | 1 + >>> include/uapi/linux/fpga-image-load.h | 18 ++++++++ >>> 4 files changed, 73 insertions(+), 10 deletions(-) >>> >>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst >>> index 487b5466f67c..f64f5ee473b8 100644 >>> --- a/Documentation/fpga/fpga-image-load.rst >>> +++ b/Documentation/fpga/fpga-image-load.rst >>> @@ -33,3 +33,9 @@ being updated. This is an exclusive operation; an attempt to start >>> concurrent image uploads for the same device will fail with EBUSY. An >>> eventfd file descriptor parameter is provided to this IOCTL. It will be >>> signalled at the completion of the image upload. >>> + >>> +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 upload, >>> +and error information in the case of a failure. >>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c >>> index f04dfc71c190..58373b9e8c02 100644 >>> --- a/drivers/fpga/fpga-image-load.c >>> +++ b/drivers/fpga/fpga-image-load.c >>> @@ -22,6 +22,22 @@ static dev_t fpga_image_devt; >>> >>> #define to_image_load(d) container_of(d, struct fpga_image_load, dev) >>> >>> +static void fpga_image_update_progress(struct fpga_image_load *imgld, >>> + u32 new_progress) >>> +{ >>> + mutex_lock(&imgld->lock); >>> + imgld->progress = new_progress; >>> + mutex_unlock(&imgld->lock); >>> +} >>> + >>> +static void fpga_image_set_error(struct fpga_image_load *imgld, u32 err_code) >>> +{ >>> + mutex_lock(&imgld->lock); >>> + imgld->err_progress = imgld->progress; >>> + imgld->err_code = err_code; >>> + mutex_unlock(&imgld->lock); >>> +} >>> + >>> static void fpga_image_prog_complete(struct fpga_image_load *imgld) >>> { >>> mutex_lock(&imgld->lock); >>> @@ -38,24 +54,24 @@ static void fpga_image_do_load(struct work_struct *work) >>> imgld = container_of(work, struct fpga_image_load, work); >>> >>> if (imgld->driver_unload) { >>> - imgld->err_code = FPGA_IMAGE_ERR_CANCELED; >>> + fpga_image_set_error(imgld, FPGA_IMAGE_ERR_CANCELED); >>> goto idle_exit; >>> } >>> >>> get_device(&imgld->dev); >>> if (!try_module_get(imgld->dev.parent->driver->owner)) { >>> - imgld->err_code = FPGA_IMAGE_ERR_BUSY; >>> + fpga_image_set_error(imgld, FPGA_IMAGE_ERR_BUSY); >>> goto putdev_exit; >>> } >>> >>> - imgld->progress = FPGA_IMAGE_PROG_PREPARING; >>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PREPARING); >>> ret = imgld->ops->prepare(imgld, imgld->data, imgld->remaining_size); >>> if (ret) { >>> - imgld->err_code = ret; >>> + fpga_image_set_error(imgld, ret); >>> goto modput_exit; >>> } >>> >>> - imgld->progress = FPGA_IMAGE_PROG_WRITING; >>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING); >>> while (imgld->remaining_size) { >>> ret = imgld->ops->write(imgld, imgld->data, offset, >>> imgld->remaining_size); >>> @@ -65,7 +81,7 @@ static void fpga_image_do_load(struct work_struct *work) >>> "write-op wrote zero data\n"); >>> ret = -FPGA_IMAGE_ERR_RW_ERROR; >>> } >>> - imgld->err_code = -ret; >>> + fpga_image_set_error(imgld, -ret); >>> goto done; >>> } >>> >>> @@ -73,10 +89,10 @@ static void fpga_image_do_load(struct work_struct *work) >>> offset += ret; >>> } >>> >>> - imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING; >>> + fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING); >>> ret = imgld->ops->poll_complete(imgld); >>> if (ret) >>> - imgld->err_code = ret; >>> + fpga_image_set_error(imgld, ret); >>> >>> done: >>> if (imgld->ops->cleanup) >>> @@ -151,20 +167,42 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld, >>> return ret; >>> } >>> >>> +static int fpga_image_load_ioctl_status(struct fpga_image_load *imgld, >>> + unsigned long arg) >>> +{ >>> + struct fpga_image_status status; >>> + >>> + memset(&status, 0, sizeof(status)); >>> + status.progress = imgld->progress; >>> + status.remaining_size = imgld->remaining_size; >>> + status.err_progress = imgld->err_progress; >>> + status.err_code = imgld->err_code; >>> + >>> + if (copy_to_user((void __user *)arg, &status, sizeof(status))) >>> + return -EFAULT; >>> + >>> + return 0; >>> +} >>> + >>> 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; >>> >>> + mutex_lock(&imgld->lock); >>> + >>> switch (cmd) { >>> case FPGA_IMAGE_LOAD_WRITE: >>> - mutex_lock(&imgld->lock); >>> ret = fpga_image_load_ioctl_write(imgld, arg); >>> - mutex_unlock(&imgld->lock); >>> + break; >>> + case FPGA_IMAGE_LOAD_STATUS: >>> + ret = fpga_image_load_ioctl_status(imgld, arg); >>> break; >>> } >>> >>> + mutex_unlock(&imgld->lock); >>> + >>> return ret; >>> } >>> >>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h >>> index 77b3c91ce073..366111d090fb 100644 >>> --- a/include/linux/fpga/fpga-image-load.h >>> +++ b/include/linux/fpga/fpga-image-load.h >>> @@ -49,6 +49,7 @@ struct fpga_image_load { >>> const u8 *data; /* pointer to update data */ >>> u32 remaining_size; /* size remaining to transfer */ >>> u32 progress; >>> + u32 err_progress; /* progress at time of error */ >>> u32 err_code; /* image load error code */ >>> bool driver_unload; >>> struct eventfd_ctx *finished; >>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h >>> index 8d2d3db92e87..1b91343961df 100644 >>> --- a/include/uapi/linux/fpga-image-load.h >>> +++ b/include/uapi/linux/fpga-image-load.h >>> @@ -51,4 +51,22 @@ struct fpga_image_write { >>> >>> #define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write) >>> >>> +/** >>> + * FPGA_IMAGE_LOAD_STATUS - _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, >>> + * struct fpga_image_status) >>> + * >>> + * Request status information for an ongoing update. >>> + * >>> + * Return: 0 on success, -errno on failure. >>> + */ >>> +struct fpga_image_status { >>> + /* Output */ >>> + __u32 remaining_size; /* size remaining to transfer */ >>> + __u32 progress; /* current progress of image load */ >>> + __u32 err_progress; /* progress at time of error */ >>> + __u32 err_code; /* error code */ >>> +}; >> Could this be extended to also collect the image detail? >> >> Image version, name, etc been successfully written to device (flash). >> >> Image version, name, etc is currently running on the device. >> >> Or maybe add another query command to do these? >> >> So the userland utility will be able to show what image is running and what image is going to run with next cold reboot. > This status call is intended specifically to show the progress and > status of the image load. Yilun's comments on patch 5 suggest that > some of these types of operations could be handled as part of the > fpga-region class driver > > I have patches that have not been submitted yet that contain some > of these types of features, although not in the class driver (yet). > Here is a link to documentation for what I have now. Take a look > at the sysfs node descriptions that are in the control subdirectory. > These are listed at the bottom of the file. > > https://github.com/OPAE/linux-dfl/blob/fpga-ofs-dev/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-sec-update
Thanks a lot. This is very helpful.
Lizhi
> > - Russ > > >> >> Thanks, >> >> Lizhi >> >>> + >>> +#define FPGA_IMAGE_LOAD_STATUS _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status) >>> + >>> #endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */ >>> -- >>> 2.25.1 >>>
|  |