[PATCH 1/2] fpga: doc: documentation for FPGA debugfs

[PATCH 1/2] fpga: doc: documentation for FPGA debugfs

[Alan Tull]

From: Alan Tull <atull@opensource.altera.com>

This patch depends on my recently submitted documentation changes
("docs: fpga: document programming fpgas using regions")

Document the DebugFS interface for the core FPGA Manager
framework.

Signed-off-by: Alan Tull <atull@kernel.org>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 Documentation/driver-api/fpga/fpga-mgr.rst | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/Documentation/driver-api/fpga/fpga-mgr.rst b/Documentation/driver-api/fpga/fpga-mgr.rst
index 576f194..d7ca320 100644
--- a/Documentation/driver-api/fpga/fpga-mgr.rst
+++ b/Documentation/driver-api/fpga/fpga-mgr.rst
@@ -125,3 +125,41 @@ API for implementing a new FPGA Manager driver
 
 .. kernel-doc:: drivers/fpga/fpga-mgr.c
    :functions: fpga_mgr_unregister
+
+FPGA Manager DebugFS
+--------------------
+
+This interface allows the user to program an FPGA from userspace.  However,
+bridges and soft IP device driver loading/unloading are not handled.  This makes
+it really easy to mess things up by doing things like reprogramming the hardware
+out from under a driver or reprogramming while a bridge is enabled, causing gunk
+to go out on a CPU bus.  It should go without saying that this interface is for
+debug and development only.  Not intended for production use.
+
+Each FPGA gets its own directory such as <debugfs>/fpga_manager/fpga0 and the
+files described below.  To program the FPGA, write the ``flags`` and/or
+``config_complete_timeout_us`` files (as needed), then use either the
+``firmware_name`` or ``image`` file to program.
+
+* ``flags`` - [RW] flags as defined in fpga-mgr.h.  For example::
+
+   $ echo 1 > /sys/kernel/debug/fpga_manager/fpga0/flags
+
+* ``config_complete_timeout_us`` - [RW] time out in microseconds to wait for the
+  FPGA to go to operating state after region has been programmed.  Not all
+  low level drivers use this.  For example::
+
+   $ echo 4 > /sys/kernel/debug/fpga_manager/fpga0/config_complete_timeout_us
+
+* ``firmware_name`` - [RW] Name of an FPGA image firmware file.  Writing
+  initiates an FPGA programming cycle.  Note that the image file must be in a
+  directory on the firmware search path such as /lib/firmware::
+
+   $ echo image.rbf > /sys/kernel/debug/fpga_manager/fpga0/firmware_name
+
+* ``image`` - [WO] Raw FPGA image data.  Writing the FPGA image data will
+  initiate an FPGA programming cycle.  Data must be written in one chunk, for
+  example::
+
+   $ dd bs=10M if=./image.rbf of=/sys/kernel/debug/fpga_manager/fpga0/image
+    (where image.rbf < 10M)
-- 
2.7.4

[PATCH 2/2] fpga: add FPGA manager debugfs

[Alan Tull]

From: Alan Tull <atull@opensource.altera.com>

Implement DebugFS for the FPGA Manager Framework for
debugging and developmental use.

Enabled by CONFIG_FPGA_MGR_DEBUG_FS

Each FPGA gets its own directory such as
 <debugfs>/fpga_manager/fpga0 and three files:

 * [RW] flags          = flags as defined in fpga-mgr.h
 * [RW] firmware_name  = write/read back name of FPGA image
                         firmware file to program
 * [WO] image          = write-only file for directly writing
                         fpga image w/o firmware layer
 * [RW] config_complete_timeout_us = maximum for the FPGA to
                         go to the operating state after
			 programming

The debugfs is implemented in a separate fpga_mgr_debugfs.c
file, but the FPGA manager core is still built as one
module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.

Signed-off-by: Alan Tull <atull@kernel.org>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/Kconfig            |   7 ++
 drivers/fpga/Makefile           |   4 +-
 drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
 drivers/fpga/fpga-mgr.c         |   8 ++
 include/linux/fpga/fpga-mgr.h   |   3 +
 6 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
 create mode 100644 drivers/fpga/fpga-mgr-debugfs.h

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 1ebcef4..600924d 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -9,6 +9,13 @@ menuconfig FPGA
 	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
 	  manager drivers.
 
+config FPGA_MGR_DEBUG_FS
+       bool "FPGA Manager DebugFS"
+       depends on FPGA && DEBUG_FS
+       help
+         Say Y here if you want to expose a DebugFS interface for the
+	 FPGA Manager Framework.
+
 if FPGA
 
 config FPGA_MGR_SOCFPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 7a2d73b..62910cc 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -4,7 +4,9 @@
 #
 
 # Core FPGA Manager Framework
-obj-$(CONFIG_FPGA)			+= fpga-mgr.o
+obj-$(CONFIG_FPGA)			+= fpga_mgr.o
+fpga_mgr-y				:= fpga-mgr.o
+fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS)	+= fpga-mgr-debugfs.o
 
 # FPGA Manager Drivers
 obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
diff --git a/drivers/fpga/fpga-mgr-debugfs.c b/drivers/fpga/fpga-mgr-debugfs.c
new file mode 100644
index 0000000..f2fdf58
--- /dev/null
+++ b/drivers/fpga/fpga-mgr-debugfs.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FPGA Manager DebugFS
+ *
+ *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
+ */
+#include <linux/debugfs.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+static struct dentry *fpga_mgr_debugfs_root;
+
+struct fpga_mgr_debugfs {
+	struct dentry *debugfs_dir;
+	struct fpga_image_info *info;
+};
+
+static ssize_t fpga_mgr_firmware_write_file(struct file *file,
+					    const char __user *user_buf,
+					    size_t count, loff_t *ppos)
+{
+	struct fpga_manager *mgr = file->private_data;
+	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
+	struct device *dev = &mgr->dev;
+	char *buf;
+	int ret;
+
+	ret = fpga_mgr_lock(mgr);
+	if (ret) {
+		dev_err(dev, "FPGA manager is busy\n");
+		return -EBUSY;
+	}
+
+	buf = devm_kzalloc(dev, count, GFP_KERNEL);
+	if (!buf) {
+		fpga_mgr_unlock(mgr);
+		return -ENOMEM;
+	}
+
+	if (copy_from_user(buf, user_buf, count)) {
+		fpga_mgr_unlock(mgr);
+		devm_kfree(dev, buf);
+		return -EFAULT;
+	}
+
+	buf[count] = 0;
+	if (buf[count - 1] == '\n')
+		buf[count - 1] = 0;
+
+	/* Release previous firmware name (if any). Save current one. */
+	if (debugfs->info->firmware_name)
+		devm_kfree(dev, debugfs->info->firmware_name);
+	debugfs->info->firmware_name = buf;
+
+	ret = fpga_mgr_load(mgr, debugfs->info);
+	if (ret)
+		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
+
+	fpga_mgr_unlock(mgr);
+
+	return count;
+}
+
+static ssize_t fpga_mgr_firmware_read_file(struct file *file,
+					   char __user *user_buf,
+					   size_t count, loff_t *ppos)
+{
+	struct fpga_manager *mgr = file->private_data;
+	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
+	char *buf;
+	int ret;
+
+	if (!debugfs->info->firmware_name)
+		return 0;
+
+	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info->firmware_name);
+	if (ret < 0) {
+		kfree(buf);
+		return ret;
+	}
+
+	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+	kfree(buf);
+
+	return ret;
+}
+
+static const struct file_operations fpga_mgr_firmware_fops = {
+	.open = simple_open,
+	.read = fpga_mgr_firmware_read_file,
+	.write = fpga_mgr_firmware_write_file,
+	.llseek = default_llseek,
+};
+
+static ssize_t fpga_mgr_image_write_file(struct file *file,
+					 const char __user *user_buf,
+					 size_t count, loff_t *ppos)
+{
+	struct fpga_manager *mgr = file->private_data;
+	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
+	struct device *dev = &mgr->dev;
+	char *buf;
+	int ret;
+
+	dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
+
+	ret = fpga_mgr_lock(mgr);
+	if (ret) {
+		dev_err(dev, "FPGA manager is busy\n");
+		return -EBUSY;
+	}
+
+	buf = kzalloc(count, GFP_KERNEL);
+	if (!buf) {
+		fpga_mgr_unlock(mgr);
+		return -ENOMEM;
+	}
+
+	if (copy_from_user(buf, user_buf, count)) {
+		fpga_mgr_unlock(mgr);
+		kfree(buf);
+		return -EFAULT;
+	}
+
+	/* If firmware interface was previously used, forget it. */
+	if (debugfs->info->firmware_name)
+		devm_kfree(dev, debugfs->info->firmware_name);
+	debugfs->info->firmware_name = NULL;
+
+	debugfs->info->buf = buf;
+	debugfs->info->count = count;
+
+	ret = fpga_mgr_load(mgr, debugfs->info);
+	if (ret)
+		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
+
+	fpga_mgr_unlock(mgr);
+
+	debugfs->info->buf = NULL;
+	debugfs->info->count = 0;
+
+	kfree(buf);
+
+	return count;
+}
+
+static const struct file_operations fpga_mgr_image_fops = {
+	.open = simple_open,
+	.write = fpga_mgr_image_write_file,
+	.llseek = default_llseek,
+};
+
+void fpga_mgr_debugfs_add(struct fpga_manager *mgr)
+{
+	struct device *dev = &mgr->dev;
+	struct fpga_mgr_debugfs *debugfs;
+	struct fpga_image_info *info;
+
+	if (!fpga_mgr_debugfs_root)
+		return;
+
+	debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
+	if (!debugfs)
+		return;
+
+	info = fpga_image_info_alloc(dev);
+	if (!info) {
+		kfree(debugfs);
+		return;
+	}
+	debugfs->info = info;
+
+	debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
+						  fpga_mgr_debugfs_root);
+
+	debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir, mgr,
+			    &fpga_mgr_firmware_fops);
+
+	debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
+			    &fpga_mgr_image_fops);
+
+	debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info->flags);
+
+	debugfs_create_u32("config_complete_timeout_us", 0600,
+			   debugfs->debugfs_dir,
+			   &info->config_complete_timeout_us);
+
+	mgr->debugfs = debugfs;
+}
+
+void fpga_mgr_debugfs_remove(struct fpga_manager *mgr)
+{
+	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
+
+	if (!fpga_mgr_debugfs_root)
+		return;
+
+	debugfs_remove_recursive(debugfs->debugfs_dir);
+
+	/* this function also frees debugfs->info->firmware_name */
+	fpga_image_info_free(debugfs->info);
+
+	kfree(debugfs);
+}
+
+void fpga_mgr_debugfs_init(void)
+{
+	fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
+	if (!fpga_mgr_debugfs_root)
+		pr_warn("fpga_mgr: Failed to create debugfs root\n");
+}
+
+void fpga_mgr_debugfs_uninit(void)
+{
+	debugfs_remove_recursive(fpga_mgr_debugfs_root);
+}
diff --git a/drivers/fpga/fpga-mgr-debugfs.h b/drivers/fpga/fpga-mgr-debugfs.h
new file mode 100644
index 0000000..17cd3eb
--- /dev/null
+++ b/drivers/fpga/fpga-mgr-debugfs.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
+#define _LINUX_FPGA_MGR_DEBUGFS_H
+
+#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
+
+void fpga_mgr_debugfs_add(struct fpga_manager *mgr);
+void fpga_mgr_debugfs_remove(struct fpga_manager *mgr);
+void fpga_mgr_debugfs_init(void);
+void fpga_mgr_debugfs_uninit(void);
+
+#else
+
+void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {}
+void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {}
+void fpga_mgr_debugfs_init(void) {}
+void fpga_mgr_debugfs_uninit(void) {}
+
+#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
+
+#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index c8684bc..66eb6f6 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/highmem.h>
+#include "fpga-mgr-debugfs.h"
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
@@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
 	if (ret)
 		goto error_device;
 
