[RFC PATCH v1 0/1] Extend FPGA manager with async image updates

[RFC PATCH v1 0/1] Extend FPGA manager with async image updates

[Russ Weight]

Hi Moritz,

This RFC patch is a follow-on to my evaluation of the possibility of merging
secure update functionality into the FPGA Manager. My earlier RFC email
is here: https://marc.info/?l=linux-fpga&m=161783978715092&w=2.

This RFC patch implements the core fpga_sec_mgr_update() function from the
security manager patchset:
https://marc.info/?l=linux-fpga&m=161525020621455&w=2.

Specifically, it is a port of this patch:
https://marc.info/?l=linux-fpga&m=161525020721457&w=2

I think this patch provides enough context for further discussion. It
extends functionality without leveraging any common code (because I
didn't see an opportunity to share code).

In this patch, I am using the term "async" (in reference to the kernel
worker thread) instead of the term "security". While the security manager
patches were originally created specifically to support Intel secure image
updates, there is nothing inherently secure about the driver support, other
than the fact that the operation is essentially atomic: one write to the
"filename" sysfs entry is all that is required from user-space to do
an update. Our convention is to use signed, self-describing images that are
interpreted by the card BMC, but one could use a non-signed image or even
interpret the contents of the image based on the context of the parent
driver. I think the main differentiating factors are:

(1) sysfs interface: an update is an atomic operation accomplished with a
    single write.
(2) self-describing: The type of information contained in the FPGA Manager
    fpga_image_info structure would have to be included in the image file
    and interpreted by the parent driver (not the class driver).
(3) asynchronous: A write to the "filename" sysfs node write will return
    immediately and the update will proceed in the context of a kernel
    worker thread. Additional sysfs interfaces would be used to monitor the
    progress and determine the ultimate success or failure of the update.
(4) No notion of regions, bridges, or FPGA state. For Intel PAC cards, some
    image files don't even contain an FPGA image. If they do, the image could
    become active on the next power-cycle, or it could be activated through
    some other trigger mechanism.

Can existing ops be leveraged?
==============================
write: The current write op _could_ be used if the prototype were modified to
accept an additional offset parameter. For the async update, writes are done
in chunks, and the target offset needs to be passed on each write.

write_init and write_complete _could_ be used without change.

Other ops would have to be added: cancel, cleanup, hw_errinfo

I chose to implement all new ops because of the return data types. The fpga-mgr
ops use the standard negative errno values. More descriptive and relevant error
information can be provided via sysfs by defining a set of enum error codes.
For example, it is very helpful to be able to tell the user that they are in a
FLASH-wearout state, but standard errno values do not facilitate the
communication of a wearout error.

Would it be better to share the two or three ops that can be shared, and be
content with the standard error numbers? Or is it OK to use separate ops?

Should async updates be available via exported symbol?
======================================================
As I understand it, current image updates through the FPGA Manager are all
started with a call to the exported symbol fpga_mgr_load(). It would be
possible to export an fpga_mgr_async_load() symbol, but there would need
to be additional exported symbols to facilitate the collection of status
information. Is there a use case for this?

Can a common update function be used?
=====================================
fpga_mgr_async_update() is analagous to fpga_mgr_load(). However, all async
updates use the request_firmware framework. The FPGA Manager supports two
separate flows: a single image buffer or scatter-gather. It would be possible
to integrate these features for async updates, but I'm not sure that there is a
need for such functionality.

I look forward to your feedback. Do you see value in integrating the two
drivers? Should I continue this process?

- Russ

Russ Weight (1):
  fpga: mgr: enable asynchronous image updates

 .../ABI/testing/sysfs-class-fpga-manager      |   9 +
 drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
 include/linux/fpga/fpga-mgr.h                 |  52 +++++
 3 files changed, 259 insertions(+), 1 deletion(-)

-- 
2.25.1

[RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Russ Weight]

Extend the FPGA Manager class driver to support asynchronous image
updates in the context of a kernel worker thread. These updates are
managed through sysfs file entries. This patch specifically creates
the async_update/filename sysfs node that can be used to initiate an
asynchronous update. The filename of a update image can be written to
this sysfs entry to cause the update to occur.

The write of the filename will return immediately, and the update will
begin in the context of a kernel worker thread.  This tool utilizes
the request_firmware framework, which requires that the image file
reside under /lib/firmware.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 .../ABI/testing/sysfs-class-fpga-manager      |   9 +
 drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
 include/linux/fpga/fpga-mgr.h                 |  52 +++++
 3 files changed, 259 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
index d78689c357a5..39ff8764f261 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-manager
+++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
@@ -58,3 +58,12 @@ Description:	Read fpga manager status as a string.
 						  reconfiguration hardware
 		* reconfig fifo overflow error	- FIFO overflow detected by
 						  reconfiguration hardware
+
+What:		/sys/class/fpga_manager/<fpga>/async_update/filename
+Date:		April 2021
+KernelVersion:	5.13
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Write only. Write the filename of a self-describing  image
+		file to this sysfs file to initiate an image update. The
+		write will return immediately, and the image update will
+		proceed in the background.
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index b85bc47c91a9..5d4449c82af5 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -8,6 +8,7 @@
  * With code from the mailing list:
  * Copyright (C) 2013 Xilinx, Inc.
  */
+#include <linux/device.h>
 #include <linux/firmware.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/idr.h>
@@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
 	&dev_attr_status.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(fpga_mgr);
+
+static const struct attribute_group fpga_mgr_group = {
+	.attrs = fpga_mgr_attrs,
+};
+
+#define WRITE_BLOCK_SIZE 0x4000	/* Update remaining_size every 0x4000 bytes */
+
+static void fpga_async_dev_error(struct fpga_manager *mgr,
+				 enum fpga_async_err err_code)
+{
+	mgr->async_update.err_code = err_code;
+	mgr->mops->async_cancel(mgr);
+}
+
+static void progress_complete(struct fpga_manager *mgr)
+{
+	struct fpga_async_update *update = &mgr->async_update;
+
+	mutex_lock(&update->lock);
+	update->progress = FPGA_ASYNC_PROG_IDLE;
+	complete_all(&update->done);
+	mutex_unlock(&update->lock);
+}
+
+static void fpga_mgr_async_update(struct work_struct *work)
+{
+	struct fpga_async_update *update;
+	u32 size, blk_size, offset = 0;
+	struct fpga_manager *mgr;
+	const struct firmware *fw;
+	enum fpga_async_err ret;
+
+	update = container_of(work, struct fpga_async_update, work);
+	mgr = container_of(update, struct fpga_manager, async_update);
+
+	get_device(&mgr->dev);
+	if (request_firmware(&fw, update->filename, &mgr->dev)) {
+		update->err_code = FPGA_ASYNC_ERR_FILE_READ;
+		goto idle_exit;
+	}
+
+	update->data = fw->data;
+	update->remaining_size = fw->size;
+
+	if (!try_module_get(mgr->dev.parent->driver->owner)) {
+		update->err_code = FPGA_ASYNC_ERR_BUSY;
+		goto release_fw_exit;
+	}
+
+	update->progress = FPGA_ASYNC_PROG_PREPARING;
+	ret = mgr->mops->async_write_init(mgr, fw->size);
+	if (ret != FPGA_ASYNC_ERR_NONE) {
+		fpga_async_dev_error(mgr, ret);
+		goto modput_exit;
+	}
+
+	update->progress = FPGA_ASYNC_PROG_WRITING;
+	size = update->remaining_size;
+	while (size) {
+		blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
+		size -= blk_size;
+		ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
+						 offset, blk_size);
+		if (ret != FPGA_ASYNC_ERR_NONE) {
+			fpga_async_dev_error(mgr, ret);
+			goto done;
+		}
+
+		update->remaining_size = size;
+		offset += blk_size;
+	}
+
+	update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
+	ret = mgr->mops->async_write_complete(mgr);
+	if (ret != FPGA_ASYNC_ERR_NONE)
+		fpga_async_dev_error(mgr, ret);
+
+done:
+	if (mgr->mops->async_cleanup)
+		mgr->mops->async_cleanup(mgr);
+
+modput_exit:
+	module_put(mgr->dev.parent->driver->owner);
+
+release_fw_exit:
+	update->data = NULL;
+	release_firmware(fw);
+
+idle_exit:
+	/*
+	 * Note: update->remaining_size is left unmodified here to
+	 * provide additional information on errors. It will be
+	 * reinitialized when the next async update begins.
+	 */
+	kfree(update->filename);
+	update->filename = NULL;
+	put_device(&mgr->dev);
+	progress_complete(mgr);
+}
+
+static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	struct fpga_async_update *update = &mgr->async_update;
+	int ret = count;
+
+	if (count == 0 || count >= PATH_MAX)
+		return -EINVAL;
+
+	mutex_lock(&update->lock);
+	if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
+		ret = -EBUSY;
+		goto unlock_exit;
+	}
+
+	update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
+	if (!update->filename) {
+		ret = -ENOMEM;
+		goto unlock_exit;
+	}
+
+	update->err_code = FPGA_ASYNC_ERR_NONE;
+	update->progress = FPGA_ASYNC_PROG_READING;
+	reinit_completion(&update->done);
+	schedule_work(&update->work);
+
+unlock_exit:
+	mutex_unlock(&update->lock);
+	return ret;
+}
+static DEVICE_ATTR_WO(filename);
+
+static umode_t
+fpga_async_update_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
+
+	if (!mgr->mops->async_write_init)
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute *fpga_mgr_async_attrs[] = {
+	&dev_attr_filename.attr,
+	NULL,
+};
+
+static struct attribute_group fpga_mgr_async_group = {
+	.name = "async_update",
+	.attrs = fpga_mgr_async_attrs,
+	.is_visible = fpga_async_update_visible,
+};
+
+static const struct attribute_group *fpga_mgr_groups[] = {
+	&fpga_mgr_group,
+	&fpga_mgr_async_group,
+	NULL,
+};
 
 static struct fpga_manager *__fpga_mgr_get(struct device *dev)
 {
@@ -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
 		return NULL;
 	}
 
+	if (mops->async_write_init || mops->async_write_blk ||
+	    mops->async_write_complete || mops->async_cancel ||
+	    mops->async_cleanup) {
+		if (!mops->async_write_init || !mops->async_write_blk ||
+		    !mops->async_write_complete || !mops->async_cancel)
+			dev_err(dev, "Attempt to register incomplete async ops\n");
+			return NULL;
+	}
+
 	if (!name || !strlen(name)) {
 		dev_err(dev, "Attempt to register with no name!\n");
 		return NULL;
@@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
 	mgr->mops = mops;
 	mgr->priv = priv;
 
+	mgr->async_update.driver_unload = false;
+	mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
+	mutex_init(&mgr->async_update.lock);
+	init_completion(&mgr->async_update.done);
+	INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
+
 	device_initialize(&mgr->dev);
 	mgr->dev.class = fpga_mgr_class;
 	mgr->dev.groups = mops->groups;
@@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
  * @mgr: fpga manager struct
  *
  * This function is intended for use in a FPGA manager driver's remove function.
+ *
+ * For some devices, once an aysynchronous update has begun the authentication
+ * phase, the hardware cannot be signaled to stop, and the driver will not exit
+ * until the hardware signals completion.  This could be 30+ minutes of waiting.
+ * The driver_unload flag enables a force-unload of the driver (e.g. modprobe -r)
+ * by signaling the parent driver to exit even if the hardware update is incomplete.
+ * The driver_unload flag also prevents new updates from starting once the
+ * unregister process has begun.
  */
 void fpga_mgr_unregister(struct fpga_manager *mgr)
 {
+	struct fpga_async_update *update = &mgr->async_update;
+
 	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
 
+	mutex_lock(&update->lock);
+	update->driver_unload = true;
+	if (update->progress == FPGA_ASYNC_PROG_IDLE) {
+		mutex_unlock(&update->lock);
+		goto unregister;
+	}
+
+	mutex_unlock(&update->lock);
+	wait_for_completion(&update->done);
+
+unregister:
+
 	/*
 	 * If the low level driver provides a method for putting fpga into
 	 * a desired state upon unregister, do it.
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 2bc3030a69e5..9ff6c55f7a43 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -9,6 +9,7 @@
 #define _LINUX_FPGA_MGR_H
 
 #include <linux/mutex.h>
+#include <linux/completion.h>
 #include <linux/platform_device.h>
 
 struct fpga_manager;
@@ -74,6 +75,30 @@ enum fpga_mgr_states {
 #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
 #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
 
+/* Asynchronous update error codes */
+enum fpga_async_err {
+	FPGA_ASYNC_ERR_NONE,
+	FPGA_ASYNC_ERR_HW_ERROR,
+	FPGA_ASYNC_ERR_TIMEOUT,
+	FPGA_ASYNC_ERR_CANCELED,
+	FPGA_ASYNC_ERR_BUSY,
+	FPGA_ASYNC_ERR_INVALID_SIZE,
+	FPGA_ASYNC_ERR_RW_ERROR,
+	FPGA_ASYNC_ERR_WEAROUT,
+	FPGA_ASYNC_ERR_FILE_READ,
+	FPGA_ASYNC_ERR_MAX
+};
+
+/* Asynchronous update progress codes */
+enum fpga_async_prog {
+	FPGA_ASYNC_PROG_IDLE,
+	FPGA_ASYNC_PROG_READING,
+	FPGA_ASYNC_PROG_PREPARING,
+	FPGA_ASYNC_PROG_WRITING,
+	FPGA_ASYNC_PROG_PROGRAMMING,
+	FPGA_ASYNC_PROG_MAX
+};
+
 /**
  * struct fpga_image_info - information specific to a FPGA image
  * @flags: boolean flags as defined above
@@ -133,6 +158,16 @@ struct fpga_manager_ops {
 	int (*write_complete)(struct fpga_manager *mgr,
 			      struct fpga_image_info *info);
 	void (*fpga_remove)(struct fpga_manager *mgr);
+
+	/* async update ops */
+	enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
+			     size_t count);
+	enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
+			     const char *buf, u32 offset, size_t count);
+	enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
+	enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
+	void (*async_cleanup)(struct fpga_manager *mgr);
+
 	const struct attribute_group **groups;
 };
 
@@ -154,6 +189,22 @@ struct fpga_compat_id {
 	u64 id_l;
 };
 
