[RFC PATCH 0/5] Firmware Upload Framework

[RFC PATCH 0/5] Firmware Upload Framework

[Russ Weight]

The Firmware Upload framework provides a common API for uploading firmware
files to target devices. An example use case involves FPGA devices that
load FPGA, Card BMC, and firmware images from FLASH when the card boots.
Users need the ability to update these firmware images while the card is
in use.

Device drivers that instantiate the Firmware Upload class driver will
interact with the target device to transfer and authenticate the firmware
data. Uploads are performed in the context of a kernel worker thread in
order to facilitate progress indicators during lengthy uploads.

This driver was previously submitted in the context of the FPGA sub-
system as the "FPGA Image Load Framework", but the framework is generic
enough to support other devices as well. The previous submission of this
patch-set can be viewed here:

https://marc.info/?l=linux-kernel&m=163295640216820&w=2

The n3000bmc-sec-update driver is the first driver to use the Firmware
Upload API. A recent version of these patches can be viewed here:

https://marc.info/?l=linux-kernel&m=163295697217095&w=2

I don't think I am duplicating any functionality that is currently covered
in the firmware subsystem. I appreciate your feedback on these patches.

- Russ

Russ Weight (5):
  firmware: Create firmware upload framework
  firmware: upload: Enable firmware uploads
  firmware: upload: Signal eventfd when complete
  firmware: upload: Add status ioctl
  firmware: upload: Enable cancel of firmware upload

 .../driver-api/firmware/firmware-upload.rst   |  54 +++
 Documentation/driver-api/firmware/index.rst   |   1 +
 MAINTAINERS                                   |   9 +
 drivers/firmware/Kconfig                      |   8 +
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/firmware-upload.c            | 413 ++++++++++++++++++
 include/linux/firmware/firmware-upload.h      |  69 +++
 include/uapi/linux/firmware-upload.h          |  73 ++++
 8 files changed, 628 insertions(+)
 create mode 100644 Documentation/driver-api/firmware/firmware-upload.rst
 create mode 100644 drivers/firmware/firmware-upload.c
 create mode 100644 include/linux/firmware/firmware-upload.h
 create mode 100644 include/uapi/linux/firmware-upload.h

-- 
2.25.1

[RFC PATCH 1/5] firmware: Create firmware upload framework

[Russ Weight]

The Firmware Upload class driver provides a common API for uploading
firmware files to devices. The lower level device driver and/or the
target device are responsible for authenticating the data.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../driver-api/firmware/firmware-upload.rst   |  17 +++
 Documentation/driver-api/firmware/index.rst   |   1 +
 MAINTAINERS                                   |   8 ++
 drivers/firmware/Kconfig                      |   8 ++
 drivers/firmware/Makefile                     |   1 +
 drivers/firmware/firmware-upload.c            | 125 ++++++++++++++++++
 include/linux/firmware/firmware-upload.h      |  35 +++++
 7 files changed, 195 insertions(+)
 create mode 100644 Documentation/driver-api/firmware/firmware-upload.rst
 create mode 100644 drivers/firmware/firmware-upload.c
 create mode 100644 include/linux/firmware/firmware-upload.h

diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
new file mode 100644
index 000000000000..9f38f18be97a
--- /dev/null
+++ b/Documentation/driver-api/firmware/firmware-upload.rst
@@ -0,0 +1,17 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+Firmware Upload Framework
+=========================
+
+Some devices load firmware from on-board FLASH when the card initializes.
+These cards do not require the request_firmware framework to load the
+firmware when the card boots, but they to require a utility to allow
+users to update the FLASH contents.
+
+The Firmware Upload framework provides a common API for user-space tools
+to manage firmware uploads to devices. Device drivers that instantiate the
+Firmware Upload class driver will interact with the target device to
+transfer and authenticate the firmware data. Uploads are performed in the
+context of a kernel worker thread in order to facilitate progress
+indicators during lengthy uploads.
diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
index 57415d657173..90c5db512d74 100644
--- a/Documentation/driver-api/firmware/index.rst
+++ b/Documentation/driver-api/firmware/index.rst
@@ -8,6 +8,7 @@ Linux Firmware API
    core
    efi/index
    request_firmware
+   firmware-upload
    other_interfaces
 
 .. only::  subproject and html
diff --git a/MAINTAINERS b/MAINTAINERS
index 8dfd5f4ae5e6..19b3d3ccc406 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7415,6 +7415,14 @@ F:	Documentation/firmware_class/
 F:	drivers/base/firmware_loader/
 F:	include/linux/firmware.h
 
+FIRMWARE UPLOADER
+M:	Russ Weight <russell.h.weight@intel.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/driver-api/firmware/firmware-upload.rst
+F:	drivers/firmware/firmware-upload.c
+F:	include/linux/firmware/firmware-upload.h
+
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <josh.h.morris@us.ibm.com>
 M:	Philip Kelleher <pjk1939@linux.ibm.com>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 75cb91055c17..2a330371a76d 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -293,6 +293,14 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
+config FIRMWARE_UPLOAD
+	tristate "Firmware Upload Framework"
+	help
+	  The Firmware Upload class driver presents a common user API
+	  for uploading a firmware file to a device. It is the
+	  responsibility of the lower-level device driver and/or device to
+	  authenticate and disposition the firmware data.
+
 source "drivers/firmware/arm_ffa/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/cirrus/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4e58cb474a68..5dd2a3bf7fa7 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
+obj-$(CONFIG_FIRMWARE_UPLOAD)	+= firmware-upload.o
 
 obj-y				+= arm_ffa/
 obj-y				+= arm_scmi/
diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
new file mode 100644
index 000000000000..bc26df8b6b52
--- /dev/null
+++ b/drivers/firmware/firmware-upload.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Firmware Upload Framework
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ */
+
+#include <linux/firmware/firmware-upload.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+#define FW_UPLOAD_XA_LIMIT	XA_LIMIT(0, INT_MAX)
+static DEFINE_XARRAY_ALLOC(fw_upload_xa);
+
+static struct class *fw_upload_class;
+
+#define to_fw_upload(d) container_of(d, struct fw_upload, dev)
+
+/**
+ * fw_upload_register - create and register a Firmware Upload Device
+ *
+ * @parent: Firmware Upload device from pdev
+ * @ops:    Pointer to a structure of firmware upload callback functions
+ * @priv:   Firmware Upload private data
+ *
+ * Returns a struct fw_upload pointer on success, or ERR_PTR() on
+ * error. The caller of this function is responsible for calling
+ * fw_upload_unregister().
+ */
+struct fw_upload *
+fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
+		   void *priv)
+{
+	struct fw_upload *fwl;
+	int ret;
+
+	fwl = kzalloc(sizeof(*fwl), GFP_KERNEL);
+	if (!fwl)
+		return ERR_PTR(-ENOMEM);
+
+	ret = xa_alloc(&fw_upload_xa, &fwl->dev.id, fwl, FW_UPLOAD_XA_LIMIT,
+		       GFP_KERNEL);
+	if (ret)
+		goto error_kfree;
+
+	mutex_init(&fwl->lock);
+
+	fwl->priv = priv;
+	fwl->ops = ops;
+
+	fwl->dev.class = fw_upload_class;
+	fwl->dev.parent = parent;
+
+	ret = dev_set_name(&fwl->dev, "fw_upload%d", fwl->dev.id);
+	if (ret) {
+		dev_err(parent, "Failed to set device name: fw_upload%d\n",
+			fwl->dev.id);
+		goto error_device;
+	}
+
+	ret = device_register(&fwl->dev);
+	if (ret) {
+		put_device(&fwl->dev);
+		return ERR_PTR(ret);
+	}
+
+	return fwl;
+
+error_device:
+	xa_erase(&fw_upload_xa, fwl->dev.id);
+
+error_kfree:
+	kfree(fwl);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(fw_upload_register);
+
+/**
+ * fw_upload_unregister - unregister a Firmware Upload device
+ *
+ * @fwl: pointer to struct fw_upload
+ *
+ * This function is intended for use in the parent driver's remove()
+ * function.
+ */
+void fw_upload_unregister(struct fw_upload *fwl)
+{
+	device_unregister(&fwl->dev);
+}
+EXPORT_SYMBOL_GPL(fw_upload_unregister);
+
+static void fw_upload_dev_release(struct device *dev)
+{
+	struct fw_upload *fwl = to_fw_upload(dev);
+
+	xa_erase(&fw_upload_xa, fwl->dev.id);
+	kfree(fwl);
+}
+
+static int __init fw_upload_class_init(void)
+{
+	pr_info("Firmware Upload Framework\n");
+
+	fw_upload_class = class_create(THIS_MODULE, "fw_upload");
+	if (IS_ERR(fw_upload_class))
+		return PTR_ERR(fw_upload_class);
+
+	fw_upload_class->dev_release = fw_upload_dev_release;
+
+	return 0;
+}
+
+static void __exit fw_upload_class_exit(void)
+{
+	class_destroy(fw_upload_class);
+	WARN_ON(!xa_empty(&fw_upload_xa));
+}
+
+MODULE_DESCRIPTION("Firmware Upload Framework");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fw_upload_class_init);
+module_exit(fw_upload_class_exit)
diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
new file mode 100644
index 000000000000..767e0bdded7b
--- /dev/null
+++ b/include/linux/firmware/firmware-upload.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Firmware Upload Framework
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ */
+#ifndef _LINUX_FIRMWARE_UPLOAD_H
+#define _LINUX_FIRMWARE_UPLOAD_H
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+struct fw_upload;
+
+/**
+ * struct fw_upload_ops - device specific operations
+ */
+struct fw_upload_ops {
+};
+
+struct fw_upload {
+	struct device dev;
+	const struct fw_upload_ops *ops;
+	struct mutex lock;		/* protect data structure contents */
+	void *priv;
+};
+
+struct fw_upload *
+fw_upload_register(struct device *dev, const struct fw_upload_ops *ops,
+		   void *priv);
+
+void fw_upload_unregister(struct fw_upload *fwl);
+
+#endif
-- 
2.25.1

[RFC PATCH 2/5] firmware: upload: Enable firmware uploads

[Russ Weight]

Extend the Firmware Upload framework to add a character device and provide
IOCTL support (FW_UPLOAD_WRITE) for uploading a firmware file to a device.
The IOCTL will return immediately, and the upload will begin in the context
of a kernel worker thread.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../driver-api/firmware/firmware-upload.rst   |  21 ++
 MAINTAINERS                                   |   1 +
 drivers/firmware/firmware-upload.c            | 216 +++++++++++++++++-
 include/linux/firmware/firmware-upload.h      |  27 +++
 include/uapi/linux/firmware-upload.h          |  53 +++++
 5 files changed, 317 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/firmware-upload.h

diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
index 9f38f18be97a..8a7c50087028 100644
--- a/Documentation/driver-api/firmware/firmware-upload.rst
+++ b/Documentation/driver-api/firmware/firmware-upload.rst
@@ -15,3 +15,24 @@ Firmware Upload class driver will interact with the target device to
 transfer and authenticate the firmware data. Uploads are performed in the
 context of a kernel worker thread in order to facilitate progress
 indicators during lengthy uploads.
+
+User API
+========
+
+open
+----
+
+An fw_upload device is opened exclusively to control a firmware upload.
+The device must remain open throughout the duration of the firmware upload.
+An attempt to close the device while an upload is in progress will block
+until the firmware upload is complete.
+
+ioctl
+-----
+
+FW_UPLOAD_WRITE:
+
+The FW_UPLOAD_WRITE IOCTL passes in the address of a data buffer and starts
+the firmware upload. This IOCTL returns immediately after assigning the work
+to a kernel worker thread. This is an exclusive operation; an attempt to
+start concurrent firmware uploads for the same device will fail with EBUSY.
diff --git a/MAINTAINERS b/MAINTAINERS
index 19b3d3ccc406..4d59e8e11813 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7422,6 +7422,7 @@ S:	Maintained
 F:	Documentation/driver-api/firmware/firmware-upload.rst
 F:	drivers/firmware/firmware-upload.c
 F:	include/linux/firmware/firmware-upload.h
+F:	include/uapi/linux/firmware-upload.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
 M:	Joshua Morris <josh.h.morris@us.ibm.com>
diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
index bc26df8b6b52..e930611262fb 100644
--- a/drivers/firmware/firmware-upload.c
+++ b/drivers/firmware/firmware-upload.c
@@ -5,18 +5,189 @@
  * Copyright (C) 2019-2021 Intel Corporation, Inc.
  */
 
+#include <linux/delay.h>
 #include <linux/firmware/firmware-upload.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 FW_UPLOAD_XA_LIMIT	XA_LIMIT(0, INT_MAX)
 static DEFINE_XARRAY_ALLOC(fw_upload_xa);
 
 static struct class *fw_upload_class;
+static dev_t fw_upload_devt;
 
 #define to_fw_upload(d) container_of(d, struct fw_upload, dev)
 
+static void fw_upload_prog_complete(struct fw_upload *fwl)
+{
+	mutex_lock(&fwl->lock);
+	fwl->progress = FW_UPLOAD_PROG_IDLE;
+	mutex_unlock(&fwl->lock);
+}
+
+static void fw_upload_do_load(struct work_struct *work)
+{
+	struct fw_upload *fwl;
+	s32 ret, offset = 0;
+
+	fwl = container_of(work, struct fw_upload, work);
+
+	if (fwl->driver_unload) {
+		fwl->err_code = FW_UPLOAD_ERR_CANCELED;
+		goto idle_exit;
+	}
+
+	get_device(&fwl->dev);
+	if (!try_module_get(fwl->dev.parent->driver->owner)) {
+		fwl->err_code = FW_UPLOAD_ERR_BUSY;
+		goto putdev_exit;
+	}
+
+	fwl->progress = FW_UPLOAD_PROG_PREPARING;
+	ret = fwl->ops->prepare(fwl, fwl->data, fwl->remaining_size);
+	if (ret) {
+		fwl->err_code = ret;
+		goto modput_exit;
+	}
+
+	fwl->progress = FW_UPLOAD_PROG_WRITING;
+	while (fwl->remaining_size) {
+		ret = fwl->ops->write(fwl, fwl->data, offset,
+					fwl->remaining_size);
+		if (ret <= 0) {
+			if (!ret) {
+				dev_warn(&fwl->dev,
+					 "write-op wrote zero data\n");
+				ret = -FW_UPLOAD_ERR_RW_ERROR;
+			}
+			fwl->err_code = -ret;
+			goto done;
+		}
+
+		fwl->remaining_size -= ret;
+		offset += ret;
+	}
+
+	fwl->progress = FW_UPLOAD_PROG_PROGRAMMING;
+	ret = fwl->ops->poll_complete(fwl);
+	if (ret)
+		fwl->err_code = ret;
+
+done:
+	if (fwl->ops->cleanup)
+		fwl->ops->cleanup(fwl);
+
+modput_exit:
+	module_put(fwl->dev.parent->driver->owner);
+
+putdev_exit:
+	put_device(&fwl->dev);
+
+idle_exit:
+	/*
+	 * Note: fwl->remaining_size is left unmodified here to provide
+	 * additional information on errors. It will be reinitialized when
+	 * the next firmeware upload begins.
+	 */
+	vfree(fwl->data);
+	fwl->data = NULL;
+	fw_upload_prog_complete(fwl);
+}
+
+static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
+{
+	struct fw_upload_write wb;
+	unsigned long minsz;
+	u8 *buf;
+
+	if (fwl->driver_unload || fwl->progress != FW_UPLOAD_PROG_IDLE)
+		return -EBUSY;
+
+	minsz = offsetofend(struct fw_upload_write, buf);
+	if (copy_from_user(&wb, (void __user *)arg, minsz))
+		return -EFAULT;
+
+	if (wb.flags)
+		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;
+	}
+
+	fwl->data = buf;
+	fwl->remaining_size = wb.size;
+	fwl->err_code = 0;
+	fwl->progress = FW_UPLOAD_PROG_STARTING;
+	queue_work(system_long_wq, &fwl->work);
+
+	return 0;
+}
+
+static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
+			    unsigned long arg)
+{
+	struct fw_upload *fwl = filp->private_data;
+	int ret = -ENOTTY;
+
+	switch (cmd) {
+	case FW_UPLOAD_WRITE:
+		mutex_lock(&fwl->lock);
+		ret = fw_upload_ioctl_write(fwl, arg);
+		mutex_unlock(&fwl->lock);
+		break;
+	}
+
+	return ret;
+}
+
+static int fw_upload_open(struct inode *inode, struct file *filp)
+{
+	struct fw_upload *fwl = container_of(inode->i_cdev,
+						     struct fw_upload, cdev);
+
+	if (atomic_cmpxchg(&fwl->opened, 0, 1))
+		return -EBUSY;
+
+	filp->private_data = fwl;
+
+	return 0;
+}
+
+static int fw_upload_release(struct inode *inode, struct file *filp)
+{
+	struct fw_upload *fwl = filp->private_data;
+
+	mutex_lock(&fwl->lock);
+	if (fwl->progress == FW_UPLOAD_PROG_IDLE) {
+		mutex_unlock(&fwl->lock);
+		goto close_exit;
+	}
+
+	mutex_unlock(&fwl->lock);
+	flush_work(&fwl->work);
+
+close_exit:
+	atomic_set(&fwl->opened, 0);
+
+	return 0;
+}
+
+static const struct file_operations fw_upload_fops = {
+	.owner = THIS_MODULE,
+	.open = fw_upload_open,
+	.release = fw_upload_release,
+	.unlocked_ioctl = fw_upload_ioctl,
+};
+
 /**
  * fw_upload_register - create and register a Firmware Upload Device
  *
@@ -35,6 +206,11 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
 	struct fw_upload *fwl;
 	int ret;
 
+	if (!ops || !ops->prepare || !ops->write || !ops->poll_complete) {
+		dev_err(parent, "Attempt to register without all required ops\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
 	fwl = kzalloc(sizeof(*fwl), GFP_KERNEL);
 	if (!fwl)
 		return ERR_PTR(-ENOMEM);
@@ -48,9 +224,13 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
 
 	fwl->priv = priv;
 	fwl->ops = ops;
+	fwl->err_code = 0;
+	fwl->progress = FW_UPLOAD_PROG_IDLE;
+	INIT_WORK(&fwl->work, fw_upload_do_load);
 
 	fwl->dev.class = fw_upload_class;
 	fwl->dev.parent = parent;
+	fwl->dev.devt = MKDEV(MAJOR(fw_upload_devt), fwl->dev.id);
 
 	ret = dev_set_name(&fwl->dev, "fw_upload%d", fwl->dev.id);
 	if (ret) {
@@ -65,6 +245,16 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
 		return ERR_PTR(ret);
 	}
 
+	cdev_init(&fwl->cdev, &fw_upload_fops);
+	fwl->cdev.owner = parent->driver->owner;
+	cdev_set_parent(&fwl->cdev, &fwl->dev.kobj);
+
+	ret = cdev_add(&fwl->cdev, fwl->dev.devt, 1);
+	if (ret) {
+		put_device(&fwl->dev);
+		return ERR_PTR(ret);
+	}
+
 	return fwl;
 
 error_device:
@@ -83,10 +273,23 @@ EXPORT_SYMBOL_GPL(fw_upload_register);
  * @fwl: pointer to struct fw_upload
  *
  * 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 fw_upload_unregister(struct fw_upload *fwl)
 {
+	mutex_lock(&fwl->lock);
+	fwl->driver_unload = true;
+	if (fwl->progress == FW_UPLOAD_PROG_IDLE) {
+		mutex_unlock(&fwl->lock);
+		goto unregister;
+	}
+
+	mutex_unlock(&fwl->lock);
+	flush_work(&fwl->work);
+
+unregister:
+	cdev_del(&fwl->cdev);
 	device_unregister(&fwl->dev);
 }
 EXPORT_SYMBOL_GPL(fw_upload_unregister);
@@ -101,19 +304,30 @@ static void fw_upload_dev_release(struct device *dev)
 
 static int __init fw_upload_class_init(void)
 {
+	int ret;
 	pr_info("Firmware Upload Framework\n");
 
 	fw_upload_class = class_create(THIS_MODULE, "fw_upload");
 	if (IS_ERR(fw_upload_class))
 		return PTR_ERR(fw_upload_class);
 
+	ret = alloc_chrdev_region(&fw_upload_devt, 0, MINORMASK,
+				  "fw_upload");
+	if (ret)
+		goto exit_destroy_class;
+
 	fw_upload_class->dev_release = fw_upload_dev_release;
 
 	return 0;
+
+exit_destroy_class:
+	class_destroy(fw_upload_class);
+	return ret;
 }
 
 static void __exit fw_upload_class_exit(void)
 {
+	unregister_chrdev_region(fw_upload_devt, MINORMASK);
 	class_destroy(fw_upload_class);
 	WARN_ON(!xa_empty(&fw_upload_xa));
 }
diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
index 767e0bdded7b..f80d5ba0c3f1 100644
--- a/include/linux/firmware/firmware-upload.h
+++ b/include/linux/firmware/firmware-upload.h
@@ -7,22 +7,49 @@
 #ifndef _LINUX_FIRMWARE_UPLOAD_H
 #define _LINUX_FIRMWARE_UPLOAD_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <uapi/linux/firmware-upload.h>
 
 struct fw_upload;
 
 /**
  * struct fw_upload_ops - device specific operations
+ * @prepare:		    Required: Prepare secure update
+ * @write:		    Required: The write() op receives the remaining
+ *			    size to be written and must return the actual
+ *			    size written or a negative error code. The write()
+ *			    op will be called repeatedly until all data is
+ *			    written.
+ * @poll_complete:	    Required: Check for the completion of the
+ *			    HW authentication/programming process.
+ * @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 fw_upload_ops {
+	u32 (*prepare)(struct fw_upload *fwl, const u8 *data, u32 size);
+	s32 (*write)(struct fw_upload *fwl, const u8 *data,
+		     u32 offset, u32 size);
+	u32 (*poll_complete)(struct fw_upload *fwl);
+	void (*cleanup)(struct fw_upload *fwl);
 };
 
 struct fw_upload {
 	struct device dev;
+	struct cdev cdev;
 	const struct fw_upload_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;			/* upload error code */