+	fpga_mgr_debugfs_add(mgr);
+
 	dev_info(&mgr->dev, "%s registered\n", mgr->name);
 
 	return 0;
@@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
 {
 	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
 
+	fpga_mgr_debugfs_remove(mgr);
+
 	/*
 	 * If the low level driver provides a method for putting fpga into
 	 * a desired state upon unregister, do it.
@@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
 	fpga_mgr_class->dev_groups = fpga_mgr_groups;
 	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
 
+	fpga_mgr_debugfs_init();
+
 	return 0;
 }
 
 static void __exit fpga_mgr_class_exit(void)
 {
+	fpga_mgr_debugfs_uninit();
 	class_destroy(fpga_mgr_class);
 	ida_destroy(&fpga_mgr_ida);
 }
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 777c502..e8f2159 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -170,6 +170,9 @@ struct fpga_manager {
 	struct fpga_compat_id *compat_id;
 	const struct fpga_manager_ops *mops;
 	void *priv;
+#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
+	void *debugfs;
+#endif
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.7.4

Re: [PATCH 1/2] fpga: doc: documentation for FPGA debugfs

[Alan Tull]

On Wed, Aug 15, 2018 at 5:09 PM, Alan Tull <atull@kernel.org> wrote:
> From: Alan Tull <atull@opensource.altera.com>

Ugh.  This patchset is a squash-and-cleanup of some really old commits
that had my no-longer valid opensource.altera.com email address as the
commit author.  I'll need to fix that in v2.

Alan

>
> This patch depends on my recently submitted documentation changes
> ("docs: fpga: document programming fpgas using regions")
>
> Document the DebugFS interface for the core FPGA Manager
> framework.
>
> Signed-off-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Randy Dunlap]

On 08/15/2018 03:09 PM, Alan Tull wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Implement DebugFS for the FPGA Manager Framework for
> debugging and developmental use.
> 
> Enabled by CONFIG_FPGA_MGR_DEBUG_FS

Hi Alan,

for your v2 changes:

> Each FPGA gets its own directory such as
>  <debugfs>/fpga_manager/fpga0 and three files:

s/three/four/

> 
>  * [RW] flags          = flags as defined in fpga-mgr.h
>  * [RW] firmware_name  = write/read back name of FPGA image
>                          firmware file to program
>  * [WO] image          = write-only file for directly writing
>                          fpga image w/o firmware layer
>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>                          go to the operating state after
> 			 programming
> 
> The debugfs is implemented in a separate fpga_mgr_debugfs.c

      debugfs interface is

> file, but the FPGA manager core is still built as one
> module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/Kconfig            |   7 ++
>  drivers/fpga/Makefile           |   4 +-
>  drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>  drivers/fpga/fpga-mgr.c         |   8 ++
>  include/linux/fpga/fpga-mgr.h   |   3 +
>  6 files changed, 264 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.h
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 1ebcef4..600924d 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -9,6 +9,13 @@ menuconfig FPGA
>  	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
>  	  manager drivers.
>  
> +config FPGA_MGR_DEBUG_FS
> +       bool "FPGA Manager DebugFS"
> +       depends on FPGA && DEBUG_FS
> +       help
> +         Say Y here if you want to expose a DebugFS interface for the
> +	 FPGA Manager Framework.

Use tab indentation for bool, depends, and help lines.

Use tab +2 spaces for help text.

> +
>  if FPGA
>  
>  config FPGA_MGR_SOCFPGA

thanks,
-- 
~Randy

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Alan Tull]

On Wed, Aug 15, 2018 at 5:34 PM, Randy Dunlap <rdunlap@infradead.org> wrote:

Hi Randy,

> On 08/15/2018 03:09 PM, Alan Tull wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Implement DebugFS for the FPGA Manager Framework for
>> debugging and developmental use.
>>
>> Enabled by CONFIG_FPGA_MGR_DEBUG_FS
>
> Hi Alan,
>
> for your v2 changes:
>
>> Each FPGA gets its own directory such as
>>  <debugfs>/fpga_manager/fpga0 and three files:
>
> s/three/four/

Yes

>
>>
>>  * [RW] flags          = flags as defined in fpga-mgr.h
>>  * [RW] firmware_name  = write/read back name of FPGA image
>>                          firmware file to program
>>  * [WO] image          = write-only file for directly writing
>>                          fpga image w/o firmware layer
>>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>>                          go to the operating state after
>>                        programming
>>
>> The debugfs is implemented in a separate fpga_mgr_debugfs.c
>
>       debugfs interface is

Yes

>
>> file, but the FPGA manager core is still built as one
>> module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/Kconfig            |   7 ++
>>  drivers/fpga/Makefile           |   4 +-
>>  drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>>  drivers/fpga/fpga-mgr.c         |   8 ++
>>  include/linux/fpga/fpga-mgr.h   |   3 +
>>  6 files changed, 264 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.h
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 1ebcef4..600924d 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -9,6 +9,13 @@ menuconfig FPGA
>>         kernel.  The FPGA framework adds a FPGA manager class and FPGA
>>         manager drivers.
>>
>> +config FPGA_MGR_DEBUG_FS
>> +       bool "FPGA Manager DebugFS"
>> +       depends on FPGA && DEBUG_FS
>> +       help
>> +         Say Y here if you want to expose a DebugFS interface for the
>> +      FPGA Manager Framework.
>
> Use tab indentation for bool, depends, and help lines.
>
> Use tab +2 spaces for help text.

Yes, thanks for catching that.  I'll fix these in v2.

Thanks for the review!

Alan

>
>> +
>>  if FPGA
>>
>>  config FPGA_MGR_SOCFPGA
>
> thanks,
> --
> ~Randy

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Moritz Fischer]

Hi Alan,

comments inline. While I see how this is useful, I have the
suspicion that from the moment this gets merged vendor kernels
will just default to use this ...

On Wed, Aug 15, 2018 at 05:09:58PM -0500, Alan Tull wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Implement DebugFS for the FPGA Manager Framework for
> debugging and developmental use.
> 
> Enabled by CONFIG_FPGA_MGR_DEBUG_FS
> 
> Each FPGA gets its own directory such as
>  <debugfs>/fpga_manager/fpga0 and three files:
> 
>  * [RW] flags          = flags as defined in fpga-mgr.h
>  * [RW] firmware_name  = write/read back name of FPGA image
>                          firmware file to program
>  * [WO] image          = write-only file for directly writing
>                          fpga image w/o firmware layer
>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>                          go to the operating state after
> 			 programming
> 
> The debugfs is implemented in a separate fpga_mgr_debugfs.c
> file, but the FPGA manager core is still built as one
> module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/Kconfig            |   7 ++
>  drivers/fpga/Makefile           |   4 +-
>  drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>  drivers/fpga/fpga-mgr.c         |   8 ++
>  include/linux/fpga/fpga-mgr.h   |   3 +
>  6 files changed, 264 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.h
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 1ebcef4..600924d 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -9,6 +9,13 @@ menuconfig FPGA
>  	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
>  	  manager drivers.
>  
> +config FPGA_MGR_DEBUG_FS
> +       bool "FPGA Manager DebugFS"
> +       depends on FPGA && DEBUG_FS
> +       help
> +         Say Y here if you want to expose a DebugFS interface for the
> +	 FPGA Manager Framework.
> +
>  if FPGA
>  
>  config FPGA_MGR_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 7a2d73b..62910cc 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -4,7 +4,9 @@
>  #
>  
>  # Core FPGA Manager Framework
> -obj-$(CONFIG_FPGA)			+= fpga-mgr.o
> +obj-$(CONFIG_FPGA)			+= fpga_mgr.o
> +fpga_mgr-y				:= fpga-mgr.o
> +fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS)	+= fpga-mgr-debugfs.o
>  
>  # FPGA Manager Drivers
>  obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
> diff --git a/drivers/fpga/fpga-mgr-debugfs.c b/drivers/fpga/fpga-mgr-debugfs.c
> new file mode 100644
> index 0000000..f2fdf58
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr-debugfs.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Manager DebugFS
> + *
> + *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
> + */
> +#include <linux/debugfs.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +static struct dentry *fpga_mgr_debugfs_root;
> +
> +struct fpga_mgr_debugfs {
> +	struct dentry *debugfs_dir;
> +	struct fpga_image_info *info;
> +};
> +
> +static ssize_t fpga_mgr_firmware_write_file(struct file *file,
> +					    const char __user *user_buf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	struct device *dev = &mgr->dev;
> +	char *buf;
> +	int ret;
> +
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret) {
> +		dev_err(dev, "FPGA manager is busy\n");
> +		return -EBUSY;
> +	}
> +
> +	buf = devm_kzalloc(dev, count, GFP_KERNEL);
> +	if (!buf) {
> +		fpga_mgr_unlock(mgr);
> +		return -ENOMEM;
> +	}
> +
> +	if (copy_from_user(buf, user_buf, count)) {
> +		fpga_mgr_unlock(mgr);
> +		devm_kfree(dev, buf);
> +		return -EFAULT;
> +	}
> +
> +	buf[count] = 0;
> +	if (buf[count - 1] == '\n')
> +		buf[count - 1] = 0;
> +
> +	/* Release previous firmware name (if any). Save current one. */
> +	if (debugfs->info->firmware_name)
> +		devm_kfree(dev, debugfs->info->firmware_name);
> +	debugfs->info->firmware_name = buf;
> +
> +	ret = fpga_mgr_load(mgr, debugfs->info);
> +	if (ret)
> +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> +
> +	fpga_mgr_unlock(mgr);
> +
> +	return count;
> +}
> +
> +static ssize_t fpga_mgr_firmware_read_file(struct file *file,
> +					   char __user *user_buf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	char *buf;
> +	int ret;
> +
> +	if (!debugfs->info->firmware_name)
> +		return 0;
> +
> +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info->firmware_name);
snip:
---->8->8->8-----
> +	if (ret < 0) {
> +		kfree(buf);
> +		return ret;
> +	}
---->8->8->8-----
just replace with:
	if (ret < 0)
		goto out;
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);