+/**
+ * struct fpga_async_update - asynchronous image update structure
+ * @name: name of low level fpga manager
+ */
+struct fpga_async_update {
+	char *filename;
+	const u8 *data;			/* pointer to update data */
+	u32 remaining_size;		/* size remaining to transfer */
+	struct work_struct work;
+	struct completion done;
+	enum fpga_async_prog progress;
+	enum fpga_async_err err_code;	/* async update error code */
+	bool driver_unload;
+	struct mutex lock;		/* protect data structure contents */
+};
+
 /**
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
@@ -171,6 +222,7 @@ struct fpga_manager {
 	enum fpga_mgr_states state;
 	struct fpga_compat_id *compat_id;
 	const struct fpga_manager_ops *mops;
+	struct fpga_async_update async_update;
 	void *priv;
 };
 
-- 
2.25.1

Re: [RFC PATCH v1 0/1] Extend FPGA manager with async image updates

[Russ Weight]

+Richard Gong

On 4/9/21 5:38 PM, Russ Weight wrote:
> Hi Moritz,
>
> This RFC patch is a follow-on to my evaluation of the possibility of merging
> secure update functionality into the FPGA Manager. My earlier RFC email
> is here: https://marc.info/?l=linux-fpga&m=161783978715092&w=2.
>
> This RFC patch implements the core fpga_sec_mgr_update() function from the
> security manager patchset:
> https://marc.info/?l=linux-fpga&m=161525020621455&w=2.
>
> Specifically, it is a port of this patch:
> https://marc.info/?l=linux-fpga&m=161525020721457&w=2
>
> I think this patch provides enough context for further discussion. It
> extends functionality without leveraging any common code (because I
> didn't see an opportunity to share code).
>
> In this patch, I am using the term "async" (in reference to the kernel
> worker thread) instead of the term "security". While the security manager
> patches were originally created specifically to support Intel secure image
> updates, there is nothing inherently secure about the driver support, other
> than the fact that the operation is essentially atomic: one write to the
> "filename" sysfs entry is all that is required from user-space to do
> an update. Our convention is to use signed, self-describing images that are
> interpreted by the card BMC, but one could use a non-signed image or even
> interpret the contents of the image based on the context of the parent
> driver. I think the main differentiating factors are:
>
> (1) sysfs interface: an update is an atomic operation accomplished with a
>     single write.
> (2) self-describing: The type of information contained in the FPGA Manager
>     fpga_image_info structure would have to be included in the image file
>     and interpreted by the parent driver (not the class driver).
> (3) asynchronous: A write to the "filename" sysfs node write will return
>     immediately and the update will proceed in the context of a kernel
>     worker thread. Additional sysfs interfaces would be used to monitor the
>     progress and determine the ultimate success or failure of the update.
> (4) No notion of regions, bridges, or FPGA state. For Intel PAC cards, some
>     image files don't even contain an FPGA image. If they do, the image could
>     become active on the next power-cycle, or it could be activated through
>     some other trigger mechanism.
>
> Can existing ops be leveraged?
> ==============================
> write: The current write op _could_ be used if the prototype were modified to
> accept an additional offset parameter. For the async update, writes are done
> in chunks, and the target offset needs to be passed on each write.
>
> write_init and write_complete _could_ be used without change.
>
> Other ops would have to be added: cancel, cleanup, hw_errinfo
>
> I chose to implement all new ops because of the return data types. The fpga-mgr
> ops use the standard negative errno values. More descriptive and relevant error
> information can be provided via sysfs by defining a set of enum error codes.
> For example, it is very helpful to be able to tell the user that they are in a
> FLASH-wearout state, but standard errno values do not facilitate the
> communication of a wearout error.
>
> Would it be better to share the two or three ops that can be shared, and be
> content with the standard error numbers? Or is it OK to use separate ops?
>
> Should async updates be available via exported symbol?
> ======================================================
> As I understand it, current image updates through the FPGA Manager are all
> started with a call to the exported symbol fpga_mgr_load(). It would be
> possible to export an fpga_mgr_async_load() symbol, but there would need
> to be additional exported symbols to facilitate the collection of status
> information. Is there a use case for this?
>
> Can a common update function be used?
> =====================================
> fpga_mgr_async_update() is analagous to fpga_mgr_load(). However, all async
> updates use the request_firmware framework. The FPGA Manager supports two
> separate flows: a single image buffer or scatter-gather. It would be possible
> to integrate these features for async updates, but I'm not sure that there is a
> need for such functionality.
>
> I look forward to your feedback. Do you see value in integrating the two
> drivers? Should I continue this process?
>
> - Russ
>
> Russ Weight (1):
>   fpga: mgr: enable asynchronous image updates
>
>  .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>  drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>  include/linux/fpga/fpga-mgr.h                 |  52 +++++
>  3 files changed, 259 insertions(+), 1 deletion(-)
>

Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Russ Weight]

+Richard Gong

On 4/9/21 5:38 PM, Russ Weight wrote:
> Extend the FPGA Manager class driver to support asynchronous image
> updates in the context of a kernel worker thread. These updates are
> managed through sysfs file entries. This patch specifically creates
> the async_update/filename sysfs node that can be used to initiate an
> asynchronous update. The filename of a update image can be written to
> this sysfs entry to cause the update to occur.
>
> The write of the filename will return immediately, and the update will
> begin in the context of a kernel worker thread.  This tool utilizes
> the request_firmware framework, which requires that the image file
> reside under /lib/firmware.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>  drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>  include/linux/fpga/fpga-mgr.h                 |  52 +++++
>  3 files changed, 259 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
> index d78689c357a5..39ff8764f261 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
> @@ -58,3 +58,12 @@ Description:	Read fpga manager status as a string.
>  						  reconfiguration hardware
>  		* reconfig fifo overflow error	- FIFO overflow detected by
>  						  reconfiguration hardware
> +
> +What:		/sys/class/fpga_manager/<fpga>/async_update/filename
> +Date:		April 2021
> +KernelVersion:	5.13
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Write only. Write the filename of a self-describing  image
> +		file to this sysfs file to initiate an image update. The
> +		write will return immediately, and the image update will
> +		proceed in the background.
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index b85bc47c91a9..5d4449c82af5 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -8,6 +8,7 @@
>   * With code from the mailing list:
>   * Copyright (C) 2013 Xilinx, Inc.
>   */
> +#include <linux/device.h>
>  #include <linux/firmware.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/idr.h>
> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>  	&dev_attr_status.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static const struct attribute_group fpga_mgr_group = {
> +	.attrs = fpga_mgr_attrs,
> +};
> +
> +#define WRITE_BLOCK_SIZE 0x4000	/* Update remaining_size every 0x4000 bytes */
> +
> +static void fpga_async_dev_error(struct fpga_manager *mgr,
> +				 enum fpga_async_err err_code)
> +{
> +	mgr->async_update.err_code = err_code;
> +	mgr->mops->async_cancel(mgr);
> +}
> +
> +static void progress_complete(struct fpga_manager *mgr)
> +{
> +	struct fpga_async_update *update = &mgr->async_update;
> +
> +	mutex_lock(&update->lock);
> +	update->progress = FPGA_ASYNC_PROG_IDLE;
> +	complete_all(&update->done);
> +	mutex_unlock(&update->lock);
> +}
> +
> +static void fpga_mgr_async_update(struct work_struct *work)
> +{
> +	struct fpga_async_update *update;
> +	u32 size, blk_size, offset = 0;
> +	struct fpga_manager *mgr;
> +	const struct firmware *fw;
> +	enum fpga_async_err ret;
> +
> +	update = container_of(work, struct fpga_async_update, work);
> +	mgr = container_of(update, struct fpga_manager, async_update);
> +
> +	get_device(&mgr->dev);
> +	if (request_firmware(&fw, update->filename, &mgr->dev)) {
> +		update->err_code = FPGA_ASYNC_ERR_FILE_READ;
> +		goto idle_exit;
> +	}
> +
> +	update->data = fw->data;
> +	update->remaining_size = fw->size;
> +
> +	if (!try_module_get(mgr->dev.parent->driver->owner)) {
> +		update->err_code = FPGA_ASYNC_ERR_BUSY;
> +		goto release_fw_exit;
> +	}
> +
> +	update->progress = FPGA_ASYNC_PROG_PREPARING;
> +	ret = mgr->mops->async_write_init(mgr, fw->size);
> +	if (ret != FPGA_ASYNC_ERR_NONE) {
> +		fpga_async_dev_error(mgr, ret);
> +		goto modput_exit;
> +	}
> +
> +	update->progress = FPGA_ASYNC_PROG_WRITING;
> +	size = update->remaining_size;
> +	while (size) {
> +		blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
> +		size -= blk_size;
> +		ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
> +						 offset, blk_size);
> +		if (ret != FPGA_ASYNC_ERR_NONE) {
> +			fpga_async_dev_error(mgr, ret);
> +			goto done;
> +		}
> +
> +		update->remaining_size = size;
> +		offset += blk_size;
> +	}
> +
> +	update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
> +	ret = mgr->mops->async_write_complete(mgr);
> +	if (ret != FPGA_ASYNC_ERR_NONE)
> +		fpga_async_dev_error(mgr, ret);
> +
> +done:
> +	if (mgr->mops->async_cleanup)
> +		mgr->mops->async_cleanup(mgr);
> +
> +modput_exit:
> +	module_put(mgr->dev.parent->driver->owner);
> +
> +release_fw_exit:
> +	update->data = NULL;
> +	release_firmware(fw);
> +
> +idle_exit:
> +	/*
> +	 * Note: update->remaining_size is left unmodified here to
> +	 * provide additional information on errors. It will be
> +	 * reinitialized when the next async update begins.
> +	 */
> +	kfree(update->filename);
> +	update->filename = NULL;
> +	put_device(&mgr->dev);
> +	progress_complete(mgr);
> +}
> +
> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct fpga_manager *mgr = to_fpga_manager(dev);
> +	struct fpga_async_update *update = &mgr->async_update;
> +	int ret = count;
> +
> +	if (count == 0 || count >= PATH_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&update->lock);
> +	if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
> +		ret = -EBUSY;
> +		goto unlock_exit;
> +	}
> +
> +	update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
> +	if (!update->filename) {
> +		ret = -ENOMEM;
> +		goto unlock_exit;
> +	}
> +
> +	update->err_code = FPGA_ASYNC_ERR_NONE;
> +	update->progress = FPGA_ASYNC_PROG_READING;
> +	reinit_completion(&update->done);
> +	schedule_work(&update->work);
> +
> +unlock_exit:
> +	mutex_unlock(&update->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(filename);
> +
> +static umode_t
> +fpga_async_update_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
> +
> +	if (!mgr->mops->async_write_init)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
> +static struct attribute *fpga_mgr_async_attrs[] = {
> +	&dev_attr_filename.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group fpga_mgr_async_group = {
> +	.name = "async_update",
> +	.attrs = fpga_mgr_async_attrs,
> +	.is_visible = fpga_async_update_visible,
> +};
> +
> +static const struct attribute_group *fpga_mgr_groups[] = {
> +	&fpga_mgr_group,
> +	&fpga_mgr_async_group,
> +	NULL,
> +};
>  
>  static struct fpga_manager *__fpga_mgr_get(struct device *dev)
>  {
> @@ -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>  		return NULL;
>  	}
>  
> +	if (mops->async_write_init || mops->async_write_blk ||
> +	    mops->async_write_complete || mops->async_cancel ||
> +	    mops->async_cleanup) {
> +		if (!mops->async_write_init || !mops->async_write_blk ||
> +		    !mops->async_write_complete || !mops->async_cancel)
> +			dev_err(dev, "Attempt to register incomplete async ops\n");
> +			return NULL;
> +	}
> +
>  	if (!name || !strlen(name)) {
>  		dev_err(dev, "Attempt to register with no name!\n");
>  		return NULL;
> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>  	mgr->mops = mops;
>  	mgr->priv = priv;
>  
> +	mgr->async_update.driver_unload = false;
> +	mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
> +	mutex_init(&mgr->async_update.lock);
> +	init_completion(&mgr->async_update.done);
> +	INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
> +
>  	device_initialize(&mgr->dev);
>  	mgr->dev.class = fpga_mgr_class;
>  	mgr->dev.groups = mops->groups;
> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>   * @mgr: fpga manager struct
>   *
>   * This function is intended for use in a FPGA manager driver's remove function.
> + *
> + * For some devices, once an aysynchronous update has begun the authentication
> + * phase, the hardware cannot be signaled to stop, and the driver will not exit
> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
> + * The driver_unload flag enables a force-unload of the driver (e.g. modprobe -r)
> + * by signaling the parent driver to exit even if the hardware update is incomplete.
> + * The driver_unload flag also prevents new updates from starting once the
> + * unregister process has begun.
>   */
>  void fpga_mgr_unregister(struct fpga_manager *mgr)
>  {
> +	struct fpga_async_update *update = &mgr->async_update;
> +
>  	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>  
> +	mutex_lock(&update->lock);
> +	update->driver_unload = true;
> +	if (update->progress == FPGA_ASYNC_PROG_IDLE) {
> +		mutex_unlock(&update->lock);
> +		goto unregister;
> +	}
> +
> +	mutex_unlock(&update->lock);
> +	wait_for_completion(&update->done);
> +
> +unregister:
> +
>  	/*
>  	 * If the low level driver provides a method for putting fpga into
>  	 * a desired state upon unregister, do it.
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 2bc3030a69e5..9ff6c55f7a43 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -9,6 +9,7 @@
>  #define _LINUX_FPGA_MGR_H
>  
>  #include <linux/mutex.h>
> +#include <linux/completion.h>
>  #include <linux/platform_device.h>
>  
>  struct fpga_manager;
> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>  #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
>  #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
>  
> +/* Asynchronous update error codes */
> +enum fpga_async_err {
> +	FPGA_ASYNC_ERR_NONE,
> +	FPGA_ASYNC_ERR_HW_ERROR,
> +	FPGA_ASYNC_ERR_TIMEOUT,
> +	FPGA_ASYNC_ERR_CANCELED,
> +	FPGA_ASYNC_ERR_BUSY,
> +	FPGA_ASYNC_ERR_INVALID_SIZE,
> +	FPGA_ASYNC_ERR_RW_ERROR,
> +	FPGA_ASYNC_ERR_WEAROUT,
> +	FPGA_ASYNC_ERR_FILE_READ,
> +	FPGA_ASYNC_ERR_MAX
> +};
> +
> +/* Asynchronous update progress codes */
> +enum fpga_async_prog {
> +	FPGA_ASYNC_PROG_IDLE,
> +	FPGA_ASYNC_PROG_READING,
> +	FPGA_ASYNC_PROG_PREPARING,
> +	FPGA_ASYNC_PROG_WRITING,
> +	FPGA_ASYNC_PROG_PROGRAMMING,
> +	FPGA_ASYNC_PROG_MAX
> +};
> +
>  /**
>   * struct fpga_image_info - information specific to a FPGA image
>   * @flags: boolean flags as defined above
> @@ -133,6 +158,16 @@ struct fpga_manager_ops {
>  	int (*write_complete)(struct fpga_manager *mgr,
>  			      struct fpga_image_info *info);
>  	void (*fpga_remove)(struct fpga_manager *mgr);
> +
> +	/* async update ops */
> +	enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
> +			     size_t count);
> +	enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
> +			     const char *buf, u32 offset, size_t count);
> +	enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
> +	enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
> +	void (*async_cleanup)(struct fpga_manager *mgr);
> +
>  	const struct attribute_group **groups;
>  };
>  
> @@ -154,6 +189,22 @@ struct fpga_compat_id {
>  	u64 id_l;
>  };
>  
> +/**
> + * struct fpga_async_update - asynchronous image update structure
> + * @name: name of low level fpga manager
> + */
> +struct fpga_async_update {
> +	char *filename;
> +	const u8 *data;			/* pointer to update data */
> +	u32 remaining_size;		/* size remaining to transfer */
> +	struct work_struct work;
> +	struct completion done;
> +	enum fpga_async_prog progress;
> +	enum fpga_async_err err_code;	/* async update error code */
> +	bool driver_unload;
> +	struct mutex lock;		/* protect data structure contents */
> +};
> +
>  /**
>   * struct fpga_manager - fpga manager structure
>   * @name: name of low level fpga manager
> @@ -171,6 +222,7 @@ struct fpga_manager {
>  	enum fpga_mgr_states state;
>  	struct fpga_compat_id *compat_id;
>  	const struct fpga_manager_ops *mops;
> +	struct fpga_async_update async_update;
>  	void *priv;
>  };
>  

Re: FW: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Richard Gong]

> 
> 
> -----Original Message-----
> From: Weight, Russell H <russell.h.weight@intel.com>
> Sent: Wednesday, April 14, 2021 11:46 AM
> To: mdf@kernel.org; linux-fpga@vger.kernel.org
> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>; Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew <matthew.gerlach@intel.com>; Gong, Richard <richard.gong@intel.com>
> Subject: Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates
> 
> +Richard Gong
> 
> On 4/9/21 5:38 PM, Russ Weight wrote:
>> Extend the FPGA Manager class driver to support asynchronous image
>> updates in the context of a kernel worker thread. These updates are
>> managed through sysfs file entries. This patch specifically creates
>> the async_update/filename sysfs node that can be used to initiate an
>> asynchronous update. The filename of a update image can be written to
>> this sysfs entry to cause the update to occur.
>>
>> The write of the filename will return immediately, and the update will
>> begin in the context of a kernel worker thread.  This tool utilizes
>> the request_firmware framework, which requires that the image file
>> reside under /lib/firmware.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> ---
>>   .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>>   drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>>   include/linux/fpga/fpga-mgr.h                 |  52 +++++
>>   3 files changed, 259 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager
>> b/Documentation/ABI/testing/sysfs-class-fpga-manager
>> index d78689c357a5..39ff8764f261 100644
>> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
>> @@ -58,3 +58,12 @@ Description:	Read fpga manager status as a string.
>>   						  reconfiguration hardware
>>   		* reconfig fifo overflow error	- FIFO overflow detected by
>>   						  reconfiguration hardware
>> +
>> +What:		/sys/class/fpga_manager/<fpga>/async_update/filename
>> +Date:		April 2021
>> +KernelVersion:	5.13
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Write only. Write the filename of a self-describing  image
>> +		file to this sysfs file to initiate an image update. The
>> +		write will return immediately, and the image update will
>> +		proceed in the background.
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>> b85bc47c91a9..5d4449c82af5 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -8,6 +8,7 @@
>>    * With code from the mailing list:
>>    * Copyright (C) 2013 Xilinx, Inc.
>>    */
>> +#include <linux/device.h>
>>   #include <linux/firmware.h>
>>   #include <linux/fpga/fpga-mgr.h>
>>   #include <linux/idr.h>
>> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>>   	&dev_attr_status.attr,
>>   	NULL,
>>   };
>> -ATTRIBUTE_GROUPS(fpga_mgr);
>> +
>> +static const struct attribute_group fpga_mgr_group = {
>> +	.attrs = fpga_mgr_attrs,
>> +};
>> +
>> +#define WRITE_BLOCK_SIZE 0x4000	/* Update remaining_size every 0x4000 bytes */
>> +
>> +static void fpga_async_dev_error(struct fpga_manager *mgr,
>> +				 enum fpga_async_err err_code)
>> +{
>> +	mgr->async_update.err_code = err_code;
>> +	mgr->mops->async_cancel(mgr);
>> +}
>> +
>> +static void progress_complete(struct fpga_manager *mgr) {
>> +	struct fpga_async_update *update = &mgr->async_update;
>> +
>> +	mutex_lock(&update->lock);
>> +	update->progress = FPGA_ASYNC_PROG_IDLE;
>> +	complete_all(&update->done);
>> +	mutex_unlock(&update->lock);
>> +}
>> +
>> +static void fpga_mgr_async_update(struct work_struct *work) {
>> +	struct fpga_async_update *update;
>> +	u32 size, blk_size, offset = 0;
>> +	struct fpga_manager *mgr;
>> +	const struct firmware *fw;
>> +	enum fpga_async_err ret;
>> +
>> +	update = container_of(work, struct fpga_async_update, work);
>> +	mgr = container_of(update, struct fpga_manager, async_update);
>> +
>> +	get_device(&mgr->dev);
>> +	if (request_firmware(&fw, update->filename, &mgr->dev)) {
>> +		update->err_code = FPGA_ASYNC_ERR_FILE_READ;
>> +		goto idle_exit;
>> +	}
>> +
>> +	update->data = fw->data;
>> +	update->remaining_size = fw->size;
>> +
>> +	if (!try_module_get(mgr->dev.parent->driver->owner)) {
>> +		update->err_code = FPGA_ASYNC_ERR_BUSY;
>> +		goto release_fw_exit;
>> +	}
>> +
>> +	update->progress = FPGA_ASYNC_PROG_PREPARING;
>> +	ret = mgr->mops->async_write_init(mgr, fw->size);
>> +	if (ret != FPGA_ASYNC_ERR_NONE) {
>> +		fpga_async_dev_error(mgr, ret);
>> +		goto modput_exit;
>> +	}
>> +
>> +	update->progress = FPGA_ASYNC_PROG_WRITING;
>> +	size = update->remaining_size;
>> +	while (size) {
>> +		blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>> +		size -= blk_size;
>> +		ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
>> +						 offset, blk_size);

It is not good idea to set 0x400 bytes here, the size of data chunk 
should be decided by low-level driver.

The actual hardware device or firmware may be able to handle data chunk 
which is larger than 0x400 bytes in one transaction. So it is good for 
FPGA manager to pass the data to low-level driver and leave the decision 
to low-level driver.

Regards,
Richard

>> +		if (ret != FPGA_ASYNC_ERR_NONE) {
>> +			fpga_async_dev_error(mgr, ret);
>> +			goto done;
>> +		}
>> +
>> +		update->remaining_size = size;
>> +		offset += blk_size;
>> +	}
>> +
>> +	update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
>> +	ret = mgr->mops->async_write_complete(mgr);
>> +	if (ret != FPGA_ASYNC_ERR_NONE)
>> +		fpga_async_dev_error(mgr, ret);
>> +
>> +done:
>> +	if (mgr->mops->async_cleanup)
>> +		mgr->mops->async_cleanup(mgr);
>> +
>> +modput_exit:
>> +	module_put(mgr->dev.parent->driver->owner);
>> +
>> +release_fw_exit:
>> +	update->data = NULL;
>> +	release_firmware(fw);
>> +
>> +idle_exit:
>> +	/*
>> +	 * Note: update->remaining_size is left unmodified here to
>> +	 * provide additional information on errors. It will be
>> +	 * reinitialized when the next async update begins.
>> +	 */
>> +	kfree(update->filename);
>> +	update->filename = NULL;
>> +	put_device(&mgr->dev);
>> +	progress_complete(mgr);
>> +}
>> +
>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>> +			      const char *buf, size_t count) {
>> +	struct fpga_manager *mgr = to_fpga_manager(dev);
>> +	struct fpga_async_update *update = &mgr->async_update;
>> +	int ret = count;
>> +
>> +	if (count == 0 || count >= PATH_MAX)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&update->lock);
>> +	if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
>> +		ret = -EBUSY;
>> +		goto unlock_exit;
>> +	}
>> +
>> +	update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
>> +	if (!update->filename) {
>> +		ret = -ENOMEM;
>> +		goto unlock_exit;
>> +	}
>> +
>> +	update->err_code = FPGA_ASYNC_ERR_NONE;
>> +	update->progress = FPGA_ASYNC_PROG_READING;
>> +	reinit_completion(&update->done);
>> +	schedule_work(&update->work);
>> +
>> +unlock_exit:
>> +	mutex_unlock(&update->lock);
>> +	return ret;
>> +}
>> +static DEVICE_ATTR_WO(filename);
>> +
>> +static umode_t
>> +fpga_async_update_visible(struct kobject *kobj, struct attribute
>> +*attr, int n) {
>> +	struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
>> +
>> +	if (!mgr->mops->async_write_init)
>> +		return 0;
>> +
>> +	return attr->mode;
>> +}
>> +
>> +static struct attribute *fpga_mgr_async_attrs[] = {
>> +	&dev_attr_filename.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group fpga_mgr_async_group = {
>> +	.name = "async_update",
>> +	.attrs = fpga_mgr_async_attrs,
>> +	.is_visible = fpga_async_update_visible, };
>> +
>> +static const struct attribute_group *fpga_mgr_groups[] = {
>> +	&fpga_mgr_group,
>> +	&fpga_mgr_async_group,
>> +	NULL,
>> +};
>>   
>>   static struct fpga_manager *__fpga_mgr_get(struct device *dev)  { @@
>> -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>   		return NULL;
>>   	}
>>   
>> +	if (mops->async_write_init || mops->async_write_blk ||
>> +	    mops->async_write_complete || mops->async_cancel ||
>> +	    mops->async_cleanup) {
>> +		if (!mops->async_write_init || !mops->async_write_blk ||
>> +		    !mops->async_write_complete || !mops->async_cancel)
>> +			dev_err(dev, "Attempt to register incomplete async ops\n");
>> +			return NULL;
>> +	}
>> +
>>   	if (!name || !strlen(name)) {
>>   		dev_err(dev, "Attempt to register with no name!\n");
>>   		return NULL;
>> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>   	mgr->mops = mops;
>>   	mgr->priv = priv;
>>   
>> +	mgr->async_update.driver_unload = false;
>> +	mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
>> +	mutex_init(&mgr->async_update.lock);
>> +	init_completion(&mgr->async_update.done);
>> +	INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
>> +
>>   	device_initialize(&mgr->dev);
>>   	mgr->dev.class = fpga_mgr_class;
>>   	mgr->dev.groups = mops->groups;
>> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>    * @mgr: fpga manager struct
>>    *
>>    * This function is intended for use in a FPGA manager driver's remove function.
>> + *
>> + * For some devices, once an aysynchronous update has begun the
>> + authentication
>> + * phase, the hardware cannot be signaled to stop, and the driver
>> + will not exit
>> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
>> + * The driver_unload flag enables a force-unload of the driver (e.g.
>> + modprobe -r)
>> + * by signaling the parent driver to exit even if the hardware update is incomplete.
>> + * The driver_unload flag also prevents new updates from starting
>> + once the
>> + * unregister process has begun.
>>    */
>>   void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>> +	struct fpga_async_update *update = &mgr->async_update;
>> +
>>   	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>   
>> +	mutex_lock(&update->lock);
>> +	update->driver_unload = true;
>> +	if (update->progress == FPGA_ASYNC_PROG_IDLE) {
>> +		mutex_unlock(&update->lock);
>> +		goto unregister;
>> +	}
>> +
>> +	mutex_unlock(&update->lock);
>> +	wait_for_completion(&update->done);
>> +
>> +unregister:
>> +
>>   	/*
>>   	 * If the low level driver provides a method for putting fpga into
>>   	 * a desired state upon unregister, do it.
>> diff --git a/include/linux/fpga/fpga-mgr.h
>> b/include/linux/fpga/fpga-mgr.h index 2bc3030a69e5..9ff6c55f7a43
>> 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -9,6 +9,7 @@
>>   #define _LINUX_FPGA_MGR_H
>>   
>>   #include <linux/mutex.h>
>> +#include <linux/completion.h>
>>   #include <linux/platform_device.h>
>>   
>>   struct fpga_manager;
>> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>>   #define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
>>   #define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
>>   
>> +/* Asynchronous update error codes */ enum fpga_async_err {
>> +	FPGA_ASYNC_ERR_NONE,
>> +	FPGA_ASYNC_ERR_HW_ERROR,
>> +	FPGA_ASYNC_ERR_TIMEOUT,
>> +	FPGA_ASYNC_ERR_CANCELED,
>> +	FPGA_ASYNC_ERR_BUSY,
>> +	FPGA_ASYNC_ERR_INVALID_SIZE,
>> +	FPGA_ASYNC_ERR_RW_ERROR,
>> +	FPGA_ASYNC_ERR_WEAROUT,
>> +	FPGA_ASYNC_ERR_FILE_READ,
>> +	FPGA_ASYNC_ERR_MAX
>> +};
>> +
>> +/* Asynchronous update progress codes */ enum fpga_async_prog {
>> +	FPGA_ASYNC_PROG_IDLE,
>> +	FPGA_ASYNC_PROG_READING,
>> +	FPGA_ASYNC_PROG_PREPARING,
>> +	FPGA_ASYNC_PROG_WRITING,
>> +	FPGA_ASYNC_PROG_PROGRAMMING,
>> +	FPGA_ASYNC_PROG_MAX
>> +};
>> +
>>   /**
>>    * struct fpga_image_info - information specific to a FPGA image
>>    * @flags: boolean flags as defined above @@ -133,6 +158,16 @@ struct
>> fpga_manager_ops {
>>   	int (*write_complete)(struct fpga_manager *mgr,
>>   			      struct fpga_image_info *info);
>>   	void (*fpga_remove)(struct fpga_manager *mgr);
>> +
>> +	/* async update ops */
>> +	enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
>> +			     size_t count);
>> +	enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
>> +			     const char *buf, u32 offset, size_t count);
>> +	enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
>> +	enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
>> +	void (*async_cleanup)(struct fpga_manager *mgr);
>> +
>>   	const struct attribute_group **groups;  };
>>   
>> @@ -154,6 +189,22 @@ struct fpga_compat_id {
>>   	u64 id_l;
>>   };
>>   
>> +/**
>> + * struct fpga_async_update - asynchronous image update structure
>> + * @name: name of low level fpga manager  */ struct fpga_async_update
>> +{
>> +	char *filename;
>> +	const u8 *data;			/* pointer to update data */
>> +	u32 remaining_size;		/* size remaining to transfer */
>> +	struct work_struct work;
>> +	struct completion done;
>> +	enum fpga_async_prog progress;
>> +	enum fpga_async_err err_code;	/* async update error code */
>> +	bool driver_unload;
>> +	struct mutex lock;		/* protect data structure contents */
>> +};
>> +
>>   /**
>>    * struct fpga_manager - fpga manager structure
>>    * @name: name of low level fpga manager @@ -171,6 +222,7 @@ struct
>> fpga_manager {
>>   	enum fpga_mgr_states state;
>>   	struct fpga_compat_id *compat_id;
>>   	const struct fpga_manager_ops *mops;
>> +	struct fpga_async_update async_update;
>>   	void *priv;
>>   };
>>   
> 

Re: FW: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Russ Weight]

On 4/15/21 5:13 AM, Richard Gong wrote:
>
>>
>>
>> -----Original Message-----
>> From: Weight, Russell H <russell.h.weight@intel.com>
>> Sent: Wednesday, April 14, 2021 11:46 AM
>> To: mdf@kernel.org; linux-fpga@vger.kernel.org
>> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>; Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew <matthew.gerlach@intel.com>; Gong, Richard <richard.gong@intel.com>
>> Subject: Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates
>>
>> +Richard Gong
>>
>> On 4/9/21 5:38 PM, Russ Weight wrote:
>>> Extend the FPGA Manager class driver to support asynchronous image
>>> updates in the context of a kernel worker thread. These updates are
>>> managed through sysfs file entries. This patch specifically creates
>>> the async_update/filename sysfs node that can be used to initiate an
>>> asynchronous update. The filename of a update image can be written to
>>> this sysfs entry to cause the update to occur.
>>>
>>> The write of the filename will return immediately, and the update will
>>> begin in the context of a kernel worker thread.  This tool utilizes
>>> the request_firmware framework, which requires that the image file
>>> reside under /lib/firmware.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> ---
>>>   .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>>>   drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>>>   include/linux/fpga/fpga-mgr.h                 |  52 +++++
>>>   3 files changed, 259 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>> b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>> index d78689c357a5..39ff8764f261 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>> @@ -58,3 +58,12 @@ Description:    Read fpga manager status as a string.
>>>                             reconfiguration hardware
>>>           * reconfig fifo overflow error    - FIFO overflow detected by
>>>                             reconfiguration hardware
>>> +
>>> +What:        /sys/class/fpga_manager/<fpga>/async_update/filename
>>> +Date:        April 2021
>>> +KernelVersion:    5.13
>>> +Contact:    Russ Weight <russell.h.weight@intel.com>
>>> +Description:    Write only. Write the filename of a self-describing  image
>>> +        file to this sysfs file to initiate an image update. The
>>> +        write will return immediately, and the image update will
>>> +        proceed in the background.
>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>> b85bc47c91a9..5d4449c82af5 100644
>>> --- a/drivers/fpga/fpga-mgr.c
>>> +++ b/drivers/fpga/fpga-mgr.c
>>> @@ -8,6 +8,7 @@
>>>    * With code from the mailing list:
>>>    * Copyright (C) 2013 Xilinx, Inc.
>>>    */
>>> +#include <linux/device.h>
>>>   #include <linux/firmware.h>
>>>   #include <linux/fpga/fpga-mgr.h>
>>>   #include <linux/idr.h>
>>> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>>>       &dev_attr_status.attr,
>>>       NULL,
>>>   };
>>> -ATTRIBUTE_GROUPS(fpga_mgr);
>>> +
>>> +static const struct attribute_group fpga_mgr_group = {
>>> +    .attrs = fpga_mgr_attrs,
>>> +};
>>> +
>>> +#define WRITE_BLOCK_SIZE 0x4000    /* Update remaining_size every 0x4000 bytes */
>>> +
>>> +static void fpga_async_dev_error(struct fpga_manager *mgr,
>>> +                 enum fpga_async_err err_code)
>>> +{
>>> +    mgr->async_update.err_code = err_code;
>>> +    mgr->mops->async_cancel(mgr);
>>> +}
>>> +
>>> +static void progress_complete(struct fpga_manager *mgr) {
>>> +    struct fpga_async_update *update = &mgr->async_update;
>>> +
>>> +    mutex_lock(&update->lock);
>>> +    update->progress = FPGA_ASYNC_PROG_IDLE;
>>> +    complete_all(&update->done);
>>> +    mutex_unlock(&update->lock);
>>> +}
>>> +
>>> +static void fpga_mgr_async_update(struct work_struct *work) {
>>> +    struct fpga_async_update *update;
>>> +    u32 size, blk_size, offset = 0;
>>> +    struct fpga_manager *mgr;
>>> +    const struct firmware *fw;
>>> +    enum fpga_async_err ret;
>>> +
>>> +    update = container_of(work, struct fpga_async_update, work);
>>> +    mgr = container_of(update, struct fpga_manager, async_update);
>>> +
>>> +    get_device(&mgr->dev);
>>> +    if (request_firmware(&fw, update->filename, &mgr->dev)) {
>>> +        update->err_code = FPGA_ASYNC_ERR_FILE_READ;
>>> +        goto idle_exit;
>>> +    }
>>> +
>>> +    update->data = fw->data;
>>> +    update->remaining_size = fw->size;
>>> +
>>> +    if (!try_module_get(mgr->dev.parent->driver->owner)) {
>>> +        update->err_code = FPGA_ASYNC_ERR_BUSY;
>>> +        goto release_fw_exit;
>>> +    }
>>> +
>>> +    update->progress = FPGA_ASYNC_PROG_PREPARING;
>>> +    ret = mgr->mops->async_write_init(mgr, fw->size);
>>> +    if (ret != FPGA_ASYNC_ERR_NONE) {
>>> +        fpga_async_dev_error(mgr, ret);
>>> +        goto modput_exit;
>>> +    }
>>> +
>>> +    update->progress = FPGA_ASYNC_PROG_WRITING;
>>> +    size = update->remaining_size;
>>> +    while (size) {
>>> +        blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>>> +        size -= blk_size;
>>> +        ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
>>> +                         offset, blk_size);
>
> It is not good idea to set 0x400 bytes here, the size of data chunk should be decided by low-level driver.

0x4000

>
> The actual hardware device or firmware may be able to handle data chunk which is larger than 0x400 bytes in one transaction. So it is good for FPGA manager to pass the data to low-level driver and leave the decision to low-level driver.

I understand the concern. The decision to break the write up into multiple
writes is done for the purpose of providing a progress indication to the
user. This is only the first of several patches. Another patch will expose
the remaining_size value via sysfs so that it can be monitored from user
space. For the devices that I am working with, in the worst case the update
takes about 40 minutes.

0x4000 could be changed to a larger number, or it could be left to the lower
level driver to choose the size. The only thing that is happening between
writes is update of the remaining_size, and checking to see if the user wants
to cancel the update, so I don't think this design introduces an appreciable
delay.

- Russ

>
> Regards,
> Richard
>
>>> +        if (ret != FPGA_ASYNC_ERR_NONE) {
>>> +            fpga_async_dev_error(mgr, ret);
>>> +            goto done;
>>> +        }
>>> +
>>> +        update->remaining_size = size;
>>> +        offset += blk_size;
>>> +    }
>>> +
>>> +    update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
>>> +    ret = mgr->mops->async_write_complete(mgr);
>>> +    if (ret != FPGA_ASYNC_ERR_NONE)
>>> +        fpga_async_dev_error(mgr, ret);
>>> +
>>> +done:
>>> +    if (mgr->mops->async_cleanup)
>>> +        mgr->mops->async_cleanup(mgr);
>>> +
>>> +modput_exit:
>>> +    module_put(mgr->dev.parent->driver->owner);
>>> +
>>> +release_fw_exit:
>>> +    update->data = NULL;
>>> +    release_firmware(fw);
>>> +
>>> +idle_exit:
>>> +    /*
>>> +     * Note: update->remaining_size is left unmodified here to
>>> +     * provide additional information on errors. It will be
>>> +     * reinitialized when the next async update begins.
>>> +     */
>>> +    kfree(update->filename);
>>> +    update->filename = NULL;
>>> +    put_device(&mgr->dev);
>>> +    progress_complete(mgr);
>>> +}
>>> +
>>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>> +                  const char *buf, size_t count) {
>>> +    struct fpga_manager *mgr = to_fpga_manager(dev);
>>> +    struct fpga_async_update *update = &mgr->async_update;
>>> +    int ret = count;
>>> +
>>> +    if (count == 0 || count >= PATH_MAX)
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&update->lock);
>>> +    if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
>>> +        ret = -EBUSY;
>>> +        goto unlock_exit;
>>> +    }
>>> +
>>> +    update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
>>> +    if (!update->filename) {
>>> +        ret = -ENOMEM;
>>> +        goto unlock_exit;
>>> +    }
>>> +
>>> +    update->err_code = FPGA_ASYNC_ERR_NONE;
>>> +    update->progress = FPGA_ASYNC_PROG_READING;
>>> +    reinit_completion(&update->done);
>>> +    schedule_work(&update->work);
>>> +
>>> +unlock_exit:
>>> +    mutex_unlock(&update->lock);
>>> +    return ret;
>>> +}
>>> +static DEVICE_ATTR_WO(filename);
>>> +
>>> +static umode_t
>>> +fpga_async_update_visible(struct kobject *kobj, struct attribute
>>> +*attr, int n) {
>>> +    struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
>>> +
>>> +    if (!mgr->mops->async_write_init)
>>> +        return 0;
>>> +
>>> +    return attr->mode;
>>> +}
>>> +
>>> +static struct attribute *fpga_mgr_async_attrs[] = {
>>> +    &dev_attr_filename.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static struct attribute_group fpga_mgr_async_group = {
>>> +    .name = "async_update",
>>> +    .attrs = fpga_mgr_async_attrs,
>>> +    .is_visible = fpga_async_update_visible, };
>>> +
>>> +static const struct attribute_group *fpga_mgr_groups[] = {
>>> +    &fpga_mgr_group,
>>> +    &fpga_mgr_async_group,
>>> +    NULL,
>>> +};
>>>     static struct fpga_manager *__fpga_mgr_get(struct device *dev)  { @@
>>> -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>           return NULL;
>>>       }
>>>   +    if (mops->async_write_init || mops->async_write_blk ||
>>> +        mops->async_write_complete || mops->async_cancel ||
>>> +        mops->async_cleanup) {
>>> +        if (!mops->async_write_init || !mops->async_write_blk ||
>>> +            !mops->async_write_complete || !mops->async_cancel)
>>> +            dev_err(dev, "Attempt to register incomplete async ops\n");
>>> +            return NULL;
>>> +    }
>>> +
>>>       if (!name || !strlen(name)) {
>>>           dev_err(dev, "Attempt to register with no name!\n");
>>>           return NULL;
>>> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>       mgr->mops = mops;
>>>       mgr->priv = priv;
>>>   +    mgr->async_update.driver_unload = false;
>>> +    mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
>>> +    mutex_init(&mgr->async_update.lock);
>>> +    init_completion(&mgr->async_update.done);
>>> +    INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
>>> +
>>>       device_initialize(&mgr->dev);
>>>       mgr->dev.class = fpga_mgr_class;
>>>       mgr->dev.groups = mops->groups;
>>> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>>    * @mgr: fpga manager struct
>>>    *
>>>    * This function is intended for use in a FPGA manager driver's remove function.
>>> + *
>>> + * For some devices, once an aysynchronous update has begun the
>>> + authentication
>>> + * phase, the hardware cannot be signaled to stop, and the driver
>>> + will not exit
>>> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
>>> + * The driver_unload flag enables a force-unload of the driver (e.g.
>>> + modprobe -r)
>>> + * by signaling the parent driver to exit even if the hardware update is incomplete.
>>> + * The driver_unload flag also prevents new updates from starting
>>> + once the
>>> + * unregister process has begun.
>>>    */
>>>   void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>>> +    struct fpga_async_update *update = &mgr->async_update;
>>> +
>>>       dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>   +    mutex_lock(&update->lock);
>>> +    update->driver_unload = true;
>>> +    if (update->progress == FPGA_ASYNC_PROG_IDLE) {
>>> +        mutex_unlock(&update->lock);
>>> +        goto unregister;
>>> +    }
>>> +
>>> +    mutex_unlock(&update->lock);
>>> +    wait_for_completion(&update->done);
>>> +
>>> +unregister:
>>> +
>>>       /*
>>>        * If the low level driver provides a method for putting fpga into
>>>        * a desired state upon unregister, do it.
>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>> b/include/linux/fpga/fpga-mgr.h index 2bc3030a69e5..9ff6c55f7a43
>>> 100644
>>> --- a/include/linux/fpga/fpga-mgr.h
>>> +++ b/include/linux/fpga/fpga-mgr.h
>>> @@ -9,6 +9,7 @@
>>>   #define _LINUX_FPGA_MGR_H
>>>     #include <linux/mutex.h>
>>> +#include <linux/completion.h>
>>>   #include <linux/platform_device.h>
>>>     struct fpga_manager;
>>> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>>>   #define FPGA_MGR_BITSTREAM_LSB_FIRST    BIT(3)
>>>   #define FPGA_MGR_COMPRESSED_BITSTREAM    BIT(4)
>>>   +/* Asynchronous update error codes */ enum fpga_async_err {
>>> +    FPGA_ASYNC_ERR_NONE,
>>> +    FPGA_ASYNC_ERR_HW_ERROR,
>>> +    FPGA_ASYNC_ERR_TIMEOUT,
>>> +    FPGA_ASYNC_ERR_CANCELED,
>>> +    FPGA_ASYNC_ERR_BUSY,
>>> +    FPGA_ASYNC_ERR_INVALID_SIZE,
>>> +    FPGA_ASYNC_ERR_RW_ERROR,
>>> +    FPGA_ASYNC_ERR_WEAROUT,
>>> +    FPGA_ASYNC_ERR_FILE_READ,
>>> +    FPGA_ASYNC_ERR_MAX
>>> +};
>>> +
>>> +/* Asynchronous update progress codes */ enum fpga_async_prog {
>>> +    FPGA_ASYNC_PROG_IDLE,
>>> +    FPGA_ASYNC_PROG_READING,
>>> +    FPGA_ASYNC_PROG_PREPARING,
>>> +    FPGA_ASYNC_PROG_WRITING,
>>> +    FPGA_ASYNC_PROG_PROGRAMMING,
>>> +    FPGA_ASYNC_PROG_MAX
>>> +};
>>> +
>>>   /**
>>>    * struct fpga_image_info - information specific to a FPGA image
>>>    * @flags: boolean flags as defined above @@ -133,6 +158,16 @@ struct
>>> fpga_manager_ops {
>>>       int (*write_complete)(struct fpga_manager *mgr,
>>>                     struct fpga_image_info *info);
>>>       void (*fpga_remove)(struct fpga_manager *mgr);
>>> +
>>> +    /* async update ops */
>>> +    enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
>>> +                 size_t count);
>>> +    enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
>>> +                 const char *buf, u32 offset, size_t count);
>>> +    enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
>>> +    enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
>>> +    void (*async_cleanup)(struct fpga_manager *mgr);
>>> +
>>>       const struct attribute_group **groups;  };
>>>   @@ -154,6 +189,22 @@ struct fpga_compat_id {
>>>       u64 id_l;
>>>   };
>>>   +/**
>>> + * struct fpga_async_update - asynchronous image update structure
>>> + * @name: name of low level fpga manager  */ struct fpga_async_update
>>> +{
>>> +    char *filename;
>>> +    const u8 *data;            /* pointer to update data */
>>> +    u32 remaining_size;        /* size remaining to transfer */
>>> +    struct work_struct work;
>>> +    struct completion done;
>>> +    enum fpga_async_prog progress;
>>> +    enum fpga_async_err err_code;    /* async update error code */
>>> +    bool driver_unload;
>>> +    struct mutex lock;        /* protect data structure contents */
>>> +};
>>> +
>>>   /**
>>>    * struct fpga_manager - fpga manager structure
>>>    * @name: name of low level fpga manager @@ -171,6 +222,7 @@ struct
>>> fpga_manager {
>>>       enum fpga_mgr_states state;
>>>       struct fpga_compat_id *compat_id;
>>>       const struct fpga_manager_ops *mops;
>>> +    struct fpga_async_update async_update;
>>>       void *priv;
>>>   };
>>>   
>>

Re: FW: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Richard Gong]

On 4/15/21 11:03 AM, Russ Weight wrote:
> 
> 
> On 4/15/21 5:13 AM, Richard Gong wrote:
>>
>>>
>>>
>>> -----Original Message-----
>>> From: Weight, Russell H <russell.h.weight@intel.com>
>>> Sent: Wednesday, April 14, 2021 11:46 AM
>>> To: mdf@kernel.org; linux-fpga@vger.kernel.org
>>> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>; Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew <matthew.gerlach@intel.com>; Gong, Richard <richard.gong@intel.com>
>>> Subject: Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates
>>>
>>> +Richard Gong
>>>
>>> On 4/9/21 5:38 PM, Russ Weight wrote:
>>>> Extend the FPGA Manager class driver to support asynchronous image
>>>> updates in the context of a kernel worker thread. These updates are
>>>> managed through sysfs file entries. This patch specifically creates
>>>> the async_update/filename sysfs node that can be used to initiate an
>>>> asynchronous update. The filename of a update image can be written to
>>>> this sysfs entry to cause the update to occur.
>>>>
>>>> The write of the filename will return immediately, and the update will
>>>> begin in the context of a kernel worker thread.  This tool utilizes
>>>> the request_firmware framework, which requires that the image file
>>>> reside under /lib/firmware.
>>>>
>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>> ---
>>>>    .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>>>>    drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>>>>    include/linux/fpga/fpga-mgr.h                 |  52 +++++
>>>>    3 files changed, 259 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>> b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>> index d78689c357a5..39ff8764f261 100644
>>>> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>> @@ -58,3 +58,12 @@ Description:    Read fpga manager status as a string.
>>>>                              reconfiguration hardware
>>>>            * reconfig fifo overflow error    - FIFO overflow detected by
>>>>                              reconfiguration hardware
>>>> +
>>>> +What:        /sys/class/fpga_manager/<fpga>/async_update/filename
>>>> +Date:        April 2021
>>>> +KernelVersion:    5.13
>>>> +Contact:    Russ Weight <russell.h.weight@intel.com>
>>>> +Description:    Write only. Write the filename of a self-describing  image
>>>> +        file to this sysfs file to initiate an image update. The
>>>> +        write will return immediately, and the image update will
>>>> +        proceed in the background.
>>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>>> b85bc47c91a9..5d4449c82af5 100644
>>>> --- a/drivers/fpga/fpga-mgr.c
>>>> +++ b/drivers/fpga/fpga-mgr.c
>>>> @@ -8,6 +8,7 @@
>>>>     * With code from the mailing list:
>>>>     * Copyright (C) 2013 Xilinx, Inc.
>>>>     */
>>>> +#include <linux/device.h>
>>>>    #include <linux/firmware.h>
>>>>    #include <linux/fpga/fpga-mgr.h>
>>>>    #include <linux/idr.h>
>>>> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>>>>        &dev_attr_status.attr,
>>>>        NULL,
>>>>    };
>>>> -ATTRIBUTE_GROUPS(fpga_mgr);
>>>> +
>>>> +static const struct attribute_group fpga_mgr_group = {
>>>> +    .attrs = fpga_mgr_attrs,
>>>> +};
>>>> +
>>>> +#define WRITE_BLOCK_SIZE 0x4000    /* Update remaining_size every 0x4000 bytes */
>>>> +
>>>> +static void fpga_async_dev_error(struct fpga_manager *mgr,
>>>> +                 enum fpga_async_err err_code)
>>>> +{
>>>> +    mgr->async_update.err_code = err_code;
>>>> +    mgr->mops->async_cancel(mgr);
>>>> +}
>>>> +
>>>> +static void progress_complete(struct fpga_manager *mgr) {
>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>> +
>>>> +    mutex_lock(&update->lock);
>>>> +    update->progress = FPGA_ASYNC_PROG_IDLE;
>>>> +    complete_all(&update->done);
>>>> +    mutex_unlock(&update->lock);
>>>> +}
>>>> +
>>>> +static void fpga_mgr_async_update(struct work_struct *work) {
>>>> +    struct fpga_async_update *update;
>>>> +    u32 size, blk_size, offset = 0;
>>>> +    struct fpga_manager *mgr;
>>>> +    const struct firmware *fw;
>>>> +    enum fpga_async_err ret;
>>>> +
>>>> +    update = container_of(work, struct fpga_async_update, work);
>>>> +    mgr = container_of(update, struct fpga_manager, async_update);
>>>> +
>>>> +    get_device(&mgr->dev);
>>>> +    if (request_firmware(&fw, update->filename, &mgr->dev)) {
>>>> +        update->err_code = FPGA_ASYNC_ERR_FILE_READ;
>>>> +        goto idle_exit;
>>>> +    }
>>>> +
>>>> +    update->data = fw->data;
>>>> +    update->remaining_size = fw->size;
>>>> +
>>>> +    if (!try_module_get(mgr->dev.parent->driver->owner)) {
>>>> +        update->err_code = FPGA_ASYNC_ERR_BUSY;
>>>> +        goto release_fw_exit;
>>>> +    }
>>>> +
>>>> +    update->progress = FPGA_ASYNC_PROG_PREPARING;
>>>> +    ret = mgr->mops->async_write_init(mgr, fw->size);
>>>> +    if (ret != FPGA_ASYNC_ERR_NONE) {
>>>> +        fpga_async_dev_error(mgr, ret);
>>>> +        goto modput_exit;
>>>> +    }
>>>> +
>>>> +    update->progress = FPGA_ASYNC_PROG_WRITING;
>>>> +    size = update->remaining_size;
>>>> +    while (size) {
>>>> +        blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>>>> +        size -= blk_size;
>>>> +        ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
>>>> +                         offset, blk_size);
>>
>> It is not good idea to set 0x400 bytes here, the size of data chunk should be decided by low-level driver.
> 
> 0x4000
> 
>>
>> The actual hardware device or firmware may be able to handle data chunk which is larger than 0x400 bytes in one transaction. So it is good for FPGA manager to pass the data to low-level driver and leave the decision to low-level driver.
> 
> I understand the concern. The decision to break the write up into multiple
> writes is done for the purpose of providing a progress indication to the
> user. This is only the first of several patches. Another patch will expose
> the remaining_size value via sysfs so that it can be monitored from user
> space. For the devices that I am working with, in the worst case the update
> takes about 40 minutes.
> 
> 0x4000 could be changed to a larger number, or it could be left to the lower
> level driver to choose the size. The only thing that is happening between
> writes is update of the remaining_size, and checking to see if the user wants
> to cancel the update, so I don't think this design introduces an appreciable
> delay.

I still think low-level driver rather than FPGA manager dirver should 
handle this.

Regards,
Richard

> 
> - Russ
> 
>>
>> Regards,
>> Richard
>>
>>>> +        if (ret != FPGA_ASYNC_ERR_NONE) {
>>>> +            fpga_async_dev_error(mgr, ret);
>>>> +            goto done;
>>>> +        }
>>>> +
>>>> +        update->remaining_size = size;
>>>> +        offset += blk_size;
>>>> +    }
>>>> +
>>>> +    update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
>>>> +    ret = mgr->mops->async_write_complete(mgr);
>>>> +    if (ret != FPGA_ASYNC_ERR_NONE)
>>>> +        fpga_async_dev_error(mgr, ret);
>>>> +
>>>> +done:
>>>> +    if (mgr->mops->async_cleanup)
>>>> +        mgr->mops->async_cleanup(mgr);
>>>> +
>>>> +modput_exit:
>>>> +    module_put(mgr->dev.parent->driver->owner);
>>>> +
>>>> +release_fw_exit:
>>>> +    update->data = NULL;
>>>> +    release_firmware(fw);
>>>> +
>>>> +idle_exit:
>>>> +    /*
>>>> +     * Note: update->remaining_size is left unmodified here to
>>>> +     * provide additional information on errors. It will be
>>>> +     * reinitialized when the next async update begins.
>>>> +     */
>>>> +    kfree(update->filename);
>>>> +    update->filename = NULL;
>>>> +    put_device(&mgr->dev);
>>>> +    progress_complete(mgr);
>>>> +}
>>>> +
>>>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>>> +                  const char *buf, size_t count) {
>>>> +    struct fpga_manager *mgr = to_fpga_manager(dev);
>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>> +    int ret = count;
>>>> +
>>>> +    if (count == 0 || count >= PATH_MAX)
>>>> +        return -EINVAL;
>>>> +
>>>> +    mutex_lock(&update->lock);
>>>> +    if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
>>>> +        ret = -EBUSY;
>>>> +        goto unlock_exit;
>>>> +    }
>>>> +
>>>> +    update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
>>>> +    if (!update->filename) {
>>>> +        ret = -ENOMEM;
>>>> +        goto unlock_exit;
>>>> +    }
>>>> +
>>>> +    update->err_code = FPGA_ASYNC_ERR_NONE;
>>>> +    update->progress = FPGA_ASYNC_PROG_READING;
>>>> +    reinit_completion(&update->done);
>>>> +    schedule_work(&update->work);
>>>> +
>>>> +unlock_exit:
>>>> +    mutex_unlock(&update->lock);
>>>> +    return ret;
>>>> +}
>>>> +static DEVICE_ATTR_WO(filename);
>>>> +
>>>> +static umode_t
>>>> +fpga_async_update_visible(struct kobject *kobj, struct attribute
>>>> +*attr, int n) {
>>>> +    struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
>>>> +
>>>> +    if (!mgr->mops->async_write_init)
>>>> +        return 0;
>>>> +
>>>> +    return attr->mode;
>>>> +}
>>>> +
>>>> +static struct attribute *fpga_mgr_async_attrs[] = {
>>>> +    &dev_attr_filename.attr,
>>>> +    NULL,
>>>> +};
>>>> +
>>>> +static struct attribute_group fpga_mgr_async_group = {
>>>> +    .name = "async_update",
>>>> +    .attrs = fpga_mgr_async_attrs,
>>>> +    .is_visible = fpga_async_update_visible, };
>>>> +
>>>> +static const struct attribute_group *fpga_mgr_groups[] = {
>>>> +    &fpga_mgr_group,
>>>> +    &fpga_mgr_async_group,
>>>> +    NULL,
>>>> +};
>>>>      static struct fpga_manager *__fpga_mgr_get(struct device *dev)  { @@
>>>> -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>            return NULL;
>>>>        }
>>>>    +    if (mops->async_write_init || mops->async_write_blk ||
>>>> +        mops->async_write_complete || mops->async_cancel ||
>>>> +        mops->async_cleanup) {
>>>> +        if (!mops->async_write_init || !mops->async_write_blk ||
>>>> +            !mops->async_write_complete || !mops->async_cancel)
>>>> +            dev_err(dev, "Attempt to register incomplete async ops\n");
>>>> +            return NULL;
>>>> +    }
>>>> +
>>>>        if (!name || !strlen(name)) {
>>>>            dev_err(dev, "Attempt to register with no name!\n");
>>>>            return NULL;
>>>> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>        mgr->mops = mops;
>>>>        mgr->priv = priv;
>>>>    +    mgr->async_update.driver_unload = false;
>>>> +    mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
>>>> +    mutex_init(&mgr->async_update.lock);
>>>> +    init_completion(&mgr->async_update.done);
>>>> +    INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
>>>> +
>>>>        device_initialize(&mgr->dev);
>>>>        mgr->dev.class = fpga_mgr_class;
>>>>        mgr->dev.groups = mops->groups;
>>>> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>>>     * @mgr: fpga manager struct
>>>>     *
>>>>     * This function is intended for use in a FPGA manager driver's remove function.
>>>> + *
>>>> + * For some devices, once an aysynchronous update has begun the
>>>> + authentication
>>>> + * phase, the hardware cannot be signaled to stop, and the driver
>>>> + will not exit
>>>> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
>>>> + * The driver_unload flag enables a force-unload of the driver (e.g.
>>>> + modprobe -r)
>>>> + * by signaling the parent driver to exit even if the hardware update is incomplete.
>>>> + * The driver_unload flag also prevents new updates from starting
>>>> + once the
>>>> + * unregister process has begun.
>>>>     */
>>>>    void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>> +
>>>>        dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>>    +    mutex_lock(&update->lock);
>>>> +    update->driver_unload = true;
>>>> +    if (update->progress == FPGA_ASYNC_PROG_IDLE) {
>>>> +        mutex_unlock(&update->lock);
>>>> +        goto unregister;
>>>> +    }
>>>> +
>>>> +    mutex_unlock(&update->lock);
>>>> +    wait_for_completion(&update->done);
>>>> +
>>>> +unregister:
>>>> +
>>>>        /*
>>>>         * If the low level driver provides a method for putting fpga into
>>>>         * a desired state upon unregister, do it.
>>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>>> b/include/linux/fpga/fpga-mgr.h index 2bc3030a69e5..9ff6c55f7a43
>>>> 100644
>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>> @@ -9,6 +9,7 @@
>>>>    #define _LINUX_FPGA_MGR_H
>>>>      #include <linux/mutex.h>
>>>> +#include <linux/completion.h>
>>>>    #include <linux/platform_device.h>
>>>>      struct fpga_manager;
>>>> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>>>>    #define FPGA_MGR_BITSTREAM_LSB_FIRST    BIT(3)
>>>>    #define FPGA_MGR_COMPRESSED_BITSTREAM    BIT(4)
>>>>    +/* Asynchronous update error codes */ enum fpga_async_err {
>>>> +    FPGA_ASYNC_ERR_NONE,
>>>> +    FPGA_ASYNC_ERR_HW_ERROR,
>>>> +    FPGA_ASYNC_ERR_TIMEOUT,
>>>> +    FPGA_ASYNC_ERR_CANCELED,
>>>> +    FPGA_ASYNC_ERR_BUSY,
>>>> +    FPGA_ASYNC_ERR_INVALID_SIZE,
>>>> +    FPGA_ASYNC_ERR_RW_ERROR,
>>>> +    FPGA_ASYNC_ERR_WEAROUT,
>>>> +    FPGA_ASYNC_ERR_FILE_READ,
>>>> +    FPGA_ASYNC_ERR_MAX
>>>> +};
>>>> +
>>>> +/* Asynchronous update progress codes */ enum fpga_async_prog {
>>>> +    FPGA_ASYNC_PROG_IDLE,
>>>> +    FPGA_ASYNC_PROG_READING,
>>>> +    FPGA_ASYNC_PROG_PREPARING,
>>>> +    FPGA_ASYNC_PROG_WRITING,
>>>> +    FPGA_ASYNC_PROG_PROGRAMMING,
>>>> +    FPGA_ASYNC_PROG_MAX
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct fpga_image_info - information specific to a FPGA image
>>>>     * @flags: boolean flags as defined above @@ -133,6 +158,16 @@ struct
>>>> fpga_manager_ops {
>>>>        int (*write_complete)(struct fpga_manager *mgr,
>>>>                      struct fpga_image_info *info);
>>>>        void (*fpga_remove)(struct fpga_manager *mgr);
>>>> +
>>>> +    /* async update ops */
>>>> +    enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
>>>> +                 size_t count);
>>>> +    enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
>>>> +                 const char *buf, u32 offset, size_t count);
>>>> +    enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
>>>> +    enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
>>>> +    void (*async_cleanup)(struct fpga_manager *mgr);
>>>> +
>>>>        const struct attribute_group **groups;  };
>>>>    @@ -154,6 +189,22 @@ struct fpga_compat_id {
>>>>        u64 id_l;
>>>>    };
>>>>    +/**
>>>> + * struct fpga_async_update - asynchronous image update structure
>>>> + * @name: name of low level fpga manager  */ struct fpga_async_update
>>>> +{
>>>> +    char *filename;
>>>> +    const u8 *data;            /* pointer to update data */
>>>> +    u32 remaining_size;        /* size remaining to transfer */
>>>> +    struct work_struct work;
>>>> +    struct completion done;
>>>> +    enum fpga_async_prog progress;
>>>> +    enum fpga_async_err err_code;    /* async update error code */
>>>> +    bool driver_unload;
>>>> +    struct mutex lock;        /* protect data structure contents */
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct fpga_manager - fpga manager structure
>>>>     * @name: name of low level fpga manager @@ -171,6 +222,7 @@ struct
>>>> fpga_manager {
>>>>        enum fpga_mgr_states state;
>>>>        struct fpga_compat_id *compat_id;
>>>>        const struct fpga_manager_ops *mops;
>>>> +    struct fpga_async_update async_update;
>>>>        void *priv;
>>>>    };
>>>>    
>>>
> 

Re: FW: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Russ Weight]