+	bool driver_unload;
 	void *priv;
 };
 
diff --git a/include/uapi/linux/firmware-upload.h b/include/uapi/linux/firmware-upload.h
new file mode 100644
index 000000000000..034550071911
--- /dev/null
+++ b/include/uapi/linux/firmware-upload.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Header File for Firmware Upload User API
+ *
+ * Copyright (C) 2019-2021 Intel Corporation, Inc.
+ *
+ */
+
+#ifndef _UAPI_LINUX_FW_UPLOAD__H
+#define _UAPI_LINUX_FW_UPLOAD__H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+#define FW_UPLOAD_MAGIC 0xB9
+
+/* Firmware upload progress codes */
+#define FW_UPLOAD_PROG_IDLE		0
+#define FW_UPLOAD_PROG_STARTING	1
+#define FW_UPLOAD_PROG_PREPARING	2
+#define FW_UPLOAD_PROG_WRITING		3
+#define FW_UPLOAD_PROG_PROGRAMMING	4
+#define FW_UPLOAD_PROG_MAX		5
+
+/* Firmware upload error codes */
+#define FW_UPLOAD_ERR_HW_ERROR		1
+#define FW_UPLOAD_ERR_TIMEOUT		2
+#define FW_UPLOAD_ERR_CANCELED		3
+#define FW_UPLOAD_ERR_BUSY		4
+#define FW_UPLOAD_ERR_INVALID_SIZE	5
+#define FW_UPLOAD_ERR_RW_ERROR		6
+#define FW_UPLOAD_ERR_WEAROUT		7
+#define FW_UPLOAD_ERR_MAX		8
+
+/**
+ * FW_UPLOAD_WRITE - _IOW(FW_UPLOAD_MAGIC, 0,
+ *				struct fw_upload_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 fw_upload_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 FW_UPLOAD_WRITE	_IOW(FW_UPLOAD_MAGIC, 0, struct fw_upload_write)
+
+#endif /* _UAPI_LINUX_FW_UPLOAD_H */
-- 
2.25.1

[RFC PATCH 3/5] firmware: upload: Signal eventfd when complete

[Russ Weight]

Amend the FW_UPLOAD_WRITE IOCTL implementation to include an eventfd file
descriptor as a parameter. The eventfd will be signalled when the firmware
upload completes.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../driver-api/firmware/firmware-upload.rst   |  2 ++
 drivers/firmware/firmware-upload.c            | 22 +++++++++++++++++--
 include/linux/firmware/firmware-upload.h      |  2 ++
 include/uapi/linux/firmware-upload.h          |  3 ++-
 4 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
index 8a7c50087028..0165c18a62be 100644
--- a/Documentation/driver-api/firmware/firmware-upload.rst
+++ b/Documentation/driver-api/firmware/firmware-upload.rst
@@ -36,3 +36,5 @@ The FW_UPLOAD_WRITE IOCTL passes in the address of a data buffer and starts
 the firmware upload. This IOCTL returns immediately after assigning the work
 to a kernel worker thread. This is an exclusive operation; an attempt to
 start concurrent firmware uploads for the same device will fail with EBUSY.
+An eventfd file descriptor parameter is also passed to this IOCTL. It will
+be signalled at the completion of the firmware upload.
diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
index e930611262fb..f8ddacc72b1d 100644
--- a/drivers/firmware/firmware-upload.c
+++ b/drivers/firmware/firmware-upload.c
@@ -26,6 +26,7 @@ static void fw_upload_prog_complete(struct fw_upload *fwl)
 {
 	mutex_lock(&fwl->lock);
 	fwl->progress = FW_UPLOAD_PROG_IDLE;
+	eventfd_signal(fwl->finished, 1);
 	mutex_unlock(&fwl->lock);
 }
 
@@ -96,12 +97,15 @@ static void fw_upload_do_load(struct work_struct *work)
 	vfree(fwl->data);
 	fwl->data = NULL;
 	fw_upload_prog_complete(fwl);
+	eventfd_ctx_put(fwl->finished);
+	fwl->finished = NULL;
 }
 
 static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
 {
 	struct fw_upload_write wb;
 	unsigned long minsz;
+	int ret;
 	u8 *buf;
 
 	if (fwl->driver_unload || fwl->progress != FW_UPLOAD_PROG_IDLE)
@@ -114,13 +118,23 @@ static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
 	if (wb.flags)
 		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;
+	}
+
+	fwl->finished = eventfd_ctx_fdget(wb.evtfd);
+	if (IS_ERR(fwl->finished)) {
+		ret = PTR_ERR(fwl->finished);
+		fwl->finished = NULL;
+		goto exit_free;
 	}
 
 	fwl->data = buf;
@@ -130,6 +144,10 @@ static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
 	queue_work(system_long_wq, &fwl->work);
 
 	return 0;
+
+exit_free:
+	vfree(buf);
+	return ret;
 }
 
 static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
index f80d5ba0c3f1..c96473fb1e6c 100644
--- a/include/linux/firmware/firmware-upload.h
+++ b/include/linux/firmware/firmware-upload.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/firmware-upload.h>
@@ -50,6 +51,7 @@ struct fw_upload {
 	u32 progress;
 	u32 err_code;			/* upload error code */
 	bool driver_unload;
+	struct eventfd_ctx *finished;
 	void *priv;
 };
 
diff --git a/include/uapi/linux/firmware-upload.h b/include/uapi/linux/firmware-upload.h
index 034550071911..5ac9079e1bf7 100644
--- a/include/uapi/linux/firmware-upload.h
+++ b/include/uapi/linux/firmware-upload.h
@@ -37,7 +37,7 @@
  *				struct fw_upload_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.
  */
@@ -45,6 +45,7 @@ struct fw_upload_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

[RFC PATCH 4/5] firmware: upload: Add status ioctl

[Russ Weight]

Extend the Firmware Upload framework to include a FW_UPLOAD_STATUS IOCTL
that can be used to monitor the progress of an ongoing firmware upload.
The status returned includes how much data remains to be transferred, the
progress of the firmware upload, and error information in the case of a
failure.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../driver-api/firmware/firmware-upload.rst   |  6 ++
 drivers/firmware/firmware-upload.c            | 56 +++++++++++++++----
 include/linux/firmware/firmware-upload.h      |  1 +
 include/uapi/linux/firmware-upload.h          | 17 ++++++
 4 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
index 0165c18a62be..bf079d648b5c 100644
--- a/Documentation/driver-api/firmware/firmware-upload.rst
+++ b/Documentation/driver-api/firmware/firmware-upload.rst
@@ -38,3 +38,9 @@ to a kernel worker thread. This is an exclusive operation; an attempt to
 start concurrent firmware uploads for the same device will fail with EBUSY.
 An eventfd file descriptor parameter is also passed to this IOCTL. It will
 be signalled at the completion of the firmware upload.
+
+FW_UPLOAD_STATUS:
+
+Collect status for an on-going firmware upload. The status returned includes
+how much data remains to be transferred, the progress of the upload, and
+error information in the case of a failure.
diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
index f8ddacc72b1d..507cd0f3740e 100644
--- a/drivers/firmware/firmware-upload.c
+++ b/drivers/firmware/firmware-upload.c
@@ -22,6 +22,21 @@ static dev_t fw_upload_devt;
 
 #define to_fw_upload(d) container_of(d, struct fw_upload, dev)
 
+static void fw_upload_update_progress(struct fw_upload *fwl, u32 new_progress)
+{
+	mutex_lock(&fwl->lock);
+	fwl->progress = new_progress;
+	mutex_unlock(&fwl->lock);
+}
+
+static void fw_upload_set_error(struct fw_upload *fwl, u32 err_code)
+{
+	mutex_lock(&fwl->lock);
+	fwl->err_progress = fwl->progress;
+	fwl->err_code = err_code;
+	mutex_unlock(&fwl->lock);
+}
+
 static void fw_upload_prog_complete(struct fw_upload *fwl)
 {
 	mutex_lock(&fwl->lock);
@@ -38,24 +53,24 @@ static void fw_upload_do_load(struct work_struct *work)
 	fwl = container_of(work, struct fw_upload, work);
 
 	if (fwl->driver_unload) {
-		fwl->err_code = FW_UPLOAD_ERR_CANCELED;
+		fw_upload_set_error(fwl, FW_UPLOAD_ERR_CANCELED);
 		goto idle_exit;
 	}
 
 	get_device(&fwl->dev);
 	if (!try_module_get(fwl->dev.parent->driver->owner)) {
-		fwl->err_code = FW_UPLOAD_ERR_BUSY;
+		fw_upload_set_error(fwl, FW_UPLOAD_ERR_BUSY);
 		goto putdev_exit;
 	}
 
-	fwl->progress = FW_UPLOAD_PROG_PREPARING;
+	fw_upload_update_progress(fwl, FW_UPLOAD_PROG_PREPARING);
 	ret = fwl->ops->prepare(fwl, fwl->data, fwl->remaining_size);
 	if (ret) {
-		fwl->err_code = ret;
+		fw_upload_set_error(fwl, ret);
 		goto modput_exit;
 	}
 
-	fwl->progress = FW_UPLOAD_PROG_WRITING;
+	fw_upload_update_progress(fwl, FW_UPLOAD_PROG_WRITING);
 	while (fwl->remaining_size) {
 		ret = fwl->ops->write(fwl, fwl->data, offset,
 					fwl->remaining_size);
@@ -65,7 +80,7 @@ static void fw_upload_do_load(struct work_struct *work)
 					 "write-op wrote zero data\n");
 				ret = -FW_UPLOAD_ERR_RW_ERROR;
 			}