out:
> +	kfree(buf);
> +	return ret;
> +}
> +
> +static const struct file_operations fpga_mgr_firmware_fops = {
> +	.open = simple_open,
> +	.read = fpga_mgr_firmware_read_file,
> +	.write = fpga_mgr_firmware_write_file,
> +	.llseek = default_llseek,
> +};
> +
> +static ssize_t fpga_mgr_image_write_file(struct file *file,
> +					 const char __user *user_buf,
> +					 size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	struct device *dev = &mgr->dev;
> +	char *buf;
> +	int ret;
> +
> +	dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
> +
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret) {
> +		dev_err(dev, "FPGA manager is busy\n");
> +		return -EBUSY;
> +	}
> +
> +	buf = kzalloc(count, GFP_KERNEL);
> +	if (!buf) {
> +		fpga_mgr_unlock(mgr);
> +		return -ENOMEM;
> +	}
> +
> +	if (copy_from_user(buf, user_buf, count)) {
> +		fpga_mgr_unlock(mgr);
> +		kfree(buf);
> +		return -EFAULT;
> +	}
> +
> +	/* If firmware interface was previously used, forget it. */
> +	if (debugfs->info->firmware_name)
> +		devm_kfree(dev, debugfs->info->firmware_name);
> +	debugfs->info->firmware_name = NULL;
> +
> +	debugfs->info->buf = buf;
> +	debugfs->info->count = count;
> +
> +	ret = fpga_mgr_load(mgr, debugfs->info);
> +	if (ret)
> +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> +
> +	fpga_mgr_unlock(mgr);
> +
> +	debugfs->info->buf = NULL;
> +	debugfs->info->count = 0;

