[RFC v3 0/1]Add user space interaction for FPGA programming

[RFC v3 0/1]Add user space interaction for FPGA programming

[Nava kishore Manne]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="N", Size: 2619 bytes --]

The existing FPGA manager subsystem didn't have any user space interface
(other than the status/state in sysfs) in Kernel. Basically, FPGAs are
semiconductor devices that can be reprogrammed for desired hardware
functionality.

FPGAs can be reprogrammed at runtime with different types of logic and IPs
as per user need and hence there is a need to use device tree overlays for
removing/updating/adding the devices at runtime for the IPs/controllers
that are present in FPGA. But we don't have any user interface in kernel
for updating the device tree at runtime.

Sometime back there was a series sent by Pantelis Antoniou
(https://lore.kernel.org/lkml/1414528565-10907-4-git-send-email-pantelis.antoniou@konsulko.com/).

This patch introduced a user interface configfs for Device Tree overlays,
a method of dynamically altering the kernel's live Device Tree. However,
this patch series was not accepted in mainline due to various concerns.
For more details refer to this link:
https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts#issues_and_what_needs_to_be_completed_--_Not_an_exhaustive_list

One of the major valid concerns that were raised with this configfs
interface was security. It provides a generic interface (Not specific
to the use cases) for modifying the live device tree.

In order to configure/program the FPGA devices, All the major vendors of
FPGA are using this configfs series as out-of-tree patch for configuring
the FPGAs and there was never an attempt to introduce a generic interface
to configure/program the FPGA in upstream and hence upstream kernel ended
up in not having proper support for FPGAs configure/program.
This series tries to address this gap of FPGA programmability by providing
a new ConfigFS-based interface to the user.

The newly introduced ConfigFS interface offers a generic and standardized
mechanism for configuring or reprogramming FPGAs at runtime. It supports
both Open Firmware (OF) and non-OF devices, utilizing vendor-specific
callbacks—such as pre_config, post_config, removal, and status checks.
To accommodate diverse device-specific configurations.

This solution enhances FPGA runtime management, supporting various device
types and vendors, while ensuring compatibility with the current FPGA
configuration flow.

Nava kishore Manne (1):
  fpga-region: Introduce ConfigFS interface for runtime FPGA
    configuration

 drivers/fpga/fpga-region.c       | 196 +++++++++++++
 drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
 include/linux/fpga/fpga-region.h |  34 +++
 3 files changed, 493 insertions(+), 211 deletions(-)

-- 
2.43.0

[RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Nava kishore Manne]

Introduces an ConfigFS interface within the fpga-region subsystem,
providing a generic and standardized mechanism for configuring (or)
reprogramming FPGAs during runtime. The newly added interface supports
both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
callbacks (e.g., pre_config, post_config, removal, and status) to
accommodate a wide range of device specific configurations.

The ConfigFS interface ensures compatibility with both OF and non-OF
devices, allowing for seamless FPGA reprogramming across diverse
platforms.

Vendor-specific callbacks are integrated into the interface, enabling
custom FPGA pre_config, post_config, removal, and status reporting
mechanisms, ensuring flexibility for vendor implementations.

This solution enhances FPGA runtime management, supporting various device
types and vendors, while ensuring compatibility with the current FPGA
configuration flow.

Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
---
Changes for v3:
 - As discussed with Yilun, the implementation continues to use a callback-based
 approach to seamlessly support both OF (Open Firmware) and non-OF devices via
 vendor-specific hooks. Additionally, the earlier IOCTL-based interface has been
 replaced with a more suitable ConfigFS-based mechanism to enable runtime FPGA
 configuration.

Changes for v2:
 - As discussed with Yilun, the implementation has been modified to utilize a
 callback approach, enabling seamless handling of both OF and non-OF devices.

 - As suggested by Yilun in the POC code, we have moved away from using  void *args
 as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
 utilizing the fpga_region_config_info structure to gather user inputs. Currently,
 this structure is implemented to support only OF devices, but we intend to extend
 it by incorporating new members to accommodate non-OF devices in the future.

 drivers/fpga/fpga-region.c       | 196 +++++++++++++
 drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
 include/linux/fpga/fpga-region.h |  34 +++
 3 files changed, 493 insertions(+), 211 deletions(-)

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 753cd142503e..d583fc22955b 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -5,6 +5,7 @@
  *  Copyright (C) 2013-2016 Altera Corporation
  *  Copyright (C) 2017 Intel Corporation
  */
+#include <linux/configfs.h>
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
@@ -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] = {
 };
 ATTRIBUTE_GROUPS(fpga_region);
 