-			fwl->err_code = -ret;
+			fw_upload_set_error(fwl, -ret);
 			goto done;
 		}
 
@@ -73,10 +88,10 @@ static void fw_upload_do_load(struct work_struct *work)
 		offset += ret;
 	}
 
-	fwl->progress = FW_UPLOAD_PROG_PROGRAMMING;
+	fw_upload_update_progress(fwl, FW_UPLOAD_PROG_PROGRAMMING);
 	ret = fwl->ops->poll_complete(fwl);
 	if (ret)
-		fwl->err_code = ret;
+		fw_upload_set_error(fwl, ret);
 
 done:
 	if (fwl->ops->cleanup)
@@ -150,20 +165,41 @@ static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
 	return ret;
 }
 
+static int fw_upload_ioctl_status(struct fw_upload *fwl, unsigned long arg)
+{
+	struct fw_upload_status status;
+
+	memset(&status, 0, sizeof(status));
+	status.progress = fwl->progress;
+	status.remaining_size = fwl->remaining_size;
+	status.err_progress = fwl->err_progress;
+	status.err_code = fwl->err_code;
+
+	if (copy_to_user((void __user *)arg, &status, sizeof(status)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
 			    unsigned long arg)
 {
 	struct fw_upload *fwl = filp->private_data;
 	int ret = -ENOTTY;
 
+	mutex_lock(&fwl->lock);
+
 	switch (cmd) {
 	case FW_UPLOAD_WRITE:
-		mutex_lock(&fwl->lock);
 		ret = fw_upload_ioctl_write(fwl, arg);
-		mutex_unlock(&fwl->lock);
+		break;
+	case FW_UPLOAD_STATUS:
+		ret = fw_upload_ioctl_status(fwl, arg);
 		break;
 	}
 
+	mutex_unlock(&fwl->lock);
+
 	return ret;
 }
 
diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
index c96473fb1e6c..63c6c65e7489 100644
--- a/include/linux/firmware/firmware-upload.h
+++ b/include/linux/firmware/firmware-upload.h
@@ -49,6 +49,7 @@ struct fw_upload {
 	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;			/* upload error code */
 	bool driver_unload;
 	struct eventfd_ctx *finished;
diff --git a/include/uapi/linux/firmware-upload.h b/include/uapi/linux/firmware-upload.h
index 5ac9079e1bf7..b8d96ee3f646 100644
--- a/include/uapi/linux/firmware-upload.h
+++ b/include/uapi/linux/firmware-upload.h
@@ -51,4 +51,21 @@ struct fw_upload_write {
 
 #define FW_UPLOAD_WRITE	_IOW(FW_UPLOAD_MAGIC, 0, struct fw_upload_write)
 
+/**
+ * FW_UPLOAD_STATUS - _IOR(FW_UPLOAD_MAGIC, 1, struct fw_upload_status)
+ *
+ * Request status information for an ongoing update.
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+struct fw_upload_status {
+	/* Output */
+	__u32 remaining_size;	/* size remaining to transfer */
+	__u32 progress;		/* current progress of firmware upload */
+	__u32 err_progress;	/* progress at time of error */
+	__u32 err_code;		/* error code */
+};
+
+#define FW_UPLOAD_STATUS	_IOR(FW_UPLOAD_MAGIC, 1, struct fw_upload_status)
+
 #endif /* _UAPI_LINUX_FW_UPLOAD_H */
-- 
2.25.1

[RFC PATCH 5/5] firmware: upload: Enable cancel of firmware upload

[Russ Weight]

Extend the Firmware Upload framework to include a cancel IOCTL that can be
used to request that a firmware upload be cancelled. The IOCTL may return
ENODEV if there is no update in progress.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../driver-api/firmware/firmware-upload.rst   |  8 +++++++
 drivers/firmware/firmware-upload.c            | 24 +++++++++++++++++--
 include/linux/firmware/firmware-upload.h      |  4 ++++
 include/uapi/linux/firmware-upload.h          |  2 ++
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
index bf079d648b5c..1a0a487fd4d6 100644
--- a/Documentation/driver-api/firmware/firmware-upload.rst
+++ b/Documentation/driver-api/firmware/firmware-upload.rst
@@ -44,3 +44,11 @@ FW_UPLOAD_STATUS:
 Collect status for an on-going firmware upload. The status returned includes
 how much data remains to be transferred, the progress of the upload, and
 error information in the case of a failure.
+
+FW_UPLOAD_CANCEL:
+
+Request that an on-going firmware upload be cancelled. This IOCTL will
+return ENODEV if there is no upload in progress. Depending on the
+implementation of the lower-level driver, the cancellation may take affect
+immediately or it could block until a critical operation such as a FLASH
+is safely completed.
diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
index 507cd0f3740e..7677121ba49e 100644
--- a/drivers/firmware/firmware-upload.c
+++ b/drivers/firmware/firmware-upload.c
@@ -181,11 +181,20 @@ static int fw_upload_ioctl_status(struct fw_upload *fwl, unsigned long arg)
 	return 0;
 }
 
+static int fw_upload_ioctl_cancel(struct fw_upload *fwl, unsigned long arg)
+{
+	if (fwl->progress == FW_UPLOAD_PROG_IDLE)
+		return -ENODEV;
+
+	fwl->ops->cancel(fwl);
+	return 0;
+}
+
 static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
 			    unsigned long arg)
 {
 	struct fw_upload *fwl = filp->private_data;
-	int ret = -ENOTTY;
+	int ret = 0;
 
 	mutex_lock(&fwl->lock);
 
@@ -196,6 +205,12 @@ static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
 	case FW_UPLOAD_STATUS:
 		ret = fw_upload_ioctl_status(fwl, arg);
 		break;
+	case FW_UPLOAD_CANCEL:
+		ret = fw_upload_ioctl_cancel(fwl, arg);
+		break;
+	default:
+		ret = -ENOTTY;
+		break;
 	}
 
 	mutex_unlock(&fwl->lock);
@@ -226,6 +241,8 @@ static int fw_upload_release(struct inode *inode, struct file *filp)
 		goto close_exit;
 	}
 
+	fwl->ops->cancel(fwl);
+
 	mutex_unlock(&fwl->lock);
 	flush_work(&fwl->work);
 
@@ -260,7 +277,8 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
 	struct fw_upload *fwl;
 	int ret;
 
-	if (!ops || !ops->prepare || !ops->write || !ops->poll_complete) {
+	if (!ops || !ops->cancel || !ops->prepare ||
+	    !ops->write || !ops->poll_complete) {
 		dev_err(parent, "Attempt to register without all required ops\n");
 		return ERR_PTR(-ENOMEM);
 	}
@@ -339,6 +357,8 @@ void fw_upload_unregister(struct fw_upload *fwl)
 		goto unregister;
 	}
 
+	fwl->ops->cancel(fwl);
+
 	mutex_unlock(&fwl->lock);
 	flush_work(&fwl->work);
 
diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
index 63c6c65e7489..6f3971b8e117 100644
--- a/include/linux/firmware/firmware-upload.h
+++ b/include/linux/firmware/firmware-upload.h
@@ -26,6 +26,9 @@ struct fw_upload;
  *			    written.
  * @poll_complete:	    Required: Check for the completion of the
  *			    HW authentication/programming process.
+ * @cancel:		    Required: Request cancellation of update. This op
+ *			    is called from the context of a different kernel
+ *			    thread, so race conditions need to be considered.
  * @cleanup:		    Optional: Complements the prepare()
  *			    function and is called at the completion
  *			    of the update, whether success or failure,
@@ -36,6 +39,7 @@ struct fw_upload_ops {
 	s32 (*write)(struct fw_upload *fwl, const u8 *data,
 		     u32 offset, u32 size);
 	u32 (*poll_complete)(struct fw_upload *fwl);
+	void (*cancel)(struct fw_upload *fwl);
 	void (*cleanup)(struct fw_upload *fwl);
 };
 
diff --git a/include/uapi/linux/firmware-upload.h b/include/uapi/linux/firmware-upload.h
index b8d96ee3f646..3bf985d27256 100644
--- a/include/uapi/linux/firmware-upload.h
+++ b/include/uapi/linux/firmware-upload.h
@@ -68,4 +68,6 @@ struct fw_upload_status {
 
 #define FW_UPLOAD_STATUS	_IOR(FW_UPLOAD_MAGIC, 1, struct fw_upload_status)
 
+#define FW_UPLOAD_CANCEL	_IO(FW_UPLOAD_MAGIC, 2)
+
 #endif /* _UAPI_LINUX_FW_UPLOAD_H */
-- 
2.25.1

Re: [RFC PATCH 0/5] Firmware Upload Framework

[Tom Rix]

On 11/10/21 5:13 PM, Russ Weight wrote:
> The Firmware Upload framework provides a common API for uploading firmware
> files to target devices. An example use case involves FPGA devices that
> load FPGA, Card BMC, and firmware images from FLASH when the card boots.
> Users need the ability to update these firmware images while the card is
> in use.
>
> Device drivers that instantiate the Firmware Upload class driver will
> interact with the target device to transfer and authenticate the firmware
> data. Uploads are performed in the context of a kernel worker thread in
> order to facilitate progress indicators during lengthy uploads.
>
> This driver was previously submitted in the context of the FPGA sub-
> system as the "FPGA Image Load Framework", but the framework is generic
> enough to support other devices as well. The previous submission of this
> patch-set can be viewed here:
>
> https://marc.info/?l=linux-kernel&m=163295640216820&w=2
>
> The n3000bmc-sec-update driver is the first driver to use the Firmware
> Upload API. A recent version of these patches can be viewed here:
>
> https://marc.info/?l=linux-kernel&m=163295697217095&w=2
>
> I don't think I am duplicating any functionality that is currently covered
> in the firmware subsystem. I appreciate your feedback on these patches.

This may be a common api for fpga/dfl-, but it is not likely common for 
general devices.

Could the scope of this patchset be reduced to just fpga/dfl for now ?

Something more like stratix10-rsu.

Tom

>
> - Russ
>
> Russ Weight (5):
>    firmware: Create firmware upload framework
>    firmware: upload: Enable firmware uploads
>    firmware: upload: Signal eventfd when complete
>    firmware: upload: Add status ioctl
>    firmware: upload: Enable cancel of firmware upload
>
>   .../driver-api/firmware/firmware-upload.rst   |  54 +++
>   Documentation/driver-api/firmware/index.rst   |   1 +
>   MAINTAINERS                                   |   9 +
>   drivers/firmware/Kconfig                      |   8 +
>   drivers/firmware/Makefile                     |   1 +
>   drivers/firmware/firmware-upload.c            | 413 ++++++++++++++++++
>   include/linux/firmware/firmware-upload.h      |  69 +++
>   include/uapi/linux/firmware-upload.h          |  73 ++++
>   8 files changed, 628 insertions(+)
>   create mode 100644 Documentation/driver-api/firmware/firmware-upload.rst
>   create mode 100644 drivers/firmware/firmware-upload.c
>   create mode 100644 include/linux/firmware/firmware-upload.h
>   create mode 100644 include/uapi/linux/firmware-upload.h
>

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Greg KH]