Is that ordering between unlock() and setting those correct?
> +
> +	kfree(buf);
> +
> +	return count;
> +}
> +
> +static const struct file_operations fpga_mgr_image_fops = {
> +	.open = simple_open,
> +	.write = fpga_mgr_image_write_file,
> +	.llseek = default_llseek,
> +};
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr)
> +{
> +	struct device *dev = &mgr->dev;
> +	struct fpga_mgr_debugfs *debugfs;
> +	struct fpga_image_info *info;
> +
> +	if (!fpga_mgr_debugfs_root)
> +		return;
> +
> +	debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
> +	if (!debugfs)
> +		return;
> +
> +	info = fpga_image_info_alloc(dev);
> +	if (!info) {
> +		kfree(debugfs);
> +		return;
> +	}
> +	debugfs->info = info;
> +
> +	debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
> +						  fpga_mgr_debugfs_root);
> +
> +	debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir, mgr,
> +			    &fpga_mgr_firmware_fops);
> +
> +	debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
> +			    &fpga_mgr_image_fops);
> +
> +	debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info->flags);
> +
> +	debugfs_create_u32("config_complete_timeout_us", 0600,
> +			   debugfs->debugfs_dir,
> +			   &info->config_complete_timeout_us);
> +
> +	mgr->debugfs = debugfs;
> +}
> +
> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr)
> +{
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +
> +	if (!fpga_mgr_debugfs_root)
> +		return;
> +
> +	debugfs_remove_recursive(debugfs->debugfs_dir);
> +
> +	/* this function also frees debugfs->info->firmware_name */
> +	fpga_image_info_free(debugfs->info);
> +
> +	kfree(debugfs);
> +}
> +
> +void fpga_mgr_debugfs_init(void)
> +{
> +	fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
> +	if (!fpga_mgr_debugfs_root)
> +		pr_warn("fpga_mgr: Failed to create debugfs root\n");
> +}
> +
> +void fpga_mgr_debugfs_uninit(void)
> +{
> +	debugfs_remove_recursive(fpga_mgr_debugfs_root);
> +}
> diff --git a/drivers/fpga/fpga-mgr-debugfs.h b/drivers/fpga/fpga-mgr-debugfs.h
> new file mode 100644
> index 0000000..17cd3eb
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr-debugfs.h
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
> +#define _LINUX_FPGA_MGR_DEBUGFS_H
> +
> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr);
> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr);
> +void fpga_mgr_debugfs_init(void);
> +void fpga_mgr_debugfs_uninit(void);
> +
> +#else
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {}
> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {}
> +void fpga_mgr_debugfs_init(void) {}
> +void fpga_mgr_debugfs_uninit(void) {}
> +
> +#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
> +
> +#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index c8684bc..66eb6f6 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include "fpga-mgr-debugfs.h"
>  
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
> @@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
>  	if (ret)
>  		goto error_device;
>  
> +	fpga_mgr_debugfs_add(mgr);
> +
>  	dev_info(&mgr->dev, "%s registered\n", mgr->name);
>  
>  	return 0;
> @@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>  {
>  	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>  
> +	fpga_mgr_debugfs_remove(mgr);
> +
>  	/*
>  	 * If the low level driver provides a method for putting fpga into
>  	 * a desired state upon unregister, do it.
> @@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
>  	fpga_mgr_class->dev_groups = fpga_mgr_groups;
>  	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
>  
> +	fpga_mgr_debugfs_init();
> +
>  	return 0;
>  }
>  
>  static void __exit fpga_mgr_class_exit(void)
>  {
> +	fpga_mgr_debugfs_uninit();
>  	class_destroy(fpga_mgr_class);
>  	ida_destroy(&fpga_mgr_ida);
>  }
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 777c502..e8f2159 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -170,6 +170,9 @@ struct fpga_manager {
>  	struct fpga_compat_id *compat_id;
>  	const struct fpga_manager_ops *mops;
>  	void *priv;
> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> +	void *debugfs;
> +#endif
>  };
>  
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> -- 
> 2.7.4
> 

Thanks,

Moritz

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Alan Tull]

On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> Hi Alan,

Hi Moritz,

> comments inline. While I see how this is useful, I have the
> suspicion that from the moment this gets merged vendor kernels
> will just default to use this ...

Yeah, I have that suspicion as well.  That's probably why I sat on
this and didn't upstream it for 2 years.  But on the other hand, I
keep hearing of lots of cases of people implementing this
independently anyway.  At least if it is debugfs, it makes it clear
that it's not intended for production use.

>
> On Wed, Aug 15, 2018 at 05:09:58PM -0500, Alan Tull wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Implement DebugFS for the FPGA Manager Framework for
>> debugging and developmental use.
>>
>> Enabled by CONFIG_FPGA_MGR_DEBUG_FS
>>
>> Each FPGA gets its own directory such as
>>  <debugfs>/fpga_manager/fpga0 and three files:
>>
>>  * [RW] flags          = flags as defined in fpga-mgr.h
>>  * [RW] firmware_name  = write/read back name of FPGA image
>>                          firmware file to program
>>  * [WO] image          = write-only file for directly writing
>>                          fpga image w/o firmware layer
>>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>>                          go to the operating state after
>>                        programming
>>
>> The debugfs is implemented in a separate fpga_mgr_debugfs.c
>> file, but the FPGA manager core is still built as one
>> module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.
>>
>> Signed-off-by: Alan Tull <atull@kernel.org>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/Kconfig            |   7 ++
>>  drivers/fpga/Makefile           |   4 +-
>>  drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>>  drivers/fpga/fpga-mgr.c         |   8 ++
>>  include/linux/fpga/fpga-mgr.h   |   3 +
>>  6 files changed, 264 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.h
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 1ebcef4..600924d 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -9,6 +9,13 @@ menuconfig FPGA
>>         kernel.  The FPGA framework adds a FPGA manager class and FPGA
>>         manager drivers.
>>
>> +config FPGA_MGR_DEBUG_FS
>> +       bool "FPGA Manager DebugFS"
>> +       depends on FPGA && DEBUG_FS
>> +       help
>> +         Say Y here if you want to expose a DebugFS interface for the
>> +      FPGA Manager Framework.
>> +
>>  if FPGA
>>
>>  config FPGA_MGR_SOCFPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 7a2d73b..62910cc 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -4,7 +4,9 @@
>>  #
>>
>>  # Core FPGA Manager Framework
>> -obj-$(CONFIG_FPGA)                   += fpga-mgr.o
>> +obj-$(CONFIG_FPGA)                   += fpga_mgr.o
>> +fpga_mgr-y                           := fpga-mgr.o
>> +fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS) += fpga-mgr-debugfs.o
>>
>>  # FPGA Manager Drivers
>>  obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)    += altera-cvp.o
>> diff --git a/drivers/fpga/fpga-mgr-debugfs.c b/drivers/fpga/fpga-mgr-debugfs.c
>> new file mode 100644
>> index 0000000..f2fdf58
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-mgr-debugfs.c
>> @@ -0,0 +1,221 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * FPGA Manager DebugFS
>> + *
>> + *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
>> + */
>> +#include <linux/debugfs.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +static struct dentry *fpga_mgr_debugfs_root;
>> +
>> +struct fpga_mgr_debugfs {
>> +     struct dentry *debugfs_dir;
>> +     struct fpga_image_info *info;
>> +};
>> +
>> +static ssize_t fpga_mgr_firmware_write_file(struct file *file,
>> +                                         const char __user *user_buf,
>> +                                         size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     struct device *dev = &mgr->dev;
>> +     char *buf;
>> +     int ret;
>> +
>> +     ret = fpga_mgr_lock(mgr);
>> +     if (ret) {
>> +             dev_err(dev, "FPGA manager is busy\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     buf = devm_kzalloc(dev, count, GFP_KERNEL);
>> +     if (!buf) {
>> +             fpga_mgr_unlock(mgr);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     if (copy_from_user(buf, user_buf, count)) {
>> +             fpga_mgr_unlock(mgr);
>> +             devm_kfree(dev, buf);
>> +             return -EFAULT;
>> +     }
>> +
>> +     buf[count] = 0;
>> +     if (buf[count - 1] == '\n')
>> +             buf[count - 1] = 0;
>> +
>> +     /* Release previous firmware name (if any). Save current one. */
>> +     if (debugfs->info->firmware_name)
>> +             devm_kfree(dev, debugfs->info->firmware_name);
>> +     debugfs->info->firmware_name = buf;
>> +
>> +     ret = fpga_mgr_load(mgr, debugfs->info);
>> +     if (ret)
>> +             dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
>> +
>> +     fpga_mgr_unlock(mgr);
>> +
>> +     return count;
>> +}
>> +
>> +static ssize_t fpga_mgr_firmware_read_file(struct file *file,
>> +                                        char __user *user_buf,
>> +                                        size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     char *buf;
>> +     int ret;
>> +
>> +     if (!debugfs->info->firmware_name)
>> +             return 0;
>> +
>> +     buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>> +
>> +     ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info->firmware_name);
> snip:
> ---->8->8->8-----
>> +     if (ret < 0) {
>> +             kfree(buf);
>> +             return ret;
>> +     }
> ---->8->8->8-----
> just replace with:
>         if (ret < 0)
>                 goto out;
>> +
>> +     ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>
> out:
>> +     kfree(buf);
>> +     return ret;
>> +}

Yes, that's better. I'll include that in v2.

>> +
>> +static const struct file_operations fpga_mgr_firmware_fops = {
>> +     .open = simple_open,
>> +     .read = fpga_mgr_firmware_read_file,
>> +     .write = fpga_mgr_firmware_write_file,
>> +     .llseek = default_llseek,
>> +};
>> +
>> +static ssize_t fpga_mgr_image_write_file(struct file *file,
>> +                                      const char __user *user_buf,
>> +                                      size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     struct device *dev = &mgr->dev;
>> +     char *buf;
>> +     int ret;
>> +
>> +     dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
>> +
>> +     ret = fpga_mgr_lock(mgr);
>> +     if (ret) {
>> +             dev_err(dev, "FPGA manager is busy\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     buf = kzalloc(count, GFP_KERNEL);
>> +     if (!buf) {
>> +             fpga_mgr_unlock(mgr);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     if (copy_from_user(buf, user_buf, count)) {
>> +             fpga_mgr_unlock(mgr);
>> +             kfree(buf);
>> +             return -EFAULT;
>> +     }
>> +
>> +     /* If firmware interface was previously used, forget it. */
>> +     if (debugfs->info->firmware_name)
>> +             devm_kfree(dev, debugfs->info->firmware_name);
>> +     debugfs->info->firmware_name = NULL;
>> +
>> +     debugfs->info->buf = buf;
>> +     debugfs->info->count = count;
>> +
>> +     ret = fpga_mgr_load(mgr, debugfs->info);
>> +     if (ret)
>> +             dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
>> +
>> +     fpga_mgr_unlock(mgr);
>> +
>> +     debugfs->info->buf = NULL;
>> +     debugfs->info->count = 0;
>
> Is that ordering between unlock() and setting those correct?

This order should be alright, but I'll switch it anyway to be more
obviously correct.

>> +
>> +     kfree(buf);
>> +
>> +     return count;
>> +}
>> +
>> +static const struct file_operations fpga_mgr_image_fops = {
>> +     .open = simple_open,
>> +     .write = fpga_mgr_image_write_file,
>> +     .llseek = default_llseek,
>> +};
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr)
>> +{
>> +     struct device *dev = &mgr->dev;
>> +     struct fpga_mgr_debugfs *debugfs;
>> +     struct fpga_image_info *info;
>> +
>> +     if (!fpga_mgr_debugfs_root)
>> +             return;
>> +
>> +     debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
>> +     if (!debugfs)
>> +             return;
>> +
>> +     info = fpga_image_info_alloc(dev);
>> +     if (!info) {
>> +             kfree(debugfs);
>> +             return;
>> +     }
>> +     debugfs->info = info;
>> +
>> +     debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
>> +                                               fpga_mgr_debugfs_root);
>> +
>> +     debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir, mgr,
>> +                         &fpga_mgr_firmware_fops);
>> +
>> +     debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
>> +                         &fpga_mgr_image_fops);
>> +
>> +     debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info->flags);
>> +
>> +     debugfs_create_u32("config_complete_timeout_us", 0600,
>> +                        debugfs->debugfs_dir,
>> +                        &info->config_complete_timeout_us);
>> +
>> +     mgr->debugfs = debugfs;
>> +}
>> +
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr)
>> +{
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +
>> +     if (!fpga_mgr_debugfs_root)
>> +             return;
>> +
>> +     debugfs_remove_recursive(debugfs->debugfs_dir);
>> +
>> +     /* this function also frees debugfs->info->firmware_name */
>> +     fpga_image_info_free(debugfs->info);
>> +
>> +     kfree(debugfs);
>> +}
>> +
>> +void fpga_mgr_debugfs_init(void)
>> +{
>> +     fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
>> +     if (!fpga_mgr_debugfs_root)
>> +             pr_warn("fpga_mgr: Failed to create debugfs root\n");
>> +}
>> +
>> +void fpga_mgr_debugfs_uninit(void)
>> +{
>> +     debugfs_remove_recursive(fpga_mgr_debugfs_root);
>> +}
>> diff --git a/drivers/fpga/fpga-mgr-debugfs.h b/drivers/fpga/fpga-mgr-debugfs.h
>> new file mode 100644
>> index 0000000..17cd3eb
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-mgr-debugfs.h
>> @@ -0,0 +1,22 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
>> +#define _LINUX_FPGA_MGR_DEBUGFS_H
>> +
>> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr);
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr);
>> +void fpga_mgr_debugfs_init(void);
>> +void fpga_mgr_debugfs_uninit(void);
>> +
>> +#else
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {}
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {}
>> +void fpga_mgr_debugfs_init(void) {}
>> +void fpga_mgr_debugfs_uninit(void) {}
>> +
>> +#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
>> +
>> +#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index c8684bc..66eb6f6 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/scatterlist.h>
>>  #include <linux/highmem.h>
>> +#include "fpga-mgr-debugfs.h"
>>
>>  static DEFINE_IDA(fpga_mgr_ida);
>>  static struct class *fpga_mgr_class;
>> @@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
>>       if (ret)
>>               goto error_device;
>>
>> +     fpga_mgr_debugfs_add(mgr);
>> +
>>       dev_info(&mgr->dev, "%s registered\n", mgr->name);
>>
>>       return 0;
>> @@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>>  {
>>       dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>
>> +     fpga_mgr_debugfs_remove(mgr);
>> +
>>       /*
>>        * If the low level driver provides a method for putting fpga into
>>        * a desired state upon unregister, do it.
>> @@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
>>       fpga_mgr_class->dev_groups = fpga_mgr_groups;
>>       fpga_mgr_class->dev_release = fpga_mgr_dev_release;
>>
>> +     fpga_mgr_debugfs_init();
>> +
>>       return 0;
>>  }
>>
>>  static void __exit fpga_mgr_class_exit(void)
>>  {
>> +     fpga_mgr_debugfs_uninit();
>>       class_destroy(fpga_mgr_class);
>>       ida_destroy(&fpga_mgr_ida);
>>  }
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 777c502..e8f2159 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -170,6 +170,9 @@ struct fpga_manager {
>>       struct fpga_compat_id *compat_id;
>>       const struct fpga_manager_ops *mops;
>>       void *priv;
>> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
>> +     void *debugfs;
>> +#endif
>>  };
>>
>>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
>> --
>> 2.7.4
>>
>
> Thanks,
>
> Moritz

Thanks for the review!

Alan

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Federico Vaga]

Hi,

On Thursday, August 16, 2018 10:04:51 PM CEST Alan Tull wrote:
> On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> > Hi Alan,
> 
> Hi Moritz,
> 
> > comments inline. While I see how this is useful, I have the
> > suspicion that from the moment this gets merged vendor kernels
> > will just default to use this ...
> 
> Yeah, I have that suspicion as well.  That's probably why I sat on
> this and didn't upstream it for 2 years.  But on the other hand, I
> keep hearing of lots of cases of people implementing this
> independently anyway.  At least if it is debugfs, it makes it clear
> that it's not intended for production use.

I'm one of those guys who implemented this independently.

@Mortiz
I do not see how this can be a bad thing (from what you wrote I guess you 
prefer another interface). Which interface to use depends on the use case.
If you have this suspicion it's, I guess, because such interface it is 
extremely easy to use.

@Alan
DebugFS can be a first step, but I would go for a normal device in /dev at 
some point. I do not see why this should not be used in production

Below, again, my code that does the same thing. I already posted this in 
another thread [1] but probably this is the best place. As I said in the 
other thread, this is what I did quickly last year to make things working 
for us (where this kind of interface is a must). Unfortunately, I do not 
have time to review my own code and improve it.


[1] https://lkml.org/lkml/2018/8/16/107


-----
From d6a6df9515c5e92cc74b8948c3c10ac9cbeec6d2 Mon Sep 17 00:00:00 2001
From: Federico Vaga <federico.vaga@vaga.pv.it>
Date: Mon, 20 Nov 2017 14:40:26 +0100
Subject: [PATCH] fpga: program from user-space

Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
---
 Documentation/fpga/fpga-mgr.txt |   3 +
 drivers/fpga/fpga-mgr.c         | 261 ++++++++++++++++++++++++++++++++++++
++
+-
 include/linux/fpga/fpga-mgr.h   |  19 +++
 3 files changed, 281 insertions(+), 2 deletions(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-
mgr.txt
index 78f197f..397dae9 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -197,3 +197,6 @@ to put the FPGA into operating mode.
 The ops include a .state function which will read the hardware FPGA 
manager 
and
 return a code of type enum fpga_mgr_states.  It doesn't result in a change 
in
 hardware state.
+
+Configuring the FPGA from user-space
+====================================
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 6441f91..964b7e4 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -27,10 +27,56 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/highmem.h>
+#include <linux/spinlock.h>
 #include <linux/version.h>
+#include <linux/cdev.h>
+#include <linux/list.h>
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
+static dev_t fpga_mgr_devt;
+
+/**
+ * struct fpga_image_chunck - FPGA configuration chunck
+ * @data: chunk data
+ * @size: chunk data size
+ * @list: for the linked list of chunks
+ */
+struct fpga_image_chunk {
+       void *data;
+       size_t size;
+       struct list_head list;
+};
+#define CHUNK_MAX_SIZE (PAGE_SIZE)
+
+/**
+ * struct fpga_user_load - structure to handle configuration from user-
space
+ * @mgr: pointer to the FPGA manager
+ * @chunk_list: HEAD point of a linked list of FPGA chunks
+ * @n_chunks: number of chunks in the list
+ * @lock: it protects: chunk_list, n_chunks
+ */
+struct fpga_user_load {
+       struct fpga_manager *mgr;
+       struct list_head chunk_list;
+       unsigned int n_chunks;
+       struct spinlock lock;
+};
+
+
+/**
+ * It sets by default a huge timeout for configuration coming from user-
space
+ * just to play safe.
+ *
+ * FIXME what about sysfs parameters to adjust it? The flag bit in 
particular
+ */
+struct fpga_image_info default_user_info = {
+       .flags = 0,
+       .enable_timeout_us = 10000000, /* 10s */
+       .disable_timeout_us = 10000000, /* 10s */
+       .config_complete_timeout_us = 10000000, /* 10s */
+};
+
 
 /*
  * Call the low level driver's write_init function.  This will do the
@@ -310,6 +356,158 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
 
+
+static int fpga_mgr_open(struct inode *inode, struct file *file)
+{
+       struct fpga_manager *mgr = container_of(inode->i_cdev,
+                                               struct fpga_manager,
+                                               cdev);
+       struct fpga_user_load *usr;
+       int ret;
+
+       /* Allow the user-space programming only if user unlocked the FPGA 
*/
+       if (!test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags)) {
+               dev_info(&mgr->dev, "The FPGA programming is locked\n");
+               return -EPERM;
+       }
+
+       ret = mutex_trylock(&mgr->usr_mutex);
+       if (ret == 0)
+               return -EBUSY;
+
+       usr = kzalloc(sizeof(struct fpga_user_load), GFP_KERNEL);
+       if (!usr) {
+               ret = -ENOMEM;
+               goto err_alloc;
+       }
+
+       usr->mgr = mgr;
+       spin_lock_init(&usr->lock);
+       INIT_LIST_HEAD(&usr->chunk_list);
+       file->private_data = usr;
+
+       return 0;
+
+err_alloc:
+       spin_lock(&mgr->lock);
+       clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+       spin_unlock(&mgr->lock);
+       mutex_unlock(&mgr->usr_mutex);
+       return ret;
+}
+
+
+static int fpga_mgr_flush(struct file *file, fl_owner_t id)
+{
+       struct fpga_user_load *usr = file->private_data;
+       struct fpga_image_chunk *chunk, *tmp;
+       struct sg_table sgt;
+       struct scatterlist *sg;
+       int err = 0;
+
+       if (!usr->n_chunks)
+               return 0;
+
+       err = sg_alloc_table(&sgt, usr->n_chunks, GFP_KERNEL);
+       if (err)
+               goto out;
+
+       sg = sgt.sgl;
+       list_for_each_entry(chunk, &usr->chunk_list, list) {
+               sg_set_buf(sg, chunk->data, chunk->size);
+               sg = sg_next(sg);
+               if (!sg)
+                       break;
+       }
+
+       err = fpga_mgr_buf_load_sg(usr->mgr,
+                                  &default_user_info,
+                                  &sgt);
+       sg_free_table(&sgt);
+
+out:
+       /* Remove all chunks */
+       spin_lock(&usr->lock);
+       list_for_each_entry_safe(chunk, tmp, &usr->chunk_list, list) {
+               list_del(&chunk->list);
+               kfree(chunk->data);
+               kfree(chunk);
+               usr->n_chunks--;
+       }
+       spin_unlock(&usr->lock);
+
+       return err;
+}
+
+
+static int fpga_mgr_close(struct inode *inode, struct file *file)
+{
+       struct fpga_user_load *usr = file->private_data;
+       struct fpga_manager *mgr = usr->mgr;
+
+       kfree(usr);
+
+       spin_lock(&mgr->lock);
+       clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+       spin_unlock(&mgr->lock);
+
+       mutex_unlock(&mgr->usr_mutex);
+
+       return 0;
+}
+
+
+static ssize_t fpga_mgr_write(struct file *file, const char __user *buf,
+                             size_t count, loff_t *offp)
+{
+       struct fpga_user_load *usr = file->private_data;
+       struct fpga_image_chunk *chunk;
+       int err;
+
+       chunk = kmalloc(sizeof(struct fpga_image_chunk), GFP_KERNEL);
+       if (!chunk)
+               return -ENOMEM;
+
+       chunk->size = count > CHUNK_MAX_SIZE ? CHUNK_MAX_SIZE : count;
+       chunk->data = kmalloc(chunk->size, GFP_KERNEL);
+       if (!chunk->data) {
+               err = -ENOMEM;
+               goto err_buf_alloc;
+       }
+
+       err = copy_from_user(chunk->data, buf, chunk->size);
+       if(err)
+               goto err_buf_copy;
+
+       spin_lock(&usr->lock);
+       list_add_tail(&chunk->list, &usr->chunk_list);
+       usr->n_chunks++;
+       spin_unlock(&usr->lock);
+
+       *offp += count;
+
+       return chunk->size;
+
+err_buf_copy:
+       kfree(chunk->data);
+err_buf_alloc:
+       kfree(chunk);
+       return err;
+}
+
+
+/**
+ * Char device operation
+ */
+static const struct file_operations fpga_mgr_fops = {
+       .owner = THIS_MODULE,
+       .open = fpga_mgr_open,
+       .flush = fpga_mgr_flush,
+       .release = fpga_mgr_close,
+       .write  = fpga_mgr_write,
+};
+
+
 static const char * const state_str[] = {
        [FPGA_MGR_STATE_UNKNOWN] =              "unknown",
        [FPGA_MGR_STATE_POWER_OFF] =            "power off",
@@ -352,13 +550,43 @@ static ssize_t state_show(struct device *dev,
        return sprintf(buf, "%s\n", state_str[mgr->state]);
 }
 
+static ssize_t config_lock_show(struct device *dev,
+                               struct device_attribute *attr, char *buf)
+{
+       struct fpga_manager *mgr = to_fpga_manager(dev);
+
+       if (test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags))
+               return sprintf(buf, "unlock\n");
+       return sprintf(buf, "lock\n");
+}
+
+static ssize_t config_lock_store(struct device *dev,
+                                struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+       struct fpga_manager *mgr = to_fpga_manager(dev);
+
+       spin_lock(&mgr->lock);
+       if (strncmp(buf, "lock" , min(4, (int)count)) == 0)
+               clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+       else if (strncmp(buf, "unlock" , min(6, (int)count)) == 0)
+               set_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+       else
+               count = -EINVAL;
+       spin_unlock(&mgr->lock);
+
+       return count;
+}
+
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RO(state);
+static DEVICE_ATTR_RW(config_lock);
 
 static struct attribute *fpga_mgr_attrs[] = {
        &dev_attr_name.attr,
        &dev_attr_state.attr,
+       &dev_attr_lock.attr,
        NULL,
 };
 ATTRIBUTE_GROUPS(fpga_mgr);