+static struct fpga_region *item_to_fpga_region(struct config_item *item)
+{
+	return container_of(to_configfs_subsystem(to_config_group(item)),
+			    struct fpga_region, subsys);
+}
+
+/**
+ * fpga_region_image_store - Set firmware image name for FPGA region
+ * This function sets the firmware image name for an FPGA region through configfs.
+ * @item: Configfs item representing the FPGA region
+ * @buf: Input buffer containing the firmware image name
+ * @count: Size of the input buffer
+ *
+ * Return: Number of bytes written on success, or negative errno on failure.
+ */
+static ssize_t fpga_region_image_store(struct config_item *item, const char *buf, size_t count)
+{
+	struct fpga_region *region = item_to_fpga_region(item);
+	struct device *dev = &region->dev;
+	struct fpga_image_info *info;
+	char firmware_name[NAME_MAX];
+	char *s;
+
+	if (region->info) {
+		dev_err(dev, "Region already has already configured.\n");
+		return -EINVAL;
+	}
+
+	info = fpga_image_info_alloc(dev);
+	if (!info)
+		return -ENOMEM;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
+	firmware_name[sizeof(firmware_name) - 1] = '\0';
+
+	/* strip trailing newlines */
+	s = firmware_name + strlen(firmware_name);
+	while (s > firmware_name && *--s == '\n')
+		*s = '\0';
+
+	region->firmware_name = devm_kstrdup(dev, firmware_name, GFP_KERNEL);
+	if (!region->firmware_name)
+		return -ENOMEM;
+
+	region->info = info;
+
+	return count;
+}
+
+/**
+ * fpga_region_config_store - Trigger FPGA configuration via configfs
+ * @item: Configfs item representing the FPGA region
+ * @buf: Input buffer containing the configuration command (expects "1" to program, "0" to remove)
+ * @count: Size of the input buffer
+ *
+ * If the input is "1", this function performs:
+ *   1. region_pre_config() (if defined)
+ *   2. Bitstream programming via fpga_region_program_fpga() (unless external config flag is set)
+ *   3. region_post_config() (if defined)
+ *
+ * If the input is "0", it triggers region_remove() (if defined).
+ *
+ * Return: Number of bytes processed on success, or negative errno on failure.
+ */
+static ssize_t fpga_region_config_store(struct config_item *item,
+					const char *buf, size_t count)
+{
+	struct fpga_region *region = item_to_fpga_region(item);
+	int config_value, ret = 0;
+
+	/* Parse input: must be "0" or "1" */
+	if (kstrtoint(buf, 10, &config_value) || (config_value != 0 && config_value != 1))
+		return -EINVAL;
+
+	/* Ensure fpga_image_info is available */
+	if (!region->info)
+		return -EINVAL;
+
+	if (config_value == 1) {
+		/* Pre-config */
+		if (region->region_ops->region_pre_config) {
+			ret = region->region_ops->region_pre_config(region);
+			if (ret)
+				return ret;
+		}
+
+		/* Program bitstream if not external */
+		if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
+			ret = fpga_region_program_fpga(region);
+			if (ret)
+				return ret;
+		}
+
+		/* Post-config */
+		if (region->region_ops->region_post_config) {
+			ret = region->region_ops->region_post_config(region);
+			if (ret)
+				return ret;
+		}
+
+	} else {
+		/* Remove configuration */
+		if (region->region_ops->region_remove) {
+			ret = region->region_ops->region_remove(region);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return count;
+}
+
+/* Define Attributes */
+CONFIGFS_ATTR_WO(fpga_region_, image);
+CONFIGFS_ATTR_WO(fpga_region_, config);
+
+/* Attribute List */
+static struct configfs_attribute *fpga_region_config_attrs[] = {
+	&fpga_region_attr_image,
+	&fpga_region_attr_config,
+	NULL,
+};
+
+/* ConfigFS Item Type */
+static const struct config_item_type fpga_region_item_type = {
+	.ct_attrs = fpga_region_config_attrs,
+	.ct_owner = THIS_MODULE,
+};
+
+static int fpga_region_configfs_register(struct fpga_region *region)
+{
+	struct configfs_subsystem *subsys = &region->subsys;
+
+	snprintf(subsys->su_group.cg_item.ci_namebuf,
+		 sizeof(subsys->su_group.cg_item.ci_namebuf),
+		 "%s", dev_name(&region->dev));
+
+	subsys->su_group.cg_item.ci_type = &fpga_region_item_type;
+
+	config_group_init(&subsys->su_group);
+
+	return configfs_register_subsystem(subsys);
+}
+
+static void fpga_region_configfs_unregister(struct fpga_region *region)
+{
+	struct configfs_subsystem *subsys = &region->subsys;
+
+	configfs_unregister_subsystem(subsys);
+}
+
 /**
  * __fpga_region_register_full - create and register an FPGA Region device
  * @parent: device parent
@@ -229,8 +382,16 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
 	if (ret)
 		goto err_remove;
 
+	if (info->region_ops) {
+		region->region_ops = info->region_ops;
+		ret = fpga_region_configfs_register(region);
+		if (ret)
+			goto err_remove;
+	}
+
 	ret = device_register(&region->dev);
 	if (ret) {
+		fpga_region_configfs_unregister(region);
 		put_device(&region->dev);
 		return ERR_PTR(ret);
 	}
@@ -272,6 +433,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(__fpga_region_register);
 
+/**
+ * __fpga_region_register_with_ops - create and register an FPGA Region device
+ * with user interface call-backs.
+ * @parent: device parent
+ * @mgr: manager that programs this region
+ * @region_ops: ops for low level FPGA region for device enumeration/removal
+ * @priv: of-fpga-region private data
+ * @get_bridges: optional function to get bridges to a list
+ * @owner: module containing the get_bridges function
+ *
+ * This simple version of the register function should be sufficient for most users.
+ * The fpga_region_register_full() function is available for users that need to
+ * pass additional, optional parameters.
+ *
+ * Return: struct fpga_region or ERR_PTR()
+ */
+struct fpga_region *
+__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
+				const struct fpga_region_ops *region_ops,
+				void *priv,
+				int (*get_bridges)(struct fpga_region *),
+				struct module *owner)
+{
+	struct fpga_region_info info = { 0 };
+
+	info.mgr = mgr;
+	info.priv = priv;
+	info.get_bridges = get_bridges;
+	info.region_ops = region_ops;
+
+	return __fpga_region_register_full(parent, &info, owner);
+}
+EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
+
 /**
  * fpga_region_unregister - unregister an FPGA region
  * @region: FPGA region
@@ -280,6 +475,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
  */
 void fpga_region_unregister(struct fpga_region *region)
 {
+	fpga_region_configfs_unregister(region);
 	device_unregister(&region->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_region_unregister);
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 43db4bb77138..7088835a224a 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -8,16 +8,34 @@
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
+#include <linux/firmware.h>
+#include <linux/fpga-region.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
+/**
+ * struct of_fpga_region_priv - Private data structure
+ * image.
+ * @dev:	Device data structure
+ * @fw:		firmware of coeff table.
+ * @path:	path of FPGA overlay image firmware file.
+ * @ovcs_id:	overlay changeset id.
+ */
+struct of_fpga_region_priv {
+	struct device *dev;
+	const struct firmware *fw;
+	int ovcs_id;
+};
+
 static const struct of_device_id fpga_region_of_match[] = {
 	{ .compatible = "fpga-region", },
 	{},
@@ -141,273 +159,322 @@ static int of_fpga_region_get_bridges(struct fpga_region *region)
 }
 
 /**
- * child_regions_with_firmware - Used to check the child region info.
- * @overlay: device node of the overlay
+ * of_fpga_region_post_remove - post-remove overlay
  *
- * If the overlay adds child FPGA regions, they are not allowed to have
- * firmware-name property.
+ * @region: FPGA region that was targeted by the overlay that was removed
  *
- * Return: 0 for OK or -EINVAL if child FPGA region adds firmware-name.
+ * Called after an overlay has been removed if the overlay's target was a
+ * FPGA region.
  */
-static int child_regions_with_firmware(struct device_node *overlay)
+static void of_fpga_region_post_remove(struct fpga_region *region)
 {
-	struct device_node *child_region;
-	const char *child_firmware_name;
-	int ret = 0;
-
-	of_node_get(overlay);
-
-	child_region = of_find_matching_node(overlay, fpga_region_of_match);
-	while (child_region) {
-		if (!of_property_read_string(child_region, "firmware-name",
-					     &child_firmware_name)) {
-			ret = -EINVAL;
-			break;
-		}
-		child_region = of_find_matching_node(child_region,
-						     fpga_region_of_match);
-	}
+	fpga_bridges_disable(&region->bridge_list);
+	fpga_bridges_put(&region->bridge_list);
+	fpga_image_info_free(region->info);
+	region->info = NULL;
+}
 
-	of_node_put(child_region);
+static int of_fpga_region_status(struct fpga_region *region)
+{
+	struct of_fpga_region_priv *ovcs = region->priv;
 
-	if (ret)
-		pr_err("firmware-name not allowed in child FPGA region: %pOF",
-		       child_region);
+	if (ovcs->ovcs_id)
+		return FPGA_REGION_HAS_PL;
 
-	return ret;
+	return FPGA_REGION_EMPTY;
 }
 
-/**
- * of_fpga_region_parse_ov - parse and check overlay applied to region
+/*
+ * FPGA DTBO Parser
  *
- * @region: FPGA region
- * @overlay: overlay applied to the FPGA region
+ * This file contains the implementation of a function to parse Device Tree
+ * Blob Overlay (DTBO) files used for dynamic reconfiguration of  FPGAs in
+ * Linux. The `parse_dtbo()` function is responsible for:
  *
- * Given an overlay applied to an FPGA region, parse the FPGA image specific
- * info in the overlay and do some checking.
+ * - Validating the DTBO header
+ * - Extracting fixups and resolving symbolic paths to actual device nodes
+ * - Identifying the FPGA region targeted by the overlay
+ * - Allocating and populating the `fpga_image_info` structure with
+ *   relevant configuration such as flags, firmware name, and timeout values
+ * - Retrieving and associating any fpga-bridges specified in the overlay
  *
- * Return:
- *   NULL if overlay doesn't direct us to program the FPGA.
- *   fpga_image_info struct if there is an image to program.
- *   error code for invalid overlay.
+ * This function leverages the Flattened Device Tree (FDT) and OF (Open Firmware)
+ * APIs to interpret the overlay and prepare the FPGA Manager for a runtime
+ * configuration update. It is intended for use in dynamic reconfiguration
+ * scenarios where full/partial bitstreams are applied using overlay files.
+ *
+ * Returns 0 on success or a negative error code on failure.
  */
-static struct fpga_image_info *
-of_fpga_region_parse_ov(struct fpga_region *region,
-			struct device_node *overlay)
+static int parse_dtbo(const struct firmware *fw)
 {
-	struct device *dev = &region->dev;
+	int fixups_off, prop_off, overlay_off, prop_len, fw_name_len, ret;
+	const char *prop_name, *symbol_path, *fw_name, *name, *value;
+	struct device_node *symbols_node = NULL;
+	struct device_node *fpga_node = NULL;
+	struct device_node *br_node = NULL;
+	const struct fdt_property *prop;
 	struct fpga_image_info *info;
-	const char *firmware_name;
-	int ret;
+	struct fpga_region *region;
+	const fdt32_t *val;
 
-	if (region->info) {
-		dev_err(dev, "Region already has overlay applied.\n");
-		return ERR_PTR(-EINVAL);
+	/* Validate DTBO header */
+	if (!fw || fdt_check_header(fw->data) < 0) {
+		pr_err("%s: Invalid DTBO file\n", __func__);
+		return -EINVAL;
 	}
 
-	/*
-	 * Reject overlay if child FPGA Regions added in the overlay have
-	 * firmware-name property (would mean that an FPGA region that has
-	 * not been added to the live tree yet is doing FPGA programming).
-	 */
-	ret = child_regions_with_firmware(overlay);
-	if (ret)
-		return ERR_PTR(ret);
+	/* Locate __fixups__ node */
+	fixups_off = fdt_path_offset((void *)fw->data, "/__fixups__");
+	if (fixups_off < 0) {
+		pr_err("%s: __fixups__ node not found\n", __func__);
+		return -EINVAL;
+	}
 
-	info = fpga_image_info_alloc(dev);
-	if (!info)
-		return ERR_PTR(-ENOMEM);
+	/* Retrieve the first property under __fixups__ */
+	prop_off = fdt_first_property_offset((void *)fw->data, fixups_off);
+	if (prop_off < 0) {
+		pr_info("%s: No properties in __fixups__\n", __func__);
+		return -ENOENT;
+	}
+
+	prop = fdt_get_property_by_offset((void *)fw->data, prop_off, &prop_len);
+	if (!prop) {
+		pr_err("%s: Failed to get first __fixups__ property\n", __func__);
+		return -ENOENT;
+	}
+
+	prop_name = fdt_string((void *)fw->data, fdt32_to_cpu(prop->nameoff));
+
+	/* Locate __symbols__ node */
+	symbols_node = of_find_node_by_path("/__symbols__");
+	if (!symbols_node) {
+		pr_err("%s: Missing __symbols__ node\n", __func__);
+		return -ENODEV;
+	}
+
+	/* Resolve symbolic path to FPGA node */
+	symbol_path = of_get_property(symbols_node, prop_name, NULL);
+	if (!symbol_path) {
+		pr_err("%s: Symbol '%s' not found in __symbols__\n", __func__, prop_name);
+		goto err_put_symbols;
+	}
+
+	/* Retrieve FPGA region associated with the node */
+	fpga_node = of_find_node_by_path(symbol_path);
+	if (!fpga_node) {
+		pr_err("%s: FPGA node not found at path: %s\n", __func__, symbol_path);
+		goto err_put_symbols;
+	}
+
+	/* Retrieve FPGA region associated with the node */
+	region = of_fpga_region_find(fpga_node);
+	if (!region) {
+		pr_err("%s: FPGA region not found for: %s\n", __func__, symbol_path);
+		goto err_put_fpga;
+	}
+
+	/* Allocate FPGA image info structure */
+	info = fpga_image_info_alloc(&region->dev);
+	if (!info) {
+		ret = -ENOMEM;
+		goto err_put_fpga;
+	}
 
-	info->overlay = overlay;
+	/* Locate /fragment@0/__overlay__ node in the overlay */
+	overlay_off = fdt_path_offset((void *)fw->data, "/fragment@0/__overlay__");
+	if (overlay_off < 0) {
+		pr_err("%s: Missing /fragment@0/__overlay__\n", __func__);
+		ret = -ENOENT;
+		goto err_free_info;
+	}
 
-	/* Read FPGA region properties from the overlay */
-	if (of_property_read_bool(overlay, "partial-fpga-config"))
+	/* Parse optional configuration flags */
+	if (fdt_getprop(fw->data, overlay_off, "partial-fpga-config", NULL))
 		info->flags |= FPGA_MGR_PARTIAL_RECONFIG;
 
-	if (of_property_read_bool(overlay, "external-fpga-config"))
+	if (fdt_getprop(fw->data, overlay_off, "external-fpga-config", NULL))
 		info->flags |= FPGA_MGR_EXTERNAL_CONFIG;
 
-	if (of_property_read_bool(overlay, "encrypted-fpga-config"))
+	if (fdt_getprop(fw->data, overlay_off, "encrypted-fpga-config", NULL))
 		info->flags |= FPGA_MGR_ENCRYPTED_BITSTREAM;
 
-	if (!of_property_read_string(overlay, "firmware-name",
-				     &firmware_name)) {
-		info->firmware_name = devm_kstrdup(dev, firmware_name,
-						   GFP_KERNEL);
-		if (!info->firmware_name)
-			return ERR_PTR(-ENOMEM);
+	/* Retrieve firmware-name property */
+	fw_name = fdt_getprop(fw->data, overlay_off, "firmware-name", &fw_name_len);
+	if (fw_name) {
+		info->firmware_name = devm_kstrdup(&region->dev, fw_name, GFP_KERNEL);
+		if (!info->firmware_name) {
+			ret = -ENOMEM;
+			goto err_free_info;
+		}
 	}
 
-	of_property_read_u32(overlay, "region-unfreeze-timeout-us",
-			     &info->enable_timeout_us);
+	/* Parse optional timeout properties */
+	val = fdt_getprop(fw->data, overlay_off, "region-unfreeze-timeout-us", &fw_name_len);
+	if (val && fw_name_len == sizeof(fdt32_t))
+		info->enable_timeout_us = fdt32_to_cpu(*val);
 
-	of_property_read_u32(overlay, "region-freeze-timeout-us",
-			     &info->disable_timeout_us);
+	val = fdt_getprop(fw->data, overlay_off, "region-freeze-timeout-us", &fw_name_len);
+	if (val && fw_name_len == sizeof(fdt32_t))
+		info->disable_timeout_us = fdt32_to_cpu(*val);
 
-	of_property_read_u32(overlay, "config-complete-timeout-us",
-			     &info->config_complete_timeout_us);
+	val = fdt_getprop(fw->data, overlay_off, "config-complete-timeout-us", &fw_name_len);
+	if (val && fw_name_len == sizeof(fdt32_t))
+		info->config_complete_timeout_us = fdt32_to_cpu(*val);
 
-	/* If overlay is not programming the FPGA, don't need FPGA image info */
-	if (!info->firmware_name) {
-		ret = 0;
-		goto ret_no_info;
-	}
+	/* Attach parsed image info to the FPGA region */
+	region->info = info;
 
-	/*
-	 * If overlay informs us FPGA was externally programmed, specifying
-	 * firmware here would be ambiguous.
-	 */
-	if (info->flags & FPGA_MGR_EXTERNAL_CONFIG) {
-		dev_err(dev, "error: specified firmware and external-fpga-config");
-		ret = -EINVAL;
-		goto ret_no_info;
-	}
+	/* Handle optional fpga-bridges references */
+	fdt_for_each_property_offset(prop_off, (void *)fw->data, fixups_off) {
+		prop = fdt_get_property_by_offset((void *)fw->data, prop_off, NULL);
+		if (!prop)
+			continue;
 
-	return info;
-ret_no_info:
-	fpga_image_info_free(info);
-	return ERR_PTR(ret);
-}
+		name = fdt_string((void *)fw->data, fdt32_to_cpu(prop->nameoff));
+		value = prop->data;
 
-/**
- * of_fpga_region_notify_pre_apply - pre-apply overlay notification
- *
- * @region: FPGA region that the overlay was applied to
- * @nd: overlay notification data
- *
- * Called when an overlay targeted to an FPGA Region is about to be applied.
- * Parses the overlay for properties that influence how the FPGA will be
- * programmed and does some checking. If the checks pass, programs the FPGA.
- * If the checks fail, overlay is rejected and does not get added to the
- * live tree.
- *
- * Return: 0 for success or negative error code for failure.
- */
-static int of_fpga_region_notify_pre_apply(struct fpga_region *region,
-					   struct of_overlay_notify_data *nd)
-{
-	struct device *dev = &region->dev;
-	struct fpga_image_info *info;
-	int ret;
+		if (!name || !value)
+			continue;
 
-	info = of_fpga_region_parse_ov(region, nd->overlay);
-	if (IS_ERR(info))
-		return PTR_ERR(info);
+		if (strstr(value, "fpga-bridges")) {
+			symbol_path = of_get_property(symbols_node, name, NULL);
+			if (!symbol_path) {
+				pr_err("%s: Missing symbol for bridge: %s\n", __func__, name);
+				ret = -ENODEV;
+				goto err_put_symbols;
+			}
 
-	/* If overlay doesn't program the FPGA, accept it anyway. */
-	if (!info)
-		return 0;
+			br_node = of_find_node_by_path(symbol_path);
+			if (!br_node) {
+				pr_err("%s: Bridge node not found at: %s\n", __func__, symbol_path);
+				ret = -ENODEV;
+				goto err_put_symbols;
+			}
 
-	if (region->info) {
-		dev_err(dev, "Region already has overlay applied.\n");
-		return -EINVAL;
-	}
+			ret = of_fpga_bridge_get_to_list(br_node, info, &region->bridge_list);
+			of_node_put(br_node);
 
-	region->info = info;
-	ret = fpga_region_program_fpga(region);
-	if (ret) {
-		/* error; reject overlay */
-		fpga_image_info_free(info);
-		region->info = NULL;
+			if (ret == -EBUSY) {
+				fpga_bridges_put(&region->bridge_list);
+				goto err_put_symbols;
+			}
+		}
 	}
 
+	of_node_put(fpga_node);
+	of_node_put(symbols_node);
+
+	return 0;
+
+err_free_info:
+	fpga_image_info_free(info);
+err_put_fpga:
+	of_node_put(fpga_node);
+err_put_symbols:
+	of_node_put(symbols_node);
+
 	return ret;
 }
 
-/**
- * of_fpga_region_notify_post_remove - post-remove overlay notification
- *
- * @region: FPGA region that was targeted by the overlay that was removed
- * @nd: overlay notification data
- *
- * Called after an overlay has been removed if the overlay's target was a
- * FPGA region.
- */
-static void of_fpga_region_notify_post_remove(struct fpga_region *region,
-					      struct of_overlay_notify_data *nd)
+static int of_fpga_region_pre_config(struct fpga_region *region)
 {
-	fpga_bridges_disable(&region->bridge_list);
-	fpga_bridges_put(&region->bridge_list);
-	fpga_image_info_free(region->info);
-	region->info = NULL;
+	struct of_fpga_region_priv *ovcs = region->priv;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (ovcs->ovcs_id)
+		return -EPERM;
+
+	err = request_firmware(&ovcs->fw, region->firmware_name, NULL);
+	if (err)
+		return err;
+
+	err = parse_dtbo(ovcs->fw);
+	if (err)
+		return err;
+
+	release_firmware(ovcs->fw);
+
+	return 0;
 }
 
-/**
- * of_fpga_region_notify - reconfig notifier for dynamic DT changes
- * @nb:		notifier block
- * @action:	notifier action
- * @arg:	reconfig data
- *
- * This notifier handles programming an FPGA when a "firmware-name" property is
- * added to an fpga-region.
- *
- * Return: NOTIFY_OK or error if FPGA programming fails.
- */
-static int of_fpga_region_notify(struct notifier_block *nb,
-				 unsigned long action, void *arg)
+static int of_fpga_region_post_config(struct fpga_region *region)
 {
-	struct of_overlay_notify_data *nd = arg;
-	struct fpga_region *region;
-	int ret;
-
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		pr_debug("%s OF_OVERLAY_PRE_APPLY\n", __func__);
-		break;
-	case OF_OVERLAY_POST_APPLY:
-		pr_debug("%s OF_OVERLAY_POST_APPLY\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_PRE_REMOVE:
-		pr_debug("%s OF_OVERLAY_PRE_REMOVE\n", __func__);
-		return NOTIFY_OK;       /* not for us */
-	case OF_OVERLAY_POST_REMOVE:
-		pr_debug("%s OF_OVERLAY_POST_REMOVE\n", __func__);
-		break;
-	default:			/* should not happen */
-		return NOTIFY_OK;
+	struct of_fpga_region_priv *ovcs = region->priv;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (ovcs->ovcs_id)
+		return -EPERM;
+
+	err = request_firmware(&ovcs->fw, region->firmware_name, NULL);
+	if (err != 0)
+		goto out_err;
+
+	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
+				   &ovcs->ovcs_id, NULL);
+	if (err < 0) {
+		pr_err("%s: Failed to create overlay (err=%d)\n",
+		       __func__, err);
+		release_firmware(ovcs->fw);
+		goto out_err;
 	}
 
-	region = of_fpga_region_find(nd->target);
-	if (!region)
-		return NOTIFY_OK;
+	return 0;
 
-	ret = 0;
-	switch (action) {
-	case OF_OVERLAY_PRE_APPLY:
-		ret = of_fpga_region_notify_pre_apply(region, nd);
-		break;
+out_err:
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
 
-	case OF_OVERLAY_POST_REMOVE:
-		of_fpga_region_notify_post_remove(region, nd);
-		break;
-	}
+	return err;
+}
 
-	put_device(&region->dev);
+static int of_fpga_region_config_remove(struct fpga_region *region)
+{
+	struct of_fpga_region_priv *ovcs = region->priv;
+
+	if (!ovcs->ovcs_id)
+		return -EPERM;
 
-	if (ret)
-		return notifier_from_errno(ret);
+	of_overlay_remove(&ovcs->ovcs_id);
+	of_fpga_region_post_remove(region);
+	release_firmware(ovcs->fw);
 
-	return NOTIFY_OK;
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
+
+	return 0;
 }
 
-static struct notifier_block fpga_region_of_nb = {
-	.notifier_call = of_fpga_region_notify,
+static const struct fpga_region_ops region_ops = {
+	.region_status = of_fpga_region_status,
+	.region_pre_config = of_fpga_region_pre_config,
+	.region_post_config = of_fpga_region_post_config,
+	.region_remove = of_fpga_region_config_remove,
 };
 
 static int of_fpga_region_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct of_fpga_region_priv *priv;
 	struct fpga_region *region;
 	struct fpga_manager *mgr;
 	int ret;
 
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
 	/* Find the FPGA mgr specified by region or parent region. */
 	mgr = of_fpga_region_get_mgr(np);
 	if (IS_ERR(mgr))
 		return -EPROBE_DEFER;
 
-	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
+	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
+					       of_fpga_region_get_bridges);
 	if (IS_ERR(region)) {
 		ret = PTR_ERR(region);
 		goto eprobe_mgr_put;
@@ -451,27 +518,12 @@ static struct platform_driver of_fpga_region_driver = {
  */
 static int __init of_fpga_region_init(void)
 {
-	int ret;
-
-	ret = of_overlay_notifier_register(&fpga_region_of_nb);
-	if (ret)
-		return ret;
-
-	ret = platform_driver_register(&of_fpga_region_driver);
-	if (ret)
-		goto err_plat;
-
-	return 0;
-
-err_plat:
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
-	return ret;
+	return platform_driver_register(&of_fpga_region_driver);
 }
 
 static void __exit of_fpga_region_exit(void)
 {
 	platform_driver_unregister(&of_fpga_region_driver);
-	of_overlay_notifier_unregister(&fpga_region_of_nb);
 }
 
 subsys_initcall(of_fpga_region_init);
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 5fbc05fe70a6..d4e4beb62533 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -3,18 +3,37 @@
 #ifndef _FPGA_REGION_H
 #define _FPGA_REGION_H
 
+#include <linux/configfs.h>
 #include <linux/device.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga-region.h>
 
 struct fpga_region;
 
+/**
+ * struct fpga_region_ops - ops for low level FPGA region ops for device
+ * enumeration/removal
+ * @region_status: Check current status of the FPGA region.
+ * @region_pre_config: Called before starting FPGA configuration.
+ * @region_post_config: Called after FPGA configuration is done.
+ * @region_remove: Called to remove devices added during configuration.
+ */
+struct fpga_region_ops {
+	int (*region_status)(struct fpga_region *region);
+	int (*region_pre_config)(struct fpga_region *region);
+	int (*region_post_config)(struct fpga_region *region);
+	int (*region_remove)(struct fpga_region *region);
+};
+
 /**
  * struct fpga_region_info - collection of parameters an FPGA Region
  * @mgr: fpga region manager
  * @compat_id: FPGA region id for compatibility check.
  * @priv: fpga region private data
  * @get_bridges: optional function to get bridges to a list
+ * @fpga_region_ops: ops for low level FPGA region ops for device
+ * enumeration/removal
  *
  * fpga_region_info contains parameters for the register_full function.
  * These are separated into an info structure because they some are optional
@@ -26,6 +45,7 @@ struct fpga_region_info {
 	struct fpga_compat_id *compat_id;
 	void *priv;
 	int (*get_bridges)(struct fpga_region *region);
+	const struct fpga_region_ops *region_ops;
 };
 
 /**
@@ -39,6 +59,10 @@ struct fpga_region_info {
  * @ops_owner: module containing the get_bridges function
  * @priv: private data
  * @get_bridges: optional function to get bridges to a list
+ * @fpga_region_ops: ops for low level FPGA region ops for device
+ * enumeration/removal.
+ * @subsys: Configfs subsystem for exposing region configuration.
+ * @firmware_name: Firmware name set via configfs for image loading.
  */
 struct fpga_region {
 	struct device dev;
@@ -50,6 +74,9 @@ struct fpga_region {
 	struct module *ops_owner;
 	void *priv;
 	int (*get_bridges)(struct fpga_region *region);
+	const struct fpga_region_ops *region_ops;
+	struct configfs_subsystem subsys;
+	char *firmware_name;
 };
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
@@ -71,6 +98,13 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
 struct fpga_region *
 __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
 		       int (*get_bridges)(struct fpga_region *), struct module *owner);
+#define fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges) \
+	__fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges, THIS_MODULE)
+struct fpga_region *
+__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
+				const struct fpga_region_ops *region_ops, void *priv,
+				int (*get_bridges)(struct fpga_region *),
+				struct module *owner);
 void fpga_region_unregister(struct fpga_region *region);
 
 #endif /* _FPGA_REGION_H */
-- 
2.43.0

Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Xu Yilun]

On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> Introduces an ConfigFS interface within the fpga-region subsystem,
> providing a generic and standardized mechanism for configuring (or)
> reprogramming FPGAs during runtime. The newly added interface supports
> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> callbacks (e.g., pre_config, post_config, removal, and status) to
> accommodate a wide range of device specific configurations.
> 
> The ConfigFS interface ensures compatibility with both OF and non-OF
> devices, allowing for seamless FPGA reprogramming across diverse
> platforms.
> 
> Vendor-specific callbacks are integrated into the interface, enabling
> custom FPGA pre_config, post_config, removal, and status reporting
> mechanisms, ensuring flexibility for vendor implementations.
> 
> This solution enhances FPGA runtime management, supporting various device
> types and vendors, while ensuring compatibility with the current FPGA
> configuration flow.
> 
> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> ---
> Changes for v3:
>  - As discussed with Yilun, the implementation continues to use a callback-based
>  approach to seamlessly support both OF (Open Firmware) and non-OF devices via
>  vendor-specific hooks. Additionally, the earlier IOCTL-based interface has been
>  replaced with a more suitable ConfigFS-based mechanism to enable runtime FPGA
>  configuration.
> 
> Changes for v2:
>  - As discussed with Yilun, the implementation has been modified to utilize a
>  callback approach, enabling seamless handling of both OF and non-OF devices.
> 
>  - As suggested by Yilun in the POC code, we have moved away from using  void *args
>  as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
>  utilizing the fpga_region_config_info structure to gather user inputs. Currently,
>  this structure is implemented to support only OF devices, but we intend to extend
>  it by incorporating new members to accommodate non-OF devices in the future.
> 
>  drivers/fpga/fpga-region.c       | 196 +++++++++++++
>  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
>  include/linux/fpga/fpga-region.h |  34 +++
>  3 files changed, 493 insertions(+), 211 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 753cd142503e..d583fc22955b 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -5,6 +5,7 @@
>   *  Copyright (C) 2013-2016 Altera Corporation
>   *  Copyright (C) 2017 Intel Corporation
>   */
> +#include <linux/configfs.h>
>  #include <linux/fpga/fpga-bridge.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/fpga/fpga-region.h>
> @@ -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(fpga_region);
>  
> +static struct fpga_region *item_to_fpga_region(struct config_item *item)
> +{
> +	return container_of(to_configfs_subsystem(to_config_group(item)),
> +			    struct fpga_region, subsys);
> +}
> +
> +/**
> + * fpga_region_image_store - Set firmware image name for FPGA region
> + * This function sets the firmware image name for an FPGA region through configfs.
> + * @item: Configfs item representing the FPGA region
> + * @buf: Input buffer containing the firmware image name
> + * @count: Size of the input buffer
> + *
> + * Return: Number of bytes written on success, or negative errno on failure.
> + */
> +static ssize_t fpga_region_image_store(struct config_item *item, const char *buf, size_t count)
> +{
> +	struct fpga_region *region = item_to_fpga_region(item);
> +	struct device *dev = &region->dev;
> +	struct fpga_image_info *info;
> +	char firmware_name[NAME_MAX];
> +	char *s;
> +
> +	if (region->info) {
> +		dev_err(dev, "Region already has already configured.\n");
> +		return -EINVAL;
> +	}
> +
> +	info = fpga_image_info_alloc(dev);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	/* copy to path buffer (and make sure it's always zero terminated */
> +	count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> +	firmware_name[sizeof(firmware_name) - 1] = '\0';
> +
> +	/* strip trailing newlines */
> +	s = firmware_name + strlen(firmware_name);
> +	while (s > firmware_name && *--s == '\n')
> +		*s = '\0';
> +
> +	region->firmware_name = devm_kstrdup(dev, firmware_name, GFP_KERNEL);
> +	if (!region->firmware_name)
> +		return -ENOMEM;
> +
> +	region->info = info;
> +
> +	return count;
> +}
> +
> +/**
> + * fpga_region_config_store - Trigger FPGA configuration via configfs
> + * @item: Configfs item representing the FPGA region
> + * @buf: Input buffer containing the configuration command (expects "1" to program, "0" to remove)
> + * @count: Size of the input buffer
> + *
> + * If the input is "1", this function performs:
> + *   1. region_pre_config() (if defined)

Please define explicit workflow, and explicit expectation for each
callback, or this framework makes no sense. From your of-fpga-region
implementation, seems pre_config() means "parse image", post_config()
means "populate devices".

> + *   2. Bitstream programming via fpga_region_program_fpga() (unless external config flag is set)
> + *   3. region_post_config() (if defined)
> + *
> + * If the input is "0", it triggers region_remove() (if defined).
> + *
> + * Return: Number of bytes processed on success, or negative errno on failure.

Please put the uAPI description in Documentation/ABI/testing. Then we
could know the file path layout from userspace POV.

> + */
> +static ssize_t fpga_region_config_store(struct config_item *item,
> +					const char *buf, size_t count)
> +{
> +	struct fpga_region *region = item_to_fpga_region(item);
> +	int config_value, ret = 0;
> +
> +	/* Parse input: must be "0" or "1" */
> +	if (kstrtoint(buf, 10, &config_value) || (config_value != 0 && config_value != 1))
> +		return -EINVAL;
> +
> +	/* Ensure fpga_image_info is available */
> +	if (!region->info)
> +		return -EINVAL;
> +
> +	if (config_value == 1) {
> +		/* Pre-config */
> +		if (region->region_ops->region_pre_config) {
> +			ret = region->region_ops->region_pre_config(region);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Program bitstream if not external */
> +		if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> +			ret = fpga_region_program_fpga(region);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Post-config */
> +		if (region->region_ops->region_post_config) {
> +			ret = region->region_ops->region_post_config(region);
> +			if (ret)
> +				return ret;
> +		}
> +
> +	} else {
> +		/* Remove configuration */
> +		if (region->region_ops->region_remove) {
> +			ret = region->region_ops->region_remove(region);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return count;
> +}
> +
> +/* Define Attributes */
> +CONFIGFS_ATTR_WO(fpga_region_, image);
> +CONFIGFS_ATTR_WO(fpga_region_, config);
> +
> +/* Attribute List */
> +static struct configfs_attribute *fpga_region_config_attrs[] = {
> +	&fpga_region_attr_image,
> +	&fpga_region_attr_config,
> +	NULL,
> +};
> +
> +/* ConfigFS Item Type */
> +static const struct config_item_type fpga_region_item_type = {
> +	.ct_attrs = fpga_region_config_attrs,
> +	.ct_owner = THIS_MODULE,
> +};

I think this is still the sysfs methodology. My understanding from configfs.rst
is, use userspace interfaces to control the lifecycle of a kernel object.

Now for existing kernel reprogramming flow, the image object for
fpga_region is the struct fpga_image_info. We need to associate the
struct with a config_item: alloc the struct fpga_image_info instance by
mkdir, expose necessary fields (enable_timeout_us, disable_timeout_us,
firmware_name, and the most important for of-fpga-region - overlay blob ...)
for user to fill/query via configfs attributes. And finally use a writeable
attribute (e.g. load) to trigger fpga_region_program_fpga().

> +
> +static int fpga_region_configfs_register(struct fpga_region *region)
> +{
> +	struct configfs_subsystem *subsys = &region->subsys;
> +
> +	snprintf(subsys->su_group.cg_item.ci_namebuf,
> +		 sizeof(subsys->su_group.cg_item.ci_namebuf),
> +		 "%s", dev_name(&region->dev));
> +
> +	subsys->su_group.cg_item.ci_type = &fpga_region_item_type;
> +
> +	config_group_init(&subsys->su_group);

I think we'd better make a root "fpga_region" group to include all
regions.

> +
> +	return configfs_register_subsystem(subsys);
> +}
> +
> +static void fpga_region_configfs_unregister(struct fpga_region *region)
> +{
> +	struct configfs_subsystem *subsys = &region->subsys;
> +
> +	configfs_unregister_subsystem(subsys);
> +}

[...]

>  static void __exit of_fpga_region_exit(void)
>  {
>  	platform_driver_unregister(&of_fpga_region_driver);
> -	of_overlay_notifier_unregister(&fpga_region_of_nb);
>  }

Sorry, it is really hard to review if all the changes are mess up
together. Maybe I'll revisit it later. But next time please split
the patches to produce some readable diff.

Thanks,
Yilun

RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Manne, Nava kishore]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Yilun,

        Thanks for quick response.
Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Sunday, June 1, 2025 8:32 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> FPGA configuration
>
> On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > Introduces an ConfigFS interface within the fpga-region subsystem,
> > providing a generic and standardized mechanism for configuring (or)
> > reprogramming FPGAs during runtime. The newly added interface supports
> > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> > callbacks (e.g., pre_config, post_config, removal, and status) to
> > accommodate a wide range of device specific configurations.
> >
> > The ConfigFS interface ensures compatibility with both OF and non-OF
> > devices, allowing for seamless FPGA reprogramming across diverse
> > platforms.
> >
> > Vendor-specific callbacks are integrated into the interface, enabling
> > custom FPGA pre_config, post_config, removal, and status reporting
> > mechanisms, ensuring flexibility for vendor implementations.
> >
> > This solution enhances FPGA runtime management, supporting various
> > device types and vendors, while ensuring compatibility with the
> > current FPGA configuration flow.
> >
> > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > ---
> > Changes for v3:
> >  - As discussed with Yilun, the implementation continues to use a
> > callback-based  approach to seamlessly support both OF (Open Firmware)
> > and non-OF devices via  vendor-specific hooks. Additionally, the
> > earlier IOCTL-based interface has been  replaced with a more suitable
> > ConfigFS-based mechanism to enable runtime FPGA  configuration.
> >
> > Changes for v2:
> >  - As discussed with Yilun, the implementation has been modified to
> > utilize a  callback approach, enabling seamless handling of both OF and non-OF
> devices.
> >
> >  - As suggested by Yilun in the POC code, we have moved away from
> > using  void *args  as a parameter for ICOTL inputs to obtain the
> > required user inputs. Instead, we are  utilizing the
> > fpga_region_config_info structure to gather user inputs. Currently,
> > this structure is implemented to support only OF devices, but we intend to extend
> it by incorporating new members to accommodate non-OF devices in the future.
> >
> >  drivers/fpga/fpga-region.c       | 196 +++++++++++++
> >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> >  include/linux/fpga/fpga-region.h |  34 +++
> >  3 files changed, 493 insertions(+), 211 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > index 753cd142503e..d583fc22955b 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -5,6 +5,7 @@
> >   *  Copyright (C) 2013-2016 Altera Corporation
> >   *  Copyright (C) 2017 Intel Corporation
> >   */
> > +#include <linux/configfs.h>
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > @@ -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] = {
> > };  ATTRIBUTE_GROUPS(fpga_region);
> >
> > +static struct fpga_region *item_to_fpga_region(struct config_item
> > +*item) {
> > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > +                       struct fpga_region, subsys);
> > +}
> > +
> > +/**
> > + * fpga_region_image_store - Set firmware image name for FPGA region
> > + * This function sets the firmware image name for an FPGA region through
> configfs.
> > + * @item: Configfs item representing the FPGA region
> > + * @buf: Input buffer containing the firmware image name
> > + * @count: Size of the input buffer
> > + *
> > + * Return: Number of bytes written on success, or negative errno on failure.
> > + */
> > +static ssize_t fpga_region_image_store(struct config_item *item,
> > +const char *buf, size_t count) {
> > +   struct fpga_region *region = item_to_fpga_region(item);
> > +   struct device *dev = &region->dev;
> > +   struct fpga_image_info *info;
> > +   char firmware_name[NAME_MAX];
> > +   char *s;
> > +
> > +   if (region->info) {
> > +           dev_err(dev, "Region already has already configured.\n");
> > +           return -EINVAL;
> > +   }
> > +
> > +   info = fpga_image_info_alloc(dev);
> > +   if (!info)
> > +           return -ENOMEM;
> > +
> > +   /* copy to path buffer (and make sure it's always zero terminated */
> > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > +
> > +   /* strip trailing newlines */
> > +   s = firmware_name + strlen(firmware_name);
> > +   while (s > firmware_name && *--s == '\n')
> > +           *s = '\0';
> > +
> > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> GFP_KERNEL);
> > +   if (!region->firmware_name)
> > +           return -ENOMEM;
> > +
> > +   region->info = info;
> > +
> > +   return count;
> > +}
> > +
> > +/**
> > + * fpga_region_config_store - Trigger FPGA configuration via configfs
> > + * @item: Configfs item representing the FPGA region
> > + * @buf: Input buffer containing the configuration command (expects
> > +"1" to program, "0" to remove)
> > + * @count: Size of the input buffer
> > + *
> > + * If the input is "1", this function performs:
> > + *   1. region_pre_config() (if defined)
>
> Please define explicit workflow, and explicit expectation for each callback, or this
> framework makes no sense. From your of-fpga-region implementation, seems
> pre_config() means "parse image", post_config() means "populate devices".
>
> > + *   2. Bitstream programming via fpga_region_program_fpga() (unless external
> config flag is set)
> > + *   3. region_post_config() (if defined)
> > + *
> > + * If the input is "0", it triggers region_remove() (if defined).
> > + *
> > + * Return: Number of bytes processed on success, or negative errno on failure.
>
> Please put the uAPI description in Documentation/ABI/testing. Then we could know
> the file path layout from userspace POV.
>
> > + */
> > +static ssize_t fpga_region_config_store(struct config_item *item,
> > +                                   const char *buf, size_t count)
> > +{
> > +   struct fpga_region *region = item_to_fpga_region(item);
> > +   int config_value, ret = 0;
> > +
> > +   /* Parse input: must be "0" or "1" */
> > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0 && config_value !=
> 1))
> > +           return -EINVAL;
> > +
> > +   /* Ensure fpga_image_info is available */
> > +   if (!region->info)
> > +           return -EINVAL;
> > +
> > +   if (config_value == 1) {
> > +           /* Pre-config */
> > +           if (region->region_ops->region_pre_config) {
> > +                   ret = region->region_ops->region_pre_config(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +           /* Program bitstream if not external */
> > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > +                   ret = fpga_region_program_fpga(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +           /* Post-config */
> > +           if (region->region_ops->region_post_config) {
> > +                   ret = region->region_ops->region_post_config(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +   } else {
> > +           /* Remove configuration */
> > +           if (region->region_ops->region_remove) {
> > +                   ret = region->region_ops->region_remove(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +   }
> > +
> > +   return count;
> > +}
> > +
> > +/* Define Attributes */
> > +CONFIGFS_ATTR_WO(fpga_region_, image);
> CONFIGFS_ATTR_WO(fpga_region_,
> > +config);
> > +
> > +/* Attribute List */
> > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > +   &fpga_region_attr_image,
> > +   &fpga_region_attr_config,
> > +   NULL,
> > +};
> > +
> > +/* ConfigFS Item Type */
> > +static const struct config_item_type fpga_region_item_type = {
> > +   .ct_attrs = fpga_region_config_attrs,
> > +   .ct_owner = THIS_MODULE,
> > +};
>
> I think this is still the sysfs methodology. My understanding from configfs.rst is, use
> userspace interfaces to control the lifecycle of a kernel object.
>
> Now for existing kernel reprogramming flow, the image object for fpga_region is the
> struct fpga_image_info. We need to associate the struct with a config_item: alloc the
> struct fpga_image_info instance by mkdir, expose necessary fields
> (enable_timeout_us, disable_timeout_us, firmware_name, and the most important for
> of-fpga-region - overlay blob ...) for user to fill/query via configfs attributes. And finally
> use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
>

Thanks for the inputs. We've now implemented a ConfigFS-based reprogramming
interface for fpga_region that aligns with the intended lifecycle-driven model
described in configfs.rst.

New Interface Usage Instructions:
# 1. Mount configfs (if not already mounted)
mkdir -p /configfs
mount -t configfs configfs /configfs

# 2. Create a region directory (e.g., region0)
mkdir /configfs/fpga_regions/region0

# 3. Copy your bitstream or overlay files to firmware path
cp pl.dtbo /lib/firmware/

# 4. Set the firmware name (e.g., overlay .dtbo)
echo pl.dtbo > /configfs/fpga_regions/region0/image

# 5. Trigger Programming/reprogramming
echo 1 > /configfs/fpga_regions/region0/config

# 6. Unload/Reset the Programmable Logic
echo 0 > /configfs/fpga_regions/region0/config

Summary of Changes:
->Lifecycle Management:
    ->On mkdir /configfs/fpga_regions/regionX, we allocate and associate
       a struct fpga_image_info to the corresponding fpga_region.
    ->On rmdir, we clean up the associated image info via the new
       fpga_regions_drop_item() callback.
->Device Lookup:
    ->class_find_device_by_name() is now used in fpga_regions_make_item()
       to map a region name (e.g., region0) directly to a registered
       fpga_region device.

Please share your thoughts on this approach.

Regards,
Navakishore.

Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Xu Yilun]

On Thu, Jun 12, 2025 at 09:05:21AM +0000, Manne, Nava kishore wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Yilun,
> 
>         Thanks for quick response.
> Please find my response inline.
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Sunday, June 1, 2025 8:32 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> > robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>
> > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> > FPGA configuration
> >
> > On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > > Introduces an ConfigFS interface within the fpga-region subsystem,
> > > providing a generic and standardized mechanism for configuring (or)
> > > reprogramming FPGAs during runtime. The newly added interface supports
> > > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> > > callbacks (e.g., pre_config, post_config, removal, and status) to
> > > accommodate a wide range of device specific configurations.
> > >
> > > The ConfigFS interface ensures compatibility with both OF and non-OF
> > > devices, allowing for seamless FPGA reprogramming across diverse
> > > platforms.
> > >
> > > Vendor-specific callbacks are integrated into the interface, enabling
> > > custom FPGA pre_config, post_config, removal, and status reporting
> > > mechanisms, ensuring flexibility for vendor implementations.
> > >
> > > This solution enhances FPGA runtime management, supporting various
> > > device types and vendors, while ensuring compatibility with the
> > > current FPGA configuration flow.
> > >
> > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > > ---
> > > Changes for v3:
> > >  - As discussed with Yilun, the implementation continues to use a
> > > callback-based  approach to seamlessly support both OF (Open Firmware)
> > > and non-OF devices via  vendor-specific hooks. Additionally, the
> > > earlier IOCTL-based interface has been  replaced with a more suitable
> > > ConfigFS-based mechanism to enable runtime FPGA  configuration.
> > >
> > > Changes for v2:
> > >  - As discussed with Yilun, the implementation has been modified to
> > > utilize a  callback approach, enabling seamless handling of both OF and non-OF
> > devices.
> > >
> > >  - As suggested by Yilun in the POC code, we have moved away from
> > > using  void *args  as a parameter for ICOTL inputs to obtain the
> > > required user inputs. Instead, we are  utilizing the
> > > fpga_region_config_info structure to gather user inputs. Currently,
> > > this structure is implemented to support only OF devices, but we intend to extend
> > it by incorporating new members to accommodate non-OF devices in the future.
> > >
> > >  drivers/fpga/fpga-region.c       | 196 +++++++++++++
> > >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> > >  include/linux/fpga/fpga-region.h |  34 +++
> > >  3 files changed, 493 insertions(+), 211 deletions(-)
> > >
> > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > > index 753cd142503e..d583fc22955b 100644
> > > --- a/drivers/fpga/fpga-region.c
> > > +++ b/drivers/fpga/fpga-region.c
> > > @@ -5,6 +5,7 @@
> > >   *  Copyright (C) 2013-2016 Altera Corporation
> > >   *  Copyright (C) 2017 Intel Corporation
> > >   */
> > > +#include <linux/configfs.h>
> > >  #include <linux/fpga/fpga-bridge.h>
> > >  #include <linux/fpga/fpga-mgr.h>
> > >  #include <linux/fpga/fpga-region.h>
> > > @@ -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] = {
> > > };  ATTRIBUTE_GROUPS(fpga_region);
> > >
> > > +static struct fpga_region *item_to_fpga_region(struct config_item
> > > +*item) {
> > > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > > +                       struct fpga_region, subsys);
> > > +}
> > > +
> > > +/**
> > > + * fpga_region_image_store - Set firmware image name for FPGA region
> > > + * This function sets the firmware image name for an FPGA region through
> > configfs.
> > > + * @item: Configfs item representing the FPGA region
> > > + * @buf: Input buffer containing the firmware image name
> > > + * @count: Size of the input buffer
> > > + *
> > > + * Return: Number of bytes written on success, or negative errno on failure.
> > > + */
> > > +static ssize_t fpga_region_image_store(struct config_item *item,
> > > +const char *buf, size_t count) {
> > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > +   struct device *dev = &region->dev;
> > > +   struct fpga_image_info *info;
> > > +   char firmware_name[NAME_MAX];
> > > +   char *s;
> > > +
> > > +   if (region->info) {
> > > +           dev_err(dev, "Region already has already configured.\n");
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   info = fpga_image_info_alloc(dev);
> > > +   if (!info)
> > > +           return -ENOMEM;
> > > +
> > > +   /* copy to path buffer (and make sure it's always zero terminated */
> > > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > > +
> > > +   /* strip trailing newlines */
> > > +   s = firmware_name + strlen(firmware_name);
> > > +   while (s > firmware_name && *--s == '\n')
> > > +           *s = '\0';
> > > +
> > > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> > GFP_KERNEL);
> > > +   if (!region->firmware_name)
> > > +           return -ENOMEM;
> > > +
> > > +   region->info = info;
> > > +
> > > +   return count;
> > > +}
> > > +
> > > +/**
> > > + * fpga_region_config_store - Trigger FPGA configuration via configfs
> > > + * @item: Configfs item representing the FPGA region
> > > + * @buf: Input buffer containing the configuration command (expects
> > > +"1" to program, "0" to remove)
> > > + * @count: Size of the input buffer
> > > + *
> > > + * If the input is "1", this function performs:
> > > + *   1. region_pre_config() (if defined)
> >
> > Please define explicit workflow, and explicit expectation for each callback, or this
> > framework makes no sense. From your of-fpga-region implementation, seems
> > pre_config() means "parse image", post_config() means "populate devices".
> >
> > > + *   2. Bitstream programming via fpga_region_program_fpga() (unless external
> > config flag is set)
> > > + *   3. region_post_config() (if defined)
> > > + *
> > > + * If the input is "0", it triggers region_remove() (if defined).
> > > + *
> > > + * Return: Number of bytes processed on success, or negative errno on failure.
> >
> > Please put the uAPI description in Documentation/ABI/testing. Then we could know
> > the file path layout from userspace POV.
> >
> > > + */
> > > +static ssize_t fpga_region_config_store(struct config_item *item,
> > > +                                   const char *buf, size_t count)
> > > +{
> > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > +   int config_value, ret = 0;
> > > +
> > > +   /* Parse input: must be "0" or "1" */
> > > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0 && config_value !=
> > 1))
> > > +           return -EINVAL;
> > > +
> > > +   /* Ensure fpga_image_info is available */
> > > +   if (!region->info)
> > > +           return -EINVAL;
> > > +
> > > +   if (config_value == 1) {
> > > +           /* Pre-config */
> > > +           if (region->region_ops->region_pre_config) {
> > > +                   ret = region->region_ops->region_pre_config(region);
> > > +                   if (ret)
> > > +                           return ret;
> > > +           }
> > > +
> > > +           /* Program bitstream if not external */
> > > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > > +                   ret = fpga_region_program_fpga(region);
> > > +                   if (ret)
> > > +                           return ret;
> > > +           }
> > > +
> > > +           /* Post-config */
> > > +           if (region->region_ops->region_post_config) {
> > > +                   ret = region->region_ops->region_post_config(region);
> > > +                   if (ret)
> > > +                           return ret;
> > > +           }
> > > +
> > > +   } else {
> > > +           /* Remove configuration */
> > > +           if (region->region_ops->region_remove) {
> > > +                   ret = region->region_ops->region_remove(region);
> > > +                   if (ret)
> > > +                           return ret;
> > > +           }
> > > +   }
> > > +
> > > +   return count;
> > > +}
> > > +
> > > +/* Define Attributes */
> > > +CONFIGFS_ATTR_WO(fpga_region_, image);
> > CONFIGFS_ATTR_WO(fpga_region_,
> > > +config);
> > > +
> > > +/* Attribute List */
> > > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > > +   &fpga_region_attr_image,
> > > +   &fpga_region_attr_config,
> > > +   NULL,
> > > +};
> > > +
> > > +/* ConfigFS Item Type */
> > > +static const struct config_item_type fpga_region_item_type = {
> > > +   .ct_attrs = fpga_region_config_attrs,
> > > +   .ct_owner = THIS_MODULE,
> > > +};
> >
> > I think this is still the sysfs methodology. My understanding from configfs.rst is, use
> > userspace interfaces to control the lifecycle of a kernel object.
> >
> > Now for existing kernel reprogramming flow, the image object for fpga_region is the
> > struct fpga_image_info. We need to associate the struct with a config_item: alloc the
> > struct fpga_image_info instance by mkdir, expose necessary fields
> > (enable_timeout_us, disable_timeout_us, firmware_name, and the most important for
> > of-fpga-region - overlay blob ...) for user to fill/query via configfs attributes. And finally
> > use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
> >
> 
> Thanks for the inputs. We've now implemented a ConfigFS-based reprogramming
> interface for fpga_region that aligns with the intended lifecycle-driven model
> described in configfs.rst.
> 
> New Interface Usage Instructions:
> # 1. Mount configfs (if not already mounted)
> mkdir -p /configfs
> mount -t configfs configfs /configfs
> 
> # 2. Create a region directory (e.g., region0)
> mkdir /configfs/fpga_regions/region0