On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
> The Firmware Upload class driver provides a common API for uploading
> firmware files to devices.

That is exactly what the existing firmware api in the kernel is supposed
to be accomplishing.

If it is not doing what you need it to do, then you need to document the
heck out of why it is not, and why you need a different api for this.  I
do not see that here in this changelog at all :(

thanks,

greg k-h

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Russ Weight]

On 11/17/21 7:15 AM, Greg KH wrote:
> On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
>> The Firmware Upload class driver provides a common API for uploading
>> firmware files to devices.
> That is exactly what the existing firmware api in the kernel is supposed
> to be accomplishing.
>
> If it is not doing what you need it to do, then you need to document the
> heck out of why it is not, and why you need a different api for this.  I
> do not see that here in this changelog at all :(
This is part of the documentation included later in this patch. I can add
this to the changelog.

+Some devices load firmware from on-board FLASH when the card initializes.
+These cards do not require the request_firmware framework to load the
+firmware when the card boots, but they to require a utility to allow
+users to update the FLASH contents.

When you say "existing firmware api", I'm thinking request_firmware, which
requires that driver names be specified in the kernel config and wants to
load firmware automatically during device initialization.

Other support under driver/firmware is specific to certain vendors, devices.

If I add this information to the changelog, is that sufficient?

> thanks,
>
> greg k-h

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Greg KH]

On Wed, Nov 17, 2021 at 10:00:54AM -0800, Russ Weight wrote:
> 
> 
> On 11/17/21 7:15 AM, Greg KH wrote:
> > On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
> >> The Firmware Upload class driver provides a common API for uploading
> >> firmware files to devices.
> > That is exactly what the existing firmware api in the kernel is supposed
> > to be accomplishing.
> >
> > If it is not doing what you need it to do, then you need to document the
> > heck out of why it is not, and why you need a different api for this.  I
> > do not see that here in this changelog at all :(
> This is part of the documentation included later in this patch. I can add
> this to the changelog.
> 
> +Some devices load firmware from on-board FLASH when the card initializes.
> +These cards do not require the request_firmware framework to load the
> +firmware when the card boots, but they to require a utility to allow
> +users to update the FLASH contents.

There's no requirement that request_firmware only be done at boot time,
why not use it at any point in time?

> When you say "existing firmware api", I'm thinking request_firmware, which
> requires that driver names be specified in the kernel config and wants to
> load firmware automatically during device initialization.

It can be used at any time, why do you think it's restricted to init
time?

And I do not understand your issue with driver names.

> Other support under driver/firmware is specific to certain vendors, devices.
> 
> If I add this information to the changelog, is that sufficient?

Nope!

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Russ Weight]

On 11/17/21 10:18 AM, Greg KH wrote:
> On Wed, Nov 17, 2021 at 10:00:54AM -0800, Russ Weight wrote:
>>
>> On 11/17/21 7:15 AM, Greg KH wrote:
>>> On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
>>>> The Firmware Upload class driver provides a common API for uploading
>>>> firmware files to devices.
>>> That is exactly what the existing firmware api in the kernel is supposed
>>> to be accomplishing.
>>>
>>> If it is not doing what you need it to do, then you need to document the
>>> heck out of why it is not, and why you need a different api for this.  I
>>> do not see that here in this changelog at all :(
>> This is part of the documentation included later in this patch. I can add
>> this to the changelog.
>>
>> +Some devices load firmware from on-board FLASH when the card initializes.
>> +These cards do not require the request_firmware framework to load the
>> +firmware when the card boots, but they to require a utility to allow
>> +users to update the FLASH contents.
> There's no requirement that request_firmware only be done at boot time,
> why not use it at any point in time?
Calling request_firmware() is not restricted to boot time. But it requires
a firmware filename under /lib/firmware, and the convention is to specify the
filename in the kernel config. I don't see any support for a user to provide
a filename at run time to be uploaded to a device, and that is the use case
that I want to support.

>
>> When you say "existing firmware api", I'm thinking request_firmware, which
>> requires that driver names be specified in the kernel config and wants to
>> load firmware automatically during device initialization.
> It can be used at any time, why do you think it's restricted to init
> time?
>
> And I do not understand your issue with driver names.
Sorry - I meant so say "firmware file names"

In an earlier iteration of this patchset, you pointed out that allowing a user
to provide a filename for request_firmware() to use was a security issue.

The use case that I am targeting is to allow a user to provide an image file
to a device at run time.
>
>> Other support under driver/firmware is specific to certain vendors, devices.
>>
>> If I add this information to the changelog, is that sufficient?
> Nope!
>

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Greg KH]