@@ -367,6 +595,9 @@ ATTRIBUTE_GROUPS(fpga_mgr);
 static struct device_attribute fpga_mgr_attrs[] = {
        __ATTR(name, S_IRUGO, name_show, NULL),
        __ATTR(state, S_IRUGO, state_show, NULL),
+       __ATTR(config_lock, S_IRUGO | S_IWUSR | S_IRGRP | S_IWGRP,
+              config_lock_show, config_lock_store),
+       __ATTR_NULL,
 };
 #endif
 
@@ -507,6 +738,8 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
        }
 
        mutex_init(&mgr->ref_mutex);
+       mutex_init(&mgr->usr_mutex);
+       spin_lock_init(&mgr->lock);
 
        mgr->name = name;
        mgr->mops = mops;
@@ -524,12 +757,19 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
        mgr->dev.parent = dev;
        mgr->dev.of_node = dev->of_node;
        mgr->dev.id = id;
+       mgr->dev.devt = MKDEV(MAJOR(fpga_mgr_devt), id);
        dev_set_drvdata(dev, mgr);
 
        ret = dev_set_name(&mgr->dev, "fpga%d", id);
        if (ret)
                goto error_device;
 
+       cdev_init(&mgr->cdev, &fpga_mgr_fops);
+       mgr->cdev.owner = THIS_MODULE;
+       ret = cdev_add(&mgr->cdev, mgr->dev.devt, 1);
+       if (ret)
+               goto err_cdev;
+
        ret = device_add(&mgr->dev);
        if (ret)
                goto error_device;
@@ -539,6 +779,8 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
        return 0;
 
 error_device:
+       cdev_del(&mgr->cdev);
+err_cdev:
        ida_simple_remove(&fpga_mgr_ida, id);
 error_kfree:
        kfree(mgr);
@@ -572,17 +814,27 @@ static void fpga_mgr_dev_release(struct device *dev)
 {
        struct fpga_manager *mgr = to_fpga_manager(dev);
 
+       cdev_del(&mgr->cdev);
        ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
        kfree(mgr);
 }
 
 static int __init fpga_mgr_class_init(void)
 {
+       int err;
+
        pr_info("FPGA manager framework\n");
 
+       err = alloc_chrdev_region(&fpga_mgr_devt, 0, FPGA_MGR_MAX_DEV,
+                                 "fpga_mgr");
+       if (err)
+               return err;
+
        fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
-       if (IS_ERR(fpga_mgr_class))
-               return PTR_ERR(fpga_mgr_class);
+       if (IS_ERR(fpga_mgr_class)) {
+               err = PTR_ERR(fpga_mgr_class);
+               goto err_class;
+       }
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
        fpga_mgr_class->dev_groups = fpga_mgr_groups;
 #else
@@ -591,12 +843,17 @@ static int __init fpga_mgr_class_init(void)
        fpga_mgr_class->dev_release = fpga_mgr_dev_release;
 
        return 0;
+
+err_class:
+       unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
+       return err;
 }
 
 static void __exit fpga_mgr_class_exit(void)
 {
        class_destroy(fpga_mgr_class);
        ida_destroy(&fpga_mgr_ida);
+       unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
 }
 
 MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index bfa14bc..ae38e48 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -15,8 +15,10 @@
  * You should have received a copy of the GNU General Public License along 
with
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+#include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #ifndef _LINUX_FPGA_MGR_H
 #define _LINUX_FPGA_MGR_H
@@ -24,6 +26,8 @@
 struct fpga_manager;
 struct sg_table;
 
+#define FPGA_MGR_MAX_DEV (256)
+
 /**
  * enum fpga_mgr_states - fpga framework states
  * @FPGA_MGR_STATE_UNKNOWN: can't determine state
@@ -118,22 +122,37 @@ struct fpga_manager_ops {
        void (*fpga_remove)(struct fpga_manager *mgr);
 };
 
+/*
+ * List of status FLAGS for the FPGA manager
+ */
+#define FPGA_MGR_FLAG_BITS (32)
+#define FPGA_MGR_FLAG_UNLOCK BIT(0)
+
 /**
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
+ * @cdev: char device interface
  * @dev: fpga manager device
  * @ref_mutex: only allows one reference to fpga manager
  * @state: state of fpga manager
  * @mops: pointer to struct of fpga manager ops
  * @priv: low level driver private date
+ * @flags: manager status bits
+ * @lock: it protects: flags
+ * @usr_mutex: only allows one user to program the FPGA
  */
 struct fpga_manager {
        const char *name;
+       struct cdev cdev;
        struct device dev;
        struct mutex ref_mutex;
        enum fpga_mgr_states state;
        const struct fpga_manager_ops *mops;
        void *priv;
+
+       DECLARE_BITMAP(flags, FPGA_MGR_FLAG_BITS);
+       struct spinlock lock;
+       struct mutex usr_mutex;
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.15.0


-- 
Federico Vaga
[BE-CO-HT]

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Moritz Fischer]

Hi Federico,

On Thu, Aug 16, 2018 at 11:21:32PM +0200, Federico Vaga wrote:
> Hi,
> 
> On Thursday, August 16, 2018 10:04:51 PM CEST Alan Tull wrote:
> > On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> > > Hi Alan,
> > 
> > Hi Moritz,
> > 
> > > comments inline. While I see how this is useful, I have the
> > > suspicion that from the moment this gets merged vendor kernels
> > > will just default to use this ...
> > 
> > Yeah, I have that suspicion as well.  That's probably why I sat on
> > this and didn't upstream it for 2 years.  But on the other hand, I
> > keep hearing of lots of cases of people implementing this
> > independently anyway.  At least if it is debugfs, it makes it clear
> > that it's not intended for production use.
> 
> I'm one of those guys who implemented this independently.

We all have in one way or another ;) Most people on ARM run an out of tree
patch using devicetree overlays these days I hope rather than /dev/mem
and UIO ... or other vender solutions...
> 
> @Mortiz
> I do not see how this can be a bad thing (from what you wrote I guess you 
> prefer another interface). Which interface to use depends on the use case.
> If you have this suspicion it's, I guess, because such interface it is 
> extremely easy to use.

What happens to a kernel driver doing MMIO with devices while you reload
the entire FPGA from userland?
> 
> @Alan
> DebugFS can be a first step, but I would go for a normal device in /dev at 
> some point. I do not see why this should not be used in production

I'm not against having a userland interface to reprogram the FPGA, the 
Intel DFL code is a good example of a sensible one, doing so in a safe
manner.

Ideally we'll get around to have a more generic interface, as we get
time to work on it.

- Moritz

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Federico Vaga]

Hi Mortiz,

I'm not 100% into the problem to understand all cases. I'm putting on the 
table the point of view, mainly, of an user. If you say there are problems 
here or there I believe you. At the beginning, you did not say that this 
interface may introduce problems (and I'm interested in those problems since I 
implemented one and we are using it), but that you fear that it becomes the 
default (usually, being a default is a good thing).

Since you and Alan are working on this for a long time, you can read each 
other mind, but I need a more verbose email to understand ^_^'

Of course the interface must be safe, I totally agree. In order to make me 
understand what are the issues, can you list some of them? And expand your 
comment about  MMIO.

(I just did a dummy backport, and implemented that interface. So, I repeat 
myself: I do not have enough experience with this framework to understand all 
consequences, but I'm interested to know what are the risks behind this 
interface)