On 4/19/21 4:33 AM, Richard Gong wrote:
>
>
> On 4/15/21 11:03 AM, Russ Weight wrote:
>>
>>
>> On 4/15/21 5:13 AM, Richard Gong wrote:
>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Weight, Russell H <russell.h.weight@intel.com>
>>>> Sent: Wednesday, April 14, 2021 11:46 AM
>>>> To: mdf@kernel.org; linux-fpga@vger.kernel.org
>>>> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>; Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew <matthew.gerlach@intel.com>; Gong, Richard <richard.gong@intel.com>
>>>> Subject: Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates
>>>>
>>>> +Richard Gong
>>>>
>>>> On 4/9/21 5:38 PM, Russ Weight wrote:
>>>>> Extend the FPGA Manager class driver to support asynchronous image
>>>>> updates in the context of a kernel worker thread. These updates are
>>>>> managed through sysfs file entries. This patch specifically creates
>>>>> the async_update/filename sysfs node that can be used to initiate an
>>>>> asynchronous update. The filename of a update image can be written to
>>>>> this sysfs entry to cause the update to occur.
>>>>>
>>>>> The write of the filename will return immediately, and the update will
>>>>> begin in the context of a kernel worker thread.  This tool utilizes
>>>>> the request_firmware framework, which requires that the image file
>>>>> reside under /lib/firmware.
>>>>>
>>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>>> ---
>>>>>    .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>>>>>    drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>>>>>    include/linux/fpga/fpga-mgr.h                 |  52 +++++
>>>>>    3 files changed, 259 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>> b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>> index d78689c357a5..39ff8764f261 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>> @@ -58,3 +58,12 @@ Description:    Read fpga manager status as a string.
>>>>>                              reconfiguration hardware
>>>>>            * reconfig fifo overflow error    - FIFO overflow detected by
>>>>>                              reconfiguration hardware
>>>>> +
>>>>> +What:        /sys/class/fpga_manager/<fpga>/async_update/filename
>>>>> +Date:        April 2021
>>>>> +KernelVersion:    5.13
>>>>> +Contact:    Russ Weight <russell.h.weight@intel.com>
>>>>> +Description:    Write only. Write the filename of a self-describing  image
>>>>> +        file to this sysfs file to initiate an image update. The
>>>>> +        write will return immediately, and the image update will
>>>>> +        proceed in the background.
>>>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>>>> b85bc47c91a9..5d4449c82af5 100644
>>>>> --- a/drivers/fpga/fpga-mgr.c
>>>>> +++ b/drivers/fpga/fpga-mgr.c
>>>>> @@ -8,6 +8,7 @@
>>>>>     * With code from the mailing list:
>>>>>     * Copyright (C) 2013 Xilinx, Inc.
>>>>>     */
>>>>> +#include <linux/device.h>
>>>>>    #include <linux/firmware.h>
>>>>>    #include <linux/fpga/fpga-mgr.h>
>>>>>    #include <linux/idr.h>
>>>>> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>>>>>        &dev_attr_status.attr,
>>>>>        NULL,
>>>>>    };
>>>>> -ATTRIBUTE_GROUPS(fpga_mgr);
>>>>> +
>>>>> +static const struct attribute_group fpga_mgr_group = {
>>>>> +    .attrs = fpga_mgr_attrs,
>>>>> +};
>>>>> +
>>>>> +#define WRITE_BLOCK_SIZE 0x4000    /* Update remaining_size every 0x4000 bytes */
>>>>> +
>>>>> +static void fpga_async_dev_error(struct fpga_manager *mgr,
>>>>> +                 enum fpga_async_err err_code)
>>>>> +{
>>>>> +    mgr->async_update.err_code = err_code;
>>>>> +    mgr->mops->async_cancel(mgr);
>>>>> +}
>>>>> +
>>>>> +static void progress_complete(struct fpga_manager *mgr) {
>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>> +
>>>>> +    mutex_lock(&update->lock);
>>>>> +    update->progress = FPGA_ASYNC_PROG_IDLE;
>>>>> +    complete_all(&update->done);
>>>>> +    mutex_unlock(&update->lock);
>>>>> +}
>>>>> +
>>>>> +static void fpga_mgr_async_update(struct work_struct *work) {
>>>>> +    struct fpga_async_update *update;
>>>>> +    u32 size, blk_size, offset = 0;
>>>>> +    struct fpga_manager *mgr;
>>>>> +    const struct firmware *fw;
>>>>> +    enum fpga_async_err ret;
>>>>> +
>>>>> +    update = container_of(work, struct fpga_async_update, work);
>>>>> +    mgr = container_of(update, struct fpga_manager, async_update);
>>>>> +
>>>>> +    get_device(&mgr->dev);
>>>>> +    if (request_firmware(&fw, update->filename, &mgr->dev)) {
>>>>> +        update->err_code = FPGA_ASYNC_ERR_FILE_READ;
>>>>> +        goto idle_exit;
>>>>> +    }
>>>>> +
>>>>> +    update->data = fw->data;
>>>>> +    update->remaining_size = fw->size;
>>>>> +
>>>>> +    if (!try_module_get(mgr->dev.parent->driver->owner)) {
>>>>> +        update->err_code = FPGA_ASYNC_ERR_BUSY;
>>>>> +        goto release_fw_exit;
>>>>> +    }
>>>>> +
>>>>> +    update->progress = FPGA_ASYNC_PROG_PREPARING;
>>>>> +    ret = mgr->mops->async_write_init(mgr, fw->size);
>>>>> +    if (ret != FPGA_ASYNC_ERR_NONE) {
>>>>> +        fpga_async_dev_error(mgr, ret);
>>>>> +        goto modput_exit;
>>>>> +    }
>>>>> +
>>>>> +    update->progress = FPGA_ASYNC_PROG_WRITING;
>>>>> +    size = update->remaining_size;
>>>>> +    while (size) {
>>>>> +        blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>>>>> +        size -= blk_size;
>>>>> +        ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
>>>>> +                         offset, blk_size);
>>>
>>> It is not good idea to set 0x400 bytes here, the size of data chunk should be decided by low-level driver.
>>
>> 0x4000
>>
>>>
>>> The actual hardware device or firmware may be able to handle data chunk which is larger than 0x400 bytes in one transaction. So it is good for FPGA manager to pass the data to low-level driver and leave the decision to low-level driver.
>>
>> I understand the concern. The decision to break the write up into multiple
>> writes is done for the purpose of providing a progress indication to the
>> user. This is only the first of several patches. Another patch will expose
>> the remaining_size value via sysfs so that it can be monitored from user
>> space. For the devices that I am working with, in the worst case the update
>> takes about 40 minutes.
>>
>> 0x4000 could be changed to a larger number, or it could be left to the lower
>> level driver to choose the size. The only thing that is happening between
>> writes is update of the remaining_size, and checking to see if the user wants
>> to cancel the update, so I don't think this design introduces an appreciable
>> delay.
>
> I still think low-level driver rather than FPGA manager dirver should handle this.

Thanks Richard, I don't think it would be hard to make the change. I could
incorporate that into and RFC v2 patch, or into the original patch-set. I think I
need to get feedback from Moritz to know which direction he would like to go.

To summarize, I think you and I agree that what were are trying to accomplish is
not as similar as we first thought - there really is no overlap - right? Your driver
would not be able to leverage the code that I am working on?

The remaining question for me is whether or not it really makes sense to integrate
the asynchronous image transfer/update into the FPGA Manager.

- Russ


>
> Regards,
> Richard
>
>>
>> - Russ
>>
>>>
>>> Regards,
>>> Richard
>>>
>>>>> +        if (ret != FPGA_ASYNC_ERR_NONE) {
>>>>> +            fpga_async_dev_error(mgr, ret);
>>>>> +            goto done;
>>>>> +        }
>>>>> +
>>>>> +        update->remaining_size = size;
>>>>> +        offset += blk_size;
>>>>> +    }
>>>>> +
>>>>> +    update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
>>>>> +    ret = mgr->mops->async_write_complete(mgr);
>>>>> +    if (ret != FPGA_ASYNC_ERR_NONE)
>>>>> +        fpga_async_dev_error(mgr, ret);
>>>>> +
>>>>> +done:
>>>>> +    if (mgr->mops->async_cleanup)
>>>>> +        mgr->mops->async_cleanup(mgr);
>>>>> +
>>>>> +modput_exit:
>>>>> +    module_put(mgr->dev.parent->driver->owner);
>>>>> +
>>>>> +release_fw_exit:
>>>>> +    update->data = NULL;
>>>>> +    release_firmware(fw);
>>>>> +
>>>>> +idle_exit:
>>>>> +    /*
>>>>> +     * Note: update->remaining_size is left unmodified here to
>>>>> +     * provide additional information on errors. It will be
>>>>> +     * reinitialized when the next async update begins.
>>>>> +     */
>>>>> +    kfree(update->filename);
>>>>> +    update->filename = NULL;
>>>>> +    put_device(&mgr->dev);
>>>>> +    progress_complete(mgr);
>>>>> +}
>>>>> +
>>>>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>>>> +                  const char *buf, size_t count) {
>>>>> +    struct fpga_manager *mgr = to_fpga_manager(dev);
>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>> +    int ret = count;
>>>>> +
>>>>> +    if (count == 0 || count >= PATH_MAX)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    mutex_lock(&update->lock);
>>>>> +    if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
>>>>> +        ret = -EBUSY;
>>>>> +        goto unlock_exit;
>>>>> +    }
>>>>> +
>>>>> +    update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
>>>>> +    if (!update->filename) {
>>>>> +        ret = -ENOMEM;
>>>>> +        goto unlock_exit;
>>>>> +    }
>>>>> +
>>>>> +    update->err_code = FPGA_ASYNC_ERR_NONE;
>>>>> +    update->progress = FPGA_ASYNC_PROG_READING;
>>>>> +    reinit_completion(&update->done);
>>>>> +    schedule_work(&update->work);
>>>>> +
>>>>> +unlock_exit:
>>>>> +    mutex_unlock(&update->lock);
>>>>> +    return ret;
>>>>> +}
>>>>> +static DEVICE_ATTR_WO(filename);
>>>>> +
>>>>> +static umode_t
>>>>> +fpga_async_update_visible(struct kobject *kobj, struct attribute
>>>>> +*attr, int n) {
>>>>> +    struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
>>>>> +
>>>>> +    if (!mgr->mops->async_write_init)
>>>>> +        return 0;
>>>>> +
>>>>> +    return attr->mode;
>>>>> +}
>>>>> +
>>>>> +static struct attribute *fpga_mgr_async_attrs[] = {
>>>>> +    &dev_attr_filename.attr,
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group fpga_mgr_async_group = {
>>>>> +    .name = "async_update",
>>>>> +    .attrs = fpga_mgr_async_attrs,
>>>>> +    .is_visible = fpga_async_update_visible, };
>>>>> +
>>>>> +static const struct attribute_group *fpga_mgr_groups[] = {
>>>>> +    &fpga_mgr_group,
>>>>> +    &fpga_mgr_async_group,
>>>>> +    NULL,
>>>>> +};
>>>>>      static struct fpga_manager *__fpga_mgr_get(struct device *dev)  { @@
>>>>> -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>>            return NULL;
>>>>>        }
>>>>>    +    if (mops->async_write_init || mops->async_write_blk ||
>>>>> +        mops->async_write_complete || mops->async_cancel ||
>>>>> +        mops->async_cleanup) {
>>>>> +        if (!mops->async_write_init || !mops->async_write_blk ||
>>>>> +            !mops->async_write_complete || !mops->async_cancel)
>>>>> +            dev_err(dev, "Attempt to register incomplete async ops\n");
>>>>> +            return NULL;
>>>>> +    }
>>>>> +
>>>>>        if (!name || !strlen(name)) {
>>>>>            dev_err(dev, "Attempt to register with no name!\n");
>>>>>            return NULL;
>>>>> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>>        mgr->mops = mops;
>>>>>        mgr->priv = priv;
>>>>>    +    mgr->async_update.driver_unload = false;
>>>>> +    mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
>>>>> +    mutex_init(&mgr->async_update.lock);
>>>>> +    init_completion(&mgr->async_update.done);
>>>>> +    INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
>>>>> +
>>>>>        device_initialize(&mgr->dev);
>>>>>        mgr->dev.class = fpga_mgr_class;
>>>>>        mgr->dev.groups = mops->groups;
>>>>> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>>>>     * @mgr: fpga manager struct
>>>>>     *
>>>>>     * This function is intended for use in a FPGA manager driver's remove function.
>>>>> + *
>>>>> + * For some devices, once an aysynchronous update has begun the
>>>>> + authentication
>>>>> + * phase, the hardware cannot be signaled to stop, and the driver
>>>>> + will not exit
>>>>> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
>>>>> + * The driver_unload flag enables a force-unload of the driver (e.g.
>>>>> + modprobe -r)
>>>>> + * by signaling the parent driver to exit even if the hardware update is incomplete.
>>>>> + * The driver_unload flag also prevents new updates from starting
>>>>> + once the
>>>>> + * unregister process has begun.
>>>>>     */
>>>>>    void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>> +
>>>>>        dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>>>    +    mutex_lock(&update->lock);
>>>>> +    update->driver_unload = true;
>>>>> +    if (update->progress == FPGA_ASYNC_PROG_IDLE) {
>>>>> +        mutex_unlock(&update->lock);
>>>>> +        goto unregister;
>>>>> +    }
>>>>> +
>>>>> +    mutex_unlock(&update->lock);
>>>>> +    wait_for_completion(&update->done);
>>>>> +
>>>>> +unregister:
>>>>> +
>>>>>        /*
>>>>>         * If the low level driver provides a method for putting fpga into
>>>>>         * a desired state upon unregister, do it.
>>>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>>>> b/include/linux/fpga/fpga-mgr.h index 2bc3030a69e5..9ff6c55f7a43
>>>>> 100644
>>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>>> @@ -9,6 +9,7 @@
>>>>>    #define _LINUX_FPGA_MGR_H
>>>>>      #include <linux/mutex.h>
>>>>> +#include <linux/completion.h>
>>>>>    #include <linux/platform_device.h>
>>>>>      struct fpga_manager;
>>>>> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>>>>>    #define FPGA_MGR_BITSTREAM_LSB_FIRST    BIT(3)
>>>>>    #define FPGA_MGR_COMPRESSED_BITSTREAM    BIT(4)
>>>>>    +/* Asynchronous update error codes */ enum fpga_async_err {
>>>>> +    FPGA_ASYNC_ERR_NONE,
>>>>> +    FPGA_ASYNC_ERR_HW_ERROR,
>>>>> +    FPGA_ASYNC_ERR_TIMEOUT,
>>>>> +    FPGA_ASYNC_ERR_CANCELED,
>>>>> +    FPGA_ASYNC_ERR_BUSY,
>>>>> +    FPGA_ASYNC_ERR_INVALID_SIZE,
>>>>> +    FPGA_ASYNC_ERR_RW_ERROR,
>>>>> +    FPGA_ASYNC_ERR_WEAROUT,
>>>>> +    FPGA_ASYNC_ERR_FILE_READ,
>>>>> +    FPGA_ASYNC_ERR_MAX
>>>>> +};
>>>>> +
>>>>> +/* Asynchronous update progress codes */ enum fpga_async_prog {
>>>>> +    FPGA_ASYNC_PROG_IDLE,
>>>>> +    FPGA_ASYNC_PROG_READING,
>>>>> +    FPGA_ASYNC_PROG_PREPARING,
>>>>> +    FPGA_ASYNC_PROG_WRITING,
>>>>> +    FPGA_ASYNC_PROG_PROGRAMMING,
>>>>> +    FPGA_ASYNC_PROG_MAX
>>>>> +};
>>>>> +
>>>>>    /**
>>>>>     * struct fpga_image_info - information specific to a FPGA image
>>>>>     * @flags: boolean flags as defined above @@ -133,6 +158,16 @@ struct
>>>>> fpga_manager_ops {
>>>>>        int (*write_complete)(struct fpga_manager *mgr,
>>>>>                      struct fpga_image_info *info);
>>>>>        void (*fpga_remove)(struct fpga_manager *mgr);
>>>>> +
>>>>> +    /* async update ops */
>>>>> +    enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
>>>>> +                 size_t count);
>>>>> +    enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
>>>>> +                 const char *buf, u32 offset, size_t count);
>>>>> +    enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
>>>>> +    enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
>>>>> +    void (*async_cleanup)(struct fpga_manager *mgr);
>>>>> +
>>>>>        const struct attribute_group **groups;  };
>>>>>    @@ -154,6 +189,22 @@ struct fpga_compat_id {
>>>>>        u64 id_l;
>>>>>    };
>>>>>    +/**
>>>>> + * struct fpga_async_update - asynchronous image update structure
>>>>> + * @name: name of low level fpga manager  */ struct fpga_async_update
>>>>> +{
>>>>> +    char *filename;
>>>>> +    const u8 *data;            /* pointer to update data */
>>>>> +    u32 remaining_size;        /* size remaining to transfer */
>>>>> +    struct work_struct work;
>>>>> +    struct completion done;
>>>>> +    enum fpga_async_prog progress;
>>>>> +    enum fpga_async_err err_code;    /* async update error code */
>>>>> +    bool driver_unload;
>>>>> +    struct mutex lock;        /* protect data structure contents */
>>>>> +};
>>>>> +
>>>>>    /**
>>>>>     * struct fpga_manager - fpga manager structure
>>>>>     * @name: name of low level fpga manager @@ -171,6 +222,7 @@ struct
>>>>> fpga_manager {
>>>>>        enum fpga_mgr_states state;
>>>>>        struct fpga_compat_id *compat_id;
>>>>>        const struct fpga_manager_ops *mops;
>>>>> +    struct fpga_async_update async_update;
>>>>>        void *priv;
>>>>>    };
>>>>>    
>>>>
>>

Re: FW: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates

[Richard Gong]

On 4/19/21 11:32 AM, Russ Weight wrote:
> 
> 
> On 4/19/21 4:33 AM, Richard Gong wrote:
>>
>>
>> On 4/15/21 11:03 AM, Russ Weight wrote:
>>>
>>>
>>> On 4/15/21 5:13 AM, Richard Gong wrote:
>>>>
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Weight, Russell H <russell.h.weight@intel.com>
>>>>> Sent: Wednesday, April 14, 2021 11:46 AM
>>>>> To: mdf@kernel.org; linux-fpga@vger.kernel.org
>>>>> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>; Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew <matthew.gerlach@intel.com>; Gong, Richard <richard.gong@intel.com>
>>>>> Subject: Re: [RFC PATCH v1 1/1] fpga: mgr: enable asynchronous image updates
>>>>>
>>>>> +Richard Gong
>>>>>
>>>>> On 4/9/21 5:38 PM, Russ Weight wrote:
>>>>>> Extend the FPGA Manager class driver to support asynchronous image
>>>>>> updates in the context of a kernel worker thread. These updates are
>>>>>> managed through sysfs file entries. This patch specifically creates
>>>>>> the async_update/filename sysfs node that can be used to initiate an
>>>>>> asynchronous update. The filename of a update image can be written to
>>>>>> this sysfs entry to cause the update to occur.
>>>>>>
>>>>>> The write of the filename will return immediately, and the update will
>>>>>> begin in the context of a kernel worker thread.  This tool utilizes
>>>>>> the request_firmware framework, which requires that the image file
>>>>>> reside under /lib/firmware.
>>>>>>
>>>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>>>> ---
>>>>>>     .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>>>>>>     drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>>>>>>     include/linux/fpga/fpga-mgr.h                 |  52 +++++
>>>>>>     3 files changed, 259 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>>> b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>>> index d78689c357a5..39ff8764f261 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
>>>>>> @@ -58,3 +58,12 @@ Description:    Read fpga manager status as a string.
>>>>>>                               reconfiguration hardware
>>>>>>             * reconfig fifo overflow error    - FIFO overflow detected by
>>>>>>                               reconfiguration hardware
>>>>>> +
>>>>>> +What:        /sys/class/fpga_manager/<fpga>/async_update/filename
>>>>>> +Date:        April 2021
>>>>>> +KernelVersion:    5.13
>>>>>> +Contact:    Russ Weight <russell.h.weight@intel.com>
>>>>>> +Description:    Write only. Write the filename of a self-describing  image
>>>>>> +        file to this sysfs file to initiate an image update. The
>>>>>> +        write will return immediately, and the image update will
>>>>>> +        proceed in the background.
>>>>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
>>>>>> b85bc47c91a9..5d4449c82af5 100644
>>>>>> --- a/drivers/fpga/fpga-mgr.c
>>>>>> +++ b/drivers/fpga/fpga-mgr.c
>>>>>> @@ -8,6 +8,7 @@
>>>>>>      * With code from the mailing list:
>>>>>>      * Copyright (C) 2013 Xilinx, Inc.
>>>>>>      */
>>>>>> +#include <linux/device.h>
>>>>>>     #include <linux/firmware.h>
>>>>>>     #include <linux/fpga/fpga-mgr.h>
>>>>>>     #include <linux/idr.h>
>>>>>> @@ -446,7 +447,166 @@ static struct attribute *fpga_mgr_attrs[] = {
>>>>>>         &dev_attr_status.attr,
>>>>>>         NULL,
>>>>>>     };
>>>>>> -ATTRIBUTE_GROUPS(fpga_mgr);
>>>>>> +
>>>>>> +static const struct attribute_group fpga_mgr_group = {
>>>>>> +    .attrs = fpga_mgr_attrs,
>>>>>> +};
>>>>>> +
>>>>>> +#define WRITE_BLOCK_SIZE 0x4000    /* Update remaining_size every 0x4000 bytes */
>>>>>> +
>>>>>> +static void fpga_async_dev_error(struct fpga_manager *mgr,
>>>>>> +                 enum fpga_async_err err_code)
>>>>>> +{
>>>>>> +    mgr->async_update.err_code = err_code;
>>>>>> +    mgr->mops->async_cancel(mgr);
>>>>>> +}
>>>>>> +
>>>>>> +static void progress_complete(struct fpga_manager *mgr) {
>>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>>> +
>>>>>> +    mutex_lock(&update->lock);
>>>>>> +    update->progress = FPGA_ASYNC_PROG_IDLE;
>>>>>> +    complete_all(&update->done);
>>>>>> +    mutex_unlock(&update->lock);
>>>>>> +}
>>>>>> +
>>>>>> +static void fpga_mgr_async_update(struct work_struct *work) {
>>>>>> +    struct fpga_async_update *update;
>>>>>> +    u32 size, blk_size, offset = 0;
>>>>>> +    struct fpga_manager *mgr;
>>>>>> +    const struct firmware *fw;
>>>>>> +    enum fpga_async_err ret;
>>>>>> +
>>>>>> +    update = container_of(work, struct fpga_async_update, work);
>>>>>> +    mgr = container_of(update, struct fpga_manager, async_update);
>>>>>> +
>>>>>> +    get_device(&mgr->dev);
>>>>>> +    if (request_firmware(&fw, update->filename, &mgr->dev)) {
>>>>>> +        update->err_code = FPGA_ASYNC_ERR_FILE_READ;
>>>>>> +        goto idle_exit;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->data = fw->data;
>>>>>> +    update->remaining_size = fw->size;
>>>>>> +
>>>>>> +    if (!try_module_get(mgr->dev.parent->driver->owner)) {
>>>>>> +        update->err_code = FPGA_ASYNC_ERR_BUSY;
>>>>>> +        goto release_fw_exit;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->progress = FPGA_ASYNC_PROG_PREPARING;
>>>>>> +    ret = mgr->mops->async_write_init(mgr, fw->size);
>>>>>> +    if (ret != FPGA_ASYNC_ERR_NONE) {
>>>>>> +        fpga_async_dev_error(mgr, ret);
>>>>>> +        goto modput_exit;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->progress = FPGA_ASYNC_PROG_WRITING;
>>>>>> +    size = update->remaining_size;
>>>>>> +    while (size) {
>>>>>> +        blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>>>>>> +        size -= blk_size;
>>>>>> +        ret = mgr->mops->async_write_blk(mgr, mgr->async_update.data,
>>>>>> +                         offset, blk_size);
>>>>
>>>> It is not good idea to set 0x400 bytes here, the size of data chunk should be decided by low-level driver.
>>>
>>> 0x4000
>>>
>>>>
>>>> The actual hardware device or firmware may be able to handle data chunk which is larger than 0x400 bytes in one transaction. So it is good for FPGA manager to pass the data to low-level driver and leave the decision to low-level driver.
>>>
>>> I understand the concern. The decision to break the write up into multiple
>>> writes is done for the purpose of providing a progress indication to the
>>> user. This is only the first of several patches. Another patch will expose
>>> the remaining_size value via sysfs so that it can be monitored from user
>>> space. For the devices that I am working with, in the worst case the update
>>> takes about 40 minutes.
>>>
>>> 0x4000 could be changed to a larger number, or it could be left to the lower
>>> level driver to choose the size. The only thing that is happening between
>>> writes is update of the remaining_size, and checking to see if the user wants
>>> to cancel the update, so I don't think this design introduces an appreciable
>>> delay.
>>
>> I still think low-level driver rather than FPGA manager dirver should handle this.
> 
> Thanks Richard, I don't think it would be hard to make the change. I could
> incorporate that into and RFC v2 patch, or into the original patch-set. 

Thanks Russ!
I think I
> need to get feedback from Moritz to know which direction he would like to go.
> 
> To summarize, I think you and I agree that what were are trying to accomplish is
> not as similar as we first thought - there really is no overlap - right? 

That's is correct.
Your driver
> would not be able to leverage the code that I am working on?

It depends on the direction Moritz likes. If he doesn't like device tree 
overlay mechanism used for bitstream authentication, I will have to 
leverage your codes.

Regards,
Richard

> 
> The remaining question for me is whether or not it really makes sense to integrate
> the asynchronous image transfer/update into the FPGA Manager.
> 
> - Russ
> 
> 
>>
>> Regards,
>> Richard
>>
>>>
>>> - Russ
>>>
>>>>
>>>> Regards,
>>>> Richard
>>>>
>>>>>> +        if (ret != FPGA_ASYNC_ERR_NONE) {
>>>>>> +            fpga_async_dev_error(mgr, ret);
>>>>>> +            goto done;
>>>>>> +        }
>>>>>> +
>>>>>> +        update->remaining_size = size;
>>>>>> +        offset += blk_size;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->progress = FPGA_ASYNC_PROG_PROGRAMMING;
>>>>>> +    ret = mgr->mops->async_write_complete(mgr);
>>>>>> +    if (ret != FPGA_ASYNC_ERR_NONE)
>>>>>> +        fpga_async_dev_error(mgr, ret);
>>>>>> +
>>>>>> +done:
>>>>>> +    if (mgr->mops->async_cleanup)
>>>>>> +        mgr->mops->async_cleanup(mgr);
>>>>>> +
>>>>>> +modput_exit:
>>>>>> +    module_put(mgr->dev.parent->driver->owner);
>>>>>> +
>>>>>> +release_fw_exit:
>>>>>> +    update->data = NULL;
>>>>>> +    release_firmware(fw);
>>>>>> +
>>>>>> +idle_exit:
>>>>>> +    /*
>>>>>> +     * Note: update->remaining_size is left unmodified here to
>>>>>> +     * provide additional information on errors. It will be
>>>>>> +     * reinitialized when the next async update begins.
>>>>>> +     */
>>>>>> +    kfree(update->filename);
>>>>>> +    update->filename = NULL;
>>>>>> +    put_device(&mgr->dev);
>>>>>> +    progress_complete(mgr);
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>>>>> +                  const char *buf, size_t count) {
>>>>>> +    struct fpga_manager *mgr = to_fpga_manager(dev);
>>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>>> +    int ret = count;
>>>>>> +
>>>>>> +    if (count == 0 || count >= PATH_MAX)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    mutex_lock(&update->lock);
>>>>>> +    if (update->driver_unload || update->progress != FPGA_ASYNC_PROG_IDLE) {
>>>>>> +        ret = -EBUSY;
>>>>>> +        goto unlock_exit;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->filename = kmemdup_nul(buf, count, GFP_KERNEL);
>>>>>> +    if (!update->filename) {
>>>>>> +        ret = -ENOMEM;
>>>>>> +        goto unlock_exit;
>>>>>> +    }
>>>>>> +
>>>>>> +    update->err_code = FPGA_ASYNC_ERR_NONE;
>>>>>> +    update->progress = FPGA_ASYNC_PROG_READING;
>>>>>> +    reinit_completion(&update->done);
>>>>>> +    schedule_work(&update->work);
>>>>>> +
>>>>>> +unlock_exit:
>>>>>> +    mutex_unlock(&update->lock);
>>>>>> +    return ret;
>>>>>> +}
>>>>>> +static DEVICE_ATTR_WO(filename);
>>>>>> +
>>>>>> +static umode_t
>>>>>> +fpga_async_update_visible(struct kobject *kobj, struct attribute
>>>>>> +*attr, int n) {
>>>>>> +    struct fpga_manager *mgr = to_fpga_manager(kobj_to_dev(kobj));
>>>>>> +
>>>>>> +    if (!mgr->mops->async_write_init)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    return attr->mode;
>>>>>> +}
>>>>>> +
>>>>>> +static struct attribute *fpga_mgr_async_attrs[] = {
>>>>>> +    &dev_attr_filename.attr,
>>>>>> +    NULL,
>>>>>> +};
>>>>>> +
>>>>>> +static struct attribute_group fpga_mgr_async_group = {
>>>>>> +    .name = "async_update",
>>>>>> +    .attrs = fpga_mgr_async_attrs,
>>>>>> +    .is_visible = fpga_async_update_visible, };
>>>>>> +
>>>>>> +static const struct attribute_group *fpga_mgr_groups[] = {
>>>>>> +    &fpga_mgr_group,
>>>>>> +    &fpga_mgr_async_group,
>>>>>> +    NULL,
>>>>>> +};
>>>>>>       static struct fpga_manager *__fpga_mgr_get(struct device *dev)  { @@
>>>>>> -575,6 +735,15 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>>>             return NULL;
>>>>>>         }
>>>>>>     +    if (mops->async_write_init || mops->async_write_blk ||
>>>>>> +        mops->async_write_complete || mops->async_cancel ||
>>>>>> +        mops->async_cleanup) {
>>>>>> +        if (!mops->async_write_init || !mops->async_write_blk ||
>>>>>> +            !mops->async_write_complete || !mops->async_cancel)
>>>>>> +            dev_err(dev, "Attempt to register incomplete async ops\n");
>>>>>> +            return NULL;
>>>>>> +    }
>>>>>> +
>>>>>>         if (!name || !strlen(name)) {
>>>>>>             dev_err(dev, "Attempt to register with no name!\n");
>>>>>>             return NULL;
>>>>>> @@ -594,6 +763,12 @@ struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>>>>>>         mgr->mops = mops;
>>>>>>         mgr->priv = priv;
>>>>>>     +    mgr->async_update.driver_unload = false;
>>>>>> +    mgr->async_update.progress = FPGA_ASYNC_PROG_IDLE;
>>>>>> +    mutex_init(&mgr->async_update.lock);
>>>>>> +    init_completion(&mgr->async_update.done);
>>>>>> +    INIT_WORK(&mgr->async_update.work, fpga_mgr_async_update);
>>>>>> +
>>>>>>         device_initialize(&mgr->dev);
>>>>>>         mgr->dev.class = fpga_mgr_class;
>>>>>>         mgr->dev.groups = mops->groups;
>>>>>> @@ -710,11 +885,33 @@ EXPORT_SYMBOL_GPL(fpga_mgr_register);
>>>>>>      * @mgr: fpga manager struct
>>>>>>      *
>>>>>>      * This function is intended for use in a FPGA manager driver's remove function.
>>>>>> + *
>>>>>> + * For some devices, once an aysynchronous update has begun the
>>>>>> + authentication
>>>>>> + * phase, the hardware cannot be signaled to stop, and the driver
>>>>>> + will not exit
>>>>>> + * until the hardware signals completion.  This could be 30+ minutes of waiting.
>>>>>> + * The driver_unload flag enables a force-unload of the driver (e.g.
>>>>>> + modprobe -r)
>>>>>> + * by signaling the parent driver to exit even if the hardware update is incomplete.
>>>>>> + * The driver_unload flag also prevents new updates from starting
>>>>>> + once the
>>>>>> + * unregister process has begun.
>>>>>>      */
>>>>>>     void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>>>>>> +    struct fpga_async_update *update = &mgr->async_update;
>>>>>> +
>>>>>>         dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>>>>>     +    mutex_lock(&update->lock);
>>>>>> +    update->driver_unload = true;
>>>>>> +    if (update->progress == FPGA_ASYNC_PROG_IDLE) {
>>>>>> +        mutex_unlock(&update->lock);
>>>>>> +        goto unregister;
>>>>>> +    }
>>>>>> +
>>>>>> +    mutex_unlock(&update->lock);
>>>>>> +    wait_for_completion(&update->done);
>>>>>> +
>>>>>> +unregister:
>>>>>> +
>>>>>>         /*
>>>>>>          * If the low level driver provides a method for putting fpga into
>>>>>>          * a desired state upon unregister, do it.
>>>>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>>>>> b/include/linux/fpga/fpga-mgr.h index 2bc3030a69e5..9ff6c55f7a43
>>>>>> 100644
>>>>>> --- a/include/linux/fpga/fpga-mgr.h
>>>>>> +++ b/include/linux/fpga/fpga-mgr.h
>>>>>> @@ -9,6 +9,7 @@
>>>>>>     #define _LINUX_FPGA_MGR_H
>>>>>>       #include <linux/mutex.h>
>>>>>> +#include <linux/completion.h>
>>>>>>     #include <linux/platform_device.h>
>>>>>>       struct fpga_manager;
>>>>>> @@ -74,6 +75,30 @@ enum fpga_mgr_states {
>>>>>>     #define FPGA_MGR_BITSTREAM_LSB_FIRST    BIT(3)
>>>>>>     #define FPGA_MGR_COMPRESSED_BITSTREAM    BIT(4)
>>>>>>     +/* Asynchronous update error codes */ enum fpga_async_err {
>>>>>> +    FPGA_ASYNC_ERR_NONE,
>>>>>> +    FPGA_ASYNC_ERR_HW_ERROR,
>>>>>> +    FPGA_ASYNC_ERR_TIMEOUT,
>>>>>> +    FPGA_ASYNC_ERR_CANCELED,
>>>>>> +    FPGA_ASYNC_ERR_BUSY,
>>>>>> +    FPGA_ASYNC_ERR_INVALID_SIZE,
>>>>>> +    FPGA_ASYNC_ERR_RW_ERROR,
>>>>>> +    FPGA_ASYNC_ERR_WEAROUT,
>>>>>> +    FPGA_ASYNC_ERR_FILE_READ,
>>>>>> +    FPGA_ASYNC_ERR_MAX
>>>>>> +};
>>>>>> +
>>>>>> +/* Asynchronous update progress codes */ enum fpga_async_prog {
>>>>>> +    FPGA_ASYNC_PROG_IDLE,
>>>>>> +    FPGA_ASYNC_PROG_READING,
>>>>>> +    FPGA_ASYNC_PROG_PREPARING,
>>>>>> +    FPGA_ASYNC_PROG_WRITING,
>>>>>> +    FPGA_ASYNC_PROG_PROGRAMMING,
>>>>>> +    FPGA_ASYNC_PROG_MAX
>>>>>> +};
>>>>>> +
>>>>>>     /**
>>>>>>      * struct fpga_image_info - information specific to a FPGA image
>>>>>>      * @flags: boolean flags as defined above @@ -133,6 +158,16 @@ struct
>>>>>> fpga_manager_ops {
>>>>>>         int (*write_complete)(struct fpga_manager *mgr,
>>>>>>                       struct fpga_image_info *info);
>>>>>>         void (*fpga_remove)(struct fpga_manager *mgr);
>>>>>> +
>>>>>> +    /* async update ops */
>>>>>> +    enum fpga_async_err (*async_write_init)(struct fpga_manager *mgr,
>>>>>> +                 size_t count);
>>>>>> +    enum fpga_async_err (*async_write_blk)(struct fpga_manager *mgr,
>>>>>> +                 const char *buf, u32 offset, size_t count);
>>>>>> +    enum fpga_async_err (*async_write_complete)(struct fpga_manager *mgr);
>>>>>> +    enum fpga_async_err (*async_cancel)(struct fpga_manager *mgr);
>>>>>> +    void (*async_cleanup)(struct fpga_manager *mgr);
>>>>>> +
>>>>>>         const struct attribute_group **groups;  };
>>>>>>     @@ -154,6 +189,22 @@ struct fpga_compat_id {
>>>>>>         u64 id_l;
>>>>>>     };
>>>>>>     +/**
>>>>>> + * struct fpga_async_update - asynchronous image update structure
>>>>>> + * @name: name of low level fpga manager  */ struct fpga_async_update
>>>>>> +{
>>>>>> +    char *filename;
>>>>>> +    const u8 *data;            /* pointer to update data */
>>>>>> +    u32 remaining_size;        /* size remaining to transfer */
>>>>>> +    struct work_struct work;
>>>>>> +    struct completion done;
>>>>>> +    enum fpga_async_prog progress;
>>>>>> +    enum fpga_async_err err_code;    /* async update error code */
>>>>>> +    bool driver_unload;
>>>>>> +    struct mutex lock;        /* protect data structure contents */
>>>>>> +};
>>>>>> +
>>>>>>     /**
>>>>>>      * struct fpga_manager - fpga manager structure
>>>>>>      * @name: name of low level fpga manager @@ -171,6 +222,7 @@ struct
>>>>>> fpga_manager {
>>>>>>         enum fpga_mgr_states state;
>>>>>>         struct fpga_compat_id *compat_id;
>>>>>>         const struct fpga_manager_ops *mops;
>>>>>> +    struct fpga_async_update async_update;
>>>>>>         void *priv;
>>>>>>     };
>>>>>>     
>>>>>
>>>
> 

Re: [RFC PATCH v1 0/1] Extend FPGA manager with async image updates

[Moritz Fischer]

Hi Russ,

On Fri, Apr 09, 2021 at 05:38:09PM -0700, Russ Weight wrote:
> Hi Moritz,
> 
> This RFC patch is a follow-on to my evaluation of the possibility of merging
> secure update functionality into the FPGA Manager. My earlier RFC email
> is here: https://marc.info/?l=linux-fpga&m=161783978715092&w=2.
> 
> This RFC patch implements the core fpga_sec_mgr_update() function from the
> security manager patchset:
> https://marc.info/?l=linux-fpga&m=161525020621455&w=2.
> 
> Specifically, it is a port of this patch:
> https://marc.info/?l=linux-fpga&m=161525020721457&w=2
> 
> I think this patch provides enough context for further discussion. It
> extends functionality without leveraging any common code (because I
> didn't see an opportunity to share code).
> 
> In this patch, I am using the term "async" (in reference to the kernel
> worker thread) instead of the term "security". While the security manager
> patches were originally created specifically to support Intel secure image
> updates, there is nothing inherently secure about the driver support, other
> than the fact that the operation is essentially atomic: one write to the
> "filename" sysfs entry is all that is required from user-space to do
> an update. Our convention is to use signed, self-describing images that are
> interpreted by the card BMC, but one could use a non-signed image or even
> interpret the contents of the image based on the context of the parent
> driver. I think the main differentiating factors are:
> 
> (1) sysfs interface: an update is an atomic operation accomplished with a
>     single write.
> (2) self-describing: The type of information contained in the FPGA Manager
>     fpga_image_info structure would have to be included in the image file
>     and interpreted by the parent driver (not the class driver).
> (3) asynchronous: A write to the "filename" sysfs node write will return
>     immediately and the update will proceed in the context of a kernel
>     worker thread. Additional sysfs interfaces would be used to monitor the
>     progress and determine the ultimate success or failure of the update.
> (4) No notion of regions, bridges, or FPGA state. For Intel PAC cards, some
>     image files don't even contain an FPGA image. If they do, the image could
>     become active on the next power-cycle, or it could be activated through
>     some other trigger mechanism.
> 
> Can existing ops be leveraged?
> ==============================
> write: The current write op _could_ be used if the prototype were modified to
> accept an additional offset parameter. For the async update, writes are done
> in chunks, and the target offset needs to be passed on each write.
> 
> write_init and write_complete _could_ be used without change.
> 
> Other ops would have to be added: cancel, cleanup, hw_errinfo
> 
> I chose to implement all new ops because of the return data types. The fpga-mgr
> ops use the standard negative errno values. More descriptive and relevant error
> information can be provided via sysfs by defining a set of enum error codes.
> For example, it is very helpful to be able to tell the user that they are in a
> FLASH-wearout state, but standard errno values do not facilitate the
> communication of a wearout error.
> 
> Would it be better to share the two or three ops that can be shared, and be
> content with the standard error numbers? Or is it OK to use separate ops?
> 
> Should async updates be available via exported symbol?
> ======================================================
> As I understand it, current image updates through the FPGA Manager are all
> started with a call to the exported symbol fpga_mgr_load(). It would be
> possible to export an fpga_mgr_async_load() symbol, but there would need
> to be additional exported symbols to facilitate the collection of status
> information. Is there a use case for this?
> 
> Can a common update function be used?
> =====================================
> fpga_mgr_async_update() is analagous to fpga_mgr_load(). However, all async
> updates use the request_firmware framework. The FPGA Manager supports two
> separate flows: a single image buffer or scatter-gather. It would be possible
> to integrate these features for async updates, but I'm not sure that there is a
> need for such functionality.
> 
> I look forward to your feedback. Do you see value in integrating the two
> drivers? Should I continue this process?
> 
> - Russ
> 
> Russ Weight (1):
>   fpga: mgr: enable asynchronous image updates
> 
>  .../ABI/testing/sysfs-class-fpga-manager      |   9 +
>  drivers/fpga/fpga-mgr.c                       | 199 +++++++++++++++++-
>  include/linux/fpga/fpga-mgr.h                 |  52 +++++
>  3 files changed, 259 insertions(+), 1 deletion(-)
> 
> -- 
> 2.25.1
> 

Apologies for the late reply, was out on vacation.

Will take a look at this this week.

- Moritz