On Wed, Nov 17, 2021 at 10:47:38AM -0800, Russ Weight wrote:
> 
> 
> On 11/17/21 10:18 AM, Greg KH wrote:
> > On Wed, Nov 17, 2021 at 10:00:54AM -0800, Russ Weight wrote:
> >>
> >> On 11/17/21 7:15 AM, Greg KH wrote:
> >>> On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
> >>>> The Firmware Upload class driver provides a common API for uploading
> >>>> firmware files to devices.
> >>> That is exactly what the existing firmware api in the kernel is supposed
> >>> to be accomplishing.
> >>>
> >>> If it is not doing what you need it to do, then you need to document the
> >>> heck out of why it is not, and why you need a different api for this.  I
> >>> do not see that here in this changelog at all :(
> >> This is part of the documentation included later in this patch. I can add
> >> this to the changelog.
> >>
> >> +Some devices load firmware from on-board FLASH when the card initializes.
> >> +These cards do not require the request_firmware framework to load the
> >> +firmware when the card boots, but they to require a utility to allow
> >> +users to update the FLASH contents.
> > There's no requirement that request_firmware only be done at boot time,
> > why not use it at any point in time?
> Calling request_firmware() is not restricted to boot time. But it requires
> a firmware filename under /lib/firmware

Not really, there are many locations it can be in.  See the different
configuration options we have.

But why would you want firmware in another location?

>, and the convention is to specify the
> filename in the kernel config.

That is not a requirement at all.

> I don't see any support for a user to provide a filename at run time
> to be uploaded to a device, and that is the use case that I want to
> support.

If that's the only difference here, please work with the existing
framework to add that tiny thing (i.e. pass in a name) to the current
framework.  Do not create a whole new one please.

> >> When you say "existing firmware api", I'm thinking request_firmware, which
> >> requires that driver names be specified in the kernel config and wants to
> >> load firmware automatically during device initialization.
> > It can be used at any time, why do you think it's restricted to init
> > time?
> >
> > And I do not understand your issue with driver names.
> Sorry - I meant so say "firmware file names"
> 
> In an earlier iteration of this patchset, you pointed out that allowing a user
> to provide a filename for request_firmware() to use was a security issue.

It is crazy, don't you think?

> The use case that I am targeting is to allow a user to provide an image file
> to a device at run time.

That's not a limitation of the existing firmware layer.

thanks,

greg k-h

Re: [RFC PATCH 0/5] Firmware Upload Framework

[Bjorn Andersson]

On Mon 15 Nov 05:57 PST 2021, Tom Rix wrote:

> 
> On 11/10/21 5:13 PM, Russ Weight wrote:
> > The Firmware Upload framework provides a common API for uploading firmware
> > files to target devices. An example use case involves FPGA devices that
> > load FPGA, Card BMC, and firmware images from FLASH when the card boots.
> > Users need the ability to update these firmware images while the card is
> > in use.
> > 
> > Device drivers that instantiate the Firmware Upload class driver will
> > interact with the target device to transfer and authenticate the firmware
> > data. Uploads are performed in the context of a kernel worker thread in
> > order to facilitate progress indicators during lengthy uploads.
> > 
> > This driver was previously submitted in the context of the FPGA sub-
> > system as the "FPGA Image Load Framework", but the framework is generic
> > enough to support other devices as well. The previous submission of this
> > patch-set can be viewed here:
> > 
> > https://marc.info/?l=linux-kernel&m=163295640216820&w=2
> > 
> > The n3000bmc-sec-update driver is the first driver to use the Firmware
> > Upload API. A recent version of these patches can be viewed here:
> > 
> > https://marc.info/?l=linux-kernel&m=163295697217095&w=2
> > 
> > I don't think I am duplicating any functionality that is currently covered
> > in the firmware subsystem. I appreciate your feedback on these patches.
> 
> This may be a common api for fpga/dfl-, but it is not likely common for
> general devices.
> 

During my years of hacking on device drivers I've run into the need for
being able to reflash/update firmware in things such as touchscreen
controllers, hdmi bridges, usb network devices and (embedded) usb hubs.

The implementation typically manifest itself as some sysfs or debugfs
knob which when written triggers a request_firmware() followed by the
operation to write the content to flash. But the result is often quite
hacky and requires that you store the firmware-to-be-written in some
path that will be looked at by request_firmware() - and hence these
patches often doesn't end up upstream.

So I'm certainly in favor of some generic way for drivers to expose an
interface for userspace to flash new firmware to their associated
hardware.

Regards,
Bjorn

> Could the scope of this patchset be reduced to just fpga/dfl for now ?
> 
> Something more like stratix10-rsu.
> 
> Tom
> 
> > 
> > - Russ
> > 
> > Russ Weight (5):
> >    firmware: Create firmware upload framework
> >    firmware: upload: Enable firmware uploads
> >    firmware: upload: Signal eventfd when complete
> >    firmware: upload: Add status ioctl
> >    firmware: upload: Enable cancel of firmware upload
> > 
> >   .../driver-api/firmware/firmware-upload.rst   |  54 +++
> >   Documentation/driver-api/firmware/index.rst   |   1 +
> >   MAINTAINERS                                   |   9 +
> >   drivers/firmware/Kconfig                      |   8 +
> >   drivers/firmware/Makefile                     |   1 +
> >   drivers/firmware/firmware-upload.c            | 413 ++++++++++++++++++
> >   include/linux/firmware/firmware-upload.h      |  69 +++
> >   include/uapi/linux/firmware-upload.h          |  73 ++++
> >   8 files changed, 628 insertions(+)
> >   create mode 100644 Documentation/driver-api/firmware/firmware-upload.rst
> >   create mode 100644 drivers/firmware/firmware-upload.c
> >   create mode 100644 include/linux/firmware/firmware-upload.h
> >   create mode 100644 include/uapi/linux/firmware-upload.h
> > 
> 

Re: [RFC PATCH 2/5] firmware: upload: Enable firmware uploads

[Bjorn Andersson]

On Wed 10 Nov 17:13 PST 2021, Russ Weight wrote:

> Extend the Firmware Upload framework to add a character device and provide
> IOCTL support (FW_UPLOAD_WRITE) for uploading a firmware file to a device.
> The IOCTL will return immediately, and the upload will begin in the context
> of a kernel worker thread.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  .../driver-api/firmware/firmware-upload.rst   |  21 ++
>  MAINTAINERS                                   |   1 +
>  drivers/firmware/firmware-upload.c            | 216 +++++++++++++++++-
>  include/linux/firmware/firmware-upload.h      |  27 +++
>  include/uapi/linux/firmware-upload.h          |  53 +++++
>  5 files changed, 317 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/linux/firmware-upload.h
> 
> diff --git a/Documentation/driver-api/firmware/firmware-upload.rst b/Documentation/driver-api/firmware/firmware-upload.rst
> index 9f38f18be97a..8a7c50087028 100644
> --- a/Documentation/driver-api/firmware/firmware-upload.rst
> +++ b/Documentation/driver-api/firmware/firmware-upload.rst
> @@ -15,3 +15,24 @@ Firmware Upload class driver will interact with the target device to
>  transfer and authenticate the firmware data. Uploads are performed in the
>  context of a kernel worker thread in order to facilitate progress
>  indicators during lengthy uploads.
> +
> +User API
> +========
> +
> +open
> +----
> +
> +An fw_upload device is opened exclusively to control a firmware upload.
> +The device must remain open throughout the duration of the firmware upload.
> +An attempt to close the device while an upload is in progress will block
> +until the firmware upload is complete.
> +
> +ioctl
> +-----
> +
> +FW_UPLOAD_WRITE:
> +

The use of an ioctl here means that one need some userspace tool to
drive the firmware update.

Compare this with the CONFIG_FW_LOADER_USER_HELPER_FALLBACK model, which
you can easily operate from a limited environment (e.g. a busybox based
environment on an embedded device):

  echo 1 > loading
  cat /some/binary > data
  echo 0 > loading

Regards,
Bjorn

> +The FW_UPLOAD_WRITE IOCTL passes in the address of a data buffer and starts
> +the firmware upload. This IOCTL returns immediately after assigning the work
> +to a kernel worker thread. This is an exclusive operation; an attempt to
> +start concurrent firmware uploads for the same device will fail with EBUSY.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19b3d3ccc406..4d59e8e11813 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7422,6 +7422,7 @@ S:	Maintained
>  F:	Documentation/driver-api/firmware/firmware-upload.rst
>  F:	drivers/firmware/firmware-upload.c
>  F:	include/linux/firmware/firmware-upload.h
> +F:	include/uapi/linux/firmware-upload.h
>  
>  FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
>  M:	Joshua Morris <josh.h.morris@us.ibm.com>
> diff --git a/drivers/firmware/firmware-upload.c b/drivers/firmware/firmware-upload.c
> index bc26df8b6b52..e930611262fb 100644
> --- a/drivers/firmware/firmware-upload.c
> +++ b/drivers/firmware/firmware-upload.c
> @@ -5,18 +5,189 @@
>   * Copyright (C) 2019-2021 Intel Corporation, Inc.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/firmware/firmware-upload.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 FW_UPLOAD_XA_LIMIT	XA_LIMIT(0, INT_MAX)
>  static DEFINE_XARRAY_ALLOC(fw_upload_xa);
>  
>  static struct class *fw_upload_class;
> +static dev_t fw_upload_devt;
>  
>  #define to_fw_upload(d) container_of(d, struct fw_upload, dev)
>  
> +static void fw_upload_prog_complete(struct fw_upload *fwl)
> +{
> +	mutex_lock(&fwl->lock);
> +	fwl->progress = FW_UPLOAD_PROG_IDLE;
> +	mutex_unlock(&fwl->lock);
> +}
> +
> +static void fw_upload_do_load(struct work_struct *work)
> +{
> +	struct fw_upload *fwl;
> +	s32 ret, offset = 0;
> +
> +	fwl = container_of(work, struct fw_upload, work);
> +
> +	if (fwl->driver_unload) {
> +		fwl->err_code = FW_UPLOAD_ERR_CANCELED;
> +		goto idle_exit;
> +	}
> +
> +	get_device(&fwl->dev);
> +	if (!try_module_get(fwl->dev.parent->driver->owner)) {
> +		fwl->err_code = FW_UPLOAD_ERR_BUSY;
> +		goto putdev_exit;
> +	}
> +
> +	fwl->progress = FW_UPLOAD_PROG_PREPARING;
> +	ret = fwl->ops->prepare(fwl, fwl->data, fwl->remaining_size);
> +	if (ret) {
> +		fwl->err_code = ret;
> +		goto modput_exit;
> +	}
> +
> +	fwl->progress = FW_UPLOAD_PROG_WRITING;
> +	while (fwl->remaining_size) {
> +		ret = fwl->ops->write(fwl, fwl->data, offset,
> +					fwl->remaining_size);
> +		if (ret <= 0) {
> +			if (!ret) {
> +				dev_warn(&fwl->dev,
> +					 "write-op wrote zero data\n");
> +				ret = -FW_UPLOAD_ERR_RW_ERROR;
> +			}
> +			fwl->err_code = -ret;
> +			goto done;
> +		}
> +
> +		fwl->remaining_size -= ret;
> +		offset += ret;
> +	}
> +
> +	fwl->progress = FW_UPLOAD_PROG_PROGRAMMING;
> +	ret = fwl->ops->poll_complete(fwl);
> +	if (ret)
> +		fwl->err_code = ret;
> +
> +done:
> +	if (fwl->ops->cleanup)
> +		fwl->ops->cleanup(fwl);
> +
> +modput_exit:
> +	module_put(fwl->dev.parent->driver->owner);
> +
> +putdev_exit:
> +	put_device(&fwl->dev);
> +
> +idle_exit:
> +	/*
> +	 * Note: fwl->remaining_size is left unmodified here to provide
> +	 * additional information on errors. It will be reinitialized when
> +	 * the next firmeware upload begins.
> +	 */
> +	vfree(fwl->data);
> +	fwl->data = NULL;
> +	fw_upload_prog_complete(fwl);
> +}
> +
> +static int fw_upload_ioctl_write(struct fw_upload *fwl, unsigned long arg)
> +{
> +	struct fw_upload_write wb;
> +	unsigned long minsz;
> +	u8 *buf;
> +
> +	if (fwl->driver_unload || fwl->progress != FW_UPLOAD_PROG_IDLE)
> +		return -EBUSY;
> +
> +	minsz = offsetofend(struct fw_upload_write, buf);
> +	if (copy_from_user(&wb, (void __user *)arg, minsz))
> +		return -EFAULT;
> +
> +	if (wb.flags)
> +		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;
> +	}
> +
> +	fwl->data = buf;
> +	fwl->remaining_size = wb.size;
> +	fwl->err_code = 0;
> +	fwl->progress = FW_UPLOAD_PROG_STARTING;
> +	queue_work(system_long_wq, &fwl->work);
> +
> +	return 0;
> +}
> +
> +static long fw_upload_ioctl(struct file *filp, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	struct fw_upload *fwl = filp->private_data;
> +	int ret = -ENOTTY;
> +
> +	switch (cmd) {
> +	case FW_UPLOAD_WRITE:
> +		mutex_lock(&fwl->lock);
> +		ret = fw_upload_ioctl_write(fwl, arg);
> +		mutex_unlock(&fwl->lock);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int fw_upload_open(struct inode *inode, struct file *filp)
> +{
> +	struct fw_upload *fwl = container_of(inode->i_cdev,
> +						     struct fw_upload, cdev);
> +
> +	if (atomic_cmpxchg(&fwl->opened, 0, 1))
> +		return -EBUSY;
> +
> +	filp->private_data = fwl;
> +
> +	return 0;
> +}
> +
> +static int fw_upload_release(struct inode *inode, struct file *filp)
> +{
> +	struct fw_upload *fwl = filp->private_data;
> +
> +	mutex_lock(&fwl->lock);
> +	if (fwl->progress == FW_UPLOAD_PROG_IDLE) {
> +		mutex_unlock(&fwl->lock);
> +		goto close_exit;
> +	}
> +
> +	mutex_unlock(&fwl->lock);
> +	flush_work(&fwl->work);
> +
> +close_exit:
> +	atomic_set(&fwl->opened, 0);
> +
> +	return 0;
> +}
> +
> +static const struct file_operations fw_upload_fops = {
> +	.owner = THIS_MODULE,
> +	.open = fw_upload_open,
> +	.release = fw_upload_release,
> +	.unlocked_ioctl = fw_upload_ioctl,
> +};
> +
>  /**
>   * fw_upload_register - create and register a Firmware Upload Device
>   *
> @@ -35,6 +206,11 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
>  	struct fw_upload *fwl;
>  	int ret;
>  
> +	if (!ops || !ops->prepare || !ops->write || !ops->poll_complete) {
> +		dev_err(parent, "Attempt to register without all required ops\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	fwl = kzalloc(sizeof(*fwl), GFP_KERNEL);
>  	if (!fwl)
>  		return ERR_PTR(-ENOMEM);
> @@ -48,9 +224,13 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
>  
>  	fwl->priv = priv;
>  	fwl->ops = ops;
> +	fwl->err_code = 0;
> +	fwl->progress = FW_UPLOAD_PROG_IDLE;
> +	INIT_WORK(&fwl->work, fw_upload_do_load);
>  
>  	fwl->dev.class = fw_upload_class;
>  	fwl->dev.parent = parent;
> +	fwl->dev.devt = MKDEV(MAJOR(fw_upload_devt), fwl->dev.id);
>  
>  	ret = dev_set_name(&fwl->dev, "fw_upload%d", fwl->dev.id);
>  	if (ret) {
> @@ -65,6 +245,16 @@ fw_upload_register(struct device *parent, const struct fw_upload_ops *ops,
>  		return ERR_PTR(ret);
>  	}
>  
> +	cdev_init(&fwl->cdev, &fw_upload_fops);
> +	fwl->cdev.owner = parent->driver->owner;
> +	cdev_set_parent(&fwl->cdev, &fwl->dev.kobj);
> +
> +	ret = cdev_add(&fwl->cdev, fwl->dev.devt, 1);
> +	if (ret) {
> +		put_device(&fwl->dev);
> +		return ERR_PTR(ret);
> +	}
> +
>  	return fwl;
>  
>  error_device:
> @@ -83,10 +273,23 @@ EXPORT_SYMBOL_GPL(fw_upload_register);
>   * @fwl: pointer to struct fw_upload
>   *
>   * 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 fw_upload_unregister(struct fw_upload *fwl)
>  {
> +	mutex_lock(&fwl->lock);
> +	fwl->driver_unload = true;
> +	if (fwl->progress == FW_UPLOAD_PROG_IDLE) {
> +		mutex_unlock(&fwl->lock);
> +		goto unregister;
> +	}
> +
> +	mutex_unlock(&fwl->lock);
> +	flush_work(&fwl->work);
> +
> +unregister:
> +	cdev_del(&fwl->cdev);
>  	device_unregister(&fwl->dev);
>  }
>  EXPORT_SYMBOL_GPL(fw_upload_unregister);
> @@ -101,19 +304,30 @@ static void fw_upload_dev_release(struct device *dev)
>  
>  static int __init fw_upload_class_init(void)
>  {
> +	int ret;
>  	pr_info("Firmware Upload Framework\n");
>  
>  	fw_upload_class = class_create(THIS_MODULE, "fw_upload");
>  	if (IS_ERR(fw_upload_class))
>  		return PTR_ERR(fw_upload_class);
>  
> +	ret = alloc_chrdev_region(&fw_upload_devt, 0, MINORMASK,
> +				  "fw_upload");
> +	if (ret)
> +		goto exit_destroy_class;
> +
>  	fw_upload_class->dev_release = fw_upload_dev_release;
>  
>  	return 0;
> +
> +exit_destroy_class:
> +	class_destroy(fw_upload_class);
> +	return ret;
>  }
>  
>  static void __exit fw_upload_class_exit(void)
>  {
> +	unregister_chrdev_region(fw_upload_devt, MINORMASK);
>  	class_destroy(fw_upload_class);
>  	WARN_ON(!xa_empty(&fw_upload_xa));
>  }
> diff --git a/include/linux/firmware/firmware-upload.h b/include/linux/firmware/firmware-upload.h
> index 767e0bdded7b..f80d5ba0c3f1 100644
> --- a/include/linux/firmware/firmware-upload.h
> +++ b/include/linux/firmware/firmware-upload.h
> @@ -7,22 +7,49 @@
>  #ifndef _LINUX_FIRMWARE_UPLOAD_H
>  #define _LINUX_FIRMWARE_UPLOAD_H
>  
> +#include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/types.h>
> +#include <uapi/linux/firmware-upload.h>
>  
>  struct fw_upload;
>  
>  /**
>   * struct fw_upload_ops - device specific operations
> + * @prepare:		    Required: Prepare secure update
> + * @write:		    Required: The write() op receives the remaining
> + *			    size to be written and must return the actual
> + *			    size written or a negative error code. The write()
> + *			    op will be called repeatedly until all data is
> + *			    written.
> + * @poll_complete:	    Required: Check for the completion of the
> + *			    HW authentication/programming process.
> + * @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 fw_upload_ops {
> +	u32 (*prepare)(struct fw_upload *fwl, const u8 *data, u32 size);
> +	s32 (*write)(struct fw_upload *fwl, const u8 *data,
> +		     u32 offset, u32 size);
> +	u32 (*poll_complete)(struct fw_upload *fwl);
> +	void (*cleanup)(struct fw_upload *fwl);
>  };
>  
>  struct fw_upload {
>  	struct device dev;
> +	struct cdev cdev;
>  	const struct fw_upload_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;			/* upload error code */
> +	bool driver_unload;
>  	void *priv;
>  };
>  
> diff --git a/include/uapi/linux/firmware-upload.h b/include/uapi/linux/firmware-upload.h
> new file mode 100644
> index 000000000000..034550071911
> --- /dev/null
> +++ b/include/uapi/linux/firmware-upload.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header File for Firmware Upload User API
> + *
> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
> + *
> + */
> +
> +#ifndef _UAPI_LINUX_FW_UPLOAD__H
> +#define _UAPI_LINUX_FW_UPLOAD__H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define FW_UPLOAD_MAGIC 0xB9
> +
> +/* Firmware upload progress codes */
> +#define FW_UPLOAD_PROG_IDLE		0
> +#define FW_UPLOAD_PROG_STARTING	1
> +#define FW_UPLOAD_PROG_PREPARING	2
> +#define FW_UPLOAD_PROG_WRITING		3
> +#define FW_UPLOAD_PROG_PROGRAMMING	4
> +#define FW_UPLOAD_PROG_MAX		5
> +
> +/* Firmware upload error codes */
> +#define FW_UPLOAD_ERR_HW_ERROR		1
> +#define FW_UPLOAD_ERR_TIMEOUT		2
> +#define FW_UPLOAD_ERR_CANCELED		3
> +#define FW_UPLOAD_ERR_BUSY		4
> +#define FW_UPLOAD_ERR_INVALID_SIZE	5
> +#define FW_UPLOAD_ERR_RW_ERROR		6
> +#define FW_UPLOAD_ERR_WEAROUT		7
> +#define FW_UPLOAD_ERR_MAX		8
> +
> +/**
> + * FW_UPLOAD_WRITE - _IOW(FW_UPLOAD_MAGIC, 0,
> + *				struct fw_upload_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 fw_upload_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 FW_UPLOAD_WRITE	_IOW(FW_UPLOAD_MAGIC, 0, struct fw_upload_write)
> +
> +#endif /* _UAPI_LINUX_FW_UPLOAD_H */
> -- 
> 2.25.1
> 

Re: [RFC PATCH 1/5] firmware: Create firmware upload framework

[Russ Weight]

On 11/17/21 10:54 AM, Greg KH wrote:
> On Wed, Nov 17, 2021 at 10:47:38AM -0800, Russ Weight wrote:
>>
>> On 11/17/21 10:18 AM, Greg KH wrote:
>>> On Wed, Nov 17, 2021 at 10:00:54AM -0800, Russ Weight wrote:
>>>> On 11/17/21 7:15 AM, Greg KH wrote:
>>>>> On Wed, Nov 10, 2021 at 05:13:41PM -0800, Russ Weight wrote:
>>>>>> The Firmware Upload class driver provides a common API for uploading
>>>>>> firmware files to devices.
>>>>> That is exactly what the existing firmware api in the kernel is supposed
>>>>> to be accomplishing.
>>>>>
>>>>> If it is not doing what you need it to do, then you need to document the
>>>>> heck out of why it is not, and why you need a different api for this.  I
>>>>> do not see that here in this changelog at all :(
>>>> This is part of the documentation included later in this patch. I can add
>>>> this to the changelog.
>>>>
>>>> +Some devices load firmware from on-board FLASH when the card initializes.
>>>> +These cards do not require the request_firmware framework to load the
>>>> +firmware when the card boots, but they to require a utility to allow
>>>> +users to update the FLASH contents.
>>> There's no requirement that request_firmware only be done at boot time,
>>> why not use it at any point in time?
>> Calling request_firmware() is not restricted to boot time. But it requires
>> a firmware filename under /lib/firmware
> Not really, there are many locations it can be in.  See the different
> configuration options we have.
>
> But why would you want firmware in another location?
My current use case is for a user to upload a new, signed FPGA image to an FPGA
card. The card BMC authenticates and stores the data in FLASH. From the
perspective of the card, the image in FLASH is analogous to a firmware file
for another device being stored in /lib/firmware. For the FPGA images, there
is no real value to also storing the files in /lib/firmware (or anywhere else on
the system).

>
>> , and the convention is to specify the
>> filename in the kernel config.
> That is not a requirement at all.
>
>> I don't see any support for a user to provide a filename at run time
>> to be uploaded to a device, and that is the use case that I want to
>> support.
> If that's the only difference here, please work with the existing
> framework to add that tiny thing (i.e. pass in a name) to the current
> framework.  Do not create a whole new one please.
I think another fundamental difference is that the request_firmware() API is
a driver API for the device driver to do a data "pull". The firmware-upload API
is a user API for doing a data "push" (prepare(), write(), poll_complete()) to
the lower-level driver.

I did look at the backup option of the request_firmware() framework for writing
image data via sysfs. That's a possibility, but I thought that we would be abusing
the intent. I can look more at that option...

>
>>>> When you say "existing firmware api", I'm thinking request_firmware, which
>>>> requires that driver names be specified in the kernel config and wants to
>>>> load firmware automatically during device initialization.
>>> It can be used at any time, why do you think it's restricted to init
>>> time?
>>>
>>> And I do not understand your issue with driver names.
>> Sorry - I meant so say "firmware file names"
>>
>> In an earlier iteration of this patchset, you pointed out that allowing a user
>> to provide a filename for request_firmware() to use was a security issue.
> It is crazy, don't you think?
>
>> The use case that I am targeting is to allow a user to provide an image file
>> to a device at run time.
> That's not a limitation of the existing firmware layer.
>
> thanks,
>
> greg k-h

Re: [RFC PATCH 0/5] Firmware Upload Framework

[Tom Rix]

On 11/17/21 11:20 AM, Bjorn Andersson wrote:
> On Mon 15 Nov 05:57 PST 2021, Tom Rix wrote:
>
>> On 11/10/21 5:13 PM, Russ Weight wrote:
>>> The Firmware Upload framework provides a common API for uploading firmware
>>> files to target devices. An example use case involves FPGA devices that
>>> load FPGA, Card BMC, and firmware images from FLASH when the card boots.
>>> Users need the ability to update these firmware images while the card is
>>> in use.
>>>
>>> Device drivers that instantiate the Firmware Upload class driver will
>>> interact with the target device to transfer and authenticate the firmware
>>> data. Uploads are performed in the context of a kernel worker thread in
>>> order to facilitate progress indicators during lengthy uploads.
>>>
>>> This driver was previously submitted in the context of the FPGA sub-
>>> system as the "FPGA Image Load Framework", but the framework is generic
>>> enough to support other devices as well. The previous submission of this
>>> patch-set can be viewed here:
>>>
>>> https://marc.info/?l=linux-kernel&m=163295640216820&w=2
>>>
>>> The n3000bmc-sec-update driver is the first driver to use the Firmware
>>> Upload API. A recent version of these patches can be viewed here:
>>>
>>> https://marc.info/?l=linux-kernel&m=163295697217095&w=2
>>>
>>> I don't think I am duplicating any functionality that is currently covered
>>> in the firmware subsystem. I appreciate your feedback on these patches.
>> This may be a common api for fpga/dfl-, but it is not likely common for
>> general devices.
>>
> During my years of hacking on device drivers I've run into the need for
> being able to reflash/update firmware in things such as touchscreen
> controllers, hdmi bridges, usb network devices and (embedded) usb hubs.
>
> The implementation typically manifest itself as some sysfs or debugfs
> knob which when written triggers a request_firmware() followed by the
> operation to write the content to flash. But the result is often quite
> hacky and requires that you store the firmware-to-be-written in some
> path that will be looked at by request_firmware() - and hence these
> patches often doesn't end up upstream.
>
> So I'm certainly in favor of some generic way for drivers to expose an
> interface for userspace to flash new firmware to their associated
> hardware.

The image to be loaded is not really firmware and not really a partial 
reconfiguration image.  It is both.

Which image is used depends on the end user's workload for the n3000 and 
could change and need reloading day to day.

Because the n3000 is unusable without this change, I would like to see 
updating working first for the n3000.

Then the interface generalized as other devices are found that have a 
similar use case.

This is a device specific feature so it should go somewhere like 
drivers/fpga/dfl-n3000-update.c

Tom

>
> Regards,
> Bjorn
>
>> Could the scope of this patchset be reduced to just fpga/dfl for now ?
>>
>> Something more like stratix10-rsu.
>>
>> Tom
>>
>>> - Russ
>>>
>>> Russ Weight (5):
>>>     firmware: Create firmware upload framework
>>>     firmware: upload: Enable firmware uploads
>>>     firmware: upload: Signal eventfd when complete
>>>     firmware: upload: Add status ioctl
>>>     firmware: upload: Enable cancel of firmware upload
>>>
>>>    .../driver-api/firmware/firmware-upload.rst   |  54 +++
>>>    Documentation/driver-api/firmware/index.rst   |   1 +
>>>    MAINTAINERS                                   |   9 +
>>>    drivers/firmware/Kconfig                      |   8 +
>>>    drivers/firmware/Makefile                     |   1 +
>>>    drivers/firmware/firmware-upload.c            | 413 ++++++++++++++++++
>>>    include/linux/firmware/firmware-upload.h      |  69 +++
>>>    include/uapi/linux/firmware-upload.h          |  73 ++++
>>>    8 files changed, 628 insertions(+)
>>>    create mode 100644 Documentation/driver-api/firmware/firmware-upload.rst
>>>    create mode 100644 drivers/firmware/firmware-upload.c
>>>    create mode 100644 include/linux/firmware/firmware-upload.h
>>>    create mode 100644 include/uapi/linux/firmware-upload.h
>>>

Re: [RFC PATCH 0/5] Firmware Upload Framework

[Greg KH]

On Thu, Dec 09, 2021 at 07:15:06AM -0800, Tom Rix wrote:
> 
> On 11/17/21 11:20 AM, Bjorn Andersson wrote:
> > On Mon 15 Nov 05:57 PST 2021, Tom Rix wrote:
> > 
> > > On 11/10/21 5:13 PM, Russ Weight wrote:
> > > > The Firmware Upload framework provides a common API for uploading firmware
> > > > files to target devices. An example use case involves FPGA devices that
> > > > load FPGA, Card BMC, and firmware images from FLASH when the card boots.
> > > > Users need the ability to update these firmware images while the card is
> > > > in use.
> > > > 
> > > > Device drivers that instantiate the Firmware Upload class driver will
> > > > interact with the target device to transfer and authenticate the firmware
> > > > data. Uploads are performed in the context of a kernel worker thread in
> > > > order to facilitate progress indicators during lengthy uploads.
> > > > 
> > > > This driver was previously submitted in the context of the FPGA sub-
> > > > system as the "FPGA Image Load Framework", but the framework is generic
> > > > enough to support other devices as well. The previous submission of this
> > > > patch-set can be viewed here:
> > > > 
> > > > https://marc.info/?l=linux-kernel&m=163295640216820&w=2
> > > > 
> > > > The n3000bmc-sec-update driver is the first driver to use the Firmware
> > > > Upload API. A recent version of these patches can be viewed here:
> > > > 
> > > > https://marc.info/?l=linux-kernel&m=163295697217095&w=2
> > > > 
> > > > I don't think I am duplicating any functionality that is currently covered
> > > > in the firmware subsystem. I appreciate your feedback on these patches.
> > > This may be a common api for fpga/dfl-, but it is not likely common for
> > > general devices.
> > > 
> > During my years of hacking on device drivers I've run into the need for
> > being able to reflash/update firmware in things such as touchscreen
> > controllers, hdmi bridges, usb network devices and (embedded) usb hubs.
> > 
> > The implementation typically manifest itself as some sysfs or debugfs
> > knob which when written triggers a request_firmware() followed by the
> > operation to write the content to flash. But the result is often quite
> > hacky and requires that you store the firmware-to-be-written in some
> > path that will be looked at by request_firmware() - and hence these
> > patches often doesn't end up upstream.
> > 
> > So I'm certainly in favor of some generic way for drivers to expose an
> > interface for userspace to flash new firmware to their associated
> > hardware.
> 
> The image to be loaded is not really firmware and not really a partial
> reconfiguration image.  It is both.

The kernel does not care.  It is a "blob of data to be sent to the
device".  Traditionally we have called this "firmware", and so the api
is called that.  But it could be anything, the kernel does not parse it
or care at all, it is just a pipe from userspace to the device to
transfer the data.

> Which image is used depends on the end user's workload for the n3000 and
> could change and need reloading day to day.
> 
> Because the n3000 is unusable without this change, I would like to see
> updating working first for the n3000.
> 
> Then the interface generalized as other devices are found that have a
> similar use case.
> 
> This is a device specific feature so it should go somewhere like
> drivers/fpga/dfl-n3000-update.c

Then have that driver call the firmware api, that's what it is there
for.

thanks,

greg k-h

Re: [RFC PATCH 0/5] Firmware Upload Framework

[Tom Rix]

On 12/9/21 7:34 AM, Greg KH wrote:
> On Thu, Dec 09, 2021 at 07:15:06AM -0800, Tom Rix wrote:
>> On 11/17/21 11:20 AM, Bjorn Andersson wrote:
>>> On Mon 15 Nov 05:57 PST 2021, Tom Rix wrote:
>>>
>>>> On 11/10/21 5:13 PM, Russ Weight wrote:
>>>>> The Firmware Upload framework provides a common API for uploading firmware
>>>>> files to target devices. An example use case involves FPGA devices that
>>>>> load FPGA, Card BMC, and firmware images from FLASH when the card boots.
>>>>> Users need the ability to update these firmware images while the card is
>>>>> in use.
>>>>>
>>>>> Device drivers that instantiate the Firmware Upload class driver will
>>>>> interact with the target device to transfer and authenticate the firmware
>>>>> data. Uploads are performed in the context of a kernel worker thread in
>>>>> order to facilitate progress indicators during lengthy uploads.
>>>>>
>>>>> This driver was previously submitted in the context of the FPGA sub-
>>>>> system as the "FPGA Image Load Framework", but the framework is generic
>>>>> enough to support other devices as well. The previous submission of this
>>>>> patch-set can be viewed here:
>>>>>
>>>>> https://marc.info/?l=linux-kernel&m=163295640216820&w=2
>>>>>
>>>>> The n3000bmc-sec-update driver is the first driver to use the Firmware
>>>>> Upload API. A recent version of these patches can be viewed here:
>>>>>
>>>>> https://marc.info/?l=linux-kernel&m=163295697217095&w=2
>>>>>
>>>>> I don't think I am duplicating any functionality that is currently covered
>>>>> in the firmware subsystem. I appreciate your feedback on these patches.
>>>> This may be a common api for fpga/dfl-, but it is not likely common for
>>>> general devices.
>>>>
>>> During my years of hacking on device drivers I've run into the need for
>>> being able to reflash/update firmware in things such as touchscreen
>>> controllers, hdmi bridges, usb network devices and (embedded) usb hubs.
>>>
>>> The implementation typically manifest itself as some sysfs or debugfs
>>> knob which when written triggers a request_firmware() followed by the
>>> operation to write the content to flash. But the result is often quite
>>> hacky and requires that you store the firmware-to-be-written in some
>>> path that will be looked at by request_firmware() - and hence these
>>> patches often doesn't end up upstream.
>>>
>>> So I'm certainly in favor of some generic way for drivers to expose an
>>> interface for userspace to flash new firmware to their associated
>>> hardware.
>> The image to be loaded is not really firmware and not really a partial
>> reconfiguration image.  It is both.
> The kernel does not care.  It is a "blob of data to be sent to the
> device".  Traditionally we have called this "firmware", and so the api
> is called that.  But it could be anything, the kernel does not parse it
> or care at all, it is just a pipe from userspace to the device to
> transfer the data.
>
>> Which image is used depends on the end user's workload for the n3000 and
>> could change and need reloading day to day.
>>
>> Because the n3000 is unusable without this change, I would like to see
>> updating working first for the n3000.
>>
>> Then the interface generalized as other devices are found that have a
>> similar use case.
>>
>> This is a device specific feature so it should go somewhere like
>> drivers/fpga/dfl-n3000-update.c
> Then have that driver call the firmware api, that's what it is there
> for.

Yes.

My point is this patchset has had several iterations on inventing a 
general interface that is used only by the n3000 board and possibly some 
of the other dfl boards.  Let's not do another round of a general 
interface inventing.  Let's focus on solving only n3000.

As an example of reducing the scope of the changes,

My understanding of the n3000 is that it does not have hw support for 
partial reconfiguration, there is only this full reimaging using the 
boards bmc, this is the intel-m10-bmc-sec-update change.  A solution 
would be to have a n3000 version of the fpga_manager_ops, with the ops 
using the intel-m10-bmc-sec-update lower level routines and reuse the 
fpga_manager's firmware api high level calls.

Tom


>
> thanks,
>
> greg k-h
>