FPGA region is the device driver generated kernel object, so this per
fpga_region group should be created by kernel, not userspace.  User
should create fpga_image config_item under region group.

  mkdir /configfs/fpga_region/region0/my_image

> 
> # 3. Copy your bitstream or overlay files to firmware path
> cp pl.dtbo /lib/firmware/
> 
> # 4. Set the firmware name (e.g., overlay .dtbo)
> echo pl.dtbo > /configfs/fpga_regions/region0/image
> 
> # 5. Trigger Programming/reprogramming
> echo 1 > /configfs/fpga_regions/region0/config

My idea is, when an image item is first created, it is not activated,
user needs to read/write its attributes to initialize it, then we
could have an attribute (e.g. enable) to activate/reprograme the image.

For example:

  echo 1 > /configfs/fpga_regions/region0/my_image/enable

> 
> # 6. Unload/Reset the Programmable Logic
> echo 0 > /configfs/fpga_regions/region0/config

Maybe we could just delete the image item for unloading

  rmdir /configfs/fpga_region/region0/my_image

Thanks,
Yilun

> 
> Summary of Changes:
> ->Lifecycle Management:
>     ->On mkdir /configfs/fpga_regions/regionX, we allocate and associate
>        a struct fpga_image_info to the corresponding fpga_region.
>     ->On rmdir, we clean up the associated image info via the new
>        fpga_regions_drop_item() callback.
> ->Device Lookup:
>     ->class_find_device_by_name() is now used in fpga_regions_make_item()
>        to map a region name (e.g., region0) directly to a registered
>        fpga_region device.
> 
> Please share your thoughts on this approach.
> 
> Regards,
> Navakishore.

RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Manne, Nava kishore]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Friday, June 13, 2025 3:40 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> FPGA configuration
>
> On Thu, Jun 12, 2025 at 09:05:21AM +0000, Manne, Nava kishore wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Yilun,
> >
> >         Thanks for quick response.
> > Please find my response inline.
> >
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > Sent: Sunday, June 1, 2025 8:32 PM
> > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com;
> > > trix@redhat.com; robh@kernel.org; saravanak@google.com;
> > > linux-fpga@vger.kernel.org; linux- kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface
> > > for runtime FPGA configuration
> > >
> > > On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > > > Introduces an ConfigFS interface within the fpga-region subsystem,
> > > > providing a generic and standardized mechanism for configuring
> > > > (or) reprogramming FPGAs during runtime. The newly added interface
> > > > supports both OF (Open Firmware) and non-OF devices, leveraging
> > > > vendor-specific callbacks (e.g., pre_config, post_config, removal,
> > > > and status) to accommodate a wide range of device specific configurations.
> > > >
> > > > The ConfigFS interface ensures compatibility with both OF and
> > > > non-OF devices, allowing for seamless FPGA reprogramming across
> > > > diverse platforms.
> > > >
> > > > Vendor-specific callbacks are integrated into the interface,
> > > > enabling custom FPGA pre_config, post_config, removal, and status
> > > > reporting mechanisms, ensuring flexibility for vendor implementations.
> > > >
> > > > This solution enhances FPGA runtime management, supporting various
> > > > device types and vendors, while ensuring compatibility with the
> > > > current FPGA configuration flow.
> > > >
> > > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > > > ---
> > > > Changes for v3:
> > > >  - As discussed with Yilun, the implementation continues to use a
> > > > callback-based  approach to seamlessly support both OF (Open
> > > > Firmware) and non-OF devices via  vendor-specific hooks.
> > > > Additionally, the earlier IOCTL-based interface has been  replaced
> > > > with a more suitable ConfigFS-based mechanism to enable runtime FPGA
> configuration.
> > > >
> > > > Changes for v2:
> > > >  - As discussed with Yilun, the implementation has been modified
> > > > to utilize a  callback approach, enabling seamless handling of
> > > > both OF and non-OF
> > > devices.
> > > >
> > > >  - As suggested by Yilun in the POC code, we have moved away from
> > > > using  void *args  as a parameter for ICOTL inputs to obtain the
> > > > required user inputs. Instead, we are  utilizing the
> > > > fpga_region_config_info structure to gather user inputs.
> > > > Currently, this structure is implemented to support only OF
> > > > devices, but we intend to extend
> > > it by incorporating new members to accommodate non-OF devices in the future.
> > > >
> > > >  drivers/fpga/fpga-region.c       | 196 +++++++++++++
> > > >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> > > >  include/linux/fpga/fpga-region.h |  34 +++
> > > >  3 files changed, 493 insertions(+), 211 deletions(-)
> > > >
> > > > diff --git a/drivers/fpga/fpga-region.c
> > > > b/drivers/fpga/fpga-region.c index 753cd142503e..d583fc22955b
> > > > 100644
> > > > --- a/drivers/fpga/fpga-region.c
> > > > +++ b/drivers/fpga/fpga-region.c
> > > > @@ -5,6 +5,7 @@
> > > >   *  Copyright (C) 2013-2016 Altera Corporation
> > > >   *  Copyright (C) 2017 Intel Corporation
> > > >   */
> > > > +#include <linux/configfs.h>
> > > >  #include <linux/fpga/fpga-bridge.h>  #include
> > > > <linux/fpga/fpga-mgr.h>  #include <linux/fpga/fpga-region.h> @@
> > > > -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] =
> > > > { };  ATTRIBUTE_GROUPS(fpga_region);
> > > >
> > > > +static struct fpga_region *item_to_fpga_region(struct config_item
> > > > +*item) {
> > > > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > > > +                       struct fpga_region, subsys); }
> > > > +
> > > > +/**
> > > > + * fpga_region_image_store - Set firmware image name for FPGA
> > > > +region
> > > > + * This function sets the firmware image name for an FPGA region
> > > > +through
> > > configfs.
> > > > + * @item: Configfs item representing the FPGA region
> > > > + * @buf: Input buffer containing the firmware image name
> > > > + * @count: Size of the input buffer
> > > > + *
> > > > + * Return: Number of bytes written on success, or negative errno on failure.
> > > > + */
> > > > +static ssize_t fpga_region_image_store(struct config_item *item,
> > > > +const char *buf, size_t count) {
> > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > +   struct device *dev = &region->dev;
> > > > +   struct fpga_image_info *info;
> > > > +   char firmware_name[NAME_MAX];
> > > > +   char *s;
> > > > +
> > > > +   if (region->info) {
> > > > +           dev_err(dev, "Region already has already configured.\n");
> > > > +           return -EINVAL;
> > > > +   }
> > > > +
> > > > +   info = fpga_image_info_alloc(dev);
> > > > +   if (!info)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   /* copy to path buffer (and make sure it's always zero terminated */
> > > > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > > > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > > > +
> > > > +   /* strip trailing newlines */
> > > > +   s = firmware_name + strlen(firmware_name);
> > > > +   while (s > firmware_name && *--s == '\n')
> > > > +           *s = '\0';
> > > > +
> > > > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> > > GFP_KERNEL);
> > > > +   if (!region->firmware_name)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   region->info = info;
> > > > +
> > > > +   return count;
> > > > +}
> > > > +
> > > > +/**
> > > > + * fpga_region_config_store - Trigger FPGA configuration via
> > > > +configfs
> > > > + * @item: Configfs item representing the FPGA region
> > > > + * @buf: Input buffer containing the configuration command
> > > > +(expects "1" to program, "0" to remove)
> > > > + * @count: Size of the input buffer
> > > > + *
> > > > + * If the input is "1", this function performs:
> > > > + *   1. region_pre_config() (if defined)
> > >
> > > Please define explicit workflow, and explicit expectation for each
> > > callback, or this framework makes no sense. From your of-fpga-region
> > > implementation, seems
> > > pre_config() means "parse image", post_config() means "populate devices".
> > >
> > > > + *   2. Bitstream programming via fpga_region_program_fpga() (unless
> external
> > > config flag is set)
> > > > + *   3. region_post_config() (if defined)
> > > > + *
> > > > + * If the input is "0", it triggers region_remove() (if defined).
> > > > + *
> > > > + * Return: Number of bytes processed on success, or negative errno on
> failure.
> > >
> > > Please put the uAPI description in Documentation/ABI/testing. Then
> > > we could know the file path layout from userspace POV.
> > >
> > > > + */
> > > > +static ssize_t fpga_region_config_store(struct config_item *item,
> > > > +                                   const char *buf, size_t count)
> > > > +{
> > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > +   int config_value, ret = 0;
> > > > +
> > > > +   /* Parse input: must be "0" or "1" */
> > > > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0 &&
> > > > + config_value !=
> > > 1))
> > > > +           return -EINVAL;
> > > > +
> > > > +   /* Ensure fpga_image_info is available */
> > > > +   if (!region->info)
> > > > +           return -EINVAL;
> > > > +
> > > > +   if (config_value == 1) {
> > > > +           /* Pre-config */
> > > > +           if (region->region_ops->region_pre_config) {
> > > > +                   ret = region->region_ops->region_pre_config(region);
> > > > +                   if (ret)
> > > > +                           return ret;
> > > > +           }
> > > > +
> > > > +           /* Program bitstream if not external */
> > > > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > > > +                   ret = fpga_region_program_fpga(region);
> > > > +                   if (ret)
> > > > +                           return ret;
> > > > +           }
> > > > +
> > > > +           /* Post-config */
> > > > +           if (region->region_ops->region_post_config) {
> > > > +                   ret = region->region_ops->region_post_config(region);
> > > > +                   if (ret)
> > > > +                           return ret;
> > > > +           }
> > > > +
> > > > +   } else {
> > > > +           /* Remove configuration */
> > > > +           if (region->region_ops->region_remove) {
> > > > +                   ret = region->region_ops->region_remove(region);
> > > > +                   if (ret)
> > > > +                           return ret;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   return count;
> > > > +}
> > > > +
> > > > +/* Define Attributes */
> > > > +CONFIGFS_ATTR_WO(fpga_region_, image);
> > > CONFIGFS_ATTR_WO(fpga_region_,
> > > > +config);
> > > > +
> > > > +/* Attribute List */
> > > > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > > > +   &fpga_region_attr_image,
> > > > +   &fpga_region_attr_config,
> > > > +   NULL,
> > > > +};
> > > > +
> > > > +/* ConfigFS Item Type */
> > > > +static const struct config_item_type fpga_region_item_type = {
> > > > +   .ct_attrs = fpga_region_config_attrs,
> > > > +   .ct_owner = THIS_MODULE,
> > > > +};
> > >
> > > I think this is still the sysfs methodology. My understanding from
> > > configfs.rst is, use userspace interfaces to control the lifecycle of a kernel object.
> > >
> > > Now for existing kernel reprogramming flow, the image object for
> > > fpga_region is the struct fpga_image_info. We need to associate the
> > > struct with a config_item: alloc the struct fpga_image_info instance
> > > by mkdir, expose necessary fields (enable_timeout_us,
> > > disable_timeout_us, firmware_name, and the most important for
> > > of-fpga-region - overlay blob ...) for user to fill/query via configfs attributes. And
> finally use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
> > >
> >
> > Thanks for the inputs. We've now implemented a ConfigFS-based
> > reprogramming interface for fpga_region that aligns with the intended
> > lifecycle-driven model described in configfs.rst.
> >
> > New Interface Usage Instructions:
> > # 1. Mount configfs (if not already mounted) mkdir -p /configfs mount
> > -t configfs configfs /configfs
> >
> > # 2. Create a region directory (e.g., region0) mkdir
> > /configfs/fpga_regions/region0
>
> FPGA region is the device driver generated kernel object, so this per fpga_region
> group should be created by kernel, not userspace.  User should create fpga_image
> config_item under region group.
>
>   mkdir /configfs/fpga_region/region0/my_image
>
> >
> > # 3. Copy your bitstream or overlay files to firmware path cp pl.dtbo
> > /lib/firmware/
> >
> > # 4. Set the firmware name (e.g., overlay .dtbo) echo pl.dtbo >
> > /configfs/fpga_regions/region0/image
> >
> > # 5. Trigger Programming/reprogramming echo 1 >
> > /configfs/fpga_regions/region0/config
>
> My idea is, when an image item is first created, it is not activated, user needs to
> read/write its attributes to initialize it, then we could have an attribute (e.g. enable) to
> activate/reprograme the image.
>
> For example:
>
>   echo 1 > /configfs/fpga_regions/region0/my_image/enable
>
> >
> > # 6. Unload/Reset the Programmable Logic echo 0 >
> > /configfs/fpga_regions/region0/config
>
> Maybe we could just delete the image item for unloading
>
>   rmdir /configfs/fpga_region/region0/my_image
>
I’ve implemented the FPGA Region ConfigFS interface with the following hierarchy:

