From: Alan Tull <[email protected]>
Followup to "Yet another stab at a fpga framework"
(https://lkml.org/lkml/2014/8/1/517)
One of the problems with writing this framework has been the
wide variety of use cases. My perspective has mostly been
for a fpga bolted to a SoC. Having said that, I really hope
this can be useful for as many uses as possible, while still
being integrated into the kernel in a way that is Linux-like.
Three basic methods of programming a FPGA are supported:
* Load firmware using sysfs (see patch 0002 header for example)
* Load firmware using Device Tree overlay (see patch 0003)
* The core fpga-mgr.c exports enough functions for any driver
to use to get the FPGA programmed. If someone needs another
interface, the core functions should give enough
functionality to make that possible.
If the fpga is programmed using firmware, resume is supported.
Big looming problem in firmware class for us here is that FPGA
image files can easily be 10's to 100's of megabytes. If we
have FPGA's that are chained, the image file for several FPGA's
is concatenated. So then you can multiply that.
High level changes from previous version:
* I went ahead and put the sysfs interface into the core
although I think it could just as well be separate.
* Rework the state/status stuff to stop the programming
steps. In the event of error, the state machine is left
at a state with '_ERR' in its name. state is available
in sysfs, including these error states.
* Added ability to reset the FPGA from sysfs. If people
like this, let me know. If not it may not be needed as
that should happen in the normal programming steps.
bus.c adds support for programming from Device Tree overlays.
That code was tested from Pantelis' branch; the rest of this
was tested on for-next.
Alan
Alan Tull (3):
fpga manager: add sysfs interface document
fpga manager: framework core
fpga manager: bus driver
Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/fpga/Kconfig | 28 +
drivers/fpga/Makefile | 11 +
drivers/fpga/bus.c | 124 +++++
drivers/fpga/fpga-mgr.c | 551 ++++++++++++++++++++
include/linux/fpga-mgr.h | 117 +++++
8 files changed, 872 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
create mode 100644 drivers/fpga/Kconfig
create mode 100644 drivers/fpga/Makefile
create mode 100644 drivers/fpga/bus.c
create mode 100644 drivers/fpga/fpga-mgr.c
create mode 100644 include/linux/fpga-mgr.h
--
1.7.9.5
From: Alan Tull <[email protected]>
Supports standard ops for low level FPGA drivers.
Various manufacturors' FPGAs can be supported by adding low
level drivers. Each driver needs to register its ops
using fpga_mgr_register().
Exports methods of doing operations to program FPGAs. These
should be sufficient for individual drivers to request FPGA
programming directly if desired.
Adds a sysfs interface. The sysfs interface can be compiled out
where desired in production builds.
Resume is supported by calling low level driver's resume
function, then reloading the firmware image.
The following are exported as GPL:
* fpga_mgr_reset
Reset the FGPA.
* fpga_mgr_write
Write a image (in buffer) to the FPGA.
* fpga_mgr_firmware_write
Request firmware by file name and write it to the FPGA.
* fpga_mgr_name
Get name of FPGA manager.
* fpga_mgr_state
Get a state of framework as a string.
* fpga_mgr_register and fpga_mgr_remove
Register/unregister low level fpga manager driver.
The following sysfs files are created:
* /sys/class/fpga_manager/<fpga>/name
Name of low level driver.
* /sys/class/fpga_manager/<fpga>/firmware
Name of FPGA image file to load using firmware class.
$ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
read: read back name of image file previous loaded
$ cat /sys/class/fpga_manager/<fpga>/firmware
* /sys/class/fpga_manager/<fpga>/reset
reset the FPGA
$ echo 1 > /sys/class/fpga_manager/<fpga>/reset
* /sys/class/fpga_manager/<fpga>/state
State of fpga framework state machine
Signed-off-by: Alan Tull <[email protected]>
---
v2: s/mangager/manager/
check for invalid request_nr
add fpga reset interface
rework the state code
remove FPGA_MGR_FAIL flag
add _ERR states to fpga manager states enum
low level state op now returns a state enum value
initialize framework state from driver state op
remove unused fpga read stuff
merge sysfs.c into fpga-mgr.c
move suspend/resume from bus.c to fpga-mgr.c
---
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/fpga/Kconfig | 21 ++
drivers/fpga/Makefile | 10 +
drivers/fpga/fpga-mgr.c | 528 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/fpga-mgr.h | 115 ++++++++++
6 files changed, 677 insertions(+)
create mode 100644 drivers/fpga/Kconfig
create mode 100644 drivers/fpga/Makefile
create mode 100644 drivers/fpga/fpga-mgr.c
create mode 100644 include/linux/fpga-mgr.h
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 622fa26..fc45fee 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -34,6 +34,8 @@ source "drivers/message/fusion/Kconfig"
source "drivers/firewire/Kconfig"
+source "drivers/fpga/Kconfig"
+
source "drivers/message/i2o/Kconfig"
source "drivers/macintosh/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index ebee555..637b9f0 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_RESET_CONTROLLER) += reset/
# default.
obj-y += tty/
obj-y += char/
+obj-$(CONFIG_FPGA) += fpga/
# gpu/ comes after char for AGP vs DRM startup
obj-y += gpu/
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
new file mode 100644
index 0000000..e775b17
--- /dev/null
+++ b/drivers/fpga/Kconfig
@@ -0,0 +1,21 @@
+#
+# FPGA framework configuration
+#
+
+menu "FPGA devices"
+
+config FPGA
+ tristate "FPGA Framework"
+ help
+ Say Y here if you want support for configuring FPGAs from the
+ kernel. The FPGA framework adds a FPGA manager class and FPGA
+ manager drivers.
+
+config FPGA_MGR_SYSFS
+ bool "FPGA Manager SysFS Interface"
+ depends on FPGA
+ depends on SYSFS
+ help
+ FPGA Manager SysFS interface.
+
+endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
new file mode 100644
index 0000000..c8a676f
--- /dev/null
+++ b/drivers/fpga/Makefile
@@ -0,0 +1,10 @@
+#
+# Makefile for the fpga framework and fpga manager drivers.
+#
+
+fpga-mgr-core-y += fpga-mgr.o
+
+# Core FPGA Manager Framework
+obj-$(CONFIG_FPGA) += fpga-mgr-core.o
+
+# FPGA Manager Drivers
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
new file mode 100644
index 0000000..0c7236a
--- /dev/null
+++ b/drivers/fpga/fpga-mgr.c
@@ -0,0 +1,528 @@
+/*
+ * FPGA Manager Core
+ *
+ * Copyright (C) 2013-2014 Altera Corporation
+ *
+ * With code from the mailing list:
+ * Copyright (C) 2013 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * 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/delay.h>
+#include <linux/firmware.h>
+#include <linux/fpga-mgr.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/vmalloc.h>
+
+static DEFINE_IDA(fpga_mgr_ida);
+static int fpga_mgr_major;
+static struct class *fpga_mgr_class;
+
+#define FPGA_MAX_MINORS 256
+
+static DEFINE_MUTEX(fpga_manager_mutex);
+static LIST_HEAD(fpga_manager_list);
+
+/*
+ * Get FPGA state from low level driver
+ * return value is same enum as fpga_mgr_framework_state
+ *
+ * This will be used to initialize framework state
+ */
+int fpga_mgr_low_level_state(struct fpga_manager *mgr)
+{
+ if (!mgr || !mgr->mops || !mgr->mops->state)
+ return FPGA_MGR_STATE_UNKNOWN;
+
+ return mgr->mops->state(mgr);
+}
+
+/*
+ * Unlocked version
+ */
+static int __fpga_mgr_reset(struct fpga_manager *mgr)
+{
+ int ret;
+
+ if (!mgr->mops->reset)
+ return -EINVAL;
+
+ ret = mgr->mops->reset(mgr);
+
+ mgr->state = fpga_mgr_low_level_state(mgr);
+
+ return ret;
+}
+
+int fpga_mgr_reset(struct fpga_manager *mgr)
+{
+ int ret;
+
+ if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
+ return -EBUSY;
+
+ ret = __fpga_mgr_reset(mgr);
+
+ clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_reset);
+
+/*
+ * Unlocked version of fpga_mgr_write function.
+ * Does not touch flags. So caller must grab the FPGA_MGR_BUSY bit.
+ */
+static int __fpga_mgr_write(struct fpga_manager *mgr, const char *buf,
+ size_t count)
+{
+ int ret;
+
+ if (mgr->mops->write_init) {
+ mgr->state = FPGA_MGR_STATE_WRITE_INIT;
+ ret = mgr->mops->write_init(mgr);
+ if (ret) {
+ mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+ return ret;
+ }
+ }
+
+ mgr->state = FPGA_MGR_STATE_WRITE;
+ ret = mgr->mops->write(mgr, buf, count);
+ if (ret) {
+ mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+ return ret;
+ }
+
+ if (mgr->mops->write_complete) {
+ mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
+ ret = mgr->mops->write_complete(mgr);
+ if (ret) {
+ mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
+ return ret;
+ }
+ }
+
+ mgr->state = FPGA_MGR_STATE_OPERATING;
+
+ return 0;
+}
+
+int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+ int ret;
+
+ if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
+ return -EBUSY;
+
+ dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
+
+ ret = __fpga_mgr_write(mgr, buf, count);
+ clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_write);
+
+/*
+ * Grab lock, request firmware, and write out to the FPGA.
+ * Update the state before each step to provide info on what step
+ * failed if there is a failure.
+ */
+int fpga_mgr_firmware_write(struct fpga_manager *mgr, const char *image_name)
+{
+ const struct firmware *fw;
+ int ret;
+
+ if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
+ return -EBUSY;
+
+ dev_info(mgr->dev, "writing %s to %s\n", image_name, mgr->name);
+
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+ ret = request_firmware(&fw, image_name, mgr->dev);
+ if (ret) {
+ mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+ goto fw_wr_exit;
+ }
+
+ ret = __fpga_mgr_write(mgr, fw->data, fw->size);
+ if (ret)
+ goto fw_wr_exit;
+
+ strcpy(mgr->image_name, image_name);
+
+fw_wr_exit:
+ clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_write);
+
+int fpga_mgr_name(struct fpga_manager *mgr, char *buf)
+{
+ if (!mgr)
+ return -ENODEV;
+
+ return sprintf(buf, "%s\n", mgr->name);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_name);
+
+static int fpga_mgr_get_new_minor(struct fpga_manager *mgr, int request_nr)
+{
+ int nr, start;
+
+ /* check specified minor number */
+ if (request_nr >= FPGA_MAX_MINORS) {
+ dev_err(mgr->parent, "Out of device minors (%d)\n", request_nr);
+ return -ENODEV;
+ }
+
+ if (request_nr < -1)
+ return -EINVAL;
+
+ /*
+ * If request_nr == -1, dynamically allocate number.
+ * If request_nr >= 0, attempt to get specific number.
+ */
+ if (request_nr == -1)
+ start = 0;
+ else
+ start = request_nr;
+
+ nr = ida_simple_get(&fpga_mgr_ida, start, FPGA_MAX_MINORS, GFP_KERNEL);
+
+ /* return error code */
+ if (nr < 0)
+ return nr;
+
+ if ((request_nr != -1) && (request_nr != nr)) {
+ dev_err(mgr->parent,
+ "Could not get requested device minor (%d)\n", nr);
+ ida_simple_remove(&fpga_mgr_ida, nr);
+ return -ENODEV;
+ }
+
+ mgr->nr = nr;
+
+ return 0;
+}
+
+static void fpga_mgr_free_minor(int nr)
+{
+ ida_simple_remove(&fpga_mgr_ida, nr);
+}
+
+#if IS_ENABLED(CONFIG_FPGA_MGR_SYSFS)
+const char *state_str[] = {
+ [FPGA_MGR_STATE_UNKNOWN] = "unknown",
+ [FPGA_MGR_STATE_POWER_OFF] = "power_off",
+ [FPGA_MGR_STATE_POWER_UP] = "power_up",
+ [FPGA_MGR_STATE_RESET] = "reset",
+
+ /* write sequence */
+ [FPGA_MGR_STATE_FIRMWARE_REQ] = "firmware_request",
+ [FPGA_MGR_STATE_FIRMWARE_REQ_ERR] = "firmware_request_err",
+ [FPGA_MGR_STATE_WRITE_INIT] = "write_init",
+ [FPGA_MGR_STATE_WRITE_INIT_ERR] = "write_init_err",
+ [FPGA_MGR_STATE_WRITE] = "write",
+ [FPGA_MGR_STATE_WRITE_ERR] = "write_err",
+ [FPGA_MGR_STATE_WRITE_COMPLETE] = "write_complete",
+ [FPGA_MGR_STATE_WRITE_COMPLETE_ERR] = "write_complete_err",
+
+ [FPGA_MGR_STATE_OPERATING] = "operating",
+};
+
+/*
+ * class attributes
+ */
+static ssize_t fpga_mgr_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+ return fpga_mgr_name(mgr, buf);
+}
+
+static ssize_t fpga_mgr_state_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", state_str[mgr->state]);
+}
+
+static ssize_t fpga_mgr_firmware_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", mgr->image_name);
+}
+
+static ssize_t fpga_mgr_firmware_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+ unsigned int len;
+ char image_name[NAME_MAX];
+ int ret;
+
+ /* lose terminating \n */
+ strcpy(image_name, buf);
+ len = strlen(image_name);
+ if (image_name[len - 1] == '\n')
+ image_name[len - 1] = 0;
+
+ ret = fpga_mgr_firmware_write(mgr, image_name);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+/* To reset the fpga, "echo 1 > /sys/.../reset" */
+static ssize_t fpga_mgr_reset_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ if (val == 1) {
+ ret = fpga_mgr_reset(mgr);
+ if (ret)
+ return ret;
+ } else {
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL);
+static DEVICE_ATTR(state, S_IRUGO, fpga_mgr_state_show, NULL);
+static DEVICE_ATTR(firmware, S_IRUGO | S_IWUSR,
+ fpga_mgr_firmware_show, fpga_mgr_firmware_store);
+static DEVICE_ATTR(reset, S_IWUSR, NULL, fpga_mgr_reset_store);
+
+static struct attribute *fpga_mgr_attrs[] = {
+ &dev_attr_name.attr,
+ &dev_attr_state.attr,
+ &dev_attr_firmware.attr,
+ &dev_attr_reset.attr,
+ NULL,
+};
+
+static const struct attribute_group fpga_mgr_group = {
+ .attrs = fpga_mgr_attrs,
+};
+
+const struct attribute_group *fpga_mgr_groups[] = {
+ &fpga_mgr_group,
+ NULL,
+};
+#else
+#define fpga_mgr_groups NULL
+#endif /* CONFIG_FPGA_MGR_SYSFS */
+
+static int fpga_mgr_suspend(struct device *dev)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+ if (!mgr)
+ return -ENODEV;
+
+ if (mgr->mops->suspend)
+ return mgr->mops->suspend(mgr);
+
+ return 0;
+}
+
+static int fpga_mgr_resume(struct device *dev)
+{
+ struct fpga_manager *mgr = dev_get_drvdata(dev);
+ int ret = 0;
+
+ if (!mgr)
+ return -ENODEV;
+
+ if (mgr->mops->resume) {
+ ret = mgr->mops->resume(mgr);
+ if (ret)
+ return ret;
+ }
+
+ if (strlen(mgr->image_name) != 0)
+ fpga_mgr_firmware_write(mgr, mgr->image_name);
+
+ return 0;
+}
+
+const struct dev_pm_ops fpga_mgr_dev_pm_ops = {
+ .suspend = fpga_mgr_suspend,
+ .resume = fpga_mgr_resume,
+};
+
+int fpga_mgr_register(struct platform_device *pdev,
+ struct fpga_manager_ops *mops,
+ const char *name, void *priv)
+{
+ struct fpga_manager *mgr;
+ int ret;
+
+ BUG_ON(!mops || !name || !strlen(name));
+
+ mgr = devm_kzalloc(&pdev->dev, sizeof(struct fpga_manager), GFP_KERNEL);
+ if (!mgr)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, mgr);
+ mgr->mops = mops;
+
+ mgr->np = pdev->dev.of_node;
+ mgr->parent = get_device(&pdev->dev);
+ mgr->priv = priv;
+ mgr->name = name;
+ init_completion(&mgr->status_complete);
+
+ ret = fpga_mgr_get_new_minor(mgr, pdev->id);
+ if (ret)
+ goto error_kfree;
+
+ if (mops->isr) {
+ mgr->irq = irq_of_parse_and_map(mgr->np, 0);
+ if (mgr->irq == NO_IRQ) {
+ dev_err(mgr->parent, "failed to map interrupt\n");
+ goto error_irq_map;
+ }
+
+ ret = request_irq(mgr->irq, mops->isr, 0, "fpga-mgr", mgr);
+ if (ret < 0) {
+ dev_err(mgr->parent, "error requesting interrupt\n");
+ goto error_irq_req;
+ }
+ }
+
+ mgr->dev = device_create(fpga_mgr_class, mgr->parent, MKDEV(0, 0), mgr,
+ "fpga%d", mgr->nr);
+ if (IS_ERR(mgr->dev)) {
+ ret = PTR_ERR(mgr->dev);
+ goto error_device;
+ }
+
+ fpga_mgr_class->pm = &fpga_mgr_dev_pm_ops;
+
+ /*
+ * Initialize framework state by requesting low level driver read state
+ * from device. FPGA may be in reset mode or may have been programmed
+ * by bootloader or EEPROM.
+ */
+ mgr->state = fpga_mgr_low_level_state(mgr);
+
+ dev_info(mgr->parent, "fpga manager [%s] registered as minor %d\n",
+ mgr->name, mgr->nr);
+
+ INIT_LIST_HEAD(&mgr->list);
+ mutex_lock(&fpga_manager_mutex);
+ list_add(&mgr->list, &fpga_manager_list);
+ mutex_unlock(&fpga_manager_mutex);
+
+ return 0;
+
+error_device:
+ free_irq(mgr->irq, mgr);
+error_irq_req:
+ irq_dispose_mapping(mgr->irq);
+error_irq_map:
+ fpga_mgr_free_minor(mgr->nr);
+error_kfree:
+ put_device(mgr->parent);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_register);
+
+void fpga_mgr_remove(struct platform_device *pdev)
+{
+ struct fpga_manager *mgr = platform_get_drvdata(pdev);
+
+ if (!mgr)
+ return;
+
+ if (mgr->mops->fpga_remove)
+ mgr->mops->fpga_remove(mgr);
+
+ mutex_lock(&fpga_manager_mutex);
+ list_del(&mgr->list);
+ mutex_unlock(&fpga_manager_mutex);
+
+ device_destroy(fpga_mgr_class, MKDEV(fpga_mgr_major, mgr->nr));
+ free_irq(mgr->irq, mgr);
+ irq_dispose_mapping(mgr->irq);
+ fpga_mgr_free_minor(mgr->nr);
+ put_device(mgr->parent);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_remove);
+
+static int __init fpga_mgr_dev_init(void)
+{
+ dev_t fpga_mgr_dev;
+ int ret;
+
+ pr_info("FPGA Manager framework driver\n");
+
+ fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
+ if (IS_ERR(fpga_mgr_class))
+ return PTR_ERR(fpga_mgr_class);
+
+ fpga_mgr_class->dev_groups = fpga_mgr_groups;
+
+ ret = alloc_chrdev_region(&fpga_mgr_dev, 0, FPGA_MAX_MINORS,
+ "fpga_manager");
+ if (ret) {
+ class_destroy(fpga_mgr_class);
+ return ret;
+ }
+
+ fpga_mgr_major = MAJOR(fpga_mgr_dev);
+
+ return 0;
+}
+
+static void __exit fpga_mgr_dev_exit(void)
+{
+ unregister_chrdev_region(MKDEV(fpga_mgr_major, 0), FPGA_MAX_MINORS);
+ class_destroy(fpga_mgr_class);
+ ida_destroy(&fpga_mgr_ida);
+}
+
+MODULE_AUTHOR("Alan Tull <[email protected]>");
+MODULE_DESCRIPTION("FPGA Manager framework driver");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_mgr_dev_init);
+module_exit(fpga_mgr_dev_exit);
diff --git a/include/linux/fpga-mgr.h b/include/linux/fpga-mgr.h
new file mode 100644
index 0000000..97c7e0b
--- /dev/null
+++ b/include/linux/fpga-mgr.h
@@ -0,0 +1,115 @@
+/*
+ * FPGA Framework
+ *
+ * Copyright (C) 2013-2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * 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/completion.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/limits.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#ifndef _LINUX_FPGA_MGR_H
+#define _LINUX_FPGA_MGR_H
+
+struct fpga_manager;
+
+/*
+ * fpga_manager_ops are the low level functions implemented by a specific
+ * fpga manager driver. Leaving any of these out that aren't needed is fine
+ * as they are all tested for NULL before being called.
+ */
+struct fpga_manager_ops {
+ /* Returns an enum value of the FPGA's state */
+ int (*state)(struct fpga_manager *mgr);
+
+ /* Put FPGA into reset state */
+ int (*reset)(struct fpga_manager *mgr);
+
+ /* Prepare the FPGA to receive confuration data */
+ int (*write_init)(struct fpga_manager *mgr);
+
+ /* Write count bytes of configuration data to the FPGA */
+ int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+
+ /* Return FPGA to default state after writing is done */
+ int (*write_complete)(struct fpga_manager *mgr);
+
+ /* Optional: Set FPGA into a specific state during driver remove */
+ void (*fpga_remove)(struct fpga_manager *mgr);
+
+ int (*suspend)(struct fpga_manager *mgr);
+
+ int (*resume)(struct fpga_manager *mgr);
+
+ /* FPGA manager isr */
+ irqreturn_t (*isr)(int irq, void *dev_id);
+};
+
+/* flag bits */
+#define FPGA_MGR_BUSY 0
+
+/* Valid states may vary by manufacturer; superset is: */
+enum fpga_mgr_states {
+ FPGA_MGR_STATE_UNKNOWN, /* can't determine state */
+ FPGA_MGR_STATE_POWER_OFF, /* FPGA power is off */
+ FPGA_MGR_STATE_POWER_UP, /* FPGA reports power is up */
+ FPGA_MGR_STATE_RESET, /* FPGA held in reset state */
+
+ /* write sequence */
+ FPGA_MGR_STATE_FIRMWARE_REQ, /* firmware request in progress */
+ FPGA_MGR_STATE_FIRMWARE_REQ_ERR, /* firmware request failed */
+ FPGA_MGR_STATE_WRITE_INIT, /* preparing FPGA for programming */
+ FPGA_MGR_STATE_WRITE_INIT_ERR, /* Error during write_init stage */
+ FPGA_MGR_STATE_WRITE, /* FPGA ready to receive image data */
+ FPGA_MGR_STATE_WRITE_ERR, /* Error while programming FPGA */
+ FPGA_MGR_STATE_WRITE_COMPLETE, /* Doing post programming steps */
+ FPGA_MGR_STATE_WRITE_COMPLETE_ERR, /* Error during write_complete */
+
+ FPGA_MGR_STATE_OPERATING, /* FPGA is programmed and operating */
+};
+
+struct fpga_manager {
+ const char *name;
+ int nr;
+ struct device_node *np;
+ struct device *parent;
+ struct device *dev;
+ struct list_head list;
+ unsigned long flags;
+ enum fpga_mgr_states state;
+ char image_name[NAME_MAX];
+
+ struct fpga_manager_ops *mops;
+ struct completion status_complete;
+ int irq;
+ void *priv;
+};
+
+#if IS_ENABLED(CONFIG_FPGA)
+
+int fpga_mgr_firmware_write(struct fpga_manager *mgr, const char *image_name);
+int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count);
+int fpga_mgr_name(struct fpga_manager *mgr, char *buf);
+int fpga_mgr_reset(struct fpga_manager *mgr);
+int fpga_mgr_register(struct platform_device *pdev,
+ struct fpga_manager_ops *mops,
+ const char *name, void *priv);
+void fpga_mgr_remove(struct platform_device *pdev);
+
+#endif /* CONFIG_FPGA */
+#endif /*_LINUX_FPGA_MGR_H */
--
1.7.9.5
From: Alan Tull <[email protected]>
Support programming the fpga from device tree overlays.
This patch adds one exported function to the core
(fpga-mgr.c): of_fpga_mgr_dev_lookup
Get pointer to fpga manager struct given a phandle.
This code is dependent on Pantelis Antoniou's current work
on Device Tree overlays, a method of dynamically altering
the kernel's live Device Tree. The rest of the patchset was
tested with recent for-next, but this 'bus driver' patch was
tested with Pantelis's and Grant Likely's stuff that is in git
(his git repo https://github.com/pantoniou/linux-beagle-track-mainline
v3.17-rc2-115-gbc778b8 == dt-ng/bbb) plus some dtc fixups
for compiling overlays.
Here's a simple example. Start with:
* the altera-gpio driver built in to the kernel but not in the
device tree.
* raw fpga image at /lib/firmware/soc_system.rbf
* Load appropriate device tree overlay in configfs by doing
$ mkdir /config/device-tree/overlays/1
$ echo socfpga_overlay.dtbo > /config/device-tree/overlays/1/path
* This results in the FPGA getting programmed and the altera
gpio driver getting probed.
* To remove/clear FPGA image by putting the FPGA in reset,
remove device tree overlay by doing:
$ rmdir /config/device-tree/overlays/1
Device tree overlay looks like this:
/dts-v1/;
/plugin/;
/ {
fragment@0 {
target-path="/soc";
__overlay__ {
#address-cells = <1>;
#size-cells = <1>;
bridge@0xff200000 {
compatible = "fpga-mgr-bus", "simple-bus";
reg = <0xff200000 0x200000>;
clocks = <0x2 0x2>;
clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
#address-cells = <0x2>;
#size-cells = <0x1>;
ranges = <0x1 0x10040 0xff210040 0x20>;
fpgamgr = <&hps_0_fpgamgr>;
fpga-firmware = "soc_system.rbf";
gpio@0x100010040 {
compatible = "altr,pio-14.0", "altr,pio-1.0";
reg = <0x1 0x10040 0x20>;
clocks = <0x2>;
altr,gpio-bank-width = <0x4>;
resetvalue = <0x0>;
#gpio-cells = <0x2>;
gpio-controller;
linux,phandle = <0x2d>;
};
};
};
};
};
Signed-off-by: Alan Tull <[email protected]>
v2: simplify of_fpga_mgr_dev_lookup to return NULL on failure
move suspend/resume to fpga-mgr.c core
support 'rmdir' to remove overlay
---
drivers/fpga/Kconfig | 7 +++
drivers/fpga/Makefile | 1 +
drivers/fpga/bus.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/fpga/fpga-mgr.c | 23 +++++++++
include/linux/fpga-mgr.h | 2 +
5 files changed, 157 insertions(+)
create mode 100644 drivers/fpga/bus.c
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index e775b17..2bd6c83 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -18,4 +18,11 @@ config FPGA_MGR_SYSFS
help
FPGA Manager SysFS interface.
+config FPGA_MGR_BUS
+ bool "FPGA Manager Bus"
+ depends on FPGA
+ help
+ FPGA Manager Bus interface. Allows loading FPGA images
+ from Device Tree or from other drivers.
+
endmenu
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index c8a676f..e39911f 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -6,5 +6,6 @@ fpga-mgr-core-y += fpga-mgr.o
# Core FPGA Manager Framework
obj-$(CONFIG_FPGA) += fpga-mgr-core.o
+obj-$(CONFIG_FPGA_MGR_BUS) += bus.o
# FPGA Manager Drivers
diff --git a/drivers/fpga/bus.c b/drivers/fpga/bus.c
new file mode 100644
index 0000000..999346e
--- /dev/null
+++ b/drivers/fpga/bus.c
@@ -0,0 +1,124 @@
+/*
+ * FPGA Manager Bus Driver
+ *
+ * Copyright (C) 2013-2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * 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/completion.h>
+#include <linux/fs.h>
+#include <linux/fpga-mgr.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+struct fpga_mgr_bus_priv {
+ struct device *dev;
+ struct device_node *np;
+ struct fpga_manager *mgr;
+ const char *path;
+};
+
+static int fpga_mgr_bus_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct fpga_mgr_bus_priv *priv;
+ const char *path;
+ struct fpga_manager *mgr;
+ int ret = 0;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ /* Find the FPGA Manager to associate with */
+ mgr = of_fpga_mgr_dev_lookup(np, "fpgamgr");
+ if (!mgr) {
+ dev_err(mgr->dev, "%s could not find fpga mgr\n", __func__);
+ return -ENODEV;
+ }
+
+ /* Find the FPGA image on the firmware path */
+ of_property_read_string(np, "fpga-firmware", &path);
+
+ ret = fpga_mgr_firmware_write(mgr, path);
+ if (ret != 0) {
+ dev_err(mgr->dev, "%s fpga mgr write failure\n", __func__);
+ return -EIO;
+ }
+
+ priv->dev = dev;
+ priv->np = np;
+ priv->mgr = mgr;
+ priv->path = path;
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+/* Called when the Device Tree overlay is removed */
+static int fpga_mgr_bus_remove(struct platform_device *pdev)
+{
+ struct fpga_mgr_bus_priv *priv = platform_get_drvdata(pdev);
+ struct fpga_manager *mgr = priv->mgr;
+
+ return fpga_mgr_reset(mgr);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id fpga_mgr_bus_of_match[] = {
+ { .compatible = "fpga-mgr-bus", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, fpga_mgr_bus_of_match);
+#endif
+
+static struct platform_driver fpga_mgr_bus_driver = {
+ .probe = fpga_mgr_bus_probe,
+ .remove = fpga_mgr_bus_remove,
+ .driver = {
+ .name = "fpga_manager_bus",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(fpga_mgr_bus_of_match),
+ },
+};
+
+static int __init fpga_mgr_bus_init(void)
+{
+ return platform_driver_register(&fpga_mgr_bus_driver);
+}
+
+static void __exit fpga_mgr_bus_exit(void)
+{
+ platform_driver_unregister(&fpga_mgr_bus_driver);
+}
+
+module_init(fpga_mgr_bus_init);
+module_exit(fpga_mgr_bus_exit);
+
+MODULE_AUTHOR("Alan Tull <[email protected]>");
+MODULE_DESCRIPTION("Altera FPGA Manager Bus");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 0c7236a..fed13a6 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -350,6 +350,29 @@ const struct attribute_group *fpga_mgr_groups[] = {
#define fpga_mgr_groups NULL
#endif /* CONFIG_FPGA_MGR_SYSFS */
+/* Find the fpga manager that is pointed to by a phandle */
+struct fpga_manager *of_fpga_mgr_dev_lookup(struct device_node *node,
+ const char *mgr_property)
+{
+ struct fpga_manager *mgr;
+ struct device_node *mgr_node;
+
+ mgr_node = of_parse_phandle(node, mgr_property, 0);
+ if (!mgr_node)
+ return NULL;
+
+ list_for_each_entry(mgr, &fpga_manager_list, list)
+ if (mgr_node == mgr->np) {
+ of_node_put(mgr_node);
+ return mgr;
+ }
+
+ of_node_put(mgr_node);
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(of_fpga_mgr_dev_lookup);
+
static int fpga_mgr_suspend(struct device *dev)
{
struct fpga_manager *mgr = dev_get_drvdata(dev);
diff --git a/include/linux/fpga-mgr.h b/include/linux/fpga-mgr.h
index 97c7e0b..abf2699 100644
--- a/include/linux/fpga-mgr.h
+++ b/include/linux/fpga-mgr.h
@@ -110,6 +110,8 @@ int fpga_mgr_register(struct platform_device *pdev,
struct fpga_manager_ops *mops,
const char *name, void *priv);
void fpga_mgr_remove(struct platform_device *pdev);
+struct fpga_manager *of_fpga_mgr_dev_lookup(struct device_node *node,
+ const char *mgr_property);
#endif /* CONFIG_FPGA */
#endif /*_LINUX_FPGA_MGR_H */
--
1.7.9.5
From: Alan Tull <[email protected]>
Add documentation for new fpga manager sysfs interface.
Signed-off-by: Alan Tull <[email protected]>
---
Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
new file mode 100644
index 0000000..eb600f2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
@@ -0,0 +1,38 @@
+What: /sys/class/fpga_manager/<fpga>/name
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+Description: Name of low level driver.
+
+What: /sys/class/fpga_manager/<fpga>/firmware
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+Description: Name of the FPGA image file to load using firmware class.
+
+What: /sys/class/fpga_manager/<fpga>/reset
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+Description: Write 1 to reset the FPGA
+
+What: /sys/class/fpga_manager/<fpga>/state
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+Description: Read state of fpga framework state machine as a string.
+ Valid states may vary by manufacturer; superset is:
+ * unknown = can't determine state
+ * power_off = FPGA power is off
+ * power_up = FPGA reports power is up
+ * reset = FPGA held in reset state
+ * firmware_request = firmware class request in progress
+ * firmware_request_err = firmware request failed
+ * write_init = FPGA being prepared for programming
+ * write_init_err = Error while preparing FPGA for
+ programming
+ * write = FPGA ready to receive image data
+ * write_err = Error while programming
+ * write_complete = Doing post programming steps
+ * write_complete_err = Error while doing post programming
+ * operating = FPGA is programmed and operating
--
1.7.9.5
On Wed, 22 Oct 2014, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Support programming the fpga from device tree overlays.
>
> This patch adds one exported function to the core
> (fpga-mgr.c): of_fpga_mgr_dev_lookup
> Get pointer to fpga manager struct given a phandle.
>
> This code is dependent on Pantelis Antoniou's current work
> on Device Tree overlays, a method of dynamically altering
> the kernel's live Device Tree. The rest of the patchset was
> tested with recent for-next, but this 'bus driver' patch was
> tested with Pantelis's and Grant Likely's stuff that is in git
> (his git repo https://github.com/pantoniou/linux-beagle-track-mainline
> v3.17-rc2-115-gbc778b8 == dt-ng/bbb) plus some dtc fixups
> for compiling overlays.
>
Since this patch is dependent on a branch that would be hard
to put together, I have pushed a branch. If you happen to
have a cyclone5 board sitting around, you should be able to
load a FPGA from DT overlays using the steps below.
The branch name is 'dt-ng-overlays-v2.2'. The repo is at:
git://git.rocketboards.org/linux-socfpga-next.git
Alan
> Here's a simple example. Start with:
> * the altera-gpio driver built in to the kernel but not in the
> device tree.
> * raw fpga image at /lib/firmware/soc_system.rbf
> * Load appropriate device tree overlay in configfs by doing
> $ mkdir /config/device-tree/overlays/1
> $ echo socfpga_overlay.dtbo > /config/device-tree/overlays/1/path
> * This results in the FPGA getting programmed and the altera
> gpio driver getting probed.
> * To remove/clear FPGA image by putting the FPGA in reset,
> remove device tree overlay by doing:
> $ rmdir /config/device-tree/overlays/1
>
> Device tree overlay looks like this:
> /dts-v1/;
> /plugin/;
> / {
> fragment@0 {
> target-path="/soc";
> __overlay__ {
> #address-cells = <1>;
> #size-cells = <1>;
>
> bridge@0xff200000 {
> compatible = "fpga-mgr-bus", "simple-bus";
> reg = <0xff200000 0x200000>;
> clocks = <0x2 0x2>;
> clock-names = "h2f_lw_axi_clock", "f2h_sdram0_clock";
> #address-cells = <0x2>;
> #size-cells = <0x1>;
> ranges = <0x1 0x10040 0xff210040 0x20>;
>
> fpgamgr = <&hps_0_fpgamgr>;
> fpga-firmware = "soc_system.rbf";
>
> gpio@0x100010040 {
> compatible = "altr,pio-14.0", "altr,pio-1.0";
> reg = <0x1 0x10040 0x20>;
> clocks = <0x2>;
> altr,gpio-bank-width = <0x4>;
> resetvalue = <0x0>;
> #gpio-cells = <0x2>;
> gpio-controller;
> linux,phandle = <0x2d>;
> };
> };
> };
> };
> };
>
> Signed-off-by: Alan Tull <[email protected]>
>
> v2: simplify of_fpga_mgr_dev_lookup to return NULL on failure
> move suspend/resume to fpga-mgr.c core
> support 'rmdir' to remove overlay
> ---
> drivers/fpga/Kconfig | 7 +++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/bus.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
> drivers/fpga/fpga-mgr.c | 23 +++++++++
> include/linux/fpga-mgr.h | 2 +
> 5 files changed, 157 insertions(+)
> create mode 100644 drivers/fpga/bus.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index e775b17..2bd6c83 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -18,4 +18,11 @@ config FPGA_MGR_SYSFS
> help
> FPGA Manager SysFS interface.
>
> +config FPGA_MGR_BUS
> + bool "FPGA Manager Bus"
> + depends on FPGA
> + help
> + FPGA Manager Bus interface. Allows loading FPGA images
> + from Device Tree or from other drivers.
> +
> endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index c8a676f..e39911f 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,5 +6,6 @@ fpga-mgr-core-y += fpga-mgr.o
>
> # Core FPGA Manager Framework
> obj-$(CONFIG_FPGA) += fpga-mgr-core.o
> +obj-$(CONFIG_FPGA_MGR_BUS) += bus.o
>
> # FPGA Manager Drivers
> diff --git a/drivers/fpga/bus.c b/drivers/fpga/bus.c
> new file mode 100644
> index 0000000..999346e
> --- /dev/null
> +++ b/drivers/fpga/bus.c
> @@ -0,0 +1,124 @@
> +/*
> + * FPGA Manager Bus Driver
> + *
> + * Copyright (C) 2013-2014 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * 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/completion.h>
> +#include <linux/fs.h>
> +#include <linux/fpga-mgr.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +struct fpga_mgr_bus_priv {
> + struct device *dev;
> + struct device_node *np;
> + struct fpga_manager *mgr;
> + const char *path;
> +};
> +
> +static int fpga_mgr_bus_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct fpga_mgr_bus_priv *priv;
> + const char *path;
> + struct fpga_manager *mgr;
> + int ret = 0;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + /* Find the FPGA Manager to associate with */
> + mgr = of_fpga_mgr_dev_lookup(np, "fpgamgr");
> + if (!mgr) {
> + dev_err(mgr->dev, "%s could not find fpga mgr\n", __func__);
> + return -ENODEV;
> + }
> +
> + /* Find the FPGA image on the firmware path */
> + of_property_read_string(np, "fpga-firmware", &path);
> +
> + ret = fpga_mgr_firmware_write(mgr, path);
> + if (ret != 0) {
> + dev_err(mgr->dev, "%s fpga mgr write failure\n", __func__);
> + return -EIO;
> + }
> +
> + priv->dev = dev;
> + priv->np = np;
> + priv->mgr = mgr;
> + priv->path = path;
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +/* Called when the Device Tree overlay is removed */
> +static int fpga_mgr_bus_remove(struct platform_device *pdev)
> +{
> + struct fpga_mgr_bus_priv *priv = platform_get_drvdata(pdev);
> + struct fpga_manager *mgr = priv->mgr;
> +
> + return fpga_mgr_reset(mgr);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id fpga_mgr_bus_of_match[] = {
> + { .compatible = "fpga-mgr-bus", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, fpga_mgr_bus_of_match);
> +#endif
> +
> +static struct platform_driver fpga_mgr_bus_driver = {
> + .probe = fpga_mgr_bus_probe,
> + .remove = fpga_mgr_bus_remove,
> + .driver = {
> + .name = "fpga_manager_bus",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(fpga_mgr_bus_of_match),
> + },
> +};
> +
> +static int __init fpga_mgr_bus_init(void)
> +{
> + return platform_driver_register(&fpga_mgr_bus_driver);
> +}
> +
> +static void __exit fpga_mgr_bus_exit(void)
> +{
> + platform_driver_unregister(&fpga_mgr_bus_driver);
> +}
> +
> +module_init(fpga_mgr_bus_init);
> +module_exit(fpga_mgr_bus_exit);
> +
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_DESCRIPTION("Altera FPGA Manager Bus");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 0c7236a..fed13a6 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -350,6 +350,29 @@ const struct attribute_group *fpga_mgr_groups[] = {
> #define fpga_mgr_groups NULL
> #endif /* CONFIG_FPGA_MGR_SYSFS */
>
> +/* Find the fpga manager that is pointed to by a phandle */
> +struct fpga_manager *of_fpga_mgr_dev_lookup(struct device_node *node,
> + const char *mgr_property)
> +{
> + struct fpga_manager *mgr;
> + struct device_node *mgr_node;
> +
> + mgr_node = of_parse_phandle(node, mgr_property, 0);
> + if (!mgr_node)
> + return NULL;
> +
> + list_for_each_entry(mgr, &fpga_manager_list, list)
> + if (mgr_node == mgr->np) {
> + of_node_put(mgr_node);
> + return mgr;
> + }
> +
> + of_node_put(mgr_node);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(of_fpga_mgr_dev_lookup);
> +
> static int fpga_mgr_suspend(struct device *dev)
> {
> struct fpga_manager *mgr = dev_get_drvdata(dev);
> diff --git a/include/linux/fpga-mgr.h b/include/linux/fpga-mgr.h
> index 97c7e0b..abf2699 100644
> --- a/include/linux/fpga-mgr.h
> +++ b/include/linux/fpga-mgr.h
> @@ -110,6 +110,8 @@ int fpga_mgr_register(struct platform_device *pdev,
> struct fpga_manager_ops *mops,
> const char *name, void *priv);
> void fpga_mgr_remove(struct platform_device *pdev);
> +struct fpga_manager *of_fpga_mgr_dev_lookup(struct device_node *node,
> + const char *mgr_property);
>
> #endif /* CONFIG_FPGA */
> #endif /*_LINUX_FPGA_MGR_H */
> --
> 1.7.9.5
>
>
Hi!
> * /sys/class/fpga_manager/<fpga>/firmware
> Name of FPGA image file to load using firmware class.
> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
I .. still don't think this is good idea. What about namespaces?
The path corresponds to path in which namespace?
> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
> +
> + ret = __fpga_mgr_write(mgr, buf, count);
> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
Is the EBUSY -- userspace please try again, but you don't know when to
try again -- right interface? I mean, normally kernel would wait, so
that userland does not have to poll?
> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + unsigned int len;
> + char image_name[NAME_MAX];
> + int ret;
> +
> + /* lose terminating \n */
> + strcpy(image_name, buf);
> + len = strlen(image_name);
> + if (image_name[len - 1] == '\n')
> + image_name[len - 1] = 0;
> +
> + ret = fpga_mgr_firmware_write(mgr, image_name);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
This shows why the interface is not right... Valid filename may
contain \n, right? It may even end with \n.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel,
> On Oct 24, 2014, at 1:52 PM, Pavel Machek <[email protected]> wrote:
>
> Hi!
>
>> * /sys/class/fpga_manager/<fpga>/firmware
>> Name of FPGA image file to load using firmware class.
>> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>
> I .. still don't think this is good idea. What about namespaces?
> The path corresponds to path in which namespace?
>
FWIW the overlays patchset uses binary configfs attribute to make this work.
>> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>> +{
>> + int ret;
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
>> +
>> + ret = __fpga_mgr_write(mgr, buf, count);
>> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
>
> Is the EBUSY -- userspace please try again, but you don't know when to
> try again -- right interface? I mean, normally kernel would wait, so
> that userland does not have to poll?
>
>> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + unsigned int len;
>> + char image_name[NAME_MAX];
>> + int ret;
>> +
>> + /* lose terminating \n */
>> + strcpy(image_name, buf);
>> + len = strlen(image_name);
>> + if (image_name[len - 1] == '\n')
>> + image_name[len - 1] = 0;
>> +
>> + ret = fpga_mgr_firmware_write(mgr, image_name);
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>
> This shows why the interface is not right... Valid filename may
> contain \n, right? It may even end with \n.
>
I could argue that a valid firmware file is one that’s well formed and does not
contain those characters.
I guess this is only for make echo file >foo work. You could specify that echo -n file >foo is
required.
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Regards
— Pantelis
On Fri, 24 Oct 2014, Pantelis Antoniou wrote:
> Hi Pavel,
>
> > On Oct 24, 2014, at 1:52 PM, Pavel Machek <[email protected]> wrote:
> >
> > Hi!
> >
> >> * /sys/class/fpga_manager/<fpga>/firmware
> >> Name of FPGA image file to load using firmware class.
> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
> >
> > I .. still don't think this is good idea. What about namespaces?
> > The path corresponds to path in which namespace?
> >
Hi Pavel,
Sorry if my documentation is too brief here. The context is the
firmware class. The 'image.rbf' is a image file that is located
on the filesystem in the firmware search patch.
See Documentation/firmware_class/README.
Basically we need some way of loading a FPGA image to the FPGA.
I don't think any one way is going to meet everybody's needs so
I wanted to export enough functions from fpga-mgr.c that other
interfaces can by built. I think firmware will work just fine
for some people and it is great in that it already exists in the
kernel.
If I missed your point, please let me know.
>
> FWIW the overlays patchset uses binary configfs attribute to make this work.
>
Hi Pantelis,
Yes I think you've mentioned that before. So that would be another
way of giving a device tree overlay to configfs, right? I should
that to the documentation in the patch header.
> >> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
> >> +{
> >> + int ret;
> >> +
> >> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
> >> + return -EBUSY;
> >> +
> >> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
> >> +
> >> + ret = __fpga_mgr_write(mgr, buf, count);
> >> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
> >
> > Is the EBUSY -- userspace please try again, but you don't know when to
> > try again -- right interface? I mean, normally kernel would wait, so
> > that userland does not have to poll?
> >
Yes, for fpga_mgr_write it may be more useful for this to be
a blocking call.
> >> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> >> + unsigned int len;
> >> + char image_name[NAME_MAX];
> >> + int ret;
> >> +
> >> + /* lose terminating \n */
> >> + strcpy(image_name, buf);
> >> + len = strlen(image_name);
> >> + if (image_name[len - 1] == '\n')
> >> + image_name[len - 1] = 0;
> >> +
> >> + ret = fpga_mgr_firmware_write(mgr, image_name);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return count;
> >> +}
> >
> > This shows why the interface is not right... Valid filename may
> > contain \n, right? It may even end with \n.
> >
>
> I could argue that a valid firmware file is one that’s well formed and does not
> contain those characters.
>
> I guess this is only for make echo file >foo work. You could specify that echo -n file >foo is
> required.
>
I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
while I am a bonehead and forget the '-n' and spend a few minutes
wondering why this thing that worked last week suddenly rejects all
commands. I'm just trying to make my user interface a bit user-friendly.
I will take out the '\n' stripping and update the documentation. I didn't
realize this would be controversial.
Alan
>
> > Best regards,
> > Pavel
> > --
> > (english) http://www.livejournal.com/~pavelmachek
> > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>
> Regards
>
> — Pantelis
>
>
> > +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
> > +{
> > + int ret;
> > +
> > + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
> > + return -EBUSY;
> > +
> > + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
> > +
> > + ret = __fpga_mgr_write(mgr, buf, count);
> > + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_mgr_write);
>
> Is the EBUSY -- userspace please try again, but you don't know when to
> try again -- right interface? I mean, normally kernel would wait, so
> that userland does not have to poll?
And if userland has to also support polling you must support poll/select
on the file handle so it knows when to retry the I/O.
The error should be EAGAIN for a retriable write with O_NDELAY set.
Alan
On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
> Hi!
>
>> * /sys/class/fpga_manager/<fpga>/firmware
>> Name of FPGA image file to load using firmware class.
>> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>
> I .. still don't think this is good idea. What about namespaces?
> The path corresponds to path in which namespace?
I don't understand your concern here. This allows userspace to name
the FPGA bitstream that the kernel will use during request_firmware(),
and it will show up as the $FIRMWARE value in the uevent file, but it
is still the responsibility of userspace to choose what to load, and
it can freely ignore the setting of $FIRMWARE if it needs to.
The process that actually loads the firmware into the kernel pretty
much has to run in the normal linux environment. How do namespaces
come into it? What exact problem do you see?
>
>> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>> +{
>> + int ret;
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
>> +
>> + ret = __fpga_mgr_write(mgr, buf, count);
>> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
>
> Is the EBUSY -- userspace please try again, but you don't know when to
> try again -- right interface? I mean, normally kernel would wait, so
> that userland does not have to poll?
Custom locking schemes are just wrong. A mutex is the right thing to
do here and then an -EBUSY isn't required.
>
>> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + unsigned int len;
>> + char image_name[NAME_MAX];
Hard coding a string length is a warning sign. That is the sort of
thing that can memdup() or strdup() can handle.
>> + int ret;
>> +
>> + /* lose terminating \n */
>> + strcpy(image_name, buf);
>> + len = strlen(image_name);
>> + if (image_name[len - 1] == '\n')
>> + image_name[len - 1] = 0;
>> +
>> + ret = fpga_mgr_firmware_write(mgr, image_name);
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>
> This shows why the interface is not right... Valid filename may
> contain \n, right? It may even end with \n.
That's not an interface problem, it implementation problem. The
function absolutely should scrub and validate it's input. It should
also make sure that the string doesn't have any special characters so
that /bin/sh doesn't barf on it (because the string will appear in the
uevent file). I would check other users of request_firmware() to see
if any of them allow userspace to specify the filename.
That said, the other way to handle this is not to specify a valid
filename through this interface at all. Just use a dummy placeholder
name and expect userspace to load the correct file when the request is
posted.
g.
On Fri, Oct 24, 2014 at 3:54 PM, atull <[email protected]> wrote:
> On Fri, 24 Oct 2014, Pantelis Antoniou wrote:
>
>> Hi Pavel,
>>
>> > On Oct 24, 2014, at 1:52 PM, Pavel Machek <[email protected]> wrote:
>> >
>> > Hi!
>> >
>> >> * /sys/class/fpga_manager/<fpga>/firmware
>> >> Name of FPGA image file to load using firmware class.
>> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>> >
>> > I .. still don't think this is good idea. What about namespaces?
>> > The path corresponds to path in which namespace?
>> >
>
> Hi Pavel,
>
> Sorry if my documentation is too brief here. The context is the
> firmware class. The 'image.rbf' is a image file that is located
> on the filesystem in the firmware search patch.
> See Documentation/firmware_class/README.
>
> Basically we need some way of loading a FPGA image to the FPGA.
> I don't think any one way is going to meet everybody's needs so
> I wanted to export enough functions from fpga-mgr.c that other
> interfaces can by built. I think firmware will work just fine
> for some people and it is great in that it already exists in the
> kernel.
>
> If I missed your point, please let me know.
>
>>
>> FWIW the overlays patchset uses binary configfs attribute to make this work.
>>
>
> Hi Pantelis,
>
> Yes I think you've mentioned that before. So that would be another
> way of giving a device tree overlay to configfs, right? I should
> that to the documentation in the patch header.
>
>> >> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>> >> +{
>> >> + int ret;
>> >> +
>> >> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
>> >> + return -EBUSY;
>> >> +
>> >> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
>> >> +
>> >> + ret = __fpga_mgr_write(mgr, buf, count);
>> >> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
>> >> +
>> >> + return ret;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
>> >
>> > Is the EBUSY -- userspace please try again, but you don't know when to
>> > try again -- right interface? I mean, normally kernel would wait, so
>> > that userland does not have to poll?
>> >
>
> Yes, for fpga_mgr_write it may be more useful for this to be
> a blocking call.
>
>> >> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
>> >> + struct device_attribute *attr,
>> >> + const char *buf, size_t count)
>> >> +{
>> >> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> >> + unsigned int len;
>> >> + char image_name[NAME_MAX];
>> >> + int ret;
>> >> +
>> >> + /* lose terminating \n */
>> >> + strcpy(image_name, buf);
>> >> + len = strlen(image_name);
>> >> + if (image_name[len - 1] == '\n')
>> >> + image_name[len - 1] = 0;
>> >> +
>> >> + ret = fpga_mgr_firmware_write(mgr, image_name);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + return count;
>> >> +}
>> >
>> > This shows why the interface is not right... Valid filename may
>> > contain \n, right? It may even end with \n.
>> >
>>
>> I could argue that a valid firmware file is one that's well formed and does not
>> contain those characters.
>>
>> I guess this is only for make echo file >foo work. You could specify that echo -n file >foo is
>> required.
>>
>
> I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
> while I am a bonehead and forget the '-n' and spend a few minutes
> wondering why this thing that worked last week suddenly rejects all
> commands. I'm just trying to make my user interface a bit user-friendly.
>
> I will take out the '\n' stripping and update the documentation. I didn't
> realize this would be controversial.
Don't. You're doing the right thing by scrubbing your input. Requiring
'echo -n' is just stupid when it is so easy to make work easily.
g.
Hi!
> > I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
> > while I am a bonehead and forget the '-n' and spend a few minutes
> > wondering why this thing that worked last week suddenly rejects all
> > commands. I'm just trying to make my user interface a bit user-friendly.
> >
> > I will take out the '\n' stripping and update the documentation. I didn't
> > realize this would be controversial.
>
> Don't. You're doing the right thing by scrubbing your input. Requiring
> 'echo -n' is just stupid when it is so easy to make work easily.
'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
echo filenames into files in the first place... and arbitrarily
disallowing certain filenames is not helping.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat 2014-12-06 13:00:17, Grant Likely wrote:
> On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> >> * /sys/class/fpga_manager/<fpga>/firmware
> >> Name of FPGA image file to load using firmware class.
> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
> >
> > I .. still don't think this is good idea. What about namespaces?
> > The path corresponds to path in which namespace?
>
> I don't understand your concern here. This allows userspace to name
> the FPGA bitstream that the kernel will use during request_firmware(),
> and it will show up as the $FIRMWARE value in the uevent file, but it
> is still the responsibility of userspace to choose what to load, and
> it can freely ignore the setting of $FIRMWARE if it needs to.
Well, consider chroot. I echo some filename but the file does not
exist for the userspace listening for the uevent.
> The process that actually loads the firmware into the kernel pretty
> much has to run in the normal linux environment. How do namespaces
> come into it? What exact problem do you see?
> > This shows why the interface is not right... Valid filename may
> > contain \n, right? It may even end with \n.
>
> That's not an interface problem, it implementation problem. The
> function absolutely should scrub and validate it's input. It should
> also make sure that the string doesn't have any special characters so
> that /bin/sh doesn't barf on it (because the string will appear in the
> uevent file). I would check other users of request_firmware() to see
> if any of them allow userspace to specify the filename.
> That said, the other way to handle this is not to specify a valid
> filename through this interface at all. Just use a dummy placeholder
> name and expect userspace to load the correct file when the request is
> posted.
I'd go for that -- "dummy" works. Kernel is not a place to know shell
limitations for all possible shells.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, 6 Dec 2014 14:55:33 +0100
, Pavel Machek <[email protected]>
wrote:
> Hi!
>
> > > I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
> > > while I am a bonehead and forget the '-n' and spend a few minutes
> > > wondering why this thing that worked last week suddenly rejects all
> > > commands. I'm just trying to make my user interface a bit user-friendly.
> > >
> > > I will take out the '\n' stripping and update the documentation. I didn't
> > > realize this would be controversial.
> >
> > Don't. You're doing the right thing by scrubbing your input. Requiring
> > 'echo -n' is just stupid when it is so easy to make work easily.
>
> 'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
> echo filenames into files in the first place... and arbitrarily
> disallowing certain filenames is not helping.
Meh. Just because it is a valid linux filename doesn't mean this
interface is forced to accept it. There should be tighter rules about
how the filename can be constructed. Allowing any arbitrary path for any
arbitrary valid linux filename makes for a large attack surface.
I would like to know, what is the purpose of the interface? Why is it
important to provide the firmware filename in this manor? Are there
going to be a lot of different FPGA bitstreams that may need to be
loaded? How does userspace choose between them, and is there a better
way to do the selection without passing the firmware filename through
the kernel? Is this merely intended to get udev to behave in a certain
way? If so, then maybe there is a better way to go about it.
We could for example use a UDEV 'PROGRAM=' rule to execute a userspace
app and have that program figure out which firmware file to provide to
the kernel.
g.
Hi Grant,
> On Dec 8, 2014, at 19:50 , Grant Likely <[email protected]> wrote:
>
> On Sat, 6 Dec 2014 14:55:33 +0100
> , Pavel Machek <[email protected]>
> wrote:
>> Hi!
>>
>>>> I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
>>>> while I am a bonehead and forget the '-n' and spend a few minutes
>>>> wondering why this thing that worked last week suddenly rejects all
>>>> commands. I'm just trying to make my user interface a bit user-friendly.
>>>>
>>>> I will take out the '\n' stripping and update the documentation. I didn't
>>>> realize this would be controversial.
>>>
>>> Don't. You're doing the right thing by scrubbing your input. Requiring
>>> 'echo -n' is just stupid when it is so easy to make work easily.
>>
>> 'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
>> echo filenames into files in the first place... and arbitrarily
>> disallowing certain filenames is not helping.
>
> Meh. Just because it is a valid linux filename doesn't mean this
> interface is forced to accept it. There should be tighter rules about
> how the filename can be constructed. Allowing any arbitrary path for any
> arbitrary valid linux filename makes for a large attack surface.
>
> I would like to know, what is the purpose of the interface? Why is it
> important to provide the firmware filename in this manor? Are there
> going to be a lot of different FPGA bitstreams that may need to be
> loaded? How does userspace choose between them, and is there a better
> way to do the selection without passing the firmware filename through
> the kernel? Is this merely intended to get udev to behave in a certain
> way? If so, then maybe there is a better way to go about it.
>
> We could for example use a UDEV 'PROGRAM=' rule to execute a userspace
> app and have that program figure out which firmware file to provide to
> the kernel.
>
Please, lets not depend at all on udev. Firmware can be embedded in the kernel
image, which is pretty important to work before userland if you have to have
the rootfs on a device that’s instantiated via an FPGA bitstream.
> g.
Regards
— Pantelis
On Mon, Dec 8, 2014 at 5:50 PM, Grant Likely <[email protected]> wrote:
> On Sat, 6 Dec 2014 14:55:33 +0100
> , Pavel Machek <[email protected]>
> wrote:
>> Hi!
>>
>> > > I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
>> > > while I am a bonehead and forget the '-n' and spend a few minutes
>> > > wondering why this thing that worked last week suddenly rejects all
>> > > commands. I'm just trying to make my user interface a bit user-friendly.
>> > >
>> > > I will take out the '\n' stripping and update the documentation. I didn't
>> > > realize this would be controversial.
>> >
>> > Don't. You're doing the right thing by scrubbing your input. Requiring
>> > 'echo -n' is just stupid when it is so easy to make work easily.
>>
>> 'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
>> echo filenames into files in the first place... and arbitrarily
>> disallowing certain filenames is not helping.
>
> Meh. Just because it is a valid linux filename doesn't mean this
> interface is forced to accept it. There should be tighter rules about
> how the filename can be constructed. Allowing any arbitrary path for any
> arbitrary valid linux filename makes for a large attack surface.
>
> I would like to know, what is the purpose of the interface? Why is it
> important to provide the firmware filename in this manor? Are there
> going to be a lot of different FPGA bitstreams that may need to be
> loaded? How does userspace choose between them, and is there a better
> way to do the selection without passing the firmware filename through
> the kernel? Is this merely intended to get udev to behave in a certain
> way? If so, then maybe there is a better way to go about it.
>
> We could for example use a UDEV 'PROGRAM=' rule to execute a userspace
> app and have that program figure out which firmware file to provide to
> the kernel.
Correction, a 'RUN+=' rule is actually what I mean here. The PROGRAM=
tag is for executing a program to determine if a device matches.
g.
On Mon, Dec 8, 2014 at 5:56 PM, Pantelis Antoniou
<[email protected]> wrote:
> Hi Grant,
>
>> On Dec 8, 2014, at 19:50 , Grant Likely <[email protected]> wrote:
>>
>> On Sat, 6 Dec 2014 14:55:33 +0100
>> , Pavel Machek <[email protected]>
>> wrote:
>>> Hi!
>>>
>>>>> I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
>>>>> while I am a bonehead and forget the '-n' and spend a few minutes
>>>>> wondering why this thing that worked last week suddenly rejects all
>>>>> commands. I'm just trying to make my user interface a bit user-friendly.
>>>>>
>>>>> I will take out the '\n' stripping and update the documentation. I didn't
>>>>> realize this would be controversial.
>>>>
>>>> Don't. You're doing the right thing by scrubbing your input. Requiring
>>>> 'echo -n' is just stupid when it is so easy to make work easily.
>>>
>>> 'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
>>> echo filenames into files in the first place... and arbitrarily
>>> disallowing certain filenames is not helping.
>>
>> Meh. Just because it is a valid linux filename doesn't mean this
>> interface is forced to accept it. There should be tighter rules about
>> how the filename can be constructed. Allowing any arbitrary path for any
>> arbitrary valid linux filename makes for a large attack surface.
>>
>> I would like to know, what is the purpose of the interface? Why is it
>> important to provide the firmware filename in this manor? Are there
>> going to be a lot of different FPGA bitstreams that may need to be
>> loaded? How does userspace choose between them, and is there a better
>> way to do the selection without passing the firmware filename through
>> the kernel? Is this merely intended to get udev to behave in a certain
>> way? If so, then maybe there is a better way to go about it.
>>
>> We could for example use a UDEV 'PROGRAM=' rule to execute a userspace
>> app and have that program figure out which firmware file to provide to
>> the kernel.
>>
>
> Please, lets not depend at all on udev. Firmware can be embedded in the kernel
> image, which is pretty important to work before userland if you have to have
> the rootfs on a device that's instantiated via an FPGA bitstream.
It sounds like there are two issues here. One is the loading of a
default image (like when built into the kernel), and the other is
selecting between multiple firmware files, but the kernel won't know
which without either userspace or firmware assistance. For the second
issue is sounds to me like the rules for choosing between multiple
images needs to be well defined. Echoing an arbitrary filename into
the sysfs interface is too loosely defined.
g.
On 12/08/2014 11:50 AM, Grant Likely wrote:
> On Sat, 6 Dec 2014 14:55:33 +0100
> , Pavel Machek <[email protected]>
> wrote:
>> Hi!
>>
>>>> I am accustomed to doing 'echo -n' for most of sysfs anyway. Once in a
>>>> while I am a bonehead and forget the '-n' and spend a few minutes
>>>> wondering why this thing that worked last week suddenly rejects all
>>>> commands. I'm just trying to make my user interface a bit user-friendly.
>>>>
>>>> I will take out the '\n' stripping and update the documentation. I didn't
>>>> realize this would be controversial.
>>>
>>> Don't. You're doing the right thing by scrubbing your input. Requiring
>>> 'echo -n' is just stupid when it is so easy to make work easily.
>>
>> 'foo\nbar\n' is unusual but valid filename in linux. It is bad idea to
>> echo filenames into files in the first place... and arbitrarily
>> disallowing certain filenames is not helping.
>
> Meh. Just because it is a valid linux filename doesn't mean this
> interface is forced to accept it. There should be tighter rules about
> how the filename can be constructed. Allowing any arbitrary path for any
> arbitrary valid linux filename makes for a large attack surface.
"echo /bin/mdev > /proc/sys/kernel/hotplug" has worked fine, without the
-n, for most of a decade now. Requiring -n on echo would be weird.
I note that the filenames in /proc/mounts have an escape syntax for
arbitrary embedded weirdness. (To see it in action, mount a path with a
space in it.) If you really want to support that, there's presumably
code that can be genericized and reappropriated so you can escape a
newline you want to keep. But if your input has a normal unescaped
trailing newline, it _should_ be ignored.
Rob
On Sat, 6 Dec 2014 13:00:17 +0000
Grant Likely <[email protected]> wrote:
> On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
> > Hi!
> >
> >> * /sys/class/fpga_manager/<fpga>/firmware
> >> Name of FPGA image file to load using firmware class.
> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
> >
> > I .. still don't think this is good idea. What about namespaces?
> > The path corresponds to path in which namespace?
>
> I don't understand your concern here. This allows userspace to name
> the FPGA bitstream that the kernel will use during request_firmware(),
> and it will show up as the $FIRMWARE value in the uevent file, but it
> is still the responsibility of userspace to choose what to load, and
> it can freely ignore the setting of $FIRMWARE if it needs to.
I think the entire model here is basically pedicated on a bogus
assumption that an FPGA is a one shot device. It's not. It's a fast
reloadable reusable device. A lot of work being done with FPGAs in
operating systems already involves basically task switching and
scheduling FPGAs as a shared resource pool. Trying to nail something
together with request_firmware is several years behind the curve.
>From userspace it needs to be a open, load, use, close type model, not a
static or semi-static pile of mappings.
Alan
On Mon, Dec 8, 2014 at 10:55 PM, One Thousand Gnomes
<[email protected]> wrote:
> On Sat, 6 Dec 2014 13:00:17 +0000
> Grant Likely <[email protected]> wrote:
>
>> On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
>> > Hi!
>> >
>> >> * /sys/class/fpga_manager/<fpga>/firmware
>> >> Name of FPGA image file to load using firmware class.
>> >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>> >
>> > I .. still don't think this is good idea. What about namespaces?
>> > The path corresponds to path in which namespace?
>>
>> I don't understand your concern here. This allows userspace to name
>> the FPGA bitstream that the kernel will use during request_firmware(),
>> and it will show up as the $FIRMWARE value in the uevent file, but it
>> is still the responsibility of userspace to choose what to load, and
>> it can freely ignore the setting of $FIRMWARE if it needs to.
>
> I think the entire model here is basically pedicated on a bogus
> assumption that an FPGA is a one shot device. It's not. It's a fast
> reloadable reusable device. A lot of work being done with FPGAs in
> operating systems already involves basically task switching and
> scheduling FPGAs as a shared resource pool. Trying to nail something
> together with request_firmware is several years behind the curve.
>
> From userspace it needs to be a open, load, use, close type model, not a
> static or semi-static pile of mappings.
If FPGA is a general purpose resource hanging off the side that
applications can use, then sure, but the majority of FPGA usage does
not fall into that scenario*. The majority of FPGA usage that I've
seen has core parts of the system implemented in the FPGA fabric.
Video pipelines, network switching, dma to/from main memory, control
of dedicated hardware. It's more than merely an application being able
to use the FPGA as an accelerator.
I'm certainly not dismissing the concept of FPGA scheduling and being
able to 'task switch' between bitstreams. Yes that is important, but
for most users it really does look like, as you say, "a static or
semi-static pile of mappings".
* Altera and Xilinx people - correct me if you disagree.
On 12/09/2014 02:11 PM, Grant Likely wrote:
> On Mon, Dec 8, 2014 at 10:55 PM, One Thousand Gnomes
> <[email protected]> wrote:
>> On Sat, 6 Dec 2014 13:00:17 +0000
>> Grant Likely <[email protected]> wrote:
>>
>>> On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
>>>> Hi!
>>>>
>>>>> * /sys/class/fpga_manager/<fpga>/firmware
>>>>> Name of FPGA image file to load using firmware class.
>>>>> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>>>>
>>>> I .. still don't think this is good idea. What about namespaces?
>>>> The path corresponds to path in which namespace?
>>>
>>> I don't understand your concern here. This allows userspace to name
>>> the FPGA bitstream that the kernel will use during request_firmware(),
>>> and it will show up as the $FIRMWARE value in the uevent file, but it
>>> is still the responsibility of userspace to choose what to load, and
>>> it can freely ignore the setting of $FIRMWARE if it needs to.
>>
>> I think the entire model here is basically pedicated on a bogus
>> assumption that an FPGA is a one shot device. It's not. It's a fast
>> reloadable reusable device. A lot of work being done with FPGAs in
>> operating systems already involves basically task switching and
>> scheduling FPGAs as a shared resource pool. Trying to nail something
>> together with request_firmware is several years behind the curve.
>>
>> From userspace it needs to be a open, load, use, close type model, not a
>> static or semi-static pile of mappings.
>
> If FPGA is a general purpose resource hanging off the side that
> applications can use, then sure, but the majority of FPGA usage does
> not fall into that scenario*. The majority of FPGA usage that I've
> seen has core parts of the system implemented in the FPGA fabric.
> Video pipelines, network switching, dma to/from main memory, control
> of dedicated hardware. It's more than merely an application being able
> to use the FPGA as an accelerator.
>
> I'm certainly not dismissing the concept of FPGA scheduling and being
> able to 'task switch' between bitstreams. Yes that is important, but
> for most users it really does look like, as you say, "a static or
> semi-static pile of mappings".
>
> * Altera and Xilinx people - correct me if you disagree.
We need to start to walk before we can run. We should move the part of this
patch series to drivers/staging directory. Greg is OK with that and
start to adding drivers for current drivers.
Static use case is good start and we should move forward not to stay in circle.
I have some changes to this series internally which should be done.
Please give me some days to finish other work and we can move.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Mon, 8 Dec 2014, One Thousand Gnomes wrote:
> On Sat, 6 Dec 2014 13:00:17 +0000
> Grant Likely <[email protected]> wrote:
>
> > On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <[email protected]> wrote:
> > > Hi!
> > >
> > >> * /sys/class/fpga_manager/<fpga>/firmware
> > >> Name of FPGA image file to load using firmware class.
> > >> $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
> > >
> > > I .. still don't think this is good idea. What about namespaces?
> > > The path corresponds to path in which namespace?
> >
> > I don't understand your concern here. This allows userspace to name
> > the FPGA bitstream that the kernel will use during request_firmware(),
> > and it will show up as the $FIRMWARE value in the uevent file, but it
> > is still the responsibility of userspace to choose what to load, and
> > it can freely ignore the setting of $FIRMWARE if it needs to.
>
> I think the entire model here is basically pedicated on a bogus
> assumption that an FPGA is a one shot device. It's not. It's a fast
> reloadable reusable device. A lot of work being done with FPGAs in
> operating systems already involves basically task switching and
> scheduling FPGAs as a shared resource pool. Trying to nail something
> together with request_firmware is several years behind the curve.
>
> From userspace it needs to be a open, load, use, close type model, not a
> static or semi-static pile of mappings.
>
> Alan
>
Hi Alan,
I agree with the view that a FPGA is something that can get reprogrammed a lot.
That's a flexibility we want to use. I don't see a problem with using firmware
to do the programming as long as we have a lightweight interface where we can
load an image, use it, then later reset the FGPA and load a different image
instead.
This assumes that the system will have a pile of FPGA images sitting on
the filesystem for us to switch between.
My intent is to also support loading using device tree overlays. This is a lot
more linux-like and less of something just bolted on. The flow here is:
* load a DT overlay
* this causes the fpga to get programmed
* appropriate bridges get enabled
* appropriate drivers get probed
When the DT overlay is removed, all these get undone in the reverse order.
Alan Tull
> I agree with the view that a FPGA is something that can get reprogrammed a lot.
> That's a flexibility we want to use. I don't see a problem with using firmware
> to do the programming as long as we have a lightweight interface where we can
> load an image, use it, then later reset the FGPA and load a different image
> instead.
>
> This assumes that the system will have a pile of FPGA images sitting on
> the filesystem for us to switch between.
>
> My intent is to also support loading using device tree overlays. This is a lot
> more linux-like and less of something just bolted on. The flow here is:
>
> * load a DT overlay
Don't assume DT. The entire world doesn't run DT
> * this causes the fpga to get programmed
> * appropriate bridges get enabled
> * appropriate drivers get probed
For the case of a fixed function device it's sort of equivalent to a
firmware load (in fact it *is* just a firmware load). The fixed function
cases don't actually even need a 'firmware manager' or an FPGA class. In
fact they shouldn't IMHO have one because the fact version A of the
device requires firmware bitstream X, and bitstream X is an altera FPGA
bitstream is an implementation detail. Revision B could be a
microcontroller or something else and you still just shove a bitstream
down it. No FPGA class is needed or appropriate. FPGA loader helpers yes.
In the enterprise space the model for FPGA use is usually a lot more
flexible, big racks of FPGA boards that are handed out as resources to
processes. They may be uploading fixed bitstreams but they may also be
splicing bitstreams (eg splicing in 'ROM' images) and in the future as
the Chinese break the existing FPGA market up I imagine we'll even see
open bitstream formats.
In the smaller system world emulators, real time and all sorts of maker
type projects use the FPGA boards as a dynamic resource already. It might
be running GNU radio, then driving a 3D printer, then doing processor
emulation for a games console. The FPGAs hanging off my desktop box have
been all sorts of things from video processors to emulated systems and
even block drivers for weird recalcitrant hardware. Next stop may well be
Localtalk 8)
In the academic world the model is similar, they are being treated as OS
resources by the various reconfigurable OS projects, most of which are
themselves Linux patch sets.
IMHO we have two use cases
1. Fixed function firmware - in which case the driver already handles it
and we don't care if its FPGA bitstreams or microcode or CPU code or
whatever
2. Dynamic use cases where we need a resource we own, which means
enumerate/open/close/read/write interfaces including firmware.
For use case #1 I don't believe we need magic classes for FPGA and in
fact they are actually a mistake, for use class #2 request_firmware()
doesn't work because it's intentionally quite blind to things like
namespaces and permissions models. Both benefit greatly from library
functions to handle the more standardised uploaded mechanisms.
I agree entirely with Michael about putting it in staging and working
from there.
Alan
On Tue, 9 Dec 2014, One Thousand Gnomes wrote:
Hi Alan,
> > I agree with the view that a FPGA is something that can get reprogrammed a lot.
> > That's a flexibility we want to use. I don't see a problem with using firmware
> > to do the programming as long as we have a lightweight interface where we can
> > load an image, use it, then later reset the FGPA and load a different image
> > instead.
> >
> > This assumes that the system will have a pile of FPGA images sitting on
> > the filesystem for us to switch between.
> >
> > My intent is to also support loading using device tree overlays. This is a lot
> > more linux-like and less of something just bolted on. The flow here is:
> >
> > * load a DT overlay
>
> Don't assume DT. The entire world doesn't run DT
There's all kinds of things I'm not assuming here. But at the same time I want
this to be widely useful and to not set general policies for proper FPGA use.
This patchset provides a sysfs interface for doing fpga image programming.
So I'm not assuming DT always. Also I am exporting some functions that can be
used to expose some other interface if you want. Those functions can load the
FPGA given either a buffer or a chunk of a buffer or a name of a FPGA file.
So I'm looking to enable broad use while still having a framework that is
quite lightweight.
We aren't *required* ignore DT either, are we? Exciting things are happening
with DT, now that it is starting to not be a static thing.
>
> > * this causes the fpga to get programmed
> > * appropriate bridges get enabled
> > * appropriate drivers get probed
>
> For the case of a fixed function device it's sort of equivalent to a
> firmware load (in fact it *is* just a firmware load). The fixed function
> cases don't actually even need a 'firmware manager' or an FPGA class. In
> fact they shouldn't IMHO have one because the fact version A of the
> device requires firmware bitstream X, and bitstream X is an altera FPGA
> bitstream is an implementation detail. Revision B could be a
> microcontroller or something else and you still just shove a bitstream
> down it. No FPGA class is needed or appropriate. FPGA loader helpers yes.
There are lots of FPGA use cases. Yes, a production use case could involve
the FPGA being loaded by the boot loader or from EEPROM. These patches
have nothing to do with that.
I'm not trying to set policy as to what the valid use cases for FPGA are.
I'm just providing a way to load FPGA images without rebooting Linux.
One case where that is helpful is when you are developing your FPGA
image and want to keep reloading it.
>
> In the enterprise space the model for FPGA use is usually a lot more
> flexible, big racks of FPGA boards that are handed out as resources to
> processes. They may be uploading fixed bitstreams but they may also be
> splicing bitstreams (eg splicing in 'ROM' images) and in the future as
> the Chinese break the existing FPGA market up I imagine we'll even see
> open bitstream formats.
I have no problem with any of that. For me, the 'firmware' interface is
a convenient way of getting an image. We can reprogram as much as we
want with as many 'firmware' images we like.
Also note that this framework does not assume anything about the contents
of the 'firmware' images. It does not try to parse those images. It
just loads them.
>
> In the smaller system world emulators, real time and all sorts of maker
> type projects use the FPGA boards as a dynamic resource already. It might
> be running GNU radio, then driving a 3D printer, then doing processor
> emulation for a games console. The FPGAs hanging off my desktop box have
> been all sorts of things from video processors to emulated systems and
> even block drivers for weird recalcitrant hardware. Next stop may well be
> Localtalk 8)
>
> In the academic world the model is similar, they are being treated as OS
> resources by the various reconfigurable OS projects, most of which are
> themselves Linux patch sets.
>
> IMHO we have two use cases
>
There are a lot more than two use cases. If you follow the history of
the FGPA manager on the mailing list, a lot of people who like one
use case have no use for any of the other use cases.
I want to provide something that is handles the low level stuff of
reconfiguring a FPGA.
> 1. Fixed function firmware - in which case the driver already handles it
> and we don't care if its FPGA bitstreams or microcode or CPU code or
> whatever
If your FPGA is already configured before Linux boots, these patches aren't
needed for that use.
But even in that case, it can be handy for reloading images during development.
>
> 2. Dynamic use cases where we need a resource we own, which means
> enumerate/open/close/read/write interfaces including firmware.
>
In the case of a framework that will (1) reconfigure the fpga and then turn
around and (2) expose what the fpga is providing as a resource,
you will need a way of dynamically loading the image into the FPGA. That's
what this provides.
> For use case #1 I don't believe we need magic classes for FPGA and in
Magic? Nope, it's a device driver. What I have here on my desk is a soc
with a FPGA bolted onto an ARM. There is a device in there that is called
the "fpga manager". This device's function is to configure the FPGA. So
I created a FPGA class to put the driver that controls this 'fpga manager'
device. This is a framework because I want to expose a useful interfaces
that other manufacturors could use, promote common code and common
interfaces.
> fact they are actually a mistake, for use class #2 request_firmware()
> doesn't work because it's intentionally quite blind to things like
> namespaces and permissions models. Both benefit greatly from library
> functions to handle the more standardised uploaded mechanisms.
>
> I agree entirely with Michael about putting it in staging and working
> from there.
>
Please look at my v4 patches which move this to drivers/staging. Also I
have dropped the DT stuff for now since not all of what it depends on is
in the main tree yet.
Alan Tull
> Alan
>
Hi!
> > * this causes the fpga to get programmed
> > * appropriate bridges get enabled
> > * appropriate drivers get probed
>
> For the case of a fixed function device it's sort of equivalent to a
> firmware load (in fact it *is* just a firmware load). The fixed function
> cases don't actually even need a 'firmware manager' or an FPGA class. In
> fact they shouldn't IMHO have one because the fact version A of the
> device requires firmware bitstream X, and bitstream X is an altera FPGA
> bitstream is an implementation detail. Revision B could be a
> microcontroller or something else and you still just shove a bitstream
> down it. No FPGA class is needed or appropriate. FPGA loader helpers
> yes.
Well, you'd still like to use the FGPA class to talk to the FPGA
loader, because that makes transition to different FPGA vendor easier,
right?
> 1. Fixed function firmware - in which case the driver already handles it
> and we don't care if its FPGA bitstreams or microcode or CPU code or
> whatever
>
> 2. Dynamic use cases where we need a resource we own, which means
> enumerate/open/close/read/write interfaces including firmware.
>
> For use case #1 I don't believe we need magic classes for FPGA and in
> fact they are actually a mistake,
Why? Bitstream upload code is fairly complex, and it seems the high
level steps are similar between vendors. Having FPGA class people can
work with helps, and it will help in future more dynamic cases, too...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, 6 Dec 2014, Grant Likely wrote:
> >> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
> >> +{
> >> + int ret;
> >> +
> >> + if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
> >> + return -EBUSY;
> >> +
> >> + dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
> >> +
> >> + ret = __fpga_mgr_write(mgr, buf, count);
> >> + clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
> >
> > Is the EBUSY -- userspace please try again, but you don't know when to
> > try again -- right interface? I mean, normally kernel would wait, so
> > that userland does not have to poll?
>
> Custom locking schemes are just wrong. A mutex is the right thing to
> do here and then an -EBUSY isn't required.
>
I've changed it to a mutex in the next version.
> >
> >> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> >> + unsigned int len;
> >> + char image_name[NAME_MAX];
>
> Hard coding a string length is a warning sign. That is the sort of
> thing that can memdup() or strdup() can handle.
>
OK, I'll fix it using kstrdup() in v6.
Alan