On Friday, August 17, 2018 12:00:34 AM CEST Moritz Fischer wrote:
> Hi Federico,
> 
> On Thu, Aug 16, 2018 at 11:21:32PM +0200, Federico Vaga wrote:
> > Hi,
> > 
> > On Thursday, August 16, 2018 10:04:51 PM CEST Alan Tull wrote:
> > > On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer <mdf@kernel.org> wrote:
> > > > Hi Alan,
> > > 
> > > Hi Moritz,
> > > 
> > > > comments inline. While I see how this is useful, I have the
> > > > suspicion that from the moment this gets merged vendor kernels
> > > > will just default to use this ...
> > > 
> > > Yeah, I have that suspicion as well.  That's probably why I sat on
> > > this and didn't upstream it for 2 years.  But on the other hand, I
> > > keep hearing of lots of cases of people implementing this
> > > independently anyway.  At least if it is debugfs, it makes it clear
> > > that it's not intended for production use.
> > 
> > I'm one of those guys who implemented this independently.
> 
> We all have in one way or another ;) Most people on ARM run an out of tree
> patch using devicetree overlays these days I hope rather than /dev/mem
> and UIO ... or other vender solutions...
> 
> > @Mortiz
> > I do not see how this can be a bad thing (from what you wrote I guess you
> > prefer another interface). Which interface to use depends on the use case.
> > If you have this suspicion it's, I guess, because such interface it is
> > extremely easy to use.
> 
> What happens to a kernel driver doing MMIO with devices while you reload
> the entire FPGA from userland?
> 
> > @Alan
> > DebugFS can be a first step, but I would go for a normal device in /dev at
> > some point. I do not see why this should not be used in production
> 
> I'm not against having a userland interface to reprogram the FPGA, the
> Intel DFL code is a good example of a sensible one, doing so in a safe
> manner.
>
> Ideally we'll get around to have a more generic interface, as we get
> time to work on it.
> 
> - Moritz

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Federico Vaga]

On Friday, August 17, 2018 3:19:24 PM CEST Alan Tull wrote:
> On Fri, Aug 17, 2018, 2:00 AM Federico Vaga <federico.vaga@cern.ch> wrote:
> > Hi Mortiz,
> > 
> > I'm not 100% into the problem to understand all cases. I'm putting on the
> > table the point of view, mainly, of an user. If you say there are problems
> > here or there I believe you. At the beginning, you did not say that this
> > interface may introduce problems (and I'm interested in those problems
> > since I
> > implemented one and we are using it), but that you fear that it becomes
> > the
> > default (usually, being a default is a good thing).
> > 
> > Since you and Alan are working on this for a long time, you can read each
> > other mind, but I need a more verbose email to understand ^_^'
> > 
> > Of course the interface must be safe, I totally agree. In order to make me
> > understand what are the issues, can you list some of them?
> 
> Before we repeat what the doc l posted says, could you look at it and
> comment on what I'm not saying there?
> 
> https://lkml.org/lkml/2018/8/15/525

I read it, but do you mean that the problems you foresee are only the ones 
listed in there? If this is so, comments:

loading devices
It is true that it is a problem, and probably it is clear to everyone who will 
try to use such interface: "and now how do I load my devices?". But this is 
only a possible case, the FPGA may run without device driver and in this case 
it is perfectly fine for production.

If the answer to the above question is: "ok, let me see where my devices are 
in the memory ..." well if the machine crashes, it's their problem. This 
problem exists even without the FPGA manager.

bridge
My understand is that it is optional. Developers are allowed to not implement 
the bridge's operations. Which probably means that it does not exist or it is 
not needed.
When an user uses this interface from userspace it shouldn't be hard to detect 
if the operation is risky or not (bridge enabled/disabled). And if it is 
risky, the operation fails with EPERM, EBUSY.

I have to say that I'm not familiar with the bridge design, perhaps I'm 
missing something.


Conclusions
Yes, those are problems but I think they do not justify the label "not for 
production": in some cases I think is fine.

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Moritz Fischer]

Hi Alan, Federico,

On Fri, Aug 17, 2018 at 6:19 AM, Alan Tull <atull@kernel.org> wrote:
> On Fri, Aug 17, 2018, 2:00 AM Federico Vaga <federico.vaga@cern.ch> wrote:
>>
>> Hi Mortiz,
>>
>> I'm not 100% into the problem to understand all cases. I'm putting on the
>> table the point of view, mainly, of an user. If you say there are problems
>> here or there I believe you. At the beginning, you did not say that this
>> interface may introduce problems (and I'm interested in those problems
>> since I
>> implemented one and we are using it), but that you fear that it becomes
>> the
>> default (usually, being a default is a good thing).
>>
>> Since you and Alan are working on this for a long time, you can read each
>> other mind, but I need a more verbose email to understand ^_^'
>>
>> Of course the interface must be safe, I totally agree. In order to make me
>> understand what are the issues, can you list some of them?

Say you have kernel drivers (a network driver in the FPGA, or an I2C controller)
for example bound to hardware on a MMIO bus in the the FPGA.
You reprogram the FPGA using the debugfs interface, and the drivers don't get
unloaded correctly, the driver will try to access the registers and depending
on your system / bus either give you bad values or lock up.
Now userland locked up your system. Bad.

I'm not saying it isn't possible to do this if you're careful, of
course you could
first unload the drivers using rrmod and it would work just fine.

I just feel an interface like this might make it easier to create the
wrong design.
I've seen plenty of Application notes from vendors where they literally did
"cat foo.bin > /dev/fpga" followed by mmap(/dev/mem...).

>
>
> Before we repeat what the doc l posted says, could you look at it and
> comment on what I'm not saying there?
>
> https://lkml.org/lkml/2018/8/15/525

Alan, maybe I didn't express myself well. I'm fine with the debugfs interface
as a debug interface, just not for general usage ;-) I think your document is
clear on that.

Thanks,

Moritz

Re: [PATCH 2/2] fpga: add FPGA manager debugfs

[Federico Vaga]

Hi,

On Friday, August 17, 2018 5:22:56 PM CEST Moritz Fischer wrote:
> Hi Alan, Federico,
> 
> On Fri, Aug 17, 2018 at 6:19 AM, Alan Tull <atull@kernel.org> wrote:
> > On Fri, Aug 17, 2018, 2:00 AM Federico Vaga <federico.vaga@cern.ch> 
wrote:
> >> Hi Mortiz,
> >> 
> >> I'm not 100% into the problem to understand all cases. I'm putting on
> >> the table the point of view, mainly, of an user. If you say there are
> >> problems here or there I believe you. At the beginning, you did not
> >> say that this interface may introduce problems (and I'm interested in
> >> those problems since I
> >> implemented one and we are using it), but that you fear that it
> >> becomes
> >> the
> >> default (usually, being a default is a good thing).
> >> 
> >> Since you and Alan are working on this for a long time, you can read
> >> each other mind, but I need a more verbose email to understand ^_^'
> >> 
> >> Of course the interface must be safe, I totally agree. In order to
> >> make me understand what are the issues, can you list some of them?
> 
> Say you have kernel drivers (a network driver in the FPGA, or an I2C
> controller) for example bound to hardware on a MMIO bus in the the FPGA.
> You reprogram the FPGA using the debugfs interface, and the drivers don't
> get unloaded correctly, the driver will try to access the registers and
> depending on your system / bus either give you bad values or lock up.
> Now userland locked up your system. Bad.

I think I got confused by your reference to the MMIO, but now it sound like 
it was just a very specific example of a more general problem. Because this 
is true for any device driver for FPGA soft-IP/IP-core, it is not strictly 
an MMIO problem. Am I missing something?

I get the problem, I have to fight with **this** problem daily because I'm 
loading images with:

cat hello.bin > /dev/fpga0

And then, somehow I have to load the device drivers (memory, IRQ, ...). But 
I will not say publicly what I do (it is a "don't try this at home" thing).

> I'm not saying it isn't possible to do this if you're careful, of
> course you could
> first unload the drivers using rrmod and it would work just fine.

Or having some reference counter on the last loaded FPGA image may work. 
This way it will be possible to detect if there are users of the current 
FPGA and inhibit any unwanted FPGA load (like the module counter forbid 
rmmod when the device is in use). If a device driver is using some FPGA 
component the reference counter increase. How to do it? Need more studies, 
but probably this is a safe way that perhaps worth to look at.
 
> I just feel an interface like this might make it easier to create the
> wrong design.
> I've seen plenty of Application notes from vendors where they literally
> did "cat foo.bin > /dev/fpga" followed by mmap(/dev/mem...).

Actually, I'm doing worst than this (to compensate the lack of 
infrastructure). You tried, but you are not scaring me :P
 
> > Before we repeat what the doc l posted says, could you look at it and
> > comment on what I'm not saying there?
> > 
> > https://lkml.org/lkml/2018/8/15/525
> 
> Alan, maybe I didn't express myself well. I'm fine with the debugfs
> interface as a debug interface, just not for general usage ;-) I think
> your document is clear on that.
> 
> Thanks,
> 
> Moritz


-- 
Federico Vaga
[BE-CO-HT]

RE: [PATCH 2/2] fpga: add FPGA manager debugfs

[Appana Durga Kedareswara Rao]

FYI...

Regards,
Kedar.