/configfs
└── fpga_regions             ← Registered via configfs_register_subsystem()
      └── region0                ← Added using configfs_add_default_group()
           └── my_image      ← Created via mkdir from userspace
                ├── firmware    ← Write firmware name here
                └── config        ← Trigger programming/unloading
Observation:
If configfs is not mounted before configfs_add_default_group() is invoked
(e.g., when regions are registered early via base DTB), the path
/configfs/fpga_regions/region0 does not appear in userspace,
even though it’s properly initialized in the kernel.

This appears to be due to how default groups function.
they require the configfs filesystem to be mounted prior to the group
addition in order to be visible. As a result, the mount order becomes
a strict dependency, which may affect or break early-boot FPGA flows
where regions are created before configfs is available.

Proposal:
Use configfs_register_subsystem(&region->cfg_subsys) for each FPGA region
instead of relying on configfs_add_default_group().

This approach places each FPGA region directly under /configfs/region0,
avoiding the timing issues associated with default groups.
The interface becomes available as soon as configfs is mounted.
regardless of when the region was registered
(boot time via base DTB or dynamically via overlays).

New user hierarchy:
/configfs
└── region0              ← Region appears as its own root node
      └── my_image   ← Created via mkdir from userspace
           ├── firmware ← Write firmware name here
           └── config     ← Trigger programming/unloading

