* [PATCH v16 1/5] fpga: image-load: fpga image load framework
2021-09-23 0:10 [PATCH v16 0/5] FPGA Image Load (previously Security Manager) Russ Weight
@ 2021-09-23 0:10 ` Russ Weight
2021-09-23 0:10 ` [PATCH v16 2/5] fpga: image-load: enable image uploads Russ Weight
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Russ Weight @ 2021-09-23 0:10 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, Russ Weight
The FPGA Image Load framework provides an API to transfer update
files to an FPGA device. Image files are self-describing. They could
contain FPGA images, BMC images, Root Entry Hashes, or other device
specific files. It is up to the lower level device driver and the
target device to authenticate and disposition the file data.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v16:
- Shift from "Driver" terminology to "Framework" in comments and
documentation
- Rename lops to ops for structure member and local variables
- Change device name from "fpga_image%d" to "fpga_image_load%d"
v15:
- Compare to previous patch:
[PATCH v14 1/6] fpga: sec-mgr: fpga security manager class driver
- Changed file, symbol, and config names to reflect the new driver name
- Rewrote documentation. The documentation will be added to in later patches.
- Removed signed-off/reviewed-by tags
v14:
- Updated copyright to 2021
- Removed the name sysfs entry
- Removed MAINTAINERS reference to
Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
- Use xa_alloc() instead of ida_simple_get()
- Rename dev to parent for parent devices
- Remove fpga_sec_mgr_create(), devm_fpga_sec_mgr_create(), and
fpga_sec_mgr_free() functions and update the fpga_sec_mgr_register()
function to both create and register a new security manager.
- Populate the fpga_sec_mgr_dev_release() function.
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:
- Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst
v7:
- Changed Date in documentation file to December 2020
v6:
- Removed sysfs support and documentation for the display of the
flash count, root entry hashes, and code-signing-key cancelation
vectors.
v5:
- Added the devm_fpga_sec_mgr_unregister() function, following recent
changes to the fpga_manager() implementation.
- Changed some *_show() functions to use sysfs_emit() instead of sprintf(
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:
- Modified sysfs handler check in check_sysfs_handler() to make
it more readable.
v2:
- Bumped documentation dates and versions
- Added Documentation/fpga/ifpga-sec-mgr.rst
- Removed references to bmc_flash_count & smbus_flash_count (not supported)
- Split ifpga_sec_mgr_register() into create() and register() functions
- Added devm_ifpga_sec_mgr_create()
- Removed typedefs for imgr ops
---
Documentation/fpga/fpga-image-load.rst | 10 ++
Documentation/fpga/index.rst | 1 +
MAINTAINERS | 8 ++
drivers/fpga/Kconfig | 10 ++
drivers/fpga/Makefile | 3 +
drivers/fpga/fpga-image-load.c | 125 +++++++++++++++++++++++++
include/linux/fpga/fpga-image-load.h | 35 +++++++
7 files changed, 192 insertions(+)
create mode 100644 Documentation/fpga/fpga-image-load.rst
create mode 100644 drivers/fpga/fpga-image-load.c
create mode 100644 include/linux/fpga/fpga-image-load.h
diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
new file mode 100644
index 000000000000..dda9283ef1c7
--- /dev/null
+++ b/Documentation/fpga/fpga-image-load.rst
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+FPGA Image Load Framework
+=========================
+
+The FPGA Image Load framework provides a common API for user-space
+tools to manage image uploads to FPGA devices. Device drivers that
+instantiate the FPGA Image Load framework will interact with the
+target device to transfer and authenticate the image data.
diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
index f80f95667ca2..85d25fb22c08 100644
--- a/Documentation/fpga/index.rst
+++ b/Documentation/fpga/index.rst
@@ -8,6 +8,7 @@ fpga
:maxdepth: 1
dfl
+ fpga-image-load
.. only:: subproject and html
diff --git a/MAINTAINERS b/MAINTAINERS
index 04fa4edf100b..6e312d0ddeb6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7365,6 +7365,14 @@ F: Documentation/fpga/
F: drivers/fpga/
F: include/linux/fpga/
+FPGA IMAGE UPLOAD DRIVERS
+M: Russ Weight <russell.h.weight@intel.com>
+L: linux-fpga@vger.kernel.org
+S: Maintained
+F: Documentation/fpga/fpga-image-load.rst
+F: drivers/fpga/fpga-image-load.c
+F: include/linux/fpga/fpga-image-load.h
+
FPU EMULATOR
M: Bill Metzenthen <billm@melbpc.org.au>
S: Maintained
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 991b3f361ec9..fbd580121661 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -243,4 +243,14 @@ config FPGA_MGR_VERSAL_FPGA
configure the programmable logic(PL).
To compile this as a module, choose M here.
+
+config FPGA_IMAGE_LOAD
+ tristate "FPGA Image Load Framework"
+ help
+ The FPGA Image Load framework presents a common user API for
+ uploading an image file to an FPGA device. The image file is
+ expected to be self-describing. It is up to the lower level
+ device driver and/or the device itself to authenticate and
+ disposition the image data.
+
endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 0bff783d1b61..adf228ee4f5e 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -22,6 +22,9 @@ obj-$(CONFIG_FPGA_MGR_VERSAL_FPGA) += versal-fpga.o
obj-$(CONFIG_ALTERA_PR_IP_CORE) += altera-pr-ip-core.o
obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT) += altera-pr-ip-core-plat.o
+# FPGA Image Load Framework
+obj-$(CONFIG_FPGA_IMAGE_LOAD) += fpga-image-load.o
+
# FPGA Bridge Drivers
obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o
obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o
diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
new file mode 100644
index 000000000000..799d18444f7c
--- /dev/null
+++ b/drivers/fpga/fpga-image-load.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FPGA Image Load Framework
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ */
+
+#include <linux/fpga/fpga-image-load.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+#define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
+static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
+
+static struct class *fpga_image_load_class;
+
+#define to_image_load(d) container_of(d, struct fpga_image_load, dev)
+
+/**
+ * fpga_image_load_register - create and register an FPGA Image Load Device
+ *
+ * @parent: fpga image load device from pdev
+ * @ops: pointer to a structure of image load callback functions
+ * @priv: fpga image load private data
+ *
+ * Returns a struct fpga_image_load pointer on success, or ERR_PTR() on
+ * error. The caller of this function is responsible for calling
+ * fpga_image_load_unregister().
+ */
+struct fpga_image_load *
+fpga_image_load_register(struct device *parent,
+ const struct fpga_image_load_ops *ops, void *priv)
+{
+ struct fpga_image_load *imgld;
+ int ret;
+
+ imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
+ if (!imgld)
+ return ERR_PTR(-ENOMEM);
+
+ ret = xa_alloc(&fpga_image_load_xa, &imgld->dev.id, imgld, IMAGE_LOAD_XA_LIMIT,
+ GFP_KERNEL);
+ if (ret)
+ goto error_kfree;
+
+ mutex_init(&imgld->lock);
+
+ imgld->priv = priv;
+ imgld->ops = ops;
+
+ imgld->dev.class = fpga_image_load_class;
+ imgld->dev.parent = parent;
+
+ ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
+ if (ret) {
+ dev_err(parent, "Failed to set device name: fpga_image_load%d\n",
+ imgld->dev.id);
+ goto error_device;
+ }
+
+ ret = device_register(&imgld->dev);
+ if (ret) {
+ put_device(&imgld->dev);
+ return ERR_PTR(ret);
+ }
+
+ return imgld;
+
+error_device:
+ xa_erase(&fpga_image_load_xa, imgld->dev.id);
+
+error_kfree:
+ kfree(imgld);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(fpga_image_load_register);
+
+/**
+ * fpga_image_load_unregister - unregister an FPGA image load device
+ *
+ * @imgld: pointer to struct fpga_image_load
+ *
+ * This function is intended for use in the parent driver's remove()
+ * function.
+ */
+void fpga_image_load_unregister(struct fpga_image_load *imgld)
+{
+ device_unregister(&imgld->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
+
+static void fpga_image_load_dev_release(struct device *dev)
+{
+ struct fpga_image_load *imgld = to_image_load(dev);
+
+ xa_erase(&fpga_image_load_xa, imgld->dev.id);
+ kfree(imgld);
+}
+
+static int __init fpga_image_load_class_init(void)
+{
+ pr_info("FPGA Image Load Framework\n");
+
+ fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
+ if (IS_ERR(fpga_image_load_class))
+ return PTR_ERR(fpga_image_load_class);
+
+ fpga_image_load_class->dev_release = fpga_image_load_dev_release;
+
+ return 0;
+}
+
+static void __exit fpga_image_load_class_exit(void)
+{
+ class_destroy(fpga_image_load_class);
+ WARN_ON(!xa_empty(&fpga_image_load_xa));
+}
+
+MODULE_DESCRIPTION("FPGA Image Load Framework");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_image_load_class_init);
+module_exit(fpga_image_load_class_exit)
diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
new file mode 100644
index 000000000000..8b051c82ef5f
--- /dev/null
+++ b/include/linux/fpga/fpga-image-load.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for FPGA Image Load Framework
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ */
+#ifndef _LINUX_FPGA_IMAGE_LOAD_H
+#define _LINUX_FPGA_IMAGE_LOAD_H
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+struct fpga_image_load;
+
+/**
+ * struct fpga_image_load_ops - device specific operations
+ */
+struct fpga_image_load_ops {
+};
+
+struct fpga_image_load {
+ struct device dev;
+ const struct fpga_image_load_ops *ops;
+ struct mutex lock; /* protect data structure contents */
+ void *priv;
+};
+
+struct fpga_image_load *
+fpga_image_load_register(struct device *dev,
+ const struct fpga_image_load_ops *ops, void *priv);
+
+void fpga_image_load_unregister(struct fpga_image_load *imgld);
+
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v16 2/5] fpga: image-load: enable image uploads
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 ` Russ Weight
2021-09-23 8:54 ` Xu Yilun
2021-09-24 8:12 ` Xu Yilun
2021-09-23 0:10 ` [PATCH v16 3/5] fpga: image-load: signal eventfd when complete Russ Weight
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Russ Weight @ 2021-09-23 0:10 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, Russ Weight
Extend the FPGA Image Load framework to include IOCTL support
(FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
The IOCTL will return immediately, and the update will begin in the
context of a kernel worker thread.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v16:
- Shift from "Driver" terminology to "Framework" in comments and
documentation
- Rename lops to ops for structure member and local variables
- Change the write_blk() definition to pass in *blk_size (a pointer to
a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
the maximum block size to stay within the limit of the data buffer).
The write_blk() op may use the default *blk_size or modify it to a
more optimal number for the given device, subject to the max_size limit.
- All enum values for progress and errors are changed to macros, because
they are included in the uapi header. This is done to maintain consistency
amongst the DFL related IOCTL header files. All references to the enum
types are changed to u32.
- Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
- Add a call to cond_resched() in the write_blk() loop to ensure that
we yield to higher priority tasks during long data transfers.
- Switch to the system_unbound_wq to enable calls to cond_resched().
- Switch from test_and_set_bit() to atomic_cmpxchg() to manage
imgld->opened.
- Change fpga_image_load_release() to block until the image upload is
complete.
- Remove the completion object, imgld->update_done, in favor of calling
flush_work(&imgld->work);
v15:
- Compare to previous patch:
[PATCH v14 2/6] fpga: sec-mgr: enable secure updates
- Changed file, symbol, and config names to reflect the new driver name
- Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
IOCTL for writing the image data to the FPGA card. The driver no longer
uses the request_firmware framework.
- Fixed some error return values in fpga_image_load_register()
- Removed signed-off/reviewed-by tags
v14:
- Added MAINTAINERS reference for
Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
- Updated ABI documentation date and kernel version
- Updated copyright to 2021
v13:
- Change "if (count == 0 || " to "if (!count || "
- Improve error message: "Attempt to register without all required ops\n"
v12:
- Updated Date and KernelVersion fields in ABI documentation
- Removed size parameter from write_blk() op - it is now up to
the lower-level driver to determine the appropriate size and
to update smgr->remaining_size accordingly.
v11:
- Fixed a spelling error in a comment
- Initialize smgr->err_code and smgr->progress explicitly in
fpga_sec_mgr_create() instead of accepting the default 0 value.
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
- Changed filename_store() to use kmemdup_nul() instead of
kstrndup() and changed the count to not assume a line-return.
v6:
- Changed "security update" to "secure update" in commit message
v5:
- When checking the return values for functions of type enum
fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
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:
- Removed unnecessary "goto done"
- Added a comment to explain imgr->driver_unload in
ifpga_sec_mgr_unregister()
v2:
- Bumped documentation date and version
- Removed explicit value assignments in enums
- Other minor code cleanup per review comments
---
Documentation/fpga/fpga-image-load.rst | 26 ++-
MAINTAINERS | 1 +
drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
include/linux/fpga/fpga-image-load.h | 29 +++
include/uapi/linux/fpga-image-load.h | 54 ++++++
5 files changed, 346 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/fpga-image-load.h
diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
index dda9283ef1c7..ba371c7c0ca0 100644
--- a/Documentation/fpga/fpga-image-load.rst
+++ b/Documentation/fpga/fpga-image-load.rst
@@ -7,4 +7,28 @@ FPGA Image Load Framework
The FPGA Image Load framework provides a common API for user-space
tools to manage image uploads to FPGA devices. Device drivers that
instantiate the FPGA Image Load framework will interact with the
-target device to transfer and authenticate the image data.
+target device to transfer and authenticate the image data. Image uploads
+are processed in the context of a kernel worker thread.
+
+User API
+========
+
+open
+----
+
+An fpga_image_load device is opened exclusively to control an image upload.
+The device must remain open throughout the duration of the image upload.
+An attempt to close the device while an upload is in progress will cause
+the upload to be cancelled. If unable to cancel the image upload, the close
+system call will block until the image upload is complete.
+
+ioctl
+-----
+
+FPGA_IMAGE_LOAD_WRITE:
+
+Start an image upload with the provided image buffer. This IOCTL returns
+immediately after starting a kernel worker thread to process the image
+upload which could take as long a 40 minutes depending on the actual device
+being updated. This is an exclusive operation; an attempt to start
+concurrent image uploads for the same device will fail with EBUSY.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e312d0ddeb6..4e44f62eef33 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7372,6 +7372,7 @@ S: Maintained
F: Documentation/fpga/fpga-image-load.rst
F: drivers/fpga/fpga-image-load.c
F: include/linux/fpga/fpga-image-load.h
+F: include/uapi/linux/fpga-image-load.h
FPU EMULATOR
M: Bill Metzenthen <billm@melbpc.org.au>
diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
index 799d18444f7c..65f553b59011 100644
--- a/drivers/fpga/fpga-image-load.c
+++ b/drivers/fpga/fpga-image-load.c
@@ -5,18 +5,210 @@
* Copyright (C) 2019-2021 Intel Corporation, Inc.
*/
+#include <linux/delay.h>
#include <linux/fpga/fpga-image-load.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/uaccess.h>
#include <linux/vmalloc.h>
#define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
static struct class *fpga_image_load_class;
+static dev_t fpga_image_devt;
#define to_image_load(d) container_of(d, struct fpga_image_load, dev)
+#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
+
+static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
+{
+ imgld->err_code = err_code;
+ imgld->ops->cancel(imgld);
+}
+
+static void fpga_image_prog_complete(struct fpga_image_load *imgld)
+{
+ mutex_lock(&imgld->lock);
+ imgld->progress = FPGA_IMAGE_PROG_IDLE;
+ mutex_unlock(&imgld->lock);
+}
+
+static void fpga_image_do_load(struct work_struct *work)
+{
+ struct fpga_image_load *imgld;
+ u32 ret, blk_size, offset = 0;
+
+ imgld = container_of(work, struct fpga_image_load, work);
+
+ if (imgld->driver_unload) {
+ imgld->err_code = 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;
+ goto putdev_exit;
+ }
+
+ imgld->progress = FPGA_IMAGE_PROG_PREPARING;
+ ret = imgld->ops->prepare(imgld);
+ if (ret != FPGA_IMAGE_ERR_NONE) {
+ fpga_image_dev_error(imgld, ret);
+ goto modput_exit;
+ }
+
+ imgld->progress = FPGA_IMAGE_PROG_WRITING;
+ while (imgld->remaining_size) {
+ /*
+ * The write_blk() op has the option to use the blk_size
+ * value provided here, or to modify it to something more
+ * optimal for the given device.
+ */
+ blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
+ ret = imgld->ops->write_blk(imgld, offset, &blk_size,
+ imgld->remaining_size);
+ if (ret != FPGA_IMAGE_ERR_NONE) {
+ fpga_image_dev_error(imgld, ret);
+ goto done;
+ }
+
+ imgld->remaining_size -= blk_size;
+ offset += blk_size;
+
+ /*
+ * The class driver does not have control of the overall
+ * size or the actual implementation of the write. Allow
+ * for scheduling out.
+ */
+ cond_resched();
+ }
+
+ imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
+ ret = imgld->ops->poll_complete(imgld);
+ if (ret != FPGA_IMAGE_ERR_NONE)
+ fpga_image_dev_error(imgld, ret);
+
+done:
+ if (imgld->ops->cleanup)
+ imgld->ops->cleanup(imgld);
+
+modput_exit:
+ module_put(imgld->dev.parent->driver->owner);
+
+putdev_exit:
+ put_device(&imgld->dev);
+
+idle_exit:
+ /*
+ * Note: imgld->remaining_size is left unmodified here to provide
+ * additional information on errors. It will be reinitialized when
+ * the next image load begins.
+ */
+ vfree(imgld->data);
+ imgld->data = NULL;
+ fpga_image_prog_complete(imgld);
+}
+
+static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
+ unsigned long arg)
+{
+ struct fpga_image_write wb;
+ unsigned long minsz;
+ u8 *buf;
+
+ if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
+ return -EBUSY;
+
+ minsz = offsetofend(struct fpga_image_write, buf);
+ if (copy_from_user(&wb, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (wb.flags)
+ return -EINVAL;
+
+ /* Enforce 32-bit alignment on the write data */
+ if (wb.size & 0x3)
+ return -EINVAL;
+
+ buf = vzalloc(wb.size);
+ if (!buf)
+ return -ENOMEM;
+
+ if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
+ vfree(buf);
+ return -EFAULT;
+ }
+
+ imgld->data = buf;
+ imgld->remaining_size = wb.size;
+ imgld->err_code = FPGA_IMAGE_ERR_NONE;
+ imgld->progress = FPGA_IMAGE_PROG_STARTING;
+ queue_work(system_unbound_wq, &imgld->work);
+
+ 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;
+
+ switch (cmd) {
+ case FPGA_IMAGE_LOAD_WRITE:
+ mutex_lock(&imgld->lock);
+ ret = fpga_image_load_ioctl_write(imgld, arg);
+ mutex_unlock(&imgld->lock);
+ break;
+ }
+
+ return ret;
+}
+
+static int fpga_image_load_open(struct inode *inode, struct file *filp)
+{
+ struct fpga_image_load *imgld = container_of(inode->i_cdev,
+ struct fpga_image_load, cdev);
+
+ if (atomic_cmpxchg(&imgld->opened, 0, 1))
+ return -EBUSY;
+
+ filp->private_data = imgld;
+
+ return 0;
+}
+
+static int fpga_image_load_release(struct inode *inode, struct file *filp)
+{
+ struct fpga_image_load *imgld = filp->private_data;
+
+ mutex_lock(&imgld->lock);
+ if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
+ mutex_unlock(&imgld->lock);
+ goto close_exit;
+ }
+
+ mutex_unlock(&imgld->lock);
+ flush_work(&imgld->work);
+
+close_exit:
+ atomic_set(&imgld->opened, 0);
+
+ return 0;
+}
+
+static const struct file_operations fpga_image_load_fops = {
+ .owner = THIS_MODULE,
+ .open = fpga_image_load_open,
+ .release = fpga_image_load_release,
+ .unlocked_ioctl = fpga_image_load_ioctl,
+};
+
/**
* fpga_image_load_register - create and register an FPGA Image Load Device
*
@@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
struct fpga_image_load *imgld;
int ret;
+ if (!ops || !ops->cancel || !ops->prepare ||
+ !ops->write_blk || !ops->poll_complete) {
+ dev_err(parent, "Attempt to register without all required ops\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
if (!imgld)
return ERR_PTR(-ENOMEM);
@@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
imgld->priv = priv;
imgld->ops = ops;
+ imgld->err_code = FPGA_IMAGE_ERR_NONE;
+ imgld->progress = FPGA_IMAGE_PROG_IDLE;
+ INIT_WORK(&imgld->work, fpga_image_do_load);
imgld->dev.class = fpga_image_load_class;
imgld->dev.parent = parent;
+ imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
if (ret) {
@@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
return ERR_PTR(ret);
}
+ cdev_init(&imgld->cdev, &fpga_image_load_fops);
+ imgld->cdev.owner = parent->driver->owner;
+ imgld->cdev.kobj.parent = &imgld->dev.kobj;
+
+ ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
+ if (ret) {
+ put_device(&imgld->dev);
+ return ERR_PTR(ret);
+ }
+
return imgld;
error_device:
@@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
* @imgld: pointer to struct fpga_image_load
*
* This function is intended for use in the parent driver's remove()
- * function.
+ * function. The driver_unload flag prevents new updates from starting
+ * once the unregister process has begun.
*/
void fpga_image_load_unregister(struct fpga_image_load *imgld)
{
+ mutex_lock(&imgld->lock);
+ imgld->driver_unload = true;
+ if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
+ mutex_unlock(&imgld->lock);
+ goto unregister;
+ }
+
+ mutex_unlock(&imgld->lock);
+ flush_work(&imgld->work);
+
+unregister:
+ cdev_del(&imgld->cdev);
device_unregister(&imgld->dev);
}
EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
@@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
static int __init fpga_image_load_class_init(void)
{
+ int ret;
pr_info("FPGA Image Load Framework\n");
fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
if (IS_ERR(fpga_image_load_class))
return PTR_ERR(fpga_image_load_class);
+ ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
+ "fpga_image_load");
+ if (ret)
+ goto exit_destroy_class;
+
fpga_image_load_class->dev_release = fpga_image_load_dev_release;
return 0;
+
+exit_destroy_class:
+ class_destroy(fpga_image_load_class);
+ return ret;
}
static void __exit fpga_image_load_class_exit(void)
{
+ unregister_chrdev_region(fpga_image_devt, MINORMASK);
class_destroy(fpga_image_load_class);
WARN_ON(!xa_empty(&fpga_image_load_xa));
}
diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
index 8b051c82ef5f..41ab63cf7b20 100644
--- a/include/linux/fpga/fpga-image-load.h
+++ b/include/linux/fpga/fpga-image-load.h
@@ -7,22 +7,51 @@
#ifndef _LINUX_FPGA_IMAGE_LOAD_H
#define _LINUX_FPGA_IMAGE_LOAD_H
+#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/types.h>
+#include <uapi/linux/fpga-image-load.h>
struct fpga_image_load;
/**
* struct fpga_image_load_ops - device specific operations
+ * @prepare: Required: Prepare secure update
+ * @write_blk: Required: Write a block of data. The class driver
+ * provides a default block size. The write_blk() op
+ * may choose to modify *blk_size to something more
+ * optimal for the given device. *blk_size must be
+ * less than or equal to max_size.
+ * @poll_complete: Required: Check for the completion of the
+ * HW authentication/programming process.
+ * @cancel: Required: Signal HW to cancel update
+ * @cleanup: Optional: Complements the prepare()
+ * function and is called at the completion
+ * of the update, whether success or failure,
+ * if the prepare function succeeded.
*/
struct fpga_image_load_ops {
+ u32 (*prepare)(struct fpga_image_load *imgld);
+ u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
+ u32 *blk_size, u32 max_size);
+ u32 (*poll_complete)(struct fpga_image_load *imgld);
+ u32 (*cancel)(struct fpga_image_load *imgld);
+ void (*cleanup)(struct fpga_image_load *imgld);
};
struct fpga_image_load {
struct device dev;
+ struct cdev cdev;
const struct fpga_image_load_ops *ops;
struct mutex lock; /* protect data structure contents */
+ atomic_t opened;
+ struct work_struct work;
+ const u8 *data; /* pointer to update data */
+ u32 remaining_size; /* size remaining to transfer */
+ u32 progress;
+ u32 err_code; /* image load error code */
+ bool driver_unload;
void *priv;
};
diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
new file mode 100644
index 000000000000..0382078c5a6c
--- /dev/null
+++ b/include/uapi/linux/fpga-image-load.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Header File for FPGA Image Load User API
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ *
+ */
+
+#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
+#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define FPGA_IMAGE_LOAD_MAGIC 0xB9
+
+/* Image load progress codes */
+#define FPGA_IMAGE_PROG_IDLE 0
+#define FPGA_IMAGE_PROG_STARTING 1
+#define FPGA_IMAGE_PROG_PREPARING 2
+#define FPGA_IMAGE_PROG_WRITING 3
+#define FPGA_IMAGE_PROG_PROGRAMMING 4
+#define FPGA_IMAGE_PROG_MAX 5
+
+/* Image error progress codes */
+#define FPGA_IMAGE_ERR_NONE 0
+#define FPGA_IMAGE_ERR_HW_ERROR 1
+#define FPGA_IMAGE_ERR_TIMEOUT 2
+#define FPGA_IMAGE_ERR_CANCELED 3
+#define FPGA_IMAGE_ERR_BUSY 4
+#define FPGA_IMAGE_ERR_INVALID_SIZE 5
+#define FPGA_IMAGE_ERR_RW_ERROR 6
+#define FPGA_IMAGE_ERR_WEAROUT 7
+#define FPGA_IMAGE_ERR_MAX 8
+
+/**
+ * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
+ * struct fpga_image_write)
+ *
+ * Upload a data buffer to the target device. The user must provide the
+ * data buffer and size.
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+struct fpga_image_write {
+ /* Input */
+ __u32 flags; /* Zero for now */
+ __u32 size; /* Data size (in bytes) to be written */
+ __u64 buf; /* User space address of source data */
+};
+
+#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
+
+#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
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
1 sibling, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2021-09-23 8:54 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On Wed, Sep 22, 2021 at 05:10:53PM -0700, Russ Weight wrote:
> Extend the FPGA Image Load framework to include IOCTL support
> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
> The IOCTL will return immediately, and the update will begin in the
> context of a kernel worker thread.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v16:
> - Shift from "Driver" terminology to "Framework" in comments and
> documentation
> - Rename lops to ops for structure member and local variables
> - Change the write_blk() definition to pass in *blk_size (a pointer to
> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
> the maximum block size to stay within the limit of the data buffer).
> The write_blk() op may use the default *blk_size or modify it to a
> more optimal number for the given device, subject to the max_size limit.
> - All enum values for progress and errors are changed to macros, because
> they are included in the uapi header. This is done to maintain consistency
> amongst the DFL related IOCTL header files. All references to the enum
> types are changed to u32.
> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
> - Add a call to cond_resched() in the write_blk() loop to ensure that
> we yield to higher priority tasks during long data transfers.
> - Switch to the system_unbound_wq to enable calls to cond_resched().
> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
> imgld->opened.
> - Change fpga_image_load_release() to block until the image upload is
> complete.
> - Remove the completion object, imgld->update_done, in favor of calling
> flush_work(&imgld->work);
> v15:
> - Compare to previous patch:
> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
> - Changed file, symbol, and config names to reflect the new driver name
> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
> IOCTL for writing the image data to the FPGA card. The driver no longer
> uses the request_firmware framework.
> - Fixed some error return values in fpga_image_load_register()
> - Removed signed-off/reviewed-by tags
> v14:
> - Added MAINTAINERS reference for
> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> - Updated ABI documentation date and kernel version
> - Updated copyright to 2021
> v13:
> - Change "if (count == 0 || " to "if (!count || "
> - Improve error message: "Attempt to register without all required ops\n"
> v12:
> - Updated Date and KernelVersion fields in ABI documentation
> - Removed size parameter from write_blk() op - it is now up to
> the lower-level driver to determine the appropriate size and
> to update smgr->remaining_size accordingly.
> v11:
> - Fixed a spelling error in a comment
> - Initialize smgr->err_code and smgr->progress explicitly in
> fpga_sec_mgr_create() instead of accepting the default 0 value.
> 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
> - Changed filename_store() to use kmemdup_nul() instead of
> kstrndup() and changed the count to not assume a line-return.
> v6:
> - Changed "security update" to "secure update" in commit message
> v5:
> - When checking the return values for functions of type enum
> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
> 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:
> - Removed unnecessary "goto done"
> - Added a comment to explain imgr->driver_unload in
> ifpga_sec_mgr_unregister()
> v2:
> - Bumped documentation date and version
> - Removed explicit value assignments in enums
> - Other minor code cleanup per review comments
> ---
> Documentation/fpga/fpga-image-load.rst | 26 ++-
> MAINTAINERS | 1 +
> drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
> include/linux/fpga/fpga-image-load.h | 29 +++
> include/uapi/linux/fpga-image-load.h | 54 ++++++
> 5 files changed, 346 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/fpga-image-load.h
>
> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> index dda9283ef1c7..ba371c7c0ca0 100644
> --- a/Documentation/fpga/fpga-image-load.rst
> +++ b/Documentation/fpga/fpga-image-load.rst
> @@ -7,4 +7,28 @@ FPGA Image Load Framework
> The FPGA Image Load framework provides a common API for user-space
> tools to manage image uploads to FPGA devices. Device drivers that
> instantiate the FPGA Image Load framework will interact with the
> -target device to transfer and authenticate the image data.
> +target device to transfer and authenticate the image data. Image uploads
> +are processed in the context of a kernel worker thread.
> +
> +User API
> +========
> +
> +open
> +----
> +
> +An fpga_image_load device is opened exclusively to control an image upload.
> +The device must remain open throughout the duration of the image upload.
> +An attempt to close the device while an upload is in progress will cause
> +the upload to be cancelled. If unable to cancel the image upload, the close
> +system call will block until the image upload is complete.
> +
> +ioctl
> +-----
> +
> +FPGA_IMAGE_LOAD_WRITE:
> +
> +Start an image upload with the provided image buffer. This IOCTL returns
> +immediately after starting a kernel worker thread to process the image
> +upload which could take as long a 40 minutes depending on the actual device
as long as?
> +being updated. This is an exclusive operation; an attempt to start
> +concurrent image uploads for the same device will fail with EBUSY.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e312d0ddeb6..4e44f62eef33 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7372,6 +7372,7 @@ S: Maintained
> F: Documentation/fpga/fpga-image-load.rst
> F: drivers/fpga/fpga-image-load.c
> F: include/linux/fpga/fpga-image-load.h
> +F: include/uapi/linux/fpga-image-load.h
>
> FPU EMULATOR
> M: Bill Metzenthen <billm@melbpc.org.au>
> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> index 799d18444f7c..65f553b59011 100644
> --- a/drivers/fpga/fpga-image-load.c
> +++ b/drivers/fpga/fpga-image-load.c
> @@ -5,18 +5,210 @@
> * Copyright (C) 2019-2021 Intel Corporation, Inc.
> */
>
> +#include <linux/delay.h>
> #include <linux/fpga/fpga-image-load.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/uaccess.h>
> #include <linux/vmalloc.h>
>
> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>
> static struct class *fpga_image_load_class;
> +static dev_t fpga_image_devt;
>
> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>
> +#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
> +
> +static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
> +{
> + imgld->err_code = err_code;
> + imgld->ops->cancel(imgld);
> +}
> +
> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> +{
> + mutex_lock(&imgld->lock);
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + mutex_unlock(&imgld->lock);
> +}
> +
> +static void fpga_image_do_load(struct work_struct *work)
> +{
> + struct fpga_image_load *imgld;
> + u32 ret, blk_size, offset = 0;
> +
> + imgld = container_of(work, struct fpga_image_load, work);
> +
> + if (imgld->driver_unload) {
> + imgld->err_code = 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;
> + goto putdev_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
> + ret = imgld->ops->prepare(imgld);
> + if (ret != FPGA_IMAGE_ERR_NONE) {
> + fpga_image_dev_error(imgld, ret);
> + goto modput_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
> + while (imgld->remaining_size) {
> + /*
> + * The write_blk() op has the option to use the blk_size
> + * value provided here, or to modify it to something more
> + * optimal for the given device.
> + */
> + blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
> + ret = imgld->ops->write_blk(imgld, offset, &blk_size,
> + imgld->remaining_size);
I don't suggest we must have a default block size. I just prefer a more
common API definition for this case.
How about we define like this:
u32 (*write_blk)(struct fpga_image_load *imgld, const char *buf, u32 size, u32 *written_size);
The return value indicates error type.
or
u32 (*write_blk)(struct fpga_image_load *imgld, const char *buf, u32 size);
The positive return value indicates the written size. The
negative values indicates error type.
then we could call it in framework like:
ret = imgld->ops->write_blk(imgld, imgld->data + offset, imgld->remaining_size, &written_size);
> + if (ret != FPGA_IMAGE_ERR_NONE) {
> + fpga_image_dev_error(imgld, ret);
> + goto done;
> + }
> +
> + imgld->remaining_size -= blk_size;
> + offset += blk_size;
> +
> + /*
> + * The class driver does not have control of the overall
> + * size or the actual implementation of the write. Allow
> + * for scheduling out.
> + */
> + cond_resched();
I'm not quite sure about this. We only reschedule during block write but
not in other phases. Actually they may also be time consuming, such as
programming phase.
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
> + ret = imgld->ops->poll_complete(imgld);
> + if (ret != FPGA_IMAGE_ERR_NONE)
> + fpga_image_dev_error(imgld, ret);
> +
> +done:
> + if (imgld->ops->cleanup)
> + imgld->ops->cleanup(imgld);
> +
> +modput_exit:
> + module_put(imgld->dev.parent->driver->owner);
> +
> +putdev_exit:
> + put_device(&imgld->dev);
> +
> +idle_exit:
> + /*
> + * Note: imgld->remaining_size is left unmodified here to provide
> + * additional information on errors. It will be reinitialized when
> + * the next image load begins.
> + */
> + vfree(imgld->data);
> + imgld->data = NULL;
> + fpga_image_prog_complete(imgld);
> +}
> +
> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> + unsigned long arg)
> +{
> + struct fpga_image_write wb;
> + unsigned long minsz;
> + u8 *buf;
> +
> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> + return -EBUSY;
> +
> + minsz = offsetofend(struct fpga_image_write, buf);
> + if (copy_from_user(&wb, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (wb.flags)
> + return -EINVAL;
> +
> + /* Enforce 32-bit alignment on the write data */
> + if (wb.size & 0x3)
> + return -EINVAL;
> +
> + buf = vzalloc(wb.size);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> + vfree(buf);
> + return -EFAULT;
> + }
> +
> + imgld->data = buf;
> + imgld->remaining_size = wb.size;
> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
> + queue_work(system_unbound_wq, &imgld->work);
I have little experience about the queue type, is it the best choice? I
see there are system_unbound_wq & system_long_wq, and according to the
workqueue.rst for system_unbound_wq use case:
1. Wide fluctuation in the concurrency level requirement.
2. Long running CPU intensive workloads.
Our case may not fit in.
Thanks,
Yilun
> +
> + 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;
> +
> + switch (cmd) {
> + case FPGA_IMAGE_LOAD_WRITE:
> + mutex_lock(&imgld->lock);
> + ret = fpga_image_load_ioctl_write(imgld, arg);
> + mutex_unlock(&imgld->lock);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
> + struct fpga_image_load, cdev);
> +
> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
> + return -EBUSY;
> +
> + filp->private_data = imgld;
> +
> + return 0;
> +}
> +
> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = filp->private_data;
> +
> + mutex_lock(&imgld->lock);
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto close_exit;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +close_exit:
> + atomic_set(&imgld->opened, 0);
> +
> + return 0;
> +}
> +
> +static const struct file_operations fpga_image_load_fops = {
> + .owner = THIS_MODULE,
> + .open = fpga_image_load_open,
> + .release = fpga_image_load_release,
> + .unlocked_ioctl = fpga_image_load_ioctl,
> +};
> +
> /**
> * fpga_image_load_register - create and register an FPGA Image Load Device
> *
> @@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
> struct fpga_image_load *imgld;
> int ret;
>
> + if (!ops || !ops->cancel || !ops->prepare ||
> + !ops->write_blk || !ops->poll_complete) {
> + dev_err(parent, "Attempt to register without all required ops\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
> if (!imgld)
> return ERR_PTR(-ENOMEM);
> @@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
>
> imgld->priv = priv;
> imgld->ops = ops;
> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + INIT_WORK(&imgld->work, fpga_image_do_load);
>
> imgld->dev.class = fpga_image_load_class;
> imgld->dev.parent = parent;
> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>
> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
> if (ret) {
> @@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
> return ERR_PTR(ret);
> }
>
> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
> + imgld->cdev.owner = parent->driver->owner;
> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
> +
> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
> + if (ret) {
> + put_device(&imgld->dev);
> + return ERR_PTR(ret);
> + }
> +
> return imgld;
>
> error_device:
> @@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
> * @imgld: pointer to struct fpga_image_load
> *
> * This function is intended for use in the parent driver's remove()
> - * function.
> + * function. The driver_unload flag prevents new updates from starting
> + * once the unregister process has begun.
> */
> void fpga_image_load_unregister(struct fpga_image_load *imgld)
> {
> + mutex_lock(&imgld->lock);
> + imgld->driver_unload = true;
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto unregister;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +unregister:
> + cdev_del(&imgld->cdev);
> device_unregister(&imgld->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
> @@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>
> static int __init fpga_image_load_class_init(void)
> {
> + int ret;
> pr_info("FPGA Image Load Framework\n");
>
> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
> if (IS_ERR(fpga_image_load_class))
> return PTR_ERR(fpga_image_load_class);
>
> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
> + "fpga_image_load");
> + if (ret)
> + goto exit_destroy_class;
> +
> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>
> return 0;
> +
> +exit_destroy_class:
> + class_destroy(fpga_image_load_class);
> + return ret;
> }
>
> static void __exit fpga_image_load_class_exit(void)
> {
> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
> class_destroy(fpga_image_load_class);
> WARN_ON(!xa_empty(&fpga_image_load_xa));
> }
> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> index 8b051c82ef5f..41ab63cf7b20 100644
> --- a/include/linux/fpga/fpga-image-load.h
> +++ b/include/linux/fpga/fpga-image-load.h
> @@ -7,22 +7,51 @@
> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
> #define _LINUX_FPGA_IMAGE_LOAD_H
>
> +#include <linux/cdev.h>
> #include <linux/device.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> +#include <uapi/linux/fpga-image-load.h>
>
> struct fpga_image_load;
>
> /**
> * struct fpga_image_load_ops - device specific operations
> + * @prepare: Required: Prepare secure update
> + * @write_blk: Required: Write a block of data. The class driver
> + * provides a default block size. The write_blk() op
> + * may choose to modify *blk_size to something more
> + * optimal for the given device. *blk_size must be
> + * less than or equal to max_size.
> + * @poll_complete: Required: Check for the completion of the
> + * HW authentication/programming process.
> + * @cancel: Required: Signal HW to cancel update
> + * @cleanup: Optional: Complements the prepare()
> + * function and is called at the completion
> + * of the update, whether success or failure,
> + * if the prepare function succeeded.
> */
> struct fpga_image_load_ops {
> + u32 (*prepare)(struct fpga_image_load *imgld);
> + u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
> + u32 *blk_size, u32 max_size);
> + u32 (*poll_complete)(struct fpga_image_load *imgld);
> + u32 (*cancel)(struct fpga_image_load *imgld);
> + void (*cleanup)(struct fpga_image_load *imgld);
> };
>
> struct fpga_image_load {
> struct device dev;
> + struct cdev cdev;
> const struct fpga_image_load_ops *ops;
> struct mutex lock; /* protect data structure contents */
> + atomic_t opened;
> + struct work_struct work;
> + const u8 *data; /* pointer to update data */
> + u32 remaining_size; /* size remaining to transfer */
> + u32 progress;
> + u32 err_code; /* image load error code */
> + bool driver_unload;
> void *priv;
> };
>
> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
> new file mode 100644
> index 000000000000..0382078c5a6c
> --- /dev/null
> +++ b/include/uapi/linux/fpga-image-load.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header File for FPGA Image Load User API
> + *
> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
> + *
> + */
> +
> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
> +
> +/* Image load progress codes */
> +#define FPGA_IMAGE_PROG_IDLE 0
> +#define FPGA_IMAGE_PROG_STARTING 1
> +#define FPGA_IMAGE_PROG_PREPARING 2
> +#define FPGA_IMAGE_PROG_WRITING 3
> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
> +#define FPGA_IMAGE_PROG_MAX 5
> +
> +/* Image error progress codes */
> +#define FPGA_IMAGE_ERR_NONE 0
> +#define FPGA_IMAGE_ERR_HW_ERROR 1
> +#define FPGA_IMAGE_ERR_TIMEOUT 2
> +#define FPGA_IMAGE_ERR_CANCELED 3
> +#define FPGA_IMAGE_ERR_BUSY 4
> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
> +#define FPGA_IMAGE_ERR_RW_ERROR 6
> +#define FPGA_IMAGE_ERR_WEAROUT 7
> +#define FPGA_IMAGE_ERR_MAX 8
> +
> +/**
> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
> + * struct fpga_image_write)
> + *
> + * Upload a data buffer to the target device. The user must provide the
> + * data buffer and size.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct fpga_image_write {
> + /* Input */
> + __u32 flags; /* Zero for now */
> + __u32 size; /* Data size (in bytes) to be written */
> + __u64 buf; /* User space address of source data */
> +};
> +
> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
> +
> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
2021-09-23 8:54 ` Xu Yilun
@ 2021-09-24 0:21 ` Russ Weight
0 siblings, 0 replies; 18+ messages in thread
From: Russ Weight @ 2021-09-24 0:21 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On 9/23/21 1:54 AM, Xu Yilun wrote:
> On Wed, Sep 22, 2021 at 05:10:53PM -0700, Russ Weight wrote:
>> Extend the FPGA Image Load framework to include IOCTL support
>> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
>> The IOCTL will return immediately, and the update will begin in the
>> context of a kernel worker thread.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v16:
>> - Shift from "Driver" terminology to "Framework" in comments and
>> documentation
>> - Rename lops to ops for structure member and local variables
>> - Change the write_blk() definition to pass in *blk_size (a pointer to
>> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
>> the maximum block size to stay within the limit of the data buffer).
>> The write_blk() op may use the default *blk_size or modify it to a
>> more optimal number for the given device, subject to the max_size limit.
>> - All enum values for progress and errors are changed to macros, because
>> they are included in the uapi header. This is done to maintain consistency
>> amongst the DFL related IOCTL header files. All references to the enum
>> types are changed to u32.
>> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
>> - Add a call to cond_resched() in the write_blk() loop to ensure that
>> we yield to higher priority tasks during long data transfers.
>> - Switch to the system_unbound_wq to enable calls to cond_resched().
>> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
>> imgld->opened.
>> - Change fpga_image_load_release() to block until the image upload is
>> complete.
>> - Remove the completion object, imgld->update_done, in favor of calling
>> flush_work(&imgld->work);
>> v15:
>> - Compare to previous patch:
>> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
>> - Changed file, symbol, and config names to reflect the new driver name
>> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
>> IOCTL for writing the image data to the FPGA card. The driver no longer
>> uses the request_firmware framework.
>> - Fixed some error return values in fpga_image_load_register()
>> - Removed signed-off/reviewed-by tags
>> v14:
>> - Added MAINTAINERS reference for
>> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
>> - Updated ABI documentation date and kernel version
>> - Updated copyright to 2021
>> v13:
>> - Change "if (count == 0 || " to "if (!count || "
>> - Improve error message: "Attempt to register without all required ops\n"
>> v12:
>> - Updated Date and KernelVersion fields in ABI documentation
>> - Removed size parameter from write_blk() op - it is now up to
>> the lower-level driver to determine the appropriate size and
>> to update smgr->remaining_size accordingly.
>> v11:
>> - Fixed a spelling error in a comment
>> - Initialize smgr->err_code and smgr->progress explicitly in
>> fpga_sec_mgr_create() instead of accepting the default 0 value.
>> 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
>> - Changed filename_store() to use kmemdup_nul() instead of
>> kstrndup() and changed the count to not assume a line-return.
>> v6:
>> - Changed "security update" to "secure update" in commit message
>> v5:
>> - When checking the return values for functions of type enum
>> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
>> 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:
>> - Removed unnecessary "goto done"
>> - Added a comment to explain imgr->driver_unload in
>> ifpga_sec_mgr_unregister()
>> v2:
>> - Bumped documentation date and version
>> - Removed explicit value assignments in enums
>> - Other minor code cleanup per review comments
>> ---
>> Documentation/fpga/fpga-image-load.rst | 26 ++-
>> MAINTAINERS | 1 +
>> drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
>> include/linux/fpga/fpga-image-load.h | 29 +++
>> include/uapi/linux/fpga-image-load.h | 54 ++++++
>> 5 files changed, 346 insertions(+), 2 deletions(-)
>> create mode 100644 include/uapi/linux/fpga-image-load.h
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index dda9283ef1c7..ba371c7c0ca0 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -7,4 +7,28 @@ FPGA Image Load Framework
>> The FPGA Image Load framework provides a common API for user-space
>> tools to manage image uploads to FPGA devices. Device drivers that
>> instantiate the FPGA Image Load framework will interact with the
>> -target device to transfer and authenticate the image data.
>> +target device to transfer and authenticate the image data. Image uploads
>> +are processed in the context of a kernel worker thread.
>> +
>> +User API
>> +========
>> +
>> +open
>> +----
>> +
>> +An fpga_image_load device is opened exclusively to control an image upload.
>> +The device must remain open throughout the duration of the image upload.
>> +An attempt to close the device while an upload is in progress will cause
>> +the upload to be cancelled. If unable to cancel the image upload, the close
>> +system call will block until the image upload is complete.
>> +
>> +ioctl
>> +-----
>> +
>> +FPGA_IMAGE_LOAD_WRITE:
>> +
>> +Start an image upload with the provided image buffer. This IOCTL returns
>> +immediately after starting a kernel worker thread to process the image
>> +upload which could take as long a 40 minutes depending on the actual device
> as long as?
Thanks, I'll fix it.
>
>> +being updated. This is an exclusive operation; an attempt to start
>> +concurrent image uploads for the same device will fail with EBUSY.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6e312d0ddeb6..4e44f62eef33 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7372,6 +7372,7 @@ S: Maintained
>> F: Documentation/fpga/fpga-image-load.rst
>> F: drivers/fpga/fpga-image-load.c
>> F: include/linux/fpga/fpga-image-load.h
>> +F: include/uapi/linux/fpga-image-load.h
>>
>> FPU EMULATOR
>> M: Bill Metzenthen <billm@melbpc.org.au>
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index 799d18444f7c..65f553b59011 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -5,18 +5,210 @@
>> * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> */
>>
>> +#include <linux/delay.h>
>> #include <linux/fpga/fpga-image-load.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> #include <linux/vmalloc.h>
>>
>> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
>> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>>
>> static struct class *fpga_image_load_class;
>> +static dev_t fpga_image_devt;
>>
>> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>
>> +#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
>> +
>> +static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
>> +{
>> + imgld->err_code = err_code;
>> + imgld->ops->cancel(imgld);
>> +}
>> +
>> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>> +{
>> + mutex_lock(&imgld->lock);
>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> + mutex_unlock(&imgld->lock);
>> +}
>> +
>> +static void fpga_image_do_load(struct work_struct *work)
>> +{
>> + struct fpga_image_load *imgld;
>> + u32 ret, blk_size, offset = 0;
>> +
>> + imgld = container_of(work, struct fpga_image_load, work);
>> +
>> + if (imgld->driver_unload) {
>> + imgld->err_code = 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;
>> + goto putdev_exit;
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>> + ret = imgld->ops->prepare(imgld);
>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>> + fpga_image_dev_error(imgld, ret);
>> + goto modput_exit;
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
>> + while (imgld->remaining_size) {
>> + /*
>> + * The write_blk() op has the option to use the blk_size
>> + * value provided here, or to modify it to something more
>> + * optimal for the given device.
>> + */
>> + blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
>> + ret = imgld->ops->write_blk(imgld, offset, &blk_size,
>> + imgld->remaining_size);
> I don't suggest we must have a default block size. I just prefer a more
> common API definition for this case.
>
> How about we define like this:
>
> u32 (*write_blk)(struct fpga_image_load *imgld, const char *buf, u32 size, u32 *written_size);
> The return value indicates error type.
>
> or
>
> u32 (*write_blk)(struct fpga_image_load *imgld, const char *buf, u32 size);
> The positive return value indicates the written size. The
> negative values indicates error type.
>
> then we could call it in framework like:
>
> ret = imgld->ops->write_blk(imgld, imgld->data + offset, imgld->remaining_size, &written_size);
Sure - I'll drop the default size and change the interface so that the write_blk
op does not need to access imgld structure elements unnecessarily.
>
>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>> + fpga_image_dev_error(imgld, ret);
>> + goto done;
>> + }
>> +
>> + imgld->remaining_size -= blk_size;
>> + offset += blk_size;
>> +
>> + /*
>> + * The class driver does not have control of the overall
>> + * size or the actual implementation of the write. Allow
>> + * for scheduling out.
>> + */
>> + cond_resched();
> I'm not quite sure about this. We only reschedule during block write but
> not in other phases. Actually they may also be time consuming, such as
> programming phase.
Yes, I've been trying to research this more. We can't really do anything
about the other phases, because they are managed completely by the parent
driver. I think it is OK to leave scheduling considerations to the parent
driver.
In our case, the parent driver does plenty of sleeps as part of the
polling handshakes with hardware, so I don't think we need to be concerned
about being friendly to the scheduler. Would you agree?
>
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>> + ret = imgld->ops->poll_complete(imgld);
>> + if (ret != FPGA_IMAGE_ERR_NONE)
>> + fpga_image_dev_error(imgld, ret);
>> +
>> +done:
>> + if (imgld->ops->cleanup)
>> + imgld->ops->cleanup(imgld);
>> +
>> +modput_exit:
>> + module_put(imgld->dev.parent->driver->owner);
>> +
>> +putdev_exit:
>> + put_device(&imgld->dev);
>> +
>> +idle_exit:
>> + /*
>> + * Note: imgld->remaining_size is left unmodified here to provide
>> + * additional information on errors. It will be reinitialized when
>> + * the next image load begins.
>> + */
>> + vfree(imgld->data);
>> + imgld->data = NULL;
>> + fpga_image_prog_complete(imgld);
>> +}
>> +
>> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> + unsigned long arg)
>> +{
>> + struct fpga_image_write wb;
>> + unsigned long minsz;
>> + u8 *buf;
>> +
>> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
>> + return -EBUSY;
>> +
>> + minsz = offsetofend(struct fpga_image_write, buf);
>> + if (copy_from_user(&wb, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (wb.flags)
>> + return -EINVAL;
>> +
>> + /* Enforce 32-bit alignment on the write data */
>> + if (wb.size & 0x3)
>> + return -EINVAL;
>> +
>> + buf = vzalloc(wb.size);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
>> + vfree(buf);
>> + return -EFAULT;
>> + }
>> +
>> + imgld->data = buf;
>> + imgld->remaining_size = wb.size;
>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
>> + queue_work(system_unbound_wq, &imgld->work);
> I have little experience about the queue type, is it the best choice? I
> see there are system_unbound_wq & system_long_wq, and according to the
> workqueue.rst for system_unbound_wq use case:
>
> 1. Wide fluctuation in the concurrency level requirement.
> 2. Long running CPU intensive workloads.
>
> Our case may not fit in.
If we remove the cond_resched() call, then I don't think we need this
queue. It seems like the system_long_wq is probably right for our use.
- Russ
>
> Thanks,
> Yilun
>
>> +
>> + 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;
>> +
>> + switch (cmd) {
>> + case FPGA_IMAGE_LOAD_WRITE:
>> + mutex_lock(&imgld->lock);
>> + ret = fpga_image_load_ioctl_write(imgld, arg);
>> + mutex_unlock(&imgld->lock);
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
>> +{
>> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
>> + struct fpga_image_load, cdev);
>> +
>> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
>> + return -EBUSY;
>> +
>> + filp->private_data = imgld;
>> +
>> + return 0;
>> +}
>> +
>> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
>> +{
>> + struct fpga_image_load *imgld = filp->private_data;
>> +
>> + mutex_lock(&imgld->lock);
>> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>> + mutex_unlock(&imgld->lock);
>> + goto close_exit;
>> + }
>> +
>> + mutex_unlock(&imgld->lock);
>> + flush_work(&imgld->work);
>> +
>> +close_exit:
>> + atomic_set(&imgld->opened, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations fpga_image_load_fops = {
>> + .owner = THIS_MODULE,
>> + .open = fpga_image_load_open,
>> + .release = fpga_image_load_release,
>> + .unlocked_ioctl = fpga_image_load_ioctl,
>> +};
>> +
>> /**
>> * fpga_image_load_register - create and register an FPGA Image Load Device
>> *
>> @@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
>> struct fpga_image_load *imgld;
>> int ret;
>>
>> + if (!ops || !ops->cancel || !ops->prepare ||
>> + !ops->write_blk || !ops->poll_complete) {
>> + dev_err(parent, "Attempt to register without all required ops\n");
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
>> if (!imgld)
>> return ERR_PTR(-ENOMEM);
>> @@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
>>
>> imgld->priv = priv;
>> imgld->ops = ops;
>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> + INIT_WORK(&imgld->work, fpga_image_do_load);
>>
>> imgld->dev.class = fpga_image_load_class;
>> imgld->dev.parent = parent;
>> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>>
>> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
>> if (ret) {
>> @@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
>> return ERR_PTR(ret);
>> }
>>
>> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
>> + imgld->cdev.owner = parent->driver->owner;
>> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
>> +
>> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
>> + if (ret) {
>> + put_device(&imgld->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> return imgld;
>>
>> error_device:
>> @@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
>> * @imgld: pointer to struct fpga_image_load
>> *
>> * This function is intended for use in the parent driver's remove()
>> - * function.
>> + * function. The driver_unload flag prevents new updates from starting
>> + * once the unregister process has begun.
>> */
>> void fpga_image_load_unregister(struct fpga_image_load *imgld)
>> {
>> + mutex_lock(&imgld->lock);
>> + imgld->driver_unload = true;
>> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>> + mutex_unlock(&imgld->lock);
>> + goto unregister;
>> + }
>> +
>> + mutex_unlock(&imgld->lock);
>> + flush_work(&imgld->work);
>> +
>> +unregister:
>> + cdev_del(&imgld->cdev);
>> device_unregister(&imgld->dev);
>> }
>> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
>> @@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>>
>> static int __init fpga_image_load_class_init(void)
>> {
>> + int ret;
>> pr_info("FPGA Image Load Framework\n");
>>
>> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
>> if (IS_ERR(fpga_image_load_class))
>> return PTR_ERR(fpga_image_load_class);
>>
>> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
>> + "fpga_image_load");
>> + if (ret)
>> + goto exit_destroy_class;
>> +
>> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>>
>> return 0;
>> +
>> +exit_destroy_class:
>> + class_destroy(fpga_image_load_class);
>> + return ret;
>> }
>>
>> static void __exit fpga_image_load_class_exit(void)
>> {
>> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
>> class_destroy(fpga_image_load_class);
>> WARN_ON(!xa_empty(&fpga_image_load_xa));
>> }
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 8b051c82ef5f..41ab63cf7b20 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -7,22 +7,51 @@
>> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
>> #define _LINUX_FPGA_IMAGE_LOAD_H
>>
>> +#include <linux/cdev.h>
>> #include <linux/device.h>
>> #include <linux/mutex.h>
>> #include <linux/types.h>
>> +#include <uapi/linux/fpga-image-load.h>
>>
>> struct fpga_image_load;
>>
>> /**
>> * struct fpga_image_load_ops - device specific operations
>> + * @prepare: Required: Prepare secure update
>> + * @write_blk: Required: Write a block of data. The class driver
>> + * provides a default block size. The write_blk() op
>> + * may choose to modify *blk_size to something more
>> + * optimal for the given device. *blk_size must be
>> + * less than or equal to max_size.
>> + * @poll_complete: Required: Check for the completion of the
>> + * HW authentication/programming process.
>> + * @cancel: Required: Signal HW to cancel update
>> + * @cleanup: Optional: Complements the prepare()
>> + * function and is called at the completion
>> + * of the update, whether success or failure,
>> + * if the prepare function succeeded.
>> */
>> struct fpga_image_load_ops {
>> + u32 (*prepare)(struct fpga_image_load *imgld);
>> + u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
>> + u32 *blk_size, u32 max_size);
>> + u32 (*poll_complete)(struct fpga_image_load *imgld);
>> + u32 (*cancel)(struct fpga_image_load *imgld);
>> + void (*cleanup)(struct fpga_image_load *imgld);
>> };
>>
>> struct fpga_image_load {
>> struct device dev;
>> + struct cdev cdev;
>> const struct fpga_image_load_ops *ops;
>> struct mutex lock; /* protect data structure contents */
>> + atomic_t opened;
>> + struct work_struct work;
>> + const u8 *data; /* pointer to update data */
>> + u32 remaining_size; /* size remaining to transfer */
>> + u32 progress;
>> + u32 err_code; /* image load error code */
>> + bool driver_unload;
>> void *priv;
>> };
>>
>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>> new file mode 100644
>> index 000000000000..0382078c5a6c
>> --- /dev/null
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -0,0 +1,54 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Header File for FPGA Image Load User API
>> + *
>> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> + *
>> + */
>> +
>> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
>> +
>> +/* Image load progress codes */
>> +#define FPGA_IMAGE_PROG_IDLE 0
>> +#define FPGA_IMAGE_PROG_STARTING 1
>> +#define FPGA_IMAGE_PROG_PREPARING 2
>> +#define FPGA_IMAGE_PROG_WRITING 3
>> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
>> +#define FPGA_IMAGE_PROG_MAX 5
>> +
>> +/* Image error progress codes */
>> +#define FPGA_IMAGE_ERR_NONE 0
>> +#define FPGA_IMAGE_ERR_HW_ERROR 1
>> +#define FPGA_IMAGE_ERR_TIMEOUT 2
>> +#define FPGA_IMAGE_ERR_CANCELED 3
>> +#define FPGA_IMAGE_ERR_BUSY 4
>> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
>> +#define FPGA_IMAGE_ERR_RW_ERROR 6
>> +#define FPGA_IMAGE_ERR_WEAROUT 7
>> +#define FPGA_IMAGE_ERR_MAX 8
>> +
>> +/**
>> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
>> + * struct fpga_image_write)
>> + *
>> + * Upload a data buffer to the target device. The user must provide the
>> + * data buffer and size.
>> + *
>> + * Return: 0 on success, -errno on failure.
>> + */
>> +struct fpga_image_write {
>> + /* Input */
>> + __u32 flags; /* Zero for now */
>> + __u32 size; /* Data size (in bytes) to be written */
>> + __u64 buf; /* User space address of source data */
>> +};
>> +
>> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>> +
>> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
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 8:12 ` Xu Yilun
2021-09-24 21:32 ` Russ Weight
1 sibling, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2021-09-24 8:12 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On Wed, Sep 22, 2021 at 05:10:53PM -0700, Russ Weight wrote:
> Extend the FPGA Image Load framework to include IOCTL support
> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
> The IOCTL will return immediately, and the update will begin in the
> context of a kernel worker thread.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v16:
> - Shift from "Driver" terminology to "Framework" in comments and
> documentation
> - Rename lops to ops for structure member and local variables
> - Change the write_blk() definition to pass in *blk_size (a pointer to
> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
> the maximum block size to stay within the limit of the data buffer).
> The write_blk() op may use the default *blk_size or modify it to a
> more optimal number for the given device, subject to the max_size limit.
> - All enum values for progress and errors are changed to macros, because
> they are included in the uapi header. This is done to maintain consistency
> amongst the DFL related IOCTL header files. All references to the enum
> types are changed to u32.
> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
> - Add a call to cond_resched() in the write_blk() loop to ensure that
> we yield to higher priority tasks during long data transfers.
> - Switch to the system_unbound_wq to enable calls to cond_resched().
> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
> imgld->opened.
> - Change fpga_image_load_release() to block until the image upload is
> complete.
> - Remove the completion object, imgld->update_done, in favor of calling
> flush_work(&imgld->work);
> v15:
> - Compare to previous patch:
> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
> - Changed file, symbol, and config names to reflect the new driver name
> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
> IOCTL for writing the image data to the FPGA card. The driver no longer
> uses the request_firmware framework.
> - Fixed some error return values in fpga_image_load_register()
> - Removed signed-off/reviewed-by tags
> v14:
> - Added MAINTAINERS reference for
> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> - Updated ABI documentation date and kernel version
> - Updated copyright to 2021
> v13:
> - Change "if (count == 0 || " to "if (!count || "
> - Improve error message: "Attempt to register without all required ops\n"
> v12:
> - Updated Date and KernelVersion fields in ABI documentation
> - Removed size parameter from write_blk() op - it is now up to
> the lower-level driver to determine the appropriate size and
> to update smgr->remaining_size accordingly.
> v11:
> - Fixed a spelling error in a comment
> - Initialize smgr->err_code and smgr->progress explicitly in
> fpga_sec_mgr_create() instead of accepting the default 0 value.
> 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
> - Changed filename_store() to use kmemdup_nul() instead of
> kstrndup() and changed the count to not assume a line-return.
> v6:
> - Changed "security update" to "secure update" in commit message
> v5:
> - When checking the return values for functions of type enum
> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
> 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:
> - Removed unnecessary "goto done"
> - Added a comment to explain imgr->driver_unload in
> ifpga_sec_mgr_unregister()
> v2:
> - Bumped documentation date and version
> - Removed explicit value assignments in enums
> - Other minor code cleanup per review comments
> ---
> Documentation/fpga/fpga-image-load.rst | 26 ++-
> MAINTAINERS | 1 +
> drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
> include/linux/fpga/fpga-image-load.h | 29 +++
> include/uapi/linux/fpga-image-load.h | 54 ++++++
> 5 files changed, 346 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/fpga-image-load.h
>
> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> index dda9283ef1c7..ba371c7c0ca0 100644
> --- a/Documentation/fpga/fpga-image-load.rst
> +++ b/Documentation/fpga/fpga-image-load.rst
> @@ -7,4 +7,28 @@ FPGA Image Load Framework
> The FPGA Image Load framework provides a common API for user-space
> tools to manage image uploads to FPGA devices. Device drivers that
> instantiate the FPGA Image Load framework will interact with the
> -target device to transfer and authenticate the image data.
> +target device to transfer and authenticate the image data. Image uploads
> +are processed in the context of a kernel worker thread.
> +
> +User API
> +========
> +
> +open
> +----
> +
> +An fpga_image_load device is opened exclusively to control an image upload.
> +The device must remain open throughout the duration of the image upload.
> +An attempt to close the device while an upload is in progress will cause
> +the upload to be cancelled. If unable to cancel the image upload, the close
> +system call will block until the image upload is complete.
> +
> +ioctl
> +-----
> +
> +FPGA_IMAGE_LOAD_WRITE:
> +
> +Start an image upload with the provided image buffer. This IOCTL returns
> +immediately after starting a kernel worker thread to process the image
> +upload which could take as long a 40 minutes depending on the actual device
> +being updated. This is an exclusive operation; an attempt to start
> +concurrent image uploads for the same device will fail with EBUSY.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e312d0ddeb6..4e44f62eef33 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7372,6 +7372,7 @@ S: Maintained
> F: Documentation/fpga/fpga-image-load.rst
> F: drivers/fpga/fpga-image-load.c
> F: include/linux/fpga/fpga-image-load.h
> +F: include/uapi/linux/fpga-image-load.h
>
> FPU EMULATOR
> M: Bill Metzenthen <billm@melbpc.org.au>
> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> index 799d18444f7c..65f553b59011 100644
> --- a/drivers/fpga/fpga-image-load.c
> +++ b/drivers/fpga/fpga-image-load.c
> @@ -5,18 +5,210 @@
> * Copyright (C) 2019-2021 Intel Corporation, Inc.
> */
>
> +#include <linux/delay.h>
> #include <linux/fpga/fpga-image-load.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/uaccess.h>
> #include <linux/vmalloc.h>
>
> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>
> static struct class *fpga_image_load_class;
> +static dev_t fpga_image_devt;
>
> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>
> +#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
> +
> +static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
> +{
> + imgld->err_code = err_code;
> + imgld->ops->cancel(imgld);
Do we need the cancel here? Or cleanup is just fine.
I see the description below:
* @cancel: Required: Signal HW to cancel update
* @cleanup: Optional: Complements the prepare()
* function and is called at the completion
* of the update, whether success or failure,
* if the prepare function succeeded.
My understanding is when error happens, the HW is already stopped,
and may optionally needs a cleanup to become normal again.
And cancel() is like interrupting an ongoing HW progress, which could
be called when other callback is running.
We can discuss on this.
> +}
> +
> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> +{
> + mutex_lock(&imgld->lock);
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + mutex_unlock(&imgld->lock);
> +}
> +
> +static void fpga_image_do_load(struct work_struct *work)
> +{
> + struct fpga_image_load *imgld;
> + u32 ret, blk_size, offset = 0;
> +
> + imgld = container_of(work, struct fpga_image_load, work);
> +
> + if (imgld->driver_unload) {
> + imgld->err_code = 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;
> + goto putdev_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
> + ret = imgld->ops->prepare(imgld);
> + if (ret != FPGA_IMAGE_ERR_NONE) {
> + fpga_image_dev_error(imgld, ret);
> + goto modput_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
> + while (imgld->remaining_size) {
> + /*
> + * The write_blk() op has the option to use the blk_size
> + * value provided here, or to modify it to something more
> + * optimal for the given device.
> + */
> + blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
> + ret = imgld->ops->write_blk(imgld, offset, &blk_size,
> + imgld->remaining_size);
> + if (ret != FPGA_IMAGE_ERR_NONE) {
> + fpga_image_dev_error(imgld, ret);
> + goto done;
> + }
> +
> + imgld->remaining_size -= blk_size;
> + offset += blk_size;
> +
> + /*
> + * The class driver does not have control of the overall
> + * size or the actual implementation of the write. Allow
> + * for scheduling out.
> + */
> + cond_resched();
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
> + ret = imgld->ops->poll_complete(imgld);
> + if (ret != FPGA_IMAGE_ERR_NONE)
> + fpga_image_dev_error(imgld, ret);
> +
> +done:
> + if (imgld->ops->cleanup)
> + imgld->ops->cleanup(imgld);
> +
> +modput_exit:
> + module_put(imgld->dev.parent->driver->owner);
> +
> +putdev_exit:
> + put_device(&imgld->dev);
> +
> +idle_exit:
> + /*
> + * Note: imgld->remaining_size is left unmodified here to provide
> + * additional information on errors. It will be reinitialized when
> + * the next image load begins.
> + */
> + vfree(imgld->data);
> + imgld->data = NULL;
> + fpga_image_prog_complete(imgld);
> +}
> +
> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> + unsigned long arg)
> +{
> + struct fpga_image_write wb;
> + unsigned long minsz;
> + u8 *buf;
> +
> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> + return -EBUSY;
> +
> + minsz = offsetofend(struct fpga_image_write, buf);
> + if (copy_from_user(&wb, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (wb.flags)
> + return -EINVAL;
> +
> + /* Enforce 32-bit alignment on the write data */
> + if (wb.size & 0x3)
> + return -EINVAL;
Why we enforce the alignment? It seems to be the requirement of the
low level driver. We may handle it there.
Thanks,
Yilun
> +
> + buf = vzalloc(wb.size);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> + vfree(buf);
> + return -EFAULT;
> + }
> +
> + imgld->data = buf;
> + imgld->remaining_size = wb.size;
> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
> + queue_work(system_unbound_wq, &imgld->work);
> +
> + 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;
> +
> + switch (cmd) {
> + case FPGA_IMAGE_LOAD_WRITE:
> + mutex_lock(&imgld->lock);
> + ret = fpga_image_load_ioctl_write(imgld, arg);
> + mutex_unlock(&imgld->lock);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
> + struct fpga_image_load, cdev);
> +
> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
> + return -EBUSY;
> +
> + filp->private_data = imgld;
> +
> + return 0;
> +}
> +
> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = filp->private_data;
> +
> + mutex_lock(&imgld->lock);
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto close_exit;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +close_exit:
> + atomic_set(&imgld->opened, 0);
> +
> + return 0;
> +}
> +
> +static const struct file_operations fpga_image_load_fops = {
> + .owner = THIS_MODULE,
> + .open = fpga_image_load_open,
> + .release = fpga_image_load_release,
> + .unlocked_ioctl = fpga_image_load_ioctl,
> +};
> +
> /**
> * fpga_image_load_register - create and register an FPGA Image Load Device
> *
> @@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
> struct fpga_image_load *imgld;
> int ret;
>
> + if (!ops || !ops->cancel || !ops->prepare ||
> + !ops->write_blk || !ops->poll_complete) {
> + dev_err(parent, "Attempt to register without all required ops\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
> if (!imgld)
> return ERR_PTR(-ENOMEM);
> @@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
>
> imgld->priv = priv;
> imgld->ops = ops;
> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + INIT_WORK(&imgld->work, fpga_image_do_load);
>
> imgld->dev.class = fpga_image_load_class;
> imgld->dev.parent = parent;
> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>
> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
> if (ret) {
> @@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
> return ERR_PTR(ret);
> }
>
> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
> + imgld->cdev.owner = parent->driver->owner;
> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
Could be replaced by cdev_set_parent()
> +
> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
> + if (ret) {
> + put_device(&imgld->dev);
> + return ERR_PTR(ret);
> + }
> +
> return imgld;
>
> error_device:
> @@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
> * @imgld: pointer to struct fpga_image_load
> *
> * This function is intended for use in the parent driver's remove()
> - * function.
> + * function. The driver_unload flag prevents new updates from starting
> + * once the unregister process has begun.
> */
> void fpga_image_load_unregister(struct fpga_image_load *imgld)
> {
> + mutex_lock(&imgld->lock);
> + imgld->driver_unload = true;
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto unregister;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +unregister:
> + cdev_del(&imgld->cdev);
> device_unregister(&imgld->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
> @@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>
> static int __init fpga_image_load_class_init(void)
> {
> + int ret;
> pr_info("FPGA Image Load Framework\n");
>
> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
> if (IS_ERR(fpga_image_load_class))
> return PTR_ERR(fpga_image_load_class);
>
> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
> + "fpga_image_load");
> + if (ret)
> + goto exit_destroy_class;
> +
> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>
> return 0;
> +
> +exit_destroy_class:
> + class_destroy(fpga_image_load_class);
> + return ret;
> }
>
> static void __exit fpga_image_load_class_exit(void)
> {
> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
> class_destroy(fpga_image_load_class);
> WARN_ON(!xa_empty(&fpga_image_load_xa));
> }
> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> index 8b051c82ef5f..41ab63cf7b20 100644
> --- a/include/linux/fpga/fpga-image-load.h
> +++ b/include/linux/fpga/fpga-image-load.h
> @@ -7,22 +7,51 @@
> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
> #define _LINUX_FPGA_IMAGE_LOAD_H
>
> +#include <linux/cdev.h>
> #include <linux/device.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> +#include <uapi/linux/fpga-image-load.h>
>
> struct fpga_image_load;
>
> /**
> * struct fpga_image_load_ops - device specific operations
> + * @prepare: Required: Prepare secure update
> + * @write_blk: Required: Write a block of data. The class driver
> + * provides a default block size. The write_blk() op
> + * may choose to modify *blk_size to something more
> + * optimal for the given device. *blk_size must be
> + * less than or equal to max_size.
> + * @poll_complete: Required: Check for the completion of the
> + * HW authentication/programming process.
> + * @cancel: Required: Signal HW to cancel update
> + * @cleanup: Optional: Complements the prepare()
> + * function and is called at the completion
> + * of the update, whether success or failure,
> + * if the prepare function succeeded.
> */
> struct fpga_image_load_ops {
> + u32 (*prepare)(struct fpga_image_load *imgld);
> + u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
> + u32 *blk_size, u32 max_size);
> + u32 (*poll_complete)(struct fpga_image_load *imgld);
> + u32 (*cancel)(struct fpga_image_load *imgld);
> + void (*cleanup)(struct fpga_image_load *imgld);
> };
>
> struct fpga_image_load {
> struct device dev;
> + struct cdev cdev;
> const struct fpga_image_load_ops *ops;
> struct mutex lock; /* protect data structure contents */
> + atomic_t opened;
> + struct work_struct work;
> + const u8 *data; /* pointer to update data */
> + u32 remaining_size; /* size remaining to transfer */
> + u32 progress;
> + u32 err_code; /* image load error code */
> + bool driver_unload;
> void *priv;
> };
>
> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
> new file mode 100644
> index 000000000000..0382078c5a6c
> --- /dev/null
> +++ b/include/uapi/linux/fpga-image-load.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header File for FPGA Image Load User API
> + *
> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
> + *
> + */
> +
> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
> +
> +/* Image load progress codes */
> +#define FPGA_IMAGE_PROG_IDLE 0
> +#define FPGA_IMAGE_PROG_STARTING 1
> +#define FPGA_IMAGE_PROG_PREPARING 2
> +#define FPGA_IMAGE_PROG_WRITING 3
> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
> +#define FPGA_IMAGE_PROG_MAX 5
> +
> +/* Image error progress codes */
> +#define FPGA_IMAGE_ERR_NONE 0
> +#define FPGA_IMAGE_ERR_HW_ERROR 1
> +#define FPGA_IMAGE_ERR_TIMEOUT 2
> +#define FPGA_IMAGE_ERR_CANCELED 3
> +#define FPGA_IMAGE_ERR_BUSY 4
> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
> +#define FPGA_IMAGE_ERR_RW_ERROR 6
> +#define FPGA_IMAGE_ERR_WEAROUT 7
> +#define FPGA_IMAGE_ERR_MAX 8
> +
> +/**
> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
> + * struct fpga_image_write)
> + *
> + * Upload a data buffer to the target device. The user must provide the
> + * data buffer and size.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct fpga_image_write {
> + /* Input */
> + __u32 flags; /* Zero for now */
> + __u32 size; /* Data size (in bytes) to be written */
> + __u64 buf; /* User space address of source data */
> +};
> +
> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
> +
> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
2021-09-24 8:12 ` Xu Yilun
@ 2021-09-24 21:32 ` Russ Weight
2021-09-26 1:10 ` Xu Yilun
0 siblings, 1 reply; 18+ messages in thread
From: Russ Weight @ 2021-09-24 21:32 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On 9/24/21 1:12 AM, Xu Yilun wrote:
> On Wed, Sep 22, 2021 at 05:10:53PM -0700, Russ Weight wrote:
>> Extend the FPGA Image Load framework to include IOCTL support
>> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
>> The IOCTL will return immediately, and the update will begin in the
>> context of a kernel worker thread.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v16:
>> - Shift from "Driver" terminology to "Framework" in comments and
>> documentation
>> - Rename lops to ops for structure member and local variables
>> - Change the write_blk() definition to pass in *blk_size (a pointer to
>> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
>> the maximum block size to stay within the limit of the data buffer).
>> The write_blk() op may use the default *blk_size or modify it to a
>> more optimal number for the given device, subject to the max_size limit.
>> - All enum values for progress and errors are changed to macros, because
>> they are included in the uapi header. This is done to maintain consistency
>> amongst the DFL related IOCTL header files. All references to the enum
>> types are changed to u32.
>> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
>> - Add a call to cond_resched() in the write_blk() loop to ensure that
>> we yield to higher priority tasks during long data transfers.
>> - Switch to the system_unbound_wq to enable calls to cond_resched().
>> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
>> imgld->opened.
>> - Change fpga_image_load_release() to block until the image upload is
>> complete.
>> - Remove the completion object, imgld->update_done, in favor of calling
>> flush_work(&imgld->work);
>> v15:
>> - Compare to previous patch:
>> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
>> - Changed file, symbol, and config names to reflect the new driver name
>> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
>> IOCTL for writing the image data to the FPGA card. The driver no longer
>> uses the request_firmware framework.
>> - Fixed some error return values in fpga_image_load_register()
>> - Removed signed-off/reviewed-by tags
>> v14:
>> - Added MAINTAINERS reference for
>> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
>> - Updated ABI documentation date and kernel version
>> - Updated copyright to 2021
>> v13:
>> - Change "if (count == 0 || " to "if (!count || "
>> - Improve error message: "Attempt to register without all required ops\n"
>> v12:
>> - Updated Date and KernelVersion fields in ABI documentation
>> - Removed size parameter from write_blk() op - it is now up to
>> the lower-level driver to determine the appropriate size and
>> to update smgr->remaining_size accordingly.
>> v11:
>> - Fixed a spelling error in a comment
>> - Initialize smgr->err_code and smgr->progress explicitly in
>> fpga_sec_mgr_create() instead of accepting the default 0 value.
>> 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
>> - Changed filename_store() to use kmemdup_nul() instead of
>> kstrndup() and changed the count to not assume a line-return.
>> v6:
>> - Changed "security update" to "secure update" in commit message
>> v5:
>> - When checking the return values for functions of type enum
>> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
>> 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:
>> - Removed unnecessary "goto done"
>> - Added a comment to explain imgr->driver_unload in
>> ifpga_sec_mgr_unregister()
>> v2:
>> - Bumped documentation date and version
>> - Removed explicit value assignments in enums
>> - Other minor code cleanup per review comments
>> ---
>> Documentation/fpga/fpga-image-load.rst | 26 ++-
>> MAINTAINERS | 1 +
>> drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
>> include/linux/fpga/fpga-image-load.h | 29 +++
>> include/uapi/linux/fpga-image-load.h | 54 ++++++
>> 5 files changed, 346 insertions(+), 2 deletions(-)
>> create mode 100644 include/uapi/linux/fpga-image-load.h
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index dda9283ef1c7..ba371c7c0ca0 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -7,4 +7,28 @@ FPGA Image Load Framework
>> The FPGA Image Load framework provides a common API for user-space
>> tools to manage image uploads to FPGA devices. Device drivers that
>> instantiate the FPGA Image Load framework will interact with the
>> -target device to transfer and authenticate the image data.
>> +target device to transfer and authenticate the image data. Image uploads
>> +are processed in the context of a kernel worker thread.
>> +
>> +User API
>> +========
>> +
>> +open
>> +----
>> +
>> +An fpga_image_load device is opened exclusively to control an image upload.
>> +The device must remain open throughout the duration of the image upload.
>> +An attempt to close the device while an upload is in progress will cause
>> +the upload to be cancelled. If unable to cancel the image upload, the close
>> +system call will block until the image upload is complete.
>> +
>> +ioctl
>> +-----
>> +
>> +FPGA_IMAGE_LOAD_WRITE:
>> +
>> +Start an image upload with the provided image buffer. This IOCTL returns
>> +immediately after starting a kernel worker thread to process the image
>> +upload which could take as long a 40 minutes depending on the actual device
>> +being updated. This is an exclusive operation; an attempt to start
>> +concurrent image uploads for the same device will fail with EBUSY.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6e312d0ddeb6..4e44f62eef33 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7372,6 +7372,7 @@ S: Maintained
>> F: Documentation/fpga/fpga-image-load.rst
>> F: drivers/fpga/fpga-image-load.c
>> F: include/linux/fpga/fpga-image-load.h
>> +F: include/uapi/linux/fpga-image-load.h
>>
>> FPU EMULATOR
>> M: Bill Metzenthen <billm@melbpc.org.au>
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index 799d18444f7c..65f553b59011 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -5,18 +5,210 @@
>> * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> */
>>
>> +#include <linux/delay.h>
>> #include <linux/fpga/fpga-image-load.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> #include <linux/vmalloc.h>
>>
>> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
>> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>>
>> static struct class *fpga_image_load_class;
>> +static dev_t fpga_image_devt;
>>
>> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>>
>> +#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
>> +
>> +static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
>> +{
>> + imgld->err_code = err_code;
>> + imgld->ops->cancel(imgld);
> Do we need the cancel here? Or cleanup is just fine.
>
> I see the description below:
>
> * @cancel: Required: Signal HW to cancel update
> * @cleanup: Optional: Complements the prepare()
> * function and is called at the completion
> * of the update, whether success or failure,
> * if the prepare function succeeded.
>
> My understanding is when error happens, the HW is already stopped,
> and may optionally needs a cleanup to become normal again.
>
> And cancel() is like interrupting an ongoing HW progress, which could
> be called when other callback is running.
>
> We can discuss on this.
Yes - I understand what you are saying I'll experiment with it.
In this context, cancel() is being called as a cleanup step.
>> +}
>> +
>> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>> +{
>> + mutex_lock(&imgld->lock);
>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> + mutex_unlock(&imgld->lock);
>> +}
>> +
>> +static void fpga_image_do_load(struct work_struct *work)
>> +{
>> + struct fpga_image_load *imgld;
>> + u32 ret, blk_size, offset = 0;
>> +
>> + imgld = container_of(work, struct fpga_image_load, work);
>> +
>> + if (imgld->driver_unload) {
>> + imgld->err_code = 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;
>> + goto putdev_exit;
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
>> + ret = imgld->ops->prepare(imgld);
>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>> + fpga_image_dev_error(imgld, ret);
>> + goto modput_exit;
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
>> + while (imgld->remaining_size) {
>> + /*
>> + * The write_blk() op has the option to use the blk_size
>> + * value provided here, or to modify it to something more
>> + * optimal for the given device.
>> + */
>> + blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
>> + ret = imgld->ops->write_blk(imgld, offset, &blk_size,
>> + imgld->remaining_size);
>> + if (ret != FPGA_IMAGE_ERR_NONE) {
>> + fpga_image_dev_error(imgld, ret);
>> + goto done;
>> + }
>> +
>> + imgld->remaining_size -= blk_size;
>> + offset += blk_size;
>> +
>> + /*
>> + * The class driver does not have control of the overall
>> + * size or the actual implementation of the write. Allow
>> + * for scheduling out.
>> + */
>> + cond_resched();
>> + }
>> +
>> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
>> + ret = imgld->ops->poll_complete(imgld);
>> + if (ret != FPGA_IMAGE_ERR_NONE)
>> + fpga_image_dev_error(imgld, ret);
>> +
>> +done:
>> + if (imgld->ops->cleanup)
>> + imgld->ops->cleanup(imgld);
>> +
>> +modput_exit:
>> + module_put(imgld->dev.parent->driver->owner);
>> +
>> +putdev_exit:
>> + put_device(&imgld->dev);
>> +
>> +idle_exit:
>> + /*
>> + * Note: imgld->remaining_size is left unmodified here to provide
>> + * additional information on errors. It will be reinitialized when
>> + * the next image load begins.
>> + */
>> + vfree(imgld->data);
>> + imgld->data = NULL;
>> + fpga_image_prog_complete(imgld);
>> +}
>> +
>> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> + unsigned long arg)
>> +{
>> + struct fpga_image_write wb;
>> + unsigned long minsz;
>> + u8 *buf;
>> +
>> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
>> + return -EBUSY;
>> +
>> + minsz = offsetofend(struct fpga_image_write, buf);
>> + if (copy_from_user(&wb, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (wb.flags)
>> + return -EINVAL;
>> +
>> + /* Enforce 32-bit alignment on the write data */
>> + if (wb.size & 0x3)
>> + return -EINVAL;
> Why we enforce the alignment? It seems to be the requirement of the
> low level driver. We may handle it there.
Sure - I can move this to the lower level driver.
Thanks,
- Russ
>
> Thanks,
> Yilun
>
>> +
>> + buf = vzalloc(wb.size);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
>> + vfree(buf);
>> + return -EFAULT;
>> + }
>> +
>> + imgld->data = buf;
>> + imgld->remaining_size = wb.size;
>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
>> + queue_work(system_unbound_wq, &imgld->work);
>> +
>> + 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;
>> +
>> + switch (cmd) {
>> + case FPGA_IMAGE_LOAD_WRITE:
>> + mutex_lock(&imgld->lock);
>> + ret = fpga_image_load_ioctl_write(imgld, arg);
>> + mutex_unlock(&imgld->lock);
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
>> +{
>> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
>> + struct fpga_image_load, cdev);
>> +
>> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
>> + return -EBUSY;
>> +
>> + filp->private_data = imgld;
>> +
>> + return 0;
>> +}
>> +
>> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
>> +{
>> + struct fpga_image_load *imgld = filp->private_data;
>> +
>> + mutex_lock(&imgld->lock);
>> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>> + mutex_unlock(&imgld->lock);
>> + goto close_exit;
>> + }
>> +
>> + mutex_unlock(&imgld->lock);
>> + flush_work(&imgld->work);
>> +
>> +close_exit:
>> + atomic_set(&imgld->opened, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct file_operations fpga_image_load_fops = {
>> + .owner = THIS_MODULE,
>> + .open = fpga_image_load_open,
>> + .release = fpga_image_load_release,
>> + .unlocked_ioctl = fpga_image_load_ioctl,
>> +};
>> +
>> /**
>> * fpga_image_load_register - create and register an FPGA Image Load Device
>> *
>> @@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
>> struct fpga_image_load *imgld;
>> int ret;
>>
>> + if (!ops || !ops->cancel || !ops->prepare ||
>> + !ops->write_blk || !ops->poll_complete) {
>> + dev_err(parent, "Attempt to register without all required ops\n");
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
>> if (!imgld)
>> return ERR_PTR(-ENOMEM);
>> @@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
>>
>> imgld->priv = priv;
>> imgld->ops = ops;
>> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> + INIT_WORK(&imgld->work, fpga_image_do_load);
>>
>> imgld->dev.class = fpga_image_load_class;
>> imgld->dev.parent = parent;
>> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>>
>> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
>> if (ret) {
>> @@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
>> return ERR_PTR(ret);
>> }
>>
>> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
>> + imgld->cdev.owner = parent->driver->owner;
>> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
> Could be replaced by cdev_set_parent()
>
>> +
>> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
>> + if (ret) {
>> + put_device(&imgld->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> return imgld;
>>
>> error_device:
>> @@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
>> * @imgld: pointer to struct fpga_image_load
>> *
>> * This function is intended for use in the parent driver's remove()
>> - * function.
>> + * function. The driver_unload flag prevents new updates from starting
>> + * once the unregister process has begun.
>> */
>> void fpga_image_load_unregister(struct fpga_image_load *imgld)
>> {
>> + mutex_lock(&imgld->lock);
>> + imgld->driver_unload = true;
>> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
>> + mutex_unlock(&imgld->lock);
>> + goto unregister;
>> + }
>> +
>> + mutex_unlock(&imgld->lock);
>> + flush_work(&imgld->work);
>> +
>> +unregister:
>> + cdev_del(&imgld->cdev);
>> device_unregister(&imgld->dev);
>> }
>> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
>> @@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>>
>> static int __init fpga_image_load_class_init(void)
>> {
>> + int ret;
>> pr_info("FPGA Image Load Framework\n");
>>
>> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
>> if (IS_ERR(fpga_image_load_class))
>> return PTR_ERR(fpga_image_load_class);
>>
>> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
>> + "fpga_image_load");
>> + if (ret)
>> + goto exit_destroy_class;
>> +
>> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>>
>> return 0;
>> +
>> +exit_destroy_class:
>> + class_destroy(fpga_image_load_class);
>> + return ret;
>> }
>>
>> static void __exit fpga_image_load_class_exit(void)
>> {
>> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
>> class_destroy(fpga_image_load_class);
>> WARN_ON(!xa_empty(&fpga_image_load_xa));
>> }
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 8b051c82ef5f..41ab63cf7b20 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -7,22 +7,51 @@
>> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
>> #define _LINUX_FPGA_IMAGE_LOAD_H
>>
>> +#include <linux/cdev.h>
>> #include <linux/device.h>
>> #include <linux/mutex.h>
>> #include <linux/types.h>
>> +#include <uapi/linux/fpga-image-load.h>
>>
>> struct fpga_image_load;
>>
>> /**
>> * struct fpga_image_load_ops - device specific operations
>> + * @prepare: Required: Prepare secure update
>> + * @write_blk: Required: Write a block of data. The class driver
>> + * provides a default block size. The write_blk() op
>> + * may choose to modify *blk_size to something more
>> + * optimal for the given device. *blk_size must be
>> + * less than or equal to max_size.
>> + * @poll_complete: Required: Check for the completion of the
>> + * HW authentication/programming process.
>> + * @cancel: Required: Signal HW to cancel update
>> + * @cleanup: Optional: Complements the prepare()
>> + * function and is called at the completion
>> + * of the update, whether success or failure,
>> + * if the prepare function succeeded.
>> */
>> struct fpga_image_load_ops {
>> + u32 (*prepare)(struct fpga_image_load *imgld);
>> + u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
>> + u32 *blk_size, u32 max_size);
>> + u32 (*poll_complete)(struct fpga_image_load *imgld);
>> + u32 (*cancel)(struct fpga_image_load *imgld);
>> + void (*cleanup)(struct fpga_image_load *imgld);
>> };
>>
>> struct fpga_image_load {
>> struct device dev;
>> + struct cdev cdev;
>> const struct fpga_image_load_ops *ops;
>> struct mutex lock; /* protect data structure contents */
>> + atomic_t opened;
>> + struct work_struct work;
>> + const u8 *data; /* pointer to update data */
>> + u32 remaining_size; /* size remaining to transfer */
>> + u32 progress;
>> + u32 err_code; /* image load error code */
>> + bool driver_unload;
>> void *priv;
>> };
>>
>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>> new file mode 100644
>> index 000000000000..0382078c5a6c
>> --- /dev/null
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -0,0 +1,54 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Header File for FPGA Image Load User API
>> + *
>> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
>> + *
>> + */
>> +
>> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioctl.h>
>> +
>> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
>> +
>> +/* Image load progress codes */
>> +#define FPGA_IMAGE_PROG_IDLE 0
>> +#define FPGA_IMAGE_PROG_STARTING 1
>> +#define FPGA_IMAGE_PROG_PREPARING 2
>> +#define FPGA_IMAGE_PROG_WRITING 3
>> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
>> +#define FPGA_IMAGE_PROG_MAX 5
>> +
>> +/* Image error progress codes */
>> +#define FPGA_IMAGE_ERR_NONE 0
>> +#define FPGA_IMAGE_ERR_HW_ERROR 1
>> +#define FPGA_IMAGE_ERR_TIMEOUT 2
>> +#define FPGA_IMAGE_ERR_CANCELED 3
>> +#define FPGA_IMAGE_ERR_BUSY 4
>> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
>> +#define FPGA_IMAGE_ERR_RW_ERROR 6
>> +#define FPGA_IMAGE_ERR_WEAROUT 7
>> +#define FPGA_IMAGE_ERR_MAX 8
>> +
>> +/**
>> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
>> + * struct fpga_image_write)
>> + *
>> + * Upload a data buffer to the target device. The user must provide the
>> + * data buffer and size.
>> + *
>> + * Return: 0 on success, -errno on failure.
>> + */
>> +struct fpga_image_write {
>> + /* Input */
>> + __u32 flags; /* Zero for now */
>> + __u32 size; /* Data size (in bytes) to be written */
>> + __u64 buf; /* User space address of source data */
>> +};
>> +
>> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>> +
>> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
2021-09-24 21:32 ` Russ Weight
@ 2021-09-26 1:10 ` Xu Yilun
2021-09-27 16:33 ` Russ Weight
0 siblings, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2021-09-26 1:10 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On Fri, Sep 24, 2021 at 02:32:25PM -0700, Russ Weight wrote:
>
>
> On 9/24/21 1:12 AM, Xu Yilun wrote:
> > On Wed, Sep 22, 2021 at 05:10:53PM -0700, Russ Weight wrote:
> >> Extend the FPGA Image Load framework to include IOCTL support
> >> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
> >> The IOCTL will return immediately, and the update will begin in the
> >> context of a kernel worker thread.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> >> ---
> >> v16:
> >> - Shift from "Driver" terminology to "Framework" in comments and
> >> documentation
> >> - Rename lops to ops for structure member and local variables
> >> - Change the write_blk() definition to pass in *blk_size (a pointer to
> >> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
> >> the maximum block size to stay within the limit of the data buffer).
> >> The write_blk() op may use the default *blk_size or modify it to a
> >> more optimal number for the given device, subject to the max_size limit.
> >> - All enum values for progress and errors are changed to macros, because
> >> they are included in the uapi header. This is done to maintain consistency
> >> amongst the DFL related IOCTL header files. All references to the enum
> >> types are changed to u32.
> >> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
> >> - Add a call to cond_resched() in the write_blk() loop to ensure that
> >> we yield to higher priority tasks during long data transfers.
> >> - Switch to the system_unbound_wq to enable calls to cond_resched().
> >> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
> >> imgld->opened.
> >> - Change fpga_image_load_release() to block until the image upload is
> >> complete.
> >> - Remove the completion object, imgld->update_done, in favor of calling
> >> flush_work(&imgld->work);
> >> v15:
> >> - Compare to previous patch:
> >> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
> >> - Changed file, symbol, and config names to reflect the new driver name
> >> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
> >> IOCTL for writing the image data to the FPGA card. The driver no longer
> >> uses the request_firmware framework.
> >> - Fixed some error return values in fpga_image_load_register()
> >> - Removed signed-off/reviewed-by tags
> >> v14:
> >> - Added MAINTAINERS reference for
> >> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> >> - Updated ABI documentation date and kernel version
> >> - Updated copyright to 2021
> >> v13:
> >> - Change "if (count == 0 || " to "if (!count || "
> >> - Improve error message: "Attempt to register without all required ops\n"
> >> v12:
> >> - Updated Date and KernelVersion fields in ABI documentation
> >> - Removed size parameter from write_blk() op - it is now up to
> >> the lower-level driver to determine the appropriate size and
> >> to update smgr->remaining_size accordingly.
> >> v11:
> >> - Fixed a spelling error in a comment
> >> - Initialize smgr->err_code and smgr->progress explicitly in
> >> fpga_sec_mgr_create() instead of accepting the default 0 value.
> >> 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
> >> - Changed filename_store() to use kmemdup_nul() instead of
> >> kstrndup() and changed the count to not assume a line-return.
> >> v6:
> >> - Changed "security update" to "secure update" in commit message
> >> v5:
> >> - When checking the return values for functions of type enum
> >> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
> >> 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:
> >> - Removed unnecessary "goto done"
> >> - Added a comment to explain imgr->driver_unload in
> >> ifpga_sec_mgr_unregister()
> >> v2:
> >> - Bumped documentation date and version
> >> - Removed explicit value assignments in enums
> >> - Other minor code cleanup per review comments
> >> ---
> >> Documentation/fpga/fpga-image-load.rst | 26 ++-
> >> MAINTAINERS | 1 +
> >> drivers/fpga/fpga-image-load.c | 238 ++++++++++++++++++++++++-
> >> include/linux/fpga/fpga-image-load.h | 29 +++
> >> include/uapi/linux/fpga-image-load.h | 54 ++++++
> >> 5 files changed, 346 insertions(+), 2 deletions(-)
> >> create mode 100644 include/uapi/linux/fpga-image-load.h
> >>
> >> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> >> index dda9283ef1c7..ba371c7c0ca0 100644
> >> --- a/Documentation/fpga/fpga-image-load.rst
> >> +++ b/Documentation/fpga/fpga-image-load.rst
> >> @@ -7,4 +7,28 @@ FPGA Image Load Framework
> >> The FPGA Image Load framework provides a common API for user-space
> >> tools to manage image uploads to FPGA devices. Device drivers that
> >> instantiate the FPGA Image Load framework will interact with the
> >> -target device to transfer and authenticate the image data.
> >> +target device to transfer and authenticate the image data. Image uploads
> >> +are processed in the context of a kernel worker thread.
> >> +
> >> +User API
> >> +========
> >> +
> >> +open
> >> +----
> >> +
> >> +An fpga_image_load device is opened exclusively to control an image upload.
> >> +The device must remain open throughout the duration of the image upload.
> >> +An attempt to close the device while an upload is in progress will cause
> >> +the upload to be cancelled. If unable to cancel the image upload, the close
> >> +system call will block until the image upload is complete.
> >> +
> >> +ioctl
> >> +-----
> >> +
> >> +FPGA_IMAGE_LOAD_WRITE:
> >> +
> >> +Start an image upload with the provided image buffer. This IOCTL returns
> >> +immediately after starting a kernel worker thread to process the image
> >> +upload which could take as long a 40 minutes depending on the actual device
> >> +being updated. This is an exclusive operation; an attempt to start
> >> +concurrent image uploads for the same device will fail with EBUSY.
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6e312d0ddeb6..4e44f62eef33 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -7372,6 +7372,7 @@ S: Maintained
> >> F: Documentation/fpga/fpga-image-load.rst
> >> F: drivers/fpga/fpga-image-load.c
> >> F: include/linux/fpga/fpga-image-load.h
> >> +F: include/uapi/linux/fpga-image-load.h
> >>
> >> FPU EMULATOR
> >> M: Bill Metzenthen <billm@melbpc.org.au>
> >> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> >> index 799d18444f7c..65f553b59011 100644
> >> --- a/drivers/fpga/fpga-image-load.c
> >> +++ b/drivers/fpga/fpga-image-load.c
> >> @@ -5,18 +5,210 @@
> >> * Copyright (C) 2019-2021 Intel Corporation, Inc.
> >> */
> >>
> >> +#include <linux/delay.h>
> >> #include <linux/fpga/fpga-image-load.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/kernel.h>
> >> #include <linux/module.h>
> >> #include <linux/slab.h>
> >> +#include <linux/uaccess.h>
> >> #include <linux/vmalloc.h>
> >>
> >> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
> >> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
> >>
> >> static struct class *fpga_image_load_class;
> >> +static dev_t fpga_image_devt;
> >>
> >> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
> >>
> >> +#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
> >> +
> >> +static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
> >> +{
> >> + imgld->err_code = err_code;
> >> + imgld->ops->cancel(imgld);
> > Do we need the cancel here? Or cleanup is just fine.
> >
> > I see the description below:
> >
> > * @cancel: Required: Signal HW to cancel update
> > * @cleanup: Optional: Complements the prepare()
> > * function and is called at the completion
> > * of the update, whether success or failure,
> > * if the prepare function succeeded.
> >
> > My understanding is when error happens, the HW is already stopped,
> > and may optionally needs a cleanup to become normal again.
> >
> > And cancel() is like interrupting an ongoing HW progress, which could
> > be called when other callback is running.
> >
> > We can discuss on this.
> Yes - I understand what you are saying I'll experiment with it.
> In this context, cancel() is being called as a cleanup step.
>
> >> +}
> >> +
> >> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> >> +{
> >> + mutex_lock(&imgld->lock);
> >> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> >> + mutex_unlock(&imgld->lock);
> >> +}
> >> +
> >> +static void fpga_image_do_load(struct work_struct *work)
> >> +{
> >> + struct fpga_image_load *imgld;
> >> + u32 ret, blk_size, offset = 0;
> >> +
> >> + imgld = container_of(work, struct fpga_image_load, work);
> >> +
> >> + if (imgld->driver_unload) {
> >> + imgld->err_code = 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;
> >> + goto putdev_exit;
> >> + }
> >> +
> >> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
> >> + ret = imgld->ops->prepare(imgld);
> >> + if (ret != FPGA_IMAGE_ERR_NONE) {
> >> + fpga_image_dev_error(imgld, ret);
> >> + goto modput_exit;
> >> + }
> >> +
> >> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
> >> + while (imgld->remaining_size) {
> >> + /*
> >> + * The write_blk() op has the option to use the blk_size
> >> + * value provided here, or to modify it to something more
> >> + * optimal for the given device.
> >> + */
> >> + blk_size = min_t(u32, WRITE_BLOCK_SIZE, imgld->remaining_size);
> >> + ret = imgld->ops->write_blk(imgld, offset, &blk_size,
> >> + imgld->remaining_size);
> >> + if (ret != FPGA_IMAGE_ERR_NONE) {
> >> + fpga_image_dev_error(imgld, ret);
> >> + goto done;
> >> + }
> >> +
> >> + imgld->remaining_size -= blk_size;
> >> + offset += blk_size;
> >> +
> >> + /*
> >> + * The class driver does not have control of the overall
> >> + * size or the actual implementation of the write. Allow
> >> + * for scheduling out.
> >> + */
> >> + cond_resched();
> >> + }
> >> +
> >> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
> >> + ret = imgld->ops->poll_complete(imgld);
> >> + if (ret != FPGA_IMAGE_ERR_NONE)
> >> + fpga_image_dev_error(imgld, ret);
> >> +
> >> +done:
> >> + if (imgld->ops->cleanup)
> >> + imgld->ops->cleanup(imgld);
> >> +
> >> +modput_exit:
> >> + module_put(imgld->dev.parent->driver->owner);
> >> +
> >> +putdev_exit:
> >> + put_device(&imgld->dev);
> >> +
> >> +idle_exit:
> >> + /*
> >> + * Note: imgld->remaining_size is left unmodified here to provide
> >> + * additional information on errors. It will be reinitialized when
> >> + * the next image load begins.
> >> + */
> >> + vfree(imgld->data);
> >> + imgld->data = NULL;
> >> + fpga_image_prog_complete(imgld);
> >> +}
> >> +
> >> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> + unsigned long arg)
> >> +{
> >> + struct fpga_image_write wb;
> >> + unsigned long minsz;
> >> + u8 *buf;
> >> +
> >> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> >> + return -EBUSY;
> >> +
> >> + minsz = offsetofend(struct fpga_image_write, buf);
> >> + if (copy_from_user(&wb, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (wb.flags)
> >> + return -EINVAL;
> >> +
> >> + /* Enforce 32-bit alignment on the write data */
> >> + if (wb.size & 0x3)
> >> + return -EINVAL;
> > Why we enforce the alignment? It seems to be the requirement of the
> > low level driver. We may handle it there.
> Sure - I can move this to the lower level driver.
>
> Thanks,
> - Russ
> >
> > Thanks,
> > Yilun
> >
> >> +
> >> + buf = vzalloc(wb.size);
> >> + if (!buf)
> >> + return -ENOMEM;
> >> +
> >> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> >> + vfree(buf);
> >> + return -EFAULT;
> >> + }
> >> +
> >> + imgld->data = buf;
> >> + imgld->remaining_size = wb.size;
> >> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> >> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
> >> + queue_work(system_unbound_wq, &imgld->work);
> >> +
> >> + 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;
> >> +
> >> + switch (cmd) {
> >> + case FPGA_IMAGE_LOAD_WRITE:
> >> + mutex_lock(&imgld->lock);
> >> + ret = fpga_image_load_ioctl_write(imgld, arg);
> >> + mutex_unlock(&imgld->lock);
> >> + break;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
> >> +{
> >> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
> >> + struct fpga_image_load, cdev);
> >> +
> >> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
> >> + return -EBUSY;
> >> +
> >> + filp->private_data = imgld;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
> >> +{
> >> + struct fpga_image_load *imgld = filp->private_data;
> >> +
> >> + mutex_lock(&imgld->lock);
> >> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> >> + mutex_unlock(&imgld->lock);
> >> + goto close_exit;
> >> + }
> >> +
> >> + mutex_unlock(&imgld->lock);
> >> + flush_work(&imgld->work);
> >> +
> >> +close_exit:
> >> + atomic_set(&imgld->opened, 0);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct file_operations fpga_image_load_fops = {
> >> + .owner = THIS_MODULE,
> >> + .open = fpga_image_load_open,
> >> + .release = fpga_image_load_release,
> >> + .unlocked_ioctl = fpga_image_load_ioctl,
> >> +};
> >> +
> >> /**
> >> * fpga_image_load_register - create and register an FPGA Image Load Device
> >> *
> >> @@ -35,6 +227,12 @@ fpga_image_load_register(struct device *parent,
> >> struct fpga_image_load *imgld;
> >> int ret;
> >>
> >> + if (!ops || !ops->cancel || !ops->prepare ||
> >> + !ops->write_blk || !ops->poll_complete) {
> >> + dev_err(parent, "Attempt to register without all required ops\n");
> >> + return ERR_PTR(-ENOMEM);
> >> + }
> >> +
> >> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
> >> if (!imgld)
> >> return ERR_PTR(-ENOMEM);
> >> @@ -48,9 +246,13 @@ fpga_image_load_register(struct device *parent,
> >>
> >> imgld->priv = priv;
> >> imgld->ops = ops;
> >> + imgld->err_code = FPGA_IMAGE_ERR_NONE;
> >> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> >> + INIT_WORK(&imgld->work, fpga_image_do_load);
> >>
> >> imgld->dev.class = fpga_image_load_class;
> >> imgld->dev.parent = parent;
> >> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
> >>
> >> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
> >> if (ret) {
> >> @@ -65,6 +267,16 @@ fpga_image_load_register(struct device *parent,
> >> return ERR_PTR(ret);
> >> }
> >>
> >> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
> >> + imgld->cdev.owner = parent->driver->owner;
> >> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
> > Could be replaced by cdev_set_parent()
Sorry I ended last mail before this comment, and you may not notice it.
Thanks,
Yilun
> >
> >> +
> >> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
> >> + if (ret) {
> >> + put_device(&imgld->dev);
> >> + return ERR_PTR(ret);
> >> + }
> >> +
> >> return imgld;
> >>
> >> error_device:
> >> @@ -83,10 +295,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
> >> * @imgld: pointer to struct fpga_image_load
> >> *
> >> * This function is intended for use in the parent driver's remove()
> >> - * function.
> >> + * function. The driver_unload flag prevents new updates from starting
> >> + * once the unregister process has begun.
> >> */
> >> void fpga_image_load_unregister(struct fpga_image_load *imgld)
> >> {
> >> + mutex_lock(&imgld->lock);
> >> + imgld->driver_unload = true;
> >> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> >> + mutex_unlock(&imgld->lock);
> >> + goto unregister;
> >> + }
> >> +
> >> + mutex_unlock(&imgld->lock);
> >> + flush_work(&imgld->work);
> >> +
> >> +unregister:
> >> + cdev_del(&imgld->cdev);
> >> device_unregister(&imgld->dev);
> >> }
> >> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
> >> @@ -101,19 +326,30 @@ static void fpga_image_load_dev_release(struct device *dev)
> >>
> >> static int __init fpga_image_load_class_init(void)
> >> {
> >> + int ret;
> >> pr_info("FPGA Image Load Framework\n");
> >>
> >> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
> >> if (IS_ERR(fpga_image_load_class))
> >> return PTR_ERR(fpga_image_load_class);
> >>
> >> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
> >> + "fpga_image_load");
> >> + if (ret)
> >> + goto exit_destroy_class;
> >> +
> >> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
> >>
> >> return 0;
> >> +
> >> +exit_destroy_class:
> >> + class_destroy(fpga_image_load_class);
> >> + return ret;
> >> }
> >>
> >> static void __exit fpga_image_load_class_exit(void)
> >> {
> >> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
> >> class_destroy(fpga_image_load_class);
> >> WARN_ON(!xa_empty(&fpga_image_load_xa));
> >> }
> >> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> >> index 8b051c82ef5f..41ab63cf7b20 100644
> >> --- a/include/linux/fpga/fpga-image-load.h
> >> +++ b/include/linux/fpga/fpga-image-load.h
> >> @@ -7,22 +7,51 @@
> >> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
> >> #define _LINUX_FPGA_IMAGE_LOAD_H
> >>
> >> +#include <linux/cdev.h>
> >> #include <linux/device.h>
> >> #include <linux/mutex.h>
> >> #include <linux/types.h>
> >> +#include <uapi/linux/fpga-image-load.h>
> >>
> >> struct fpga_image_load;
> >>
> >> /**
> >> * struct fpga_image_load_ops - device specific operations
> >> + * @prepare: Required: Prepare secure update
> >> + * @write_blk: Required: Write a block of data. The class driver
> >> + * provides a default block size. The write_blk() op
> >> + * may choose to modify *blk_size to something more
> >> + * optimal for the given device. *blk_size must be
> >> + * less than or equal to max_size.
> >> + * @poll_complete: Required: Check for the completion of the
> >> + * HW authentication/programming process.
> >> + * @cancel: Required: Signal HW to cancel update
> >> + * @cleanup: Optional: Complements the prepare()
> >> + * function and is called at the completion
> >> + * of the update, whether success or failure,
> >> + * if the prepare function succeeded.
> >> */
> >> struct fpga_image_load_ops {
> >> + u32 (*prepare)(struct fpga_image_load *imgld);
> >> + u32 (*write_blk)(struct fpga_image_load *imgld, u32 offset,
> >> + u32 *blk_size, u32 max_size);
> >> + u32 (*poll_complete)(struct fpga_image_load *imgld);
> >> + u32 (*cancel)(struct fpga_image_load *imgld);
> >> + void (*cleanup)(struct fpga_image_load *imgld);
> >> };
> >>
> >> struct fpga_image_load {
> >> struct device dev;
> >> + struct cdev cdev;
> >> const struct fpga_image_load_ops *ops;
> >> struct mutex lock; /* protect data structure contents */
> >> + atomic_t opened;
> >> + struct work_struct work;
> >> + const u8 *data; /* pointer to update data */
> >> + u32 remaining_size; /* size remaining to transfer */
> >> + u32 progress;
> >> + u32 err_code; /* image load error code */
> >> + bool driver_unload;
> >> void *priv;
> >> };
> >>
> >> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
> >> new file mode 100644
> >> index 000000000000..0382078c5a6c
> >> --- /dev/null
> >> +++ b/include/uapi/linux/fpga-image-load.h
> >> @@ -0,0 +1,54 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +/*
> >> + * Header File for FPGA Image Load User API
> >> + *
> >> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
> >> + *
> >> + */
> >> +
> >> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> >> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/ioctl.h>
> >> +
> >> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
> >> +
> >> +/* Image load progress codes */
> >> +#define FPGA_IMAGE_PROG_IDLE 0
> >> +#define FPGA_IMAGE_PROG_STARTING 1
> >> +#define FPGA_IMAGE_PROG_PREPARING 2
> >> +#define FPGA_IMAGE_PROG_WRITING 3
> >> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
> >> +#define FPGA_IMAGE_PROG_MAX 5
> >> +
> >> +/* Image error progress codes */
> >> +#define FPGA_IMAGE_ERR_NONE 0
> >> +#define FPGA_IMAGE_ERR_HW_ERROR 1
> >> +#define FPGA_IMAGE_ERR_TIMEOUT 2
> >> +#define FPGA_IMAGE_ERR_CANCELED 3
> >> +#define FPGA_IMAGE_ERR_BUSY 4
> >> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
> >> +#define FPGA_IMAGE_ERR_RW_ERROR 6
> >> +#define FPGA_IMAGE_ERR_WEAROUT 7
> >> +#define FPGA_IMAGE_ERR_MAX 8
> >> +
> >> +/**
> >> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
> >> + * struct fpga_image_write)
> >> + *
> >> + * Upload a data buffer to the target device. The user must provide the
> >> + * data buffer and size.
> >> + *
> >> + * Return: 0 on success, -errno on failure.
> >> + */
> >> +struct fpga_image_write {
> >> + /* Input */
> >> + __u32 flags; /* Zero for now */
> >> + __u32 size; /* Data size (in bytes) to be written */
> >> + __u64 buf; /* User space address of source data */
> >> +};
> >> +
> >> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
> >> +
> >> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
> >> --
> >> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 2/5] fpga: image-load: enable image uploads
2021-09-26 1:10 ` Xu Yilun
@ 2021-09-27 16:33 ` Russ Weight
0 siblings, 0 replies; 18+ messages in thread
From: Russ Weight @ 2021-09-27 16:33 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On 9/25/21 6:10 PM, Xu Yilun wrote:
>>>>
>>>> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
>>>> + imgld->cdev.owner = parent->driver->owner;
>>>> + imgld->cdev.kobj.parent = &imgld->dev.kobj;
>>> Could be replaced by cdev_set_parent()
> Sorry I ended last mail before this comment, and you may not notice it.
Yes - I did miss that. Thanks!
- Russ
>
> Thanks,
> Yilun
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 3/5] fpga: image-load: signal eventfd when complete
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 0:10 ` Russ Weight
2021-09-24 8:49 ` 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
4 siblings, 1 reply; 18+ messages in thread
From: Russ Weight @ 2021-09-23 0:10 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, Russ Weight
Amend the FPGA_IMAGE_LOAD_WRITE IOCTL implementation to include an
eventfd file descriptor as a parameter. The eventfd will be triggered
when the image upload completes.
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
v16:
- Some cleanup of documentation for the FPGA_IMAGE_LOAD_WRITE IOCTL.
v15:
- This patch is new to the patch-set, and adds an eventfd to the
FPGA_IMAGE_LOAD_WRITE IOCTL. The eventfd is signalled upon completion
of an update.
---
Documentation/fpga/fpga-image-load.rst | 12 +++++++-----
drivers/fpga/fpga-image-load.c | 22 ++++++++++++++++++++--
include/linux/fpga/fpga-image-load.h | 2 ++
include/uapi/linux/fpga-image-load.h | 3 ++-
4 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
index ba371c7c0ca0..22a455421bb4 100644
--- a/Documentation/fpga/fpga-image-load.rst
+++ b/Documentation/fpga/fpga-image-load.rst
@@ -27,8 +27,10 @@ ioctl
FPGA_IMAGE_LOAD_WRITE:
-Start an image upload with the provided image buffer. This IOCTL returns
-immediately after starting a kernel worker thread to process the image
-upload which could take as long a 40 minutes depending on the actual device
-being updated. This is an exclusive operation; an attempt to start
-concurrent image uploads for the same device will fail with EBUSY.
+Start an image upload with the provided image buffer. This IOCTL returns
+immediately after starting a kernel worker thread to process the image
+upload which could take as long a 40 minutes depending on the actual device
+being updated. This is an exclusive operation; an attempt to start
+concurrent image loads 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.
diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
index 65f553b59011..09164a0258a5 100644
--- a/drivers/fpga/fpga-image-load.c
+++ b/drivers/fpga/fpga-image-load.c
@@ -34,6 +34,7 @@ static void fpga_image_prog_complete(struct fpga_image_load *imgld)
{
mutex_lock(&imgld->lock);
imgld->progress = FPGA_IMAGE_PROG_IDLE;
+ eventfd_signal(imgld->finished, 1);
mutex_unlock(&imgld->lock);
}
@@ -112,6 +113,8 @@ static void fpga_image_do_load(struct work_struct *work)
vfree(imgld->data);
imgld->data = NULL;
fpga_image_prog_complete(imgld);
+ eventfd_ctx_put(imgld->finished);
+ imgld->finished = NULL;
}
static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
@@ -119,6 +122,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
{
struct fpga_image_write wb;
unsigned long minsz;
+ int ret;
u8 *buf;
if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
@@ -135,13 +139,23 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
if (wb.size & 0x3)
return -EINVAL;
+ if (wb.evtfd < 0)
+ return -EINVAL;
+
buf = vzalloc(wb.size);
if (!buf)
return -ENOMEM;
if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
- vfree(buf);
- return -EFAULT;
+ ret = -EFAULT;
+ goto exit_free;
+ }
+
+ imgld->finished = eventfd_ctx_fdget(wb.evtfd);
+ if (IS_ERR(imgld->finished)) {
+ ret = PTR_ERR(imgld->finished);
+ imgld->finished = NULL;
+ goto exit_free;
}
imgld->data = buf;
@@ -151,6 +165,10 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
queue_work(system_unbound_wq, &imgld->work);
return 0;
+
+exit_free:
+ vfree(buf);
+ return ret;
}
static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
index 41ab63cf7b20..7d39daa4d921 100644
--- a/include/linux/fpga/fpga-image-load.h
+++ b/include/linux/fpga/fpga-image-load.h
@@ -9,6 +9,7 @@
#include <linux/cdev.h>
#include <linux/device.h>
+#include <linux/eventfd.h>
#include <linux/mutex.h>
#include <linux/types.h>
#include <uapi/linux/fpga-image-load.h>
@@ -52,6 +53,7 @@ struct fpga_image_load {
u32 progress;
u32 err_code; /* image load error code */
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 0382078c5a6c..152a8e1c031f 100644
--- a/include/uapi/linux/fpga-image-load.h
+++ b/include/uapi/linux/fpga-image-load.h
@@ -38,7 +38,7 @@
* struct fpga_image_write)
*
* Upload a data buffer to the target device. The user must provide the
- * data buffer and size.
+ * data buffer, size, and an eventfd file descriptor.
*
* Return: 0 on success, -errno on failure.
*/
@@ -46,6 +46,7 @@ struct fpga_image_write {
/* Input */
__u32 flags; /* Zero for now */
__u32 size; /* Data size (in bytes) to be written */
+ __s32 evtfd; /* File descriptor for completion signal */
__u64 buf; /* User space address of source data */
};
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v16 3/5] fpga: image-load: signal eventfd when complete
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
0 siblings, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2021-09-24 8:49 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On Wed, Sep 22, 2021 at 05:10:54PM -0700, Russ Weight wrote:
> Amend the FPGA_IMAGE_LOAD_WRITE IOCTL implementation to include an
> eventfd file descriptor as a parameter. The eventfd will be triggered
> when the image upload completes.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v16:
> - Some cleanup of documentation for the FPGA_IMAGE_LOAD_WRITE IOCTL.
> v15:
> - This patch is new to the patch-set, and adds an eventfd to the
> FPGA_IMAGE_LOAD_WRITE IOCTL. The eventfd is signalled upon completion
> of an update.
> ---
> Documentation/fpga/fpga-image-load.rst | 12 +++++++-----
> drivers/fpga/fpga-image-load.c | 22 ++++++++++++++++++++--
> include/linux/fpga/fpga-image-load.h | 2 ++
> include/uapi/linux/fpga-image-load.h | 3 ++-
> 4 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> index ba371c7c0ca0..22a455421bb4 100644
> --- a/Documentation/fpga/fpga-image-load.rst
> +++ b/Documentation/fpga/fpga-image-load.rst
> @@ -27,8 +27,10 @@ ioctl
>
> FPGA_IMAGE_LOAD_WRITE:
>
> -Start an image upload with the provided image buffer. This IOCTL returns
> -immediately after starting a kernel worker thread to process the image
> -upload which could take as long a 40 minutes depending on the actual device
> -being updated. This is an exclusive operation; an attempt to start
> -concurrent image uploads for the same device will fail with EBUSY.
> +Start an image upload with the provided image buffer. This IOCTL returns
> +immediately after starting a kernel worker thread to process the image
> +upload which could take as long a 40 minutes depending on the actual device
> +being updated. This is an exclusive operation; an attempt to start
Just curious, there are marks here but seems no change.
> +concurrent image loads for the same device will fail with EBUSY. An eventfd
You want to fix "uploads" to "loads"? But there are many other "upload(s)".
Others look good to me.
> +file descriptor parameter is provided to this IOCTL. It will be signalled
> +at the completion of the image upload.
> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> index 65f553b59011..09164a0258a5 100644
> --- a/drivers/fpga/fpga-image-load.c
> +++ b/drivers/fpga/fpga-image-load.c
> @@ -34,6 +34,7 @@ static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> {
> mutex_lock(&imgld->lock);
> imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + eventfd_signal(imgld->finished, 1);
> mutex_unlock(&imgld->lock);
> }
>
> @@ -112,6 +113,8 @@ static void fpga_image_do_load(struct work_struct *work)
> vfree(imgld->data);
> imgld->data = NULL;
> fpga_image_prog_complete(imgld);
> + eventfd_ctx_put(imgld->finished);
> + imgld->finished = NULL;
> }
>
> static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> @@ -119,6 +122,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> {
> struct fpga_image_write wb;
> unsigned long minsz;
> + int ret;
> u8 *buf;
>
> if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> @@ -135,13 +139,23 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> if (wb.size & 0x3)
> return -EINVAL;
>
> + if (wb.evtfd < 0)
> + return -EINVAL;
> +
> buf = vzalloc(wb.size);
> if (!buf)
> return -ENOMEM;
>
> if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> - vfree(buf);
> - return -EFAULT;
> + ret = -EFAULT;
> + goto exit_free;
> + }
> +
> + imgld->finished = eventfd_ctx_fdget(wb.evtfd);
> + if (IS_ERR(imgld->finished)) {
> + ret = PTR_ERR(imgld->finished);
> + imgld->finished = NULL;
> + goto exit_free;
> }
>
> imgld->data = buf;
> @@ -151,6 +165,10 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> queue_work(system_unbound_wq, &imgld->work);
>
> return 0;
> +
> +exit_free:
> + vfree(buf);
> + return ret;
> }
>
> static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> index 41ab63cf7b20..7d39daa4d921 100644
> --- a/include/linux/fpga/fpga-image-load.h
> +++ b/include/linux/fpga/fpga-image-load.h
> @@ -9,6 +9,7 @@
>
> #include <linux/cdev.h>
> #include <linux/device.h>
> +#include <linux/eventfd.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> #include <uapi/linux/fpga-image-load.h>
> @@ -52,6 +53,7 @@ struct fpga_image_load {
> u32 progress;
> u32 err_code; /* image load error code */
> 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 0382078c5a6c..152a8e1c031f 100644
> --- a/include/uapi/linux/fpga-image-load.h
> +++ b/include/uapi/linux/fpga-image-load.h
> @@ -38,7 +38,7 @@
> * struct fpga_image_write)
> *
> * Upload a data buffer to the target device. The user must provide the
> - * data buffer and size.
> + * data buffer, size, and an eventfd file descriptor.
> *
> * Return: 0 on success, -errno on failure.
> */
> @@ -46,6 +46,7 @@ struct fpga_image_write {
> /* Input */
> __u32 flags; /* Zero for now */
> __u32 size; /* Data size (in bytes) to be written */
> + __s32 evtfd; /* File descriptor for completion signal */
> __u64 buf; /* User space address of source data */
> };
>
> --
> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 3/5] fpga: image-load: signal eventfd when complete
2021-09-24 8:49 ` Xu Yilun
@ 2021-09-24 21:45 ` Russ Weight
2021-09-26 1:06 ` Xu Yilun
0 siblings, 1 reply; 18+ messages in thread
From: Russ Weight @ 2021-09-24 21:45 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On 9/24/21 1:49 AM, Xu Yilun wrote:
> On Wed, Sep 22, 2021 at 05:10:54PM -0700, Russ Weight wrote:
>> Amend the FPGA_IMAGE_LOAD_WRITE IOCTL implementation to include an
>> eventfd file descriptor as a parameter. The eventfd will be triggered
>> when the image upload completes.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>> v16:
>> - Some cleanup of documentation for the FPGA_IMAGE_LOAD_WRITE IOCTL.
>> v15:
>> - This patch is new to the patch-set, and adds an eventfd to the
>> FPGA_IMAGE_LOAD_WRITE IOCTL. The eventfd is signalled upon completion
>> of an update.
>> ---
>> Documentation/fpga/fpga-image-load.rst | 12 +++++++-----
>> drivers/fpga/fpga-image-load.c | 22 ++++++++++++++++++++--
>> include/linux/fpga/fpga-image-load.h | 2 ++
>> include/uapi/linux/fpga-image-load.h | 3 ++-
>> 4 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index ba371c7c0ca0..22a455421bb4 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -27,8 +27,10 @@ ioctl
>>
>> FPGA_IMAGE_LOAD_WRITE:
>>
>> -Start an image upload with the provided image buffer. This IOCTL returns
>> -immediately after starting a kernel worker thread to process the image
>> -upload which could take as long a 40 minutes depending on the actual device
>> -being updated. This is an exclusive operation; an attempt to start
>> -concurrent image uploads for the same device will fail with EBUSY.
>> +Start an image upload with the provided image buffer. This IOCTL returns
>> +immediately after starting a kernel worker thread to process the image
>> +upload which could take as long a 40 minutes depending on the actual device
>> +being updated. This is an exclusive operation; an attempt to start
> Just curious, there are marks here but seems no change.
Hmmm - sometimes when fixing up patches, I find it easier to edit
the patch files (e.g. changing file and symbol names). The patches
applied cleanly, but I must have forgotten to regenerate them
before sending them for review.
>
>> +concurrent image loads for the same device will fail with EBUSY. An eventfd
> You want to fix "uploads" to "loads"? But there are many other "upload(s)".
My preference is upload, because load implies more than transferring
the data but the data transfer is all that this driver really does.
I preferred fpga_image_load to fpga_image_upload for the device name,
just because it was a couple of characters less. I'll change "loads"
to "uploads" here.
Do you think I should rename the driver to fpga_image_upload? Or do
you think the name is OK if it is probably documented?
- Russ
>
> Others look good to me.
>
>> +file descriptor parameter is provided to this IOCTL. It will be signalled
>> +at the completion of the image upload.
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index 65f553b59011..09164a0258a5 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -34,6 +34,7 @@ static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>> {
>> mutex_lock(&imgld->lock);
>> imgld->progress = FPGA_IMAGE_PROG_IDLE;
>> + eventfd_signal(imgld->finished, 1);
>> mutex_unlock(&imgld->lock);
>> }
>>
>> @@ -112,6 +113,8 @@ static void fpga_image_do_load(struct work_struct *work)
>> vfree(imgld->data);
>> imgld->data = NULL;
>> fpga_image_prog_complete(imgld);
>> + eventfd_ctx_put(imgld->finished);
>> + imgld->finished = NULL;
>> }
>>
>> static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> @@ -119,6 +122,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> {
>> struct fpga_image_write wb;
>> unsigned long minsz;
>> + int ret;
>> u8 *buf;
>>
>> if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
>> @@ -135,13 +139,23 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> if (wb.size & 0x3)
>> return -EINVAL;
>>
>> + if (wb.evtfd < 0)
>> + return -EINVAL;
>> +
>> buf = vzalloc(wb.size);
>> if (!buf)
>> return -ENOMEM;
>>
>> if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
>> - vfree(buf);
>> - return -EFAULT;
>> + ret = -EFAULT;
>> + goto exit_free;
>> + }
>> +
>> + imgld->finished = eventfd_ctx_fdget(wb.evtfd);
>> + if (IS_ERR(imgld->finished)) {
>> + ret = PTR_ERR(imgld->finished);
>> + imgld->finished = NULL;
>> + goto exit_free;
>> }
>>
>> imgld->data = buf;
>> @@ -151,6 +165,10 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> queue_work(system_unbound_wq, &imgld->work);
>>
>> return 0;
>> +
>> +exit_free:
>> + vfree(buf);
>> + return ret;
>> }
>>
>> static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 41ab63cf7b20..7d39daa4d921 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -9,6 +9,7 @@
>>
>> #include <linux/cdev.h>
>> #include <linux/device.h>
>> +#include <linux/eventfd.h>
>> #include <linux/mutex.h>
>> #include <linux/types.h>
>> #include <uapi/linux/fpga-image-load.h>
>> @@ -52,6 +53,7 @@ struct fpga_image_load {
>> u32 progress;
>> u32 err_code; /* image load error code */
>> 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 0382078c5a6c..152a8e1c031f 100644
>> --- a/include/uapi/linux/fpga-image-load.h
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -38,7 +38,7 @@
>> * struct fpga_image_write)
>> *
>> * Upload a data buffer to the target device. The user must provide the
>> - * data buffer and size.
>> + * data buffer, size, and an eventfd file descriptor.
>> *
>> * Return: 0 on success, -errno on failure.
>> */
>> @@ -46,6 +46,7 @@ struct fpga_image_write {
>> /* Input */
>> __u32 flags; /* Zero for now */
>> __u32 size; /* Data size (in bytes) to be written */
>> + __s32 evtfd; /* File descriptor for completion signal */
>> __u64 buf; /* User space address of source data */
>> };
>>
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 3/5] fpga: image-load: signal eventfd when complete
2021-09-24 21:45 ` Russ Weight
@ 2021-09-26 1:06 ` Xu Yilun
0 siblings, 0 replies; 18+ messages in thread
From: Xu Yilun @ 2021-09-26 1:06 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
On Fri, Sep 24, 2021 at 02:45:57PM -0700, Russ Weight wrote:
>
>
> On 9/24/21 1:49 AM, Xu Yilun wrote:
> > On Wed, Sep 22, 2021 at 05:10:54PM -0700, Russ Weight wrote:
> >> Amend the FPGA_IMAGE_LOAD_WRITE IOCTL implementation to include an
> >> eventfd file descriptor as a parameter. The eventfd will be triggered
> >> when the image upload completes.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> >> ---
> >> v16:
> >> - Some cleanup of documentation for the FPGA_IMAGE_LOAD_WRITE IOCTL.
> >> v15:
> >> - This patch is new to the patch-set, and adds an eventfd to the
> >> FPGA_IMAGE_LOAD_WRITE IOCTL. The eventfd is signalled upon completion
> >> of an update.
> >> ---
> >> Documentation/fpga/fpga-image-load.rst | 12 +++++++-----
> >> drivers/fpga/fpga-image-load.c | 22 ++++++++++++++++++++--
> >> include/linux/fpga/fpga-image-load.h | 2 ++
> >> include/uapi/linux/fpga-image-load.h | 3 ++-
> >> 4 files changed, 31 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> >> index ba371c7c0ca0..22a455421bb4 100644
> >> --- a/Documentation/fpga/fpga-image-load.rst
> >> +++ b/Documentation/fpga/fpga-image-load.rst
> >> @@ -27,8 +27,10 @@ ioctl
> >>
> >> FPGA_IMAGE_LOAD_WRITE:
> >>
> >> -Start an image upload with the provided image buffer. This IOCTL returns
> >> -immediately after starting a kernel worker thread to process the image
> >> -upload which could take as long a 40 minutes depending on the actual device
> >> -being updated. This is an exclusive operation; an attempt to start
> >> -concurrent image uploads for the same device will fail with EBUSY.
> >> +Start an image upload with the provided image buffer. This IOCTL returns
> >> +immediately after starting a kernel worker thread to process the image
> >> +upload which could take as long a 40 minutes depending on the actual device
> >> +being updated. This is an exclusive operation; an attempt to start
> > Just curious, there are marks here but seems no change.
> Hmmm - sometimes when fixing up patches, I find it easier to edit
> the patch files (e.g. changing file and symbol names). The patches
> applied cleanly, but I must have forgotten to regenerate them
> before sending them for review.
>
> >
> >> +concurrent image loads for the same device will fail with EBUSY. An eventfd
> > You want to fix "uploads" to "loads"? But there are many other "upload(s)".
> My preference is upload, because load implies more than transferring
> the data but the data transfer is all that this driver really does.
> I preferred fpga_image_load to fpga_image_upload for the device name,
> just because it was a couple of characters less. I'll change "loads"
> to "uploads" here.
That's fine. This word is initiated in Patch #1 and changed in Patch #2,
which makes me wonder why.
>
> Do you think I should rename the driver to fpga_image_upload? Or do
> you think the name is OK if it is probably documented?
The name is OK.
Thanks,
Yilun
>
> - Russ
> >
> > Others look good to me.
> >
> >> +file descriptor parameter is provided to this IOCTL. It will be signalled
> >> +at the completion of the image upload.
> >> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> >> index 65f553b59011..09164a0258a5 100644
> >> --- a/drivers/fpga/fpga-image-load.c
> >> +++ b/drivers/fpga/fpga-image-load.c
> >> @@ -34,6 +34,7 @@ static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> >> {
> >> mutex_lock(&imgld->lock);
> >> imgld->progress = FPGA_IMAGE_PROG_IDLE;
> >> + eventfd_signal(imgld->finished, 1);
> >> mutex_unlock(&imgld->lock);
> >> }
> >>
> >> @@ -112,6 +113,8 @@ static void fpga_image_do_load(struct work_struct *work)
> >> vfree(imgld->data);
> >> imgld->data = NULL;
> >> fpga_image_prog_complete(imgld);
> >> + eventfd_ctx_put(imgld->finished);
> >> + imgld->finished = NULL;
> >> }
> >>
> >> static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> @@ -119,6 +122,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> {
> >> struct fpga_image_write wb;
> >> unsigned long minsz;
> >> + int ret;
> >> u8 *buf;
> >>
> >> if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> >> @@ -135,13 +139,23 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> if (wb.size & 0x3)
> >> return -EINVAL;
> >>
> >> + if (wb.evtfd < 0)
> >> + return -EINVAL;
> >> +
> >> buf = vzalloc(wb.size);
> >> if (!buf)
> >> return -ENOMEM;
> >>
> >> if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> >> - vfree(buf);
> >> - return -EFAULT;
> >> + ret = -EFAULT;
> >> + goto exit_free;
> >> + }
> >> +
> >> + imgld->finished = eventfd_ctx_fdget(wb.evtfd);
> >> + if (IS_ERR(imgld->finished)) {
> >> + ret = PTR_ERR(imgld->finished);
> >> + imgld->finished = NULL;
> >> + goto exit_free;
> >> }
> >>
> >> imgld->data = buf;
> >> @@ -151,6 +165,10 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> >> queue_work(system_unbound_wq, &imgld->work);
> >>
> >> return 0;
> >> +
> >> +exit_free:
> >> + vfree(buf);
> >> + return ret;
> >> }
> >>
> >> static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
> >> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> >> index 41ab63cf7b20..7d39daa4d921 100644
> >> --- a/include/linux/fpga/fpga-image-load.h
> >> +++ b/include/linux/fpga/fpga-image-load.h
> >> @@ -9,6 +9,7 @@
> >>
> >> #include <linux/cdev.h>
> >> #include <linux/device.h>
> >> +#include <linux/eventfd.h>
> >> #include <linux/mutex.h>
> >> #include <linux/types.h>
> >> #include <uapi/linux/fpga-image-load.h>
> >> @@ -52,6 +53,7 @@ struct fpga_image_load {
> >> u32 progress;
> >> u32 err_code; /* image load error code */
> >> 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 0382078c5a6c..152a8e1c031f 100644
> >> --- a/include/uapi/linux/fpga-image-load.h
> >> +++ b/include/uapi/linux/fpga-image-load.h
> >> @@ -38,7 +38,7 @@
> >> * struct fpga_image_write)
> >> *
> >> * Upload a data buffer to the target device. The user must provide the
> >> - * data buffer and size.
> >> + * data buffer, size, and an eventfd file descriptor.
> >> *
> >> * Return: 0 on success, -errno on failure.
> >> */
> >> @@ -46,6 +46,7 @@ struct fpga_image_write {
> >> /* Input */
> >> __u32 flags; /* Zero for now */
> >> __u32 size; /* Data size (in bytes) to be written */
> >> + __s32 evtfd; /* File descriptor for completion signal */
> >> __u64 buf; /* User space address of source data */
> >> };
> >>
> >> --
> >> 2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v16 4/5] fpga: image-load: add status ioctl
2021-09-23 0:10 [PATCH v16 0/5] FPGA Image Load (previously Security Manager) Russ Weight
` (2 preceding siblings ...)
2021-09-23 0:10 ` [PATCH v16 3/5] fpga: image-load: signal eventfd when complete Russ Weight
@ 2021-09-23 0:10 ` Russ Weight
2021-09-23 0:10 ` [PATCH v16 5/5] fpga: image-load: enable cancel of image upload Russ Weight
4 siblings, 0 replies; 18+ messages in thread
From: Russ Weight @ 2021-09-23 0:10 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, Russ Weight
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>
---
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 | 54 ++++++++++++++++++++++----
include/linux/fpga/fpga-image-load.h | 1 +
include/uapi/linux/fpga-image-load.h | 18 +++++++++
4 files changed, 71 insertions(+), 8 deletions(-)
diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
index 22a455421bb4..572e18afebb9 100644
--- a/Documentation/fpga/fpga-image-load.rst
+++ b/Documentation/fpga/fpga-image-load.rst
@@ -34,3 +34,9 @@ being updated. This is an exclusive operation; an attempt to start
concurrent image loads 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 load,
+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 09164a0258a5..2e9a5a041535 100644
--- a/drivers/fpga/fpga-image-load.c
+++ b/drivers/fpga/fpga-image-load.c
@@ -24,9 +24,25 @@ static dev_t fpga_image_devt;
#define WRITE_BLOCK_SIZE 0x4000 /* Default write-block size is 0x4000 bytes */
-static void fpga_image_dev_error(struct fpga_image_load *imgld, u32 err_code)
+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_dev_error(struct fpga_image_load *imgld, u32 err_code)
+{
+ fpga_image_set_error(imgld, err_code);
imgld->ops->cancel(imgld);
}
@@ -46,24 +62,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);
if (ret != FPGA_IMAGE_ERR_NONE) {
fpga_image_dev_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) {
/*
* The write_blk() op has the option to use the blk_size
@@ -89,7 +105,7 @@ static void fpga_image_do_load(struct work_struct *work)
cond_resched();
}
- imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
+ fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
ret = imgld->ops->poll_complete(imgld);
if (ret != FPGA_IMAGE_ERR_NONE)
fpga_image_dev_error(imgld, ret);
@@ -171,20 +187,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 7d39daa4d921..8b58365893fc 100644
--- a/include/linux/fpga/fpga-image-load.h
+++ b/include/linux/fpga/fpga-image-load.h
@@ -51,6 +51,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 152a8e1c031f..dc0c9f1d78b1 100644
--- a/include/uapi/linux/fpga-image-load.h
+++ b/include/uapi/linux/fpga-image-load.h
@@ -52,4 +52,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 */
+};
+
+#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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v16 5/5] fpga: image-load: enable cancel of image upload
2021-09-23 0:10 [PATCH v16 0/5] FPGA Image Load (previously Security Manager) Russ Weight
` (3 preceding siblings ...)
2021-09-23 0:10 ` [PATCH v16 4/5] fpga: image-load: add status ioctl Russ Weight
@ 2021-09-23 0:10 ` Russ Weight
2021-09-24 14:53 ` Xu Yilun
4 siblings, 1 reply; 18+ messages in thread
From: Russ Weight @ 2021-09-23 0:10 UTC (permalink / raw)
To: mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, Russ Weight
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
+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 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);
+ 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) {
/*
* 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v16 5/5] fpga: image-load: enable cancel of image upload
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
0 siblings, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2021-09-24 14:53 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
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
> +EBUSY if it cannot be cancelled by software or ENODEV if there is no update
-EBUSY -ENODEV
> +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.
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 5/5] fpga: image-load: enable cancel of image upload
2021-09-24 14:53 ` Xu Yilun
@ 2021-09-25 0:29 ` Russ Weight
2021-09-26 1:16 ` Xu Yilun
0 siblings, 1 reply; 18+ messages in thread
From: Russ Weight @ 2021-09-25 0:29 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
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.
>
>> +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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v16 5/5] fpga: image-load: enable cancel of image upload
2021-09-25 0:29 ` Russ Weight
@ 2021-09-26 1:16 ` Xu Yilun
0 siblings, 0 replies; 18+ messages in thread
From: Xu Yilun @ 2021-09-26 1:16 UTC (permalink / raw)
To: Russ Weight
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, hao.wu, matthew.gerlach
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
^ permalink raw reply [flat|nested] 18+ messages in thread