> -----Original Message-----
> From: Alan Tull <atull@kernel.org>
> Sent: Thursday, August 16, 2018 3:40 AM
> To: Moritz Fischer <mdf@kernel.org>; Jonathan Corbet <corbet@lwn.net>;
> Randy Dunlap <rdunlap@infradead.org>; Dinh Nguyen
> <dinguyen@kernel.org>
> Cc: Appana Durga Kedareswara Rao <appanad@xilinx.com>; linux-
> kernel@vger.kernel.org; linux-fpga@vger.kernel.org; linux-
> doc@vger.kernel.org; Alan Tull <atull@kernel.org>; Alan Tull
> <atull@opensource.altera.com>; Matthew Gerlach
> <matthew.gerlach@linux.intel.com>
> Subject: [PATCH 2/2] fpga: add FPGA manager debugfs
> 
> From: Alan Tull <atull@opensource.altera.com>
> 
> Implement DebugFS for the FPGA Manager Framework for debugging and
> developmental use.
> 
> Enabled by CONFIG_FPGA_MGR_DEBUG_FS
> 
> Each FPGA gets its own directory such as
>  <debugfs>/fpga_manager/fpga0 and three files:
> 
>  * [RW] flags          = flags as defined in fpga-mgr.h
>  * [RW] firmware_name  = write/read back name of FPGA image
>                          firmware file to program
>  * [WO] image          = write-only file for directly writing
>                          fpga image w/o firmware layer
>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>                          go to the operating state after
> 			 programming
> 
> The debugfs is implemented in a separate fpga_mgr_debugfs.c file, but the
> FPGA manager core is still built as one module.  Note the name change from
> fpga-mgr.ko to fpga_mgr.ko.
> 
> Signed-off-by: Alan Tull <atull@kernel.org>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/Kconfig            |   7 ++
>  drivers/fpga/Makefile           |   4 +-
>  drivers/fpga/fpga-mgr-debugfs.c | 221
> ++++++++++++++++++++++++++++++++++++++++
>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>  drivers/fpga/fpga-mgr.c         |   8 ++
>  include/linux/fpga/fpga-mgr.h   |   3 +
>  6 files changed, 264 insertions(+), 1 deletion(-)  create mode 100644
> drivers/fpga/fpga-mgr-debugfs.c  create mode 100644 drivers/fpga/fpga-mgr-
> debugfs.h
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index 1ebcef4..600924d
> 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -9,6 +9,13 @@ menuconfig FPGA
>  	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
>  	  manager drivers.
> 
> +config FPGA_MGR_DEBUG_FS
> +       bool "FPGA Manager DebugFS"
> +       depends on FPGA && DEBUG_FS
> +       help
> +         Say Y here if you want to expose a DebugFS interface for the
> +	 FPGA Manager Framework.
> +
>  if FPGA
> 
>  config FPGA_MGR_SOCFPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> 7a2d73b..62910cc 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -4,7 +4,9 @@
>  #
> 
>  # Core FPGA Manager Framework
> -obj-$(CONFIG_FPGA)			+= fpga-mgr.o
> +obj-$(CONFIG_FPGA)			+= fpga_mgr.o
> +fpga_mgr-y				:= fpga-mgr.o
> +fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS)	+= fpga-mgr-debugfs.o
> 
>  # FPGA Manager Drivers
>  obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
> diff --git a/drivers/fpga/fpga-mgr-debugfs.c b/drivers/fpga/fpga-mgr-debugfs.c
> new file mode 100644 index 0000000..f2fdf58
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr-debugfs.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Manager DebugFS
> + *
> + *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
> + */
> +#include <linux/debugfs.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +static struct dentry *fpga_mgr_debugfs_root;
> +
> +struct fpga_mgr_debugfs {
> +	struct dentry *debugfs_dir;
> +	struct fpga_image_info *info;
> +};
> +
> +static ssize_t fpga_mgr_firmware_write_file(struct file *file,
> +					    const char __user *user_buf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	struct device *dev = &mgr->dev;
> +	char *buf;
> +	int ret;
> +
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret) {
> +		dev_err(dev, "FPGA manager is busy\n");
> +		return -EBUSY;
> +	}
> +
> +	buf = devm_kzalloc(dev, count, GFP_KERNEL);
> +	if (!buf) {
> +		fpga_mgr_unlock(mgr);
> +		return -ENOMEM;
> +	}
> +
> +	if (copy_from_user(buf, user_buf, count)) {
> +		fpga_mgr_unlock(mgr);
> +		devm_kfree(dev, buf);
> +		return -EFAULT;
> +	}
> +
> +	buf[count] = 0;
> +	if (buf[count - 1] == '\n')
> +		buf[count - 1] = 0;
> +
> +	/* Release previous firmware name (if any). Save current one. */
> +	if (debugfs->info->firmware_name)
> +		devm_kfree(dev, debugfs->info->firmware_name);
> +	debugfs->info->firmware_name = buf;
> +
> +	ret = fpga_mgr_load(mgr, debugfs->info);
> +	if (ret)
> +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> +
> +	fpga_mgr_unlock(mgr);
> +
> +	return count;
> +}
> +
> +static ssize_t fpga_mgr_firmware_read_file(struct file *file,
> +					   char __user *user_buf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	char *buf;
> +	int ret;
> +
> +	if (!debugfs->info->firmware_name)
> +		return 0;
> +
> +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info-
> >firmware_name);
> +	if (ret < 0) {
> +		kfree(buf);
> +		return ret;
> +	}
> +
> +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
> +static const struct file_operations fpga_mgr_firmware_fops = {
> +	.open = simple_open,
> +	.read = fpga_mgr_firmware_read_file,
> +	.write = fpga_mgr_firmware_write_file,
> +	.llseek = default_llseek,
> +};
> +
> +static ssize_t fpga_mgr_image_write_file(struct file *file,
> +					 const char __user *user_buf,
> +					 size_t count, loff_t *ppos)
> +{
> +	struct fpga_manager *mgr = file->private_data;
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +	struct device *dev = &mgr->dev;
> +	char *buf;
> +	int ret;
> +
> +	dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
> +
> +	ret = fpga_mgr_lock(mgr);
> +	if (ret) {
> +		dev_err(dev, "FPGA manager is busy\n");
> +		return -EBUSY;
> +	}
> +
> +	buf = kzalloc(count, GFP_KERNEL);
> +	if (!buf) {
> +		fpga_mgr_unlock(mgr);
> +		return -ENOMEM;
> +	}
> +
> +	if (copy_from_user(buf, user_buf, count)) {
> +		fpga_mgr_unlock(mgr);
> +		kfree(buf);
> +		return -EFAULT;
> +	}
> +
> +	/* If firmware interface was previously used, forget it. */
> +	if (debugfs->info->firmware_name)
> +		devm_kfree(dev, debugfs->info->firmware_name);
> +	debugfs->info->firmware_name = NULL;
> +
> +	debugfs->info->buf = buf;
> +	debugfs->info->count = count;
> +
> +	ret = fpga_mgr_load(mgr, debugfs->info);
> +	if (ret)
> +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> +
> +	fpga_mgr_unlock(mgr);
> +
> +	debugfs->info->buf = NULL;
> +	debugfs->info->count = 0;
> +
> +	kfree(buf);
> +
> +	return count;
> +}
> +
> +static const struct file_operations fpga_mgr_image_fops = {
> +	.open = simple_open,
> +	.write = fpga_mgr_image_write_file,
> +	.llseek = default_llseek,
> +};
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {
> +	struct device *dev = &mgr->dev;
> +	struct fpga_mgr_debugfs *debugfs;
> +	struct fpga_image_info *info;
> +
> +	if (!fpga_mgr_debugfs_root)
> +		return;
> +
> +	debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
> +	if (!debugfs)
> +		return;
> +
> +	info = fpga_image_info_alloc(dev);
> +	if (!info) {
> +		kfree(debugfs);
> +		return;
> +	}
> +	debugfs->info = info;
> +
> +	debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
> +						  fpga_mgr_debugfs_root);
> +
> +	debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir,
> mgr,
> +			    &fpga_mgr_firmware_fops);
> +
> +	debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
> +			    &fpga_mgr_image_fops);
> +
> +	debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info-
> >flags);
> +
> +	debugfs_create_u32("config_complete_timeout_us", 0600,
> +			   debugfs->debugfs_dir,
> +			   &info->config_complete_timeout_us);
> +
> +	mgr->debugfs = debugfs;
> +}
> +
> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {
> +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> +
> +	if (!fpga_mgr_debugfs_root)
> +		return;
> +
> +	debugfs_remove_recursive(debugfs->debugfs_dir);
> +
> +	/* this function also frees debugfs->info->firmware_name */
> +	fpga_image_info_free(debugfs->info);
> +
> +	kfree(debugfs);
> +}
> +
> +void fpga_mgr_debugfs_init(void)
> +{
> +	fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
> +	if (!fpga_mgr_debugfs_root)
> +		pr_warn("fpga_mgr: Failed to create debugfs root\n"); }
> +
> +void fpga_mgr_debugfs_uninit(void)
> +{
> +	debugfs_remove_recursive(fpga_mgr_debugfs_root);
> +}
> diff --git a/drivers/fpga/fpga-mgr-debugfs.h b/drivers/fpga/fpga-mgr-debugfs.h
> new file mode 100644 index 0000000..17cd3eb
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr-debugfs.h
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
> +#define _LINUX_FPGA_MGR_DEBUGFS_H
> +
> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr); void
> +fpga_mgr_debugfs_remove(struct fpga_manager *mgr); void
> +fpga_mgr_debugfs_init(void); void fpga_mgr_debugfs_uninit(void);
> +
> +#else
> +
> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {} void
> +fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {} void
> +fpga_mgr_debugfs_init(void) {} void fpga_mgr_debugfs_uninit(void) {}
> +
> +#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
> +
> +#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> c8684bc..66eb6f6 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include "fpga-mgr-debugfs.h"
> 
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
> @@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
>  	if (ret)
>  		goto error_device;
> 
> +	fpga_mgr_debugfs_add(mgr);
> +
>  	dev_info(&mgr->dev, "%s registered\n", mgr->name);
> 
>  	return 0;
> @@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)  {
>  	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> 
> +	fpga_mgr_debugfs_remove(mgr);
> +
>  	/*
>  	 * If the low level driver provides a method for putting fpga into
>  	 * a desired state upon unregister, do it.
> @@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
>  	fpga_mgr_class->dev_groups = fpga_mgr_groups;
>  	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
> 
> +	fpga_mgr_debugfs_init();
> +
>  	return 0;
>  }
> 
>  static void __exit fpga_mgr_class_exit(void)  {
> +	fpga_mgr_debugfs_uninit();
>  	class_destroy(fpga_mgr_class);
>  	ida_destroy(&fpga_mgr_ida);
>  }
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 777c502..e8f2159 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -170,6 +170,9 @@ struct fpga_manager {
>  	struct fpga_compat_id *compat_id;
>  	const struct fpga_manager_ops *mops;
>  	void *priv;
> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> +	void *debugfs;
> +#endif
>  };
> 
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> --
> 2.7.4

RE: [PATCH 2/2] fpga: add FPGA manager debugfs

[Appana Durga Kedareswara Rao]

Please ignore this mail....

Regards,
Kedar.

> -----Original Message-----
> From: Appana Durga Kedareswara Rao
> Sent: Tuesday, March 19, 2019 3:58 PM
> To: 'Alan Tull' <atull@kernel.org>; Moritz Fischer <mdf@kernel.org>; Jonathan
> Corbet <corbet@lwn.net>; Randy Dunlap <rdunlap@infradead.org>; Dinh
> Nguyen <dinguyen@kernel.org>
> Cc: linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; linux-
> doc@vger.kernel.org; Alan Tull <atull@opensource.altera.com>; Matthew
> Gerlach <matthew.gerlach@linux.intel.com>
> Subject: RE: [PATCH 2/2] fpga: add FPGA manager debugfs
> 
> FYI...
> 
> Regards,
> Kedar.
> 
> > -----Original Message-----
> > From: Alan Tull <atull@kernel.org>
> > Sent: Thursday, August 16, 2018 3:40 AM
> > To: Moritz Fischer <mdf@kernel.org>; Jonathan Corbet <corbet@lwn.net>;
> > Randy Dunlap <rdunlap@infradead.org>; Dinh Nguyen
> > <dinguyen@kernel.org>
> > Cc: Appana Durga Kedareswara Rao <appanad@xilinx.com>; linux-
> > kernel@vger.kernel.org; linux-fpga@vger.kernel.org; linux-
> > doc@vger.kernel.org; Alan Tull <atull@kernel.org>; Alan Tull
> > <atull@opensource.altera.com>; Matthew Gerlach
> > <matthew.gerlach@linux.intel.com>
> > Subject: [PATCH 2/2] fpga: add FPGA manager debugfs
> >
> > From: Alan Tull <atull@opensource.altera.com>
> >
> > Implement DebugFS for the FPGA Manager Framework for debugging and
> > developmental use.
> >
> > Enabled by CONFIG_FPGA_MGR_DEBUG_FS
> >
> > Each FPGA gets its own directory such as
> >  <debugfs>/fpga_manager/fpga0 and three files:
> >
> >  * [RW] flags          = flags as defined in fpga-mgr.h
> >  * [RW] firmware_name  = write/read back name of FPGA image
> >                          firmware file to program
> >  * [WO] image          = write-only file for directly writing
> >                          fpga image w/o firmware layer
> >  * [RW] config_complete_timeout_us = maximum for the FPGA to
> >                          go to the operating state after
> > 			 programming
> >
> > The debugfs is implemented in a separate fpga_mgr_debugfs.c file, but
> > the FPGA manager core is still built as one module.  Note the name
> > change from fpga-mgr.ko to fpga_mgr.ko.
> >
> > Signed-off-by: Alan Tull <atull@kernel.org>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > ---
> >  drivers/fpga/Kconfig            |   7 ++
> >  drivers/fpga/Makefile           |   4 +-
> >  drivers/fpga/fpga-mgr-debugfs.c | 221
> > ++++++++++++++++++++++++++++++++++++++++
> >  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
> >  drivers/fpga/fpga-mgr.c         |   8 ++
> >  include/linux/fpga/fpga-mgr.h   |   3 +
> >  6 files changed, 264 insertions(+), 1 deletion(-)  create mode 100644
> > drivers/fpga/fpga-mgr-debugfs.c  create mode 100644
> > drivers/fpga/fpga-mgr- debugfs.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > 1ebcef4..600924d
> > 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -9,6 +9,13 @@ menuconfig FPGA
> >  	  kernel.  The FPGA framework adds a FPGA manager class and FPGA
> >  	  manager drivers.
> >
> > +config FPGA_MGR_DEBUG_FS
> > +       bool "FPGA Manager DebugFS"
> > +       depends on FPGA && DEBUG_FS
> > +       help
> > +         Say Y here if you want to expose a DebugFS interface for the
> > +	 FPGA Manager Framework.
> > +
> >  if FPGA
> >
> >  config FPGA_MGR_SOCFPGA
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > 7a2d73b..62910cc 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -4,7 +4,9 @@
> >  #
> >
> >  # Core FPGA Manager Framework
> > -obj-$(CONFIG_FPGA)			+= fpga-mgr.o
> > +obj-$(CONFIG_FPGA)			+= fpga_mgr.o
> > +fpga_mgr-y				:= fpga-mgr.o
> > +fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS)	+= fpga-mgr-debugfs.o
> >
> >  # FPGA Manager Drivers
> >  obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)	+= altera-cvp.o
> > diff --git a/drivers/fpga/fpga-mgr-debugfs.c
> > b/drivers/fpga/fpga-mgr-debugfs.c new file mode 100644 index
> > 0000000..f2fdf58
> > --- /dev/null
> > +++ b/drivers/fpga/fpga-mgr-debugfs.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * FPGA Manager DebugFS
> > + *
> > + *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
> > + */
> > +#include <linux/debugfs.h>
> > +#include <linux/fpga/fpga-mgr.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +
> > +static struct dentry *fpga_mgr_debugfs_root;
> > +
> > +struct fpga_mgr_debugfs {
> > +	struct dentry *debugfs_dir;
> > +	struct fpga_image_info *info;
> > +};
> > +
> > +static ssize_t fpga_mgr_firmware_write_file(struct file *file,
> > +					    const char __user *user_buf,
> > +					    size_t count, loff_t *ppos)
> > +{
> > +	struct fpga_manager *mgr = file->private_data;
> > +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> > +	struct device *dev = &mgr->dev;
> > +	char *buf;
> > +	int ret;
> > +
> > +	ret = fpga_mgr_lock(mgr);
> > +	if (ret) {
> > +		dev_err(dev, "FPGA manager is busy\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	buf = devm_kzalloc(dev, count, GFP_KERNEL);
> > +	if (!buf) {
> > +		fpga_mgr_unlock(mgr);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (copy_from_user(buf, user_buf, count)) {
> > +		fpga_mgr_unlock(mgr);
> > +		devm_kfree(dev, buf);
> > +		return -EFAULT;
> > +	}
> > +
> > +	buf[count] = 0;
> > +	if (buf[count - 1] == '\n')
> > +		buf[count - 1] = 0;
> > +
> > +	/* Release previous firmware name (if any). Save current one. */
> > +	if (debugfs->info->firmware_name)
> > +		devm_kfree(dev, debugfs->info->firmware_name);
> > +	debugfs->info->firmware_name = buf;
> > +
> > +	ret = fpga_mgr_load(mgr, debugfs->info);
> > +	if (ret)
> > +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> > +
> > +	fpga_mgr_unlock(mgr);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t fpga_mgr_firmware_read_file(struct file *file,
> > +					   char __user *user_buf,
> > +					   size_t count, loff_t *ppos)
> > +{
> > +	struct fpga_manager *mgr = file->private_data;
> > +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> > +	char *buf;
> > +	int ret;
> > +
> > +	if (!debugfs->info->firmware_name)
> > +		return 0;
> > +
> > +	buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info-
> > >firmware_name);
> > +	if (ret < 0) {
> > +		kfree(buf);
> > +		return ret;
> > +	}
> > +
> > +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> > +	kfree(buf);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct file_operations fpga_mgr_firmware_fops = {
> > +	.open = simple_open,
> > +	.read = fpga_mgr_firmware_read_file,
> > +	.write = fpga_mgr_firmware_write_file,
> > +	.llseek = default_llseek,
> > +};
> > +
> > +static ssize_t fpga_mgr_image_write_file(struct file *file,
> > +					 const char __user *user_buf,
> > +					 size_t count, loff_t *ppos)
> > +{
> > +	struct fpga_manager *mgr = file->private_data;
> > +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> > +	struct device *dev = &mgr->dev;
> > +	char *buf;
> > +	int ret;
> > +
> > +	dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
> > +
> > +	ret = fpga_mgr_lock(mgr);
> > +	if (ret) {
> > +		dev_err(dev, "FPGA manager is busy\n");
> > +		return -EBUSY;
> > +	}
> > +
> > +	buf = kzalloc(count, GFP_KERNEL);
> > +	if (!buf) {
> > +		fpga_mgr_unlock(mgr);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (copy_from_user(buf, user_buf, count)) {
> > +		fpga_mgr_unlock(mgr);
> > +		kfree(buf);
> > +		return -EFAULT;
> > +	}
> > +
> > +	/* If firmware interface was previously used, forget it. */
> > +	if (debugfs->info->firmware_name)
> > +		devm_kfree(dev, debugfs->info->firmware_name);
> > +	debugfs->info->firmware_name = NULL;
> > +
> > +	debugfs->info->buf = buf;
> > +	debugfs->info->count = count;
> > +
> > +	ret = fpga_mgr_load(mgr, debugfs->info);
> > +	if (ret)
> > +		dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
> > +
> > +	fpga_mgr_unlock(mgr);
> > +
> > +	debugfs->info->buf = NULL;
> > +	debugfs->info->count = 0;
> > +
> > +	kfree(buf);
> > +
> > +	return count;
> > +}
> > +
> > +static const struct file_operations fpga_mgr_image_fops = {
> > +	.open = simple_open,
> > +	.write = fpga_mgr_image_write_file,
> > +	.llseek = default_llseek,
> > +};
> > +
> > +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {
> > +	struct device *dev = &mgr->dev;
> > +	struct fpga_mgr_debugfs *debugfs;
> > +	struct fpga_image_info *info;
> > +
> > +	if (!fpga_mgr_debugfs_root)
> > +		return;
> > +
> > +	debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
> > +	if (!debugfs)
> > +		return;
> > +
> > +	info = fpga_image_info_alloc(dev);
> > +	if (!info) {
> > +		kfree(debugfs);
> > +		return;
> > +	}
> > +	debugfs->info = info;
> > +
> > +	debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
> > +						  fpga_mgr_debugfs_root);
> > +
> > +	debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir,
> > mgr,
> > +			    &fpga_mgr_firmware_fops);
> > +
> > +	debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
> > +			    &fpga_mgr_image_fops);
> > +
> > +	debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info-
> > >flags);
> > +
> > +	debugfs_create_u32("config_complete_timeout_us", 0600,
> > +			   debugfs->debugfs_dir,
> > +			   &info->config_complete_timeout_us);
> > +
> > +	mgr->debugfs = debugfs;
> > +}
> > +
> > +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {
> > +	struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
> > +
> > +	if (!fpga_mgr_debugfs_root)
> > +		return;
> > +
> > +	debugfs_remove_recursive(debugfs->debugfs_dir);
> > +
> > +	/* this function also frees debugfs->info->firmware_name */
> > +	fpga_image_info_free(debugfs->info);
> > +
> > +	kfree(debugfs);
> > +}
> > +
> > +void fpga_mgr_debugfs_init(void)
> > +{
> > +	fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
> > +	if (!fpga_mgr_debugfs_root)
> > +		pr_warn("fpga_mgr: Failed to create debugfs root\n"); }
> > +
> > +void fpga_mgr_debugfs_uninit(void)
> > +{
> > +	debugfs_remove_recursive(fpga_mgr_debugfs_root);
> > +}
> > diff --git a/drivers/fpga/fpga-mgr-debugfs.h
> > b/drivers/fpga/fpga-mgr-debugfs.h new file mode 100644 index
> > 0000000..17cd3eb
> > --- /dev/null
> > +++ b/drivers/fpga/fpga-mgr-debugfs.h
> > @@ -0,0 +1,22 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
> > +#define _LINUX_FPGA_MGR_DEBUGFS_H
> > +
> > +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> > +
> > +void fpga_mgr_debugfs_add(struct fpga_manager *mgr); void
> > +fpga_mgr_debugfs_remove(struct fpga_manager *mgr); void
> > +fpga_mgr_debugfs_init(void); void fpga_mgr_debugfs_uninit(void);
> > +
> > +#else
> > +
> > +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {} void
> > +fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {} void
> > +fpga_mgr_debugfs_init(void) {} void fpga_mgr_debugfs_uninit(void) {}
> > +
> > +#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
> > +
> > +#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > c8684bc..66eb6f6 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/highmem.h>
> > +#include "fpga-mgr-debugfs.h"
> >
> >  static DEFINE_IDA(fpga_mgr_ida);
> >  static struct class *fpga_mgr_class;
> > @@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
> >  	if (ret)
> >  		goto error_device;
> >
> > +	fpga_mgr_debugfs_add(mgr);
> > +
> >  	dev_info(&mgr->dev, "%s registered\n", mgr->name);
> >
> >  	return 0;
> > @@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
> {
> >  	dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
> >
> > +	fpga_mgr_debugfs_remove(mgr);
> > +
> >  	/*
> >  	 * If the low level driver provides a method for putting fpga into
> >  	 * a desired state upon unregister, do it.
> > @@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
> >  	fpga_mgr_class->dev_groups = fpga_mgr_groups;
> >  	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
> >
> > +	fpga_mgr_debugfs_init();
> > +
> >  	return 0;
> >  }
> >
> >  static void __exit fpga_mgr_class_exit(void)  {
> > +	fpga_mgr_debugfs_uninit();
> >  	class_destroy(fpga_mgr_class);
> >  	ida_destroy(&fpga_mgr_ida);
> >  }
> > diff --git a/include/linux/fpga/fpga-mgr.h
> > b/include/linux/fpga/fpga-mgr.h index 777c502..e8f2159 100644
> > --- a/include/linux/fpga/fpga-mgr.h
> > +++ b/include/linux/fpga/fpga-mgr.h
> > @@ -170,6 +170,9 @@ struct fpga_manager {
> >  	struct fpga_compat_id *compat_id;
> >  	const struct fpga_manager_ops *mops;
> >  	void *priv;
> > +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
> > +	void *debugfs;
> > +#endif
> >  };
> >
> >  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > --
> > 2.7.4