Would like to know if this approach looks good, or if there are better
suggestions to handle this scenario?

Regards,
Navakishore.

RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Manne, Nava kishore]

Ping!

> -----Original Message-----
> From: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Sent: Friday, June 20, 2025 4:45 PM
> To: Xu Yilun <yilun.xu@linux.intel.com>
> Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> robh@kernel.org; saravanak@google.com; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> FPGA configuration
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Yilun,
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Friday, June 13, 2025 3:40 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com;
> > trix@redhat.com; robh@kernel.org; saravanak@google.com;
> > linux-fpga@vger.kernel.org; linux- kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface
> > for runtime FPGA configuration
> >
> > On Thu, Jun 12, 2025 at 09:05:21AM +0000, Manne, Nava kishore wrote:
> > > [AMD Official Use Only - AMD Internal Distribution Only]
> > >
> > > Hi Yilun,
> > >
> > >         Thanks for quick response.
> > > Please find my response inline.
> > >
> > > > -----Original Message-----
> > > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > > Sent: Sunday, June 1, 2025 8:32 PM
> > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > > Cc: mdf@kernel.org; hao.wu@intel.com; yilun.xu@intel.com;
> > > > trix@redhat.com; robh@kernel.org; saravanak@google.com;
> > > > linux-fpga@vger.kernel.org; linux- kernel@vger.kernel.org;
> > > > devicetree@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > > > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS
> > > > interface for runtime FPGA configuration
> > > >
> > > > On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > > > > Introduces an ConfigFS interface within the fpga-region
> > > > > subsystem, providing a generic and standardized mechanism for
> > > > > configuring
> > > > > (or) reprogramming FPGAs during runtime. The newly added
> > > > > interface supports both OF (Open Firmware) and non-OF devices,
> > > > > leveraging vendor-specific callbacks (e.g., pre_config,
> > > > > post_config, removal, and status) to accommodate a wide range of device
> specific configurations.
> > > > >
> > > > > The ConfigFS interface ensures compatibility with both OF and
> > > > > non-OF devices, allowing for seamless FPGA reprogramming across
> > > > > diverse platforms.
> > > > >
> > > > > Vendor-specific callbacks are integrated into the interface,
> > > > > enabling custom FPGA pre_config, post_config, removal, and
> > > > > status reporting mechanisms, ensuring flexibility for vendor implementations.
> > > > >
> > > > > This solution enhances FPGA runtime management, supporting
> > > > > various device types and vendors, while ensuring compatibility
> > > > > with the current FPGA configuration flow.
> > > > >
> > > > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > > > > ---
> > > > > Changes for v3:
> > > > >  - As discussed with Yilun, the implementation continues to use
> > > > > a callback-based  approach to seamlessly support both OF (Open
> > > > > Firmware) and non-OF devices via  vendor-specific hooks.
> > > > > Additionally, the earlier IOCTL-based interface has been
> > > > > replaced with a more suitable ConfigFS-based mechanism to enable
> > > > > runtime FPGA
> > configuration.
> > > > >
> > > > > Changes for v2:
> > > > >  - As discussed with Yilun, the implementation has been modified
> > > > > to utilize a  callback approach, enabling seamless handling of
> > > > > both OF and non-OF
> > > > devices.
> > > > >
> > > > >  - As suggested by Yilun in the POC code, we have moved away
> > > > > from using  void *args  as a parameter for ICOTL inputs to
> > > > > obtain the required user inputs. Instead, we are  utilizing the
> > > > > fpga_region_config_info structure to gather user inputs.
> > > > > Currently, this structure is implemented to support only OF
> > > > > devices, but we intend to extend
> > > > it by incorporating new members to accommodate non-OF devices in the
> future.
> > > > >
> > > > >  drivers/fpga/fpga-region.c       | 196 +++++++++++++
> > > > >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> > > > >  include/linux/fpga/fpga-region.h |  34 +++
> > > > >  3 files changed, 493 insertions(+), 211 deletions(-)
> > > > >
> > > > > diff --git a/drivers/fpga/fpga-region.c
> > > > > b/drivers/fpga/fpga-region.c index 753cd142503e..d583fc22955b
> > > > > 100644
> > > > > --- a/drivers/fpga/fpga-region.c
> > > > > +++ b/drivers/fpga/fpga-region.c
> > > > > @@ -5,6 +5,7 @@
> > > > >   *  Copyright (C) 2013-2016 Altera Corporation
> > > > >   *  Copyright (C) 2017 Intel Corporation
> > > > >   */
> > > > > +#include <linux/configfs.h>
> > > > >  #include <linux/fpga/fpga-bridge.h>  #include
> > > > > <linux/fpga/fpga-mgr.h>  #include <linux/fpga/fpga-region.h> @@
> > > > > -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[]
> > > > > = { };  ATTRIBUTE_GROUPS(fpga_region);
> > > > >
> > > > > +static struct fpga_region *item_to_fpga_region(struct
> > > > > +config_item
> > > > > +*item) {
> > > > > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > > > > +                       struct fpga_region, subsys); }
> > > > > +
> > > > > +/**
> > > > > + * fpga_region_image_store - Set firmware image name for FPGA
> > > > > +region
> > > > > + * This function sets the firmware image name for an FPGA
> > > > > +region through
> > > > configfs.
> > > > > + * @item: Configfs item representing the FPGA region
> > > > > + * @buf: Input buffer containing the firmware image name
> > > > > + * @count: Size of the input buffer
> > > > > + *
> > > > > + * Return: Number of bytes written on success, or negative errno on failure.
> > > > > + */
> > > > > +static ssize_t fpga_region_image_store(struct config_item
> > > > > +*item, const char *buf, size_t count) {
> > > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > > +   struct device *dev = &region->dev;
> > > > > +   struct fpga_image_info *info;
> > > > > +   char firmware_name[NAME_MAX];
> > > > > +   char *s;
> > > > > +
> > > > > +   if (region->info) {
> > > > > +           dev_err(dev, "Region already has already configured.\n");
> > > > > +           return -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   info = fpga_image_info_alloc(dev);
> > > > > +   if (!info)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   /* copy to path buffer (and make sure it's always zero terminated */
> > > > > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > > > > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > > > > +
> > > > > +   /* strip trailing newlines */
> > > > > +   s = firmware_name + strlen(firmware_name);
> > > > > +   while (s > firmware_name && *--s == '\n')
> > > > > +           *s = '\0';
> > > > > +
> > > > > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> > > > GFP_KERNEL);
> > > > > +   if (!region->firmware_name)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   region->info = info;
> > > > > +
> > > > > +   return count;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * fpga_region_config_store - Trigger FPGA configuration via
> > > > > +configfs
> > > > > + * @item: Configfs item representing the FPGA region
> > > > > + * @buf: Input buffer containing the configuration command
> > > > > +(expects "1" to program, "0" to remove)
> > > > > + * @count: Size of the input buffer
> > > > > + *
> > > > > + * If the input is "1", this function performs:
> > > > > + *   1. region_pre_config() (if defined)
> > > >
> > > > Please define explicit workflow, and explicit expectation for each
> > > > callback, or this framework makes no sense. From your
> > > > of-fpga-region implementation, seems
> > > > pre_config() means "parse image", post_config() means "populate devices".
> > > >
> > > > > + *   2. Bitstream programming via fpga_region_program_fpga() (unless
> > external
> > > > config flag is set)
> > > > > + *   3. region_post_config() (if defined)
> > > > > + *
> > > > > + * If the input is "0", it triggers region_remove() (if defined).
> > > > > + *
> > > > > + * Return: Number of bytes processed on success, or negative
> > > > > + errno on
> > failure.
> > > >
> > > > Please put the uAPI description in Documentation/ABI/testing. Then
> > > > we could know the file path layout from userspace POV.
> > > >
> > > > > + */
> > > > > +static ssize_t fpga_region_config_store(struct config_item *item,
> > > > > +                                   const char *buf, size_t
> > > > > +count) {
> > > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > > +   int config_value, ret = 0;
> > > > > +
> > > > > +   /* Parse input: must be "0" or "1" */
> > > > > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0
> > > > > + && config_value !=
> > > > 1))
> > > > > +           return -EINVAL;
> > > > > +
> > > > > +   /* Ensure fpga_image_info is available */
> > > > > +   if (!region->info)
> > > > > +           return -EINVAL;
> > > > > +
> > > > > +   if (config_value == 1) {
> > > > > +           /* Pre-config */
> > > > > +           if (region->region_ops->region_pre_config) {
> > > > > +                   ret = region->region_ops->region_pre_config(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +           /* Program bitstream if not external */
> > > > > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > > > > +                   ret = fpga_region_program_fpga(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +           /* Post-config */
> > > > > +           if (region->region_ops->region_post_config) {
> > > > > +                   ret = region->region_ops->region_post_config(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +   } else {
> > > > > +           /* Remove configuration */
> > > > > +           if (region->region_ops->region_remove) {
> > > > > +                   ret = region->region_ops->region_remove(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   return count;
> > > > > +}
> > > > > +
> > > > > +/* Define Attributes */
> > > > > +CONFIGFS_ATTR_WO(fpga_region_, image);
> > > > CONFIGFS_ATTR_WO(fpga_region_,
> > > > > +config);
> > > > > +
> > > > > +/* Attribute List */
> > > > > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > > > > +   &fpga_region_attr_image,
> > > > > +   &fpga_region_attr_config,
> > > > > +   NULL,
> > > > > +};
> > > > > +
> > > > > +/* ConfigFS Item Type */
> > > > > +static const struct config_item_type fpga_region_item_type = {
> > > > > +   .ct_attrs = fpga_region_config_attrs,
> > > > > +   .ct_owner = THIS_MODULE,
> > > > > +};
> > > >
> > > > I think this is still the sysfs methodology. My understanding from
> > > > configfs.rst is, use userspace interfaces to control the lifecycle of a kernel
> object.
> > > >
> > > > Now for existing kernel reprogramming flow, the image object for
> > > > fpga_region is the struct fpga_image_info. We need to associate
> > > > the struct with a config_item: alloc the struct fpga_image_info
> > > > instance by mkdir, expose necessary fields (enable_timeout_us,
> > > > disable_timeout_us, firmware_name, and the most important for
> > > > of-fpga-region - overlay blob ...) for user to fill/query via
> > > > configfs attributes. And
> > finally use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
> > > >
> > >
> > > Thanks for the inputs. We've now implemented a ConfigFS-based
> > > reprogramming interface for fpga_region that aligns with the
> > > intended lifecycle-driven model described in configfs.rst.
> > >
> > > New Interface Usage Instructions:
> > > # 1. Mount configfs (if not already mounted) mkdir -p /configfs
> > > mount -t configfs configfs /configfs
> > >
> > > # 2. Create a region directory (e.g., region0) mkdir
> > > /configfs/fpga_regions/region0
> >
> > FPGA region is the device driver generated kernel object, so this per
> > fpga_region group should be created by kernel, not userspace.  User
> > should create fpga_image config_item under region group.
> >
> >   mkdir /configfs/fpga_region/region0/my_image
> >
> > >
> > > # 3. Copy your bitstream or overlay files to firmware path cp
> > > pl.dtbo /lib/firmware/
> > >
> > > # 4. Set the firmware name (e.g., overlay .dtbo) echo pl.dtbo >
> > > /configfs/fpga_regions/region0/image
> > >
> > > # 5. Trigger Programming/reprogramming echo 1 >
> > > /configfs/fpga_regions/region0/config
> >
> > My idea is, when an image item is first created, it is not activated,
> > user needs to read/write its attributes to initialize it, then we
> > could have an attribute (e.g. enable) to activate/reprograme the image.
> >
> > For example:
> >
> >   echo 1 > /configfs/fpga_regions/region0/my_image/enable
> >
> > >
> > > # 6. Unload/Reset the Programmable Logic echo 0 >
> > > /configfs/fpga_regions/region0/config
> >
> > Maybe we could just delete the image item for unloading
> >
> >   rmdir /configfs/fpga_region/region0/my_image
> >
> I’ve implemented the FPGA Region ConfigFS interface with the following hierarchy:
> 
> /configfs
> └── fpga_regions             ← Registered via configfs_register_subsystem()
>       └── region0                ← Added using configfs_add_default_group()
>            └── my_image      ← Created via mkdir from userspace
>                 ├── firmware    ← Write firmware name here
>                 └── config        ← Trigger programming/unloading
> Observation:
> If configfs is not mounted before configfs_add_default_group() is invoked (e.g., when
> regions are registered early via base DTB), the path
> /configfs/fpga_regions/region0 does not appear in userspace, even though it’s
> properly initialized in the kernel.
> 
> This appears to be due to how default groups function.
> they require the configfs filesystem to be mounted prior to the group addition in order
> to be visible. As a result, the mount order becomes a strict dependency, which may
> affect or break early-boot FPGA flows where regions are created before configfs is
> available.
> 
> Proposal:
> Use configfs_register_subsystem(&region->cfg_subsys) for each FPGA region
> instead of relying on configfs_add_default_group().
> 
> This approach places each FPGA region directly under /configfs/region0, avoiding
> the timing issues associated with default groups.
> The interface becomes available as soon as configfs is mounted.
> regardless of when the region was registered (boot time via base DTB or
> dynamically via overlays).
> 
> New user hierarchy:
> /configfs
> └── region0              ← Region appears as its own root node
>       └── my_image   ← Created via mkdir from userspace
>            ├── firmware ← Write firmware name here
>            └── config     ← Trigger programming/unloading
> 
> Would like to know if this approach looks good, or if there are better suggestions to
> handle this scenario?
> 
> Regards,
> Navakishore.

Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime FPGA configuration

[Xu Yilun]

> I’ve implemented the FPGA Region ConfigFS interface with the following hierarchy:
> 
> /configfs
> └── fpga_regions             ← Registered via configfs_register_subsystem()
>       └── region0                ← Added using configfs_add_default_group()
>            └── my_image      ← Created via mkdir from userspace
>                 ├── firmware    ← Write firmware name here
>                 └── config        ← Trigger programming/unloading

Yes this is good to me.

> Observation:
> If configfs is not mounted before configfs_add_default_group() is invoked
> (e.g., when regions are registered early via base DTB), the path
> /configfs/fpga_regions/region0 does not appear in userspace,
> even though it’s properly initialized in the kernel.
> 
> This appears to be due to how default groups function.
> they require the configfs filesystem to be mounted prior to the group
> addition in order to be visible. As a result, the mount order becomes
> a strict dependency, which may affect or break early-boot FPGA flows
> where regions are created before configfs is available.

I don't have answer here. But IIUC you are describing some generic
problem of configfs_add_default_group(). According to configfs.rst,
subsystem is also a config_group so it doesn't make sense to me a
subsystem works but a default subgroup can't. Unless configfs people
have proper justification, your observation is a bug and should try to
fix it.

> 
> Proposal:
> Use configfs_register_subsystem(&region->cfg_subsys) for each FPGA region
> instead of relying on configfs_add_default_group().

This seems a workaround. I don't prefer we give up on it so early...

Thanks,
Yilun

> 
> This approach places each FPGA region directly under /configfs/region0,
> avoiding the timing issues associated with default groups.
> The interface becomes available as soon as configfs is mounted.
> regardless of when the region was registered
> (boot time via base DTB or dynamically via overlays).
> 
> New user hierarchy:
> /configfs
> └── region0              ← Region appears as its own root node
>       └── my_image   ← Created via mkdir from userspace
>            ├── firmware ← Write firmware name here
>            └── config     ← Trigger programming/unloading
> 
> Would like to know if this approach looks good, or if there are better
> suggestions to handle this scenario?
> 
> Regards,
> Navakishore.