[PATCHv2 0/3] Add streaming support to driver_data API

[PATCHv2 0/3] Add streaming support to driver_data API

[yi1.li]

From: Yi Li <yi1.li@linux.intel.com>

This series enables the streaming support on driver_data sync API
and add self test and FPGA mgr to test/use stream firmware in
4KB trucks.  

Changes in v2:

  - Rebase to Luis R. Rodriguez's 20170501-driver-data-try2
    branch 
  - Expended fw_get_filesystem_firmware function and added
    DRIVER_DATA_REQ_STREAMING flag for driver_data streaming mode.
  - Add self test cases.

Yi Li (3):
  firmware: Add streaming support for driver_data_request_sync API
  test: add streaming test to driver_data tester
  fpga_mgr: Add streaming support through the new driver_data API

 drivers/base/firmware_class.c                   |  94 +++++++++++++++++---
 drivers/fpga/fpga-mgr.c                         | 111 ++++++++++++++++++++++++
 include/linux/driver_data.h                     |  10 +++
 include/linux/fpga/fpga-mgr.h                   |   4 +
 lib/test_driver_data.c                          |  80 ++++++++++++++++-
 tools/testing/selftests/firmware/driver_data.sh |  32 +++++++
 6 files changed, 316 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCHv2 1/3] firmware: Add streaming support for driver_data_request_sync API

[yi1.li]

From: Yi Li <yi1.li@linux.intel.com>

By setting the driver_data_req_params req flag of DRIVER_DATA_REQ_STREAMING
and DRIVER_DATA_REQ_NO_CACHE, caller can streaming firmware image to the
pre-allocated buffer in small trunks. Caller also need to setup the
img_offset pointer and firmware image **path to avoid searching the
firmware folders repeatly. Details and examples please refer to the
trigger_config_stream function of test_driver_data.c and
fpga_mgr_firmware_stream function of fpga_mgr.c.

Signed-off-by: Yi Li <yi1.li@linux.intel.com>
---
 drivers/base/firmware_class.c | 94 +++++++++++++++++++++++++++++++++++++------
 include/linux/driver_data.h   | 10 +++++
 2 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 461c7c2..9a63124 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -152,6 +152,20 @@ struct driver_data_params {
 			     DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT,	\
 	}
 
+#define __DATA_REQ_FIRMWARE_STREAM_BUF(buf, size, offset, path)		\
+	.req_params = {							\
+		.reqs = DRIVER_DATA_REQ_NO_CACHE |			\
+			DRIVER_DATA_REQ_STREAMING,			\
+		.alloc_buf = buf,					\
+		.alloc_buf_size = size,					\
+		.img_offset = offset,					\
+		.path = path,						\
+	},								\
+	.priv_params = {						\
+		.priv_reqs = DRIVER_DATA_PRIV_REQ_FALLBACK |		\
+			     DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT,	\
+	}
+
 #define __DATA_REQ_FIRMWARE_NOWAIT(module, uevent, gfp, async_cb, async_ctx) \
 	.req_params = {							\
 		.hold_module = module,					\
@@ -180,6 +194,8 @@ struct driver_data_params {
 	(!!((params)->priv_reqs & DRIVER_DATA_PRIV_REQ_FALLBACK_UEVENT))
 #define driver_data_param_nocache(params)	\
 	(!!((params)->reqs & DRIVER_DATA_REQ_NO_CACHE))
+#define driver_data_param_streaming(params)	\
+	(!!((params)->reqs & DRIVER_DATA_REQ_STREAMING))
 
 #define driver_data_param_optional(params)	\
 	(!!((params)->reqs & DRIVER_DATA_REQ_OPTIONAL))
@@ -625,14 +641,18 @@ module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
 static int
-fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
+fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
+			   struct driver_data_params *data_params)
 {
 	loff_t size;
 	int i, len;
 	int rc = -ENOENT;
-	char *path;
+	char **path;
+	char *local_path = NULL;
+	loff_t *offset = data_params->req_params.img_offset;
 	enum kernel_read_file_id id = READING_FIRMWARE;
 	size_t msize = INT_MAX;
+	struct file *file;
 
 	/* Already populated data member means we're loading into a buffer */
 	if (buf->data) {
@@ -640,16 +660,58 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		msize = buf->allocated_size;
 	}
 
-	path = __getname();
-	if (!path)
+	buf->size = 0;
+
+	/* Save path for streaming case */
+	if (driver_data_param_streaming(&data_params->req_params)) {
+		path = data_params->req_params.path;
+		if (!path) {
+			dev_err(device, "req_params.path not initialized\n");
+			rc = -ENOENT;
+			return rc;
+		}
+	} else {
+		path = &local_path;
+	}
+
+streaming:
+	/* Skip the repeating folder searching, direct to load*/
+	if (driver_data_param_streaming(&data_params->req_params) && *path) {
+		if (!offset) {
+			dev_err(device, "img_offset not initialized\n");
+			rc = -ENOENT;
+			return rc;
+		}
+
+		file = filp_open(*path, O_RDONLY, 0);
+		if (IS_ERR(file)) {
+			rc = -ENOENT;
+			return rc;
+		}
+
+		buf->size = kernel_read(file, *offset, (char *)buf->data,
+					msize);
+		fput(file);
+		if (buf->size < msize) {
+			fw_state_done(&buf->fw_st);
+			__putname(*path);
+		}
+		return 0;
+	}
+
+	/* First time to load the firmware */
+	*path = __getname();
+	if (!*path) {
+		dev_err(device, "cannot getname\n");
 		return -ENOMEM;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
 		/* skip the unset customized path */
 		if (!fw_path[i][0])
 			continue;
 
-		len = snprintf(path, PATH_MAX, "%s/%s",
+		len = snprintf(*path, PATH_MAX, "%s/%s",
 			       fw_path[i], buf->fw_id);
 		if (len >= PATH_MAX) {
 			rc = -ENAMETOOLONG;
@@ -657,23 +719,29 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		}
 
 		buf->size = 0;
-		rc = kernel_read_file_from_path(path, &buf->data, &size, msize,
-						id);
+		rc = kernel_read_file_from_path(*path, &buf->data, &size,
+						msize, id);
 		if (rc) {
-			if (rc == -ENOENT)
+			if (driver_data_param_streaming(&data_params->req_params)
+			    && (rc == -EFBIG)) {
+				rc = 0;
+				goto streaming;
+			} else if (rc == -ENOENT)
 				dev_dbg(device, "loading %s failed with error %d\n",
-					 path, rc);
+					*path, rc);
 			else
 				dev_warn(device, "loading %s failed with error %d\n",
-					 path, rc);
+					 *path, rc);
 			continue;
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_state_done(&buf->fw_st);
+		if (buf->size < msize) {
+			fw_state_done(&buf->fw_st);
+			__putname(*path);
+		}
 		break;
 	}
-	__putname(path);
 
 	return rc;
 }
@@ -1466,7 +1534,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	ret = fw_get_filesystem_firmware(device, fw->priv);
+	ret = fw_get_filesystem_firmware(device, fw->priv, data_params);
 	if (ret) {
 		if (!driver_data_param_optional(&data_params->req_params))
 			dev_warn(device,
diff --git a/include/linux/driver_data.h b/include/linux/driver_data.h
index a3a5fbe..e9e828a 100644
--- a/include/linux/driver_data.h
+++ b/include/linux/driver_data.h
@@ -120,12 +120,16 @@ union driver_data_cbs {
  * @DRIVER_DATA_REQ_NO_CACHE: indicates that the driver data request
  *	should not set up and use the internal caching mechanism to assist
  *	drivers from fetching driver data at resume time after suspend.
+ * @DRIVER_DATA_REQ_STREAMING: indicates that the driver data request
+ *	is in the streaming mode, the buffer is allocated outside the
+ *	firmware driver.
  */
 enum driver_data_reqs {
 	DRIVER_DATA_REQ_OPTIONAL			= 1 << 0,
 	DRIVER_DATA_REQ_KEEP				= 1 << 1,
 	DRIVER_DATA_REQ_USE_API_VERSIONING		= 1 << 2,
 	DRIVER_DATA_REQ_NO_CACHE			= 1 << 3,
+	DRIVER_DATA_REQ_STREAMING			= 1 << 4,
 };
 
 /**
@@ -147,6 +151,10 @@ enum driver_data_reqs {
  * @alloc_buf: pointer of pointer to the buffer area allocated by the caller
  *	so we can place the respective driver data
  * @alloc_buf_size: size of the @alloc_buf
+ * @img_offset: optional for streaming mode, define which offset address the
+ *	firmware should be read from
+ * @path: optional for streaming mode, save the file path to avoid search over
+ *	and over again
  *
  * This data structure is intended to carry all requirements and specifications
  * required to complete the task to get the requested driver date file to the
@@ -162,6 +170,8 @@ struct driver_data_req_params {
 	const union driver_data_cbs cbs;
 	void **alloc_buf;
 	size_t alloc_buf_size;
+	loff_t *img_offset;
+	char **path;
 };
 
 /*
-- 
2.7.4

[PATCHv2 2/3] test: add streaming test to driver_data tester

[yi1.li]

From: Yi Li <yi1.li@linux.intel.com>

This adds streaming test case, which pre-allocate the buffer and ask
driver_data_request_sync API to read 4KB each. Since the dummy firmware
image is very small, the test only stream couple bytes.
A manual test method:
echo -n 'bigfile' > /sys/devices/virtual/misc/test_driver_data0/config_name
echo -n 0 > /sys/devices/virtual/misc/test_driver_data0/config_async
echo -n 1 > /sys/devices/virtual/misc/test_driver_data0/config_stream
echo -n 1 > /sys/devices/virtual/misc/test_driver_data0/config_no_cache
echo -n 1 > /sys/devices/virtual/misc/test_driver_data0/trigger_config

Signed-off-by: Yi Li <yi1.li@linux.intel.com>
---
 lib/test_driver_data.c                          | 80 ++++++++++++++++++++++++-
 tools/testing/selftests/firmware/driver_data.sh | 32 ++++++++++
 2 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/lib/test_driver_data.c b/lib/test_driver_data.c
index 13fb091..9cca0b7 100644
--- a/lib/test_driver_data.c
+++ b/lib/test_driver_data.c
@@ -72,6 +72,8 @@ int num_test_devs;
  * @prealloc: if set, driver_data will use preallocated buffer for
  *      firmware_buf->data. The caller need to setup alloc_buf_size and
  *      alloc_buf in the driver_data_req_params structure.
+ * @stream: true if you want to trigger an streaming request. This will use
+ *	driver_data_request_sync() with streaming flag.
  * @enable_opt_cb: whether or not the optional callback should be set
  *	on a trigger. There is no equivalent setting on the struct
  *	driver_data_req_params as this is implementation specific, and in
@@ -118,6 +120,7 @@ struct test_config {
 	char *name;
 	char *default_name;
 	bool async;
+	bool stream;
 	bool optional;
 	bool keep;
 	bool no_cache;
@@ -353,6 +356,9 @@ static ssize_t config_show(struct device *dev,
 	len += snprintf(buf+len, PAGE_SIZE,
 			"prealloc:\t\t%s\n",
 			config->prealloc ? "true" : "false");
+	len += snprintf(buf+len, PAGE_SIZE,
+			"stream:\t\t%s\n",
+			config->stream ? "true" : "false");
 
 	mutex_unlock(&test_dev->config_mutex);
 
@@ -502,6 +508,47 @@ static int trigger_config_sync(struct driver_data_test_device *test_dev)
 	return ret;
 }
 
+static int trigger_config_stream(struct driver_data_test_device *test_dev)
+{
+	struct test_config *config = &test_dev->config;
+	int ret;
+	char *path = NULL;
+	void *buf;
+	loff_t offset = 0;
+	size_t length = INT_MAX;
+	const struct driver_data_req_params req_params_default = {
+		DRIVER_DATA_DEFAULT_SYNC_REQS(config_sync_req_cb, test_dev,
+					      DRIVER_DATA_REQ_NO_CACHE |
+					      DRIVER_DATA_REQ_STREAMING),
+		.alloc_buf_size = SZ_4K,
+		.alloc_buf = &buf,
+		.img_offset = &offset,
+		.path = &path,
+	};
+	const struct driver_data_req_params *req_params = &req_params_default;
+
+	buf = kmalloc(SZ_4K, GFP_KERNEL);
+	if (!buf) {
+		dev_err(test_dev->dev, "%s: kmalloc buf failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	while (length > 0) {
+		ret = driver_data_request_sync(config->name, req_params,
+					       test_dev->dev);
+		if (ret)
+			dev_err(test_dev->dev, "sync load of '%s' failed: %d\n",
+				config->name, ret);
+		length -= test_dev->test_driver_data.size;
+		offset += test_dev->test_driver_data.size;
+		if (test_dev->test_driver_data.size < SZ_4K)
+			break;
+	}
+
+	kfree(buf);
+	return ret;
+}
+
 static void config_async_req_cb(const struct firmware *driver_data,
 				void *context, int unused_error)
 {
@@ -636,8 +683,12 @@ trigger_config_store(struct device *dev,
 
 	if (config->async)
 		ret = trigger_config_async(test_dev);
-	else
-		ret = trigger_config_sync(test_dev);
+	else {
+		if (config->stream)
+			ret = trigger_config_stream(test_dev);
+		else
+			ret = trigger_config_sync(test_dev);
+	}
 
 	config->test_result = ret;
 
@@ -707,6 +758,7 @@ static int __driver_data_config_init(struct test_config *config)
 		goto out;
 
 	config->async = false;
+	config->stream = false;
 	config->optional = false;
 	config->keep = false;
 	config->no_cache = false;
@@ -985,6 +1037,29 @@ static ssize_t config_async_show(struct device *dev,
 }
 static DEVICE_ATTR(config_async, 0644, config_async_show, config_async_store);
 
+static ssize_t config_stream_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_update_bool(test_dev, buf, count,
+					   &config->stream);
+}
+
+static ssize_t config_stream_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct driver_data_test_device *test_dev = dev_to_test_dev(dev);
+	struct test_config *config = &test_dev->config;
+
+	return test_dev_config_show_bool(test_dev, buf, config->stream);
+}
+static DEVICE_ATTR(config_stream, 0644, config_stream_show,
+		   config_stream_store);
+
 static ssize_t config_optional_store(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t count)
@@ -1231,6 +1306,7 @@ static struct attribute *test_dev_attrs[] = {
 	TEST_DRIVER_DATA_DEV_ATTR(config_name),
 	TEST_DRIVER_DATA_DEV_ATTR(config_default_name),
 	TEST_DRIVER_DATA_DEV_ATTR(config_async),
+	TEST_DRIVER_DATA_DEV_ATTR(config_stream),
 	TEST_DRIVER_DATA_DEV_ATTR(config_optional),
 	TEST_DRIVER_DATA_DEV_ATTR(config_keep),
 	TEST_DRIVER_DATA_DEV_ATTR(config_no_cache),
diff --git a/tools/testing/selftests/firmware/driver_data.sh b/tools/testing/selftests/firmware/driver_data.sh
index dcbbc78..f63a703 100755
--- a/tools/testing/selftests/firmware/driver_data.sh
+++ b/tools/testing/selftests/firmware/driver_data.sh
@@ -41,6 +41,7 @@ ALL_TESTS="$ALL_TESTS 0010:10:1"
 ALL_TESTS="$ALL_TESTS 0011:10:1"
 ALL_TESTS="$ALL_TESTS 0012:1:1"
 ALL_TESTS="$ALL_TESTS 0013:1:1"
+ALL_TESTS="$ALL_TESTS 0015:1:1"
 
 # Not yet sure how to automate suspend test well yet.  For now we expect a
 # manual run. If using qemu you can resume a guest using something like the
@@ -244,6 +245,22 @@ config_disable_prealloc()
 	fi
 }
 
+config_set_stream()
+{
+	if ! echo -n 1 >$DIR/config_stream ; then
+		echo "$0: Unable to unset to stream" >&2
+		exit 1
+	fi
+}
+
+config_disable_stream()
+{
+	if ! echo -n 0 >$DIR/config_stream ; then
+		echo "$0: Unable to set to stream" >&2
+		exit 1
+	fi
+}
+
 config_enable_opt_cb()
 {
 	if ! echo -n 1 >$DIR/config_enable_opt_cb; then
@@ -470,6 +487,12 @@ driver_data_set_async_defaults()
 	config_set_async
 }
 
+driver_data_set_stream_defaults()
+{
+	config_reset
+	config_set_stream
+}
+
 set_system_state()
 {
 	STATE="mem"
@@ -886,6 +909,14 @@ driver_data_test_0014()
 	driver_data_test_0014a
 }
 
+driver_data_test_0015()
+{
+	driver_data_set_stream_defaults
+	config_trigger ${FUNCNAME[0]}
+	config_file_should_match ${FUNCNAME[0]}
+	config_expect_result ${FUNCNAME[0]} SUCCESS
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -908,6 +939,7 @@ list_tests()
 	echo "0012 x $(get_test_count 0012) - Verify api call wills will hunt for files, ignore file"
 	echo "0013 x $(get_test_count 0013) - Verify api call works"
 	echo "0014 x $(get_test_count 0013) - Verify api call works with suspend + resume"
+	echo "0015 x $(get_test_count 0015) - Simple streaming loader"
 }
 
 test_reqs
-- 
2.7.4

[PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[yi1.li]

From: Yi Li <yi1.li@linux.intel.com>

Since the FPGA image are getting bigger in size, this add an new API
fpga_mgr_firmware_stream in FPGA manager, which will stream FPGA image in
4KB trunks.

Signed-off-by: Yi Li <yi1.li@linux.intel.com>
---
 drivers/fpga/fpga-mgr.c       | 111 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h |   4 ++
 2 files changed, 115 insertions(+)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 188ffef..420ee38 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -27,6 +27,8 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/highmem.h>
+#include <linux/sizes.h>
+#include <linux/driver_data.h>
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
@@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
 	return fpga_mgr_write_complete(mgr, info);
 }
 
+struct fpga_mgr_streaming_priv_params {
+	struct fpga_image_info *info;
+	struct fpga_manager *mgr;
+	loff_t offset;
+	loff_t fw_size;
+};
+
+static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw,
+				    int err)
+{
+	int ret = -EINVAL;
+	struct fpga_mgr_streaming_priv_params *params =
+		(struct fpga_mgr_streaming_priv_params *)context;
+	struct fpga_image_info *info = params->info;
+	struct fpga_manager *mgr = params->mgr;
+	struct device *dev = &mgr->dev;
+
+	params->fw_size = fw->size;
+	/*
+	 * init.
+	 */
+	if (params->offset == 0) {
+		ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Write the FPGA image to the FPGA.
+	 */
+	mgr->state = FPGA_MGR_STATE_WRITE;
+	ret = mgr->mops->write(mgr, fw->data, fw->size);
+	if (ret) {
+		dev_err(dev, "Error while writing image data to FPGA\n");
+		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+		return ret;
+	}
+
+	if (fw->size < SZ_4K)
+		ret = fpga_mgr_write_complete(mgr, info);
+
+	return ret;
+}
+
+/**
+ * fpga_mgr_firmware_stream - streaming firmware and load to fpga
+ * @mgr:	fpga manager
+ * @info:	fpga image specific information
+ * @image_name:	name of image file on the firmware search path
+ *
+ * Streaming an FPGA image using the firmware class, then write out to the FPGA.
+ * Update the state before each step to provide info on what step failed if
+ * there is a failure.  This code assumes the caller got the mgr pointer
+ * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
+ * code.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
+			     struct fpga_image_info *info,
+			     const char *image_name)
+{
+	int ret;
+	char *path = NULL;
+	void *buf;
+	size_t length = INT_MAX;
+	struct device *dev = &mgr->dev;
+	struct fpga_mgr_streaming_priv_params params = {
+		.info = info,
+		.mgr = mgr,
+		.fw_size = 0,
+		.offset = 0,
+	};
+
+	const struct driver_data_req_params req_params = {
+		DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, &params),
+		.reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING,
+		.alloc_buf_size = SZ_4K,
+		.alloc_buf = &buf,
+		.img_offset = &params.offset,
+		.path = &path,
+	};
+
+	buf = kmalloc(SZ_4K, GFP_KERNEL);
+	if (!buf) {
+		dev_err(dev, "%s: kmalloc buf failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+	while (length > 0) {
+		ret = driver_data_request_sync(image_name, &req_params, dev);
+		if (ret) {
+			dev_err(dev, "Error reading firmware %d\n", ret);
+			mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+			return ret;
+		}
+
+		length -= params.fw_size;
+		params.offset += params.fw_size;
+		if (params.fw_size < SZ_4K)
+			break;
+	}
+
+	kfree(buf);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
+
 /**
  * fpga_mgr_buf_load - load fpga from image in buffer
  * @mgr:	fpga manager
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index b4ac24c..083e091 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -143,6 +143,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 			   struct fpga_image_info *info,
 			   const char *image_name);
 
+int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
+			     struct fpga_image_info *info,
+			     const char *image_name);
+
 struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
 
 struct fpga_manager *fpga_mgr_get(struct device *dev);
-- 
2.7.4

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Alan Tull]

On Sat, May 20, 2017 at 1:49 AM,  <yi1.li@linux.intel.com> wrote:

Hi Yi,

> From: Yi Li <yi1.li@linux.intel.com>
>
> Since the FPGA image are getting bigger in size, this add an new API
> fpga_mgr_firmware_stream

You could replace the guts of the current fpga_mgr_firmware_load()
with this new API (keep the old name).  Unless anyone can think of a
reason to keep my old memory hog version.  Assuming this all works :)

> in FPGA manager, which will stream FPGA image in
> 4KB trunks.

chunks?

>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 111 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |   4 ++
>  2 files changed, 115 insertions(+)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 188ffef..420ee38 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -27,6 +27,8 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include <linux/sizes.h>
> +#include <linux/driver_data.h>
>
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>         return fpga_mgr_write_complete(mgr, info);
>  }
>
> +struct fpga_mgr_streaming_priv_params {

This name is boggling my mind. :-) Something can be private or it can
be params, but it seems like params aren't really private.  And later
it's referred to as 'params' although it's really private data.  Could
you rename and take off the _params?  That way when I'm reading
through this, I'll know that this is something that this code is
really passing to itself and I won't mistakenly think that this is a
struct that is being passed for the firmware code to consume.

> +       struct fpga_image_info *info;
> +       struct fpga_manager *mgr;
> +       loff_t offset;
> +       loff_t fw_size;
> +};
> +

Please add a comment here explaining that this is the callback
function, called back from the firmware code for each 4K chunk of FPGA
image.

> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw,
> +                                   int err)
> +{
> +       int ret = -EINVAL;
> +       struct fpga_mgr_streaming_priv_params *params =

And please call this 'priv' or something with 'priv' in the name?

> +               (struct fpga_mgr_streaming_priv_params *)context;
> +       struct fpga_image_info *info = params->info;
> +       struct fpga_manager *mgr = params->mgr;
> +       struct device *dev = &mgr->dev;
> +
> +       params->fw_size = fw->size;

Please skip a line here.

> +       /*
> +        * init.
> +        */

/* A one line comment should be like this, not 3 lines. */

> +       if (params->offset == 0) {
> +               ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /*
> +        * Write the FPGA image to the FPGA.
> +        */

/* Write the FPGA image to the FPGA. */

> +       mgr->state = FPGA_MGR_STATE_WRITE;
> +       ret = mgr->mops->write(mgr, fw->data, fw->size);
> +       if (ret) {
> +               dev_err(dev, "Error while writing image data to FPGA\n");
> +               mgr->state = FPGA_MGR_STATE_WRITE_ERR;
> +               return ret;
> +       }
> +
> +       if (fw->size < SZ_4K)

Define a macro before the function and use it everywhere SZ_4K means
the same thing here (3 occurrences), such as

#define FW_CHUNK_SZ     SZ_4K

> +               ret = fpga_mgr_write_complete(mgr, info);
> +
> +       return ret;
> +}
> +
> +/**
> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga
> + * @mgr:       fpga manager
> + * @info:      fpga image specific information
> + * @image_name:        name of image file on the firmware search path
> + *
> + * Streaming an FPGA image using the firmware class, then write out to the FPGA.
> + * Update the state before each step to provide info on what step failed if
> + * there is a failure.  This code assumes the caller got the mgr pointer
> + * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
> + * code.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
> +                            struct fpga_image_info *info,
> +                            const char *image_name)
> +{
> +       int ret;
> +       char *path = NULL;
> +       void *buf;
> +       size_t length = INT_MAX;

I guess the firmware layer doesn't give us the size of the whole
firmware, so length is used for the while() below, right?  Too bad
since everybody using streaming firmware is going to be writing this
same type of while statement.

> +       struct device *dev = &mgr->dev;
> +       struct fpga_mgr_streaming_priv_params params = {

priv

> +               .info = info,
> +               .mgr = mgr,
> +               .fw_size = 0,
> +               .offset = 0,
> +       };
> +
> +       const struct driver_data_req_params req_params = {
> +               DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, &params),
> +               .reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING,
> +               .alloc_buf_size = SZ_4K,

FW_CHUNK_SZ or whatever name is appropriate.

> +               .alloc_buf = &buf,
> +               .img_offset = &params.offset,
> +               .path = &path,
> +       };
> +
> +       buf = kmalloc(SZ_4K, GFP_KERNEL);
> +       if (!buf) {
> +               dev_err(dev, "%s: kmalloc buf failed\n", __func__);

This message isn't needed.

> +               return -ENOMEM;
> +       }
> +
> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +       while (length > 0) {

This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
&& (length > 0));" since that's what it's really doing.

> +               ret = driver_data_request_sync(image_name, &req_params, dev);
> +               if (ret) {
> +                       dev_err(dev, "Error reading firmware %d\n", ret);
> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +                       return ret;
> +               }
> +
> +               length -= params.fw_size;
> +               params.offset += params.fw_size;
> +               if (params.fw_size < SZ_4K)
> +                       break;
> +       }
> +
> +       kfree(buf);

Please skip a line before return.

> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);

Thanks for working on this!

Alan

> +
>  /**
>   * fpga_mgr_buf_load - load fpga from image in buffer
>   * @mgr:       fpga manager
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index b4ac24c..083e091 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -143,6 +143,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>                            struct fpga_image_info *info,
>                            const char *image_name);
>
> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
> +                            struct fpga_image_info *info,
> +                            const char *image_name);
> +
>  struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>
>  struct fpga_manager *fpga_mgr_get(struct device *dev);
> --
> 2.7.4
>

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Li, Yi]

hi Alan


On 5/22/2017 4:09 PM, Alan Tull wrote:
> On Sat, May 20, 2017 at 1:49 AM,  <yi1.li@linux.intel.com> wrote:
>
> Hi Yi,
>
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Since the FPGA image are getting bigger in size, this add an new API
>> fpga_mgr_firmware_stream
> You could replace the guts of the current fpga_mgr_firmware_load()
> with this new API (keep the old name).  Unless anyone can think of a
> reason to keep my old memory hog version.  Assuming this all works :)
>
>> in FPGA manager, which will stream FPGA image in
>> 4KB trunks.
> chunks?
Oops.
>
>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>> ---
>>   drivers/fpga/fpga-mgr.c       | 111 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/fpga/fpga-mgr.h |   4 ++
>>   2 files changed, 115 insertions(+)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 188ffef..420ee38 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -27,6 +27,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/scatterlist.h>
>>   #include <linux/highmem.h>
>> +#include <linux/sizes.h>
>> +#include <linux/driver_data.h>
>>
>>   static DEFINE_IDA(fpga_mgr_ida);
>>   static struct class *fpga_mgr_class;
>> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct fpga_manager *mgr,
>>          return fpga_mgr_write_complete(mgr, info);
>>   }
>>
>> +struct fpga_mgr_streaming_priv_params {
> This name is boggling my mind. :-) Something can be private or it can
> be params, but it seems like params aren't really private.  And later
> it's referred to as 'params' although it's really private data.  Could
> you rename and take off the _params?  That way when I'm reading
> through this, I'll know that this is something that this code is
> really passing to itself and I won't mistakenly think that this is a
> struct that is being passed for the firmware code to consume.

Okay, will rename the structure to fpga_mgr_streaming_context?
>
>> +       struct fpga_image_info *info;
>> +       struct fpga_manager *mgr;
>> +       loff_t offset;
>> +       loff_t fw_size;
>> +};
>> +
> Please add a comment here explaining that this is the callback
> function, called back from the firmware code for each 4K chunk of FPGA
> image.

Sure, will do.
>> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware *fw,
>> +                                   int err)
>> +{
>> +       int ret = -EINVAL;
>> +       struct fpga_mgr_streaming_priv_params *params =
> And please call this 'priv' or something with 'priv' in the name?

Okay, struct fpga_mgr_streaming_context *streaming_context?
>
>> +               (struct fpga_mgr_streaming_priv_params *)context;
>> +       struct fpga_image_info *info = params->info;
>> +       struct fpga_manager *mgr = params->mgr;
>> +       struct device *dev = &mgr->dev;
>> +
>> +       params->fw_size = fw->size;
> Please skip a line here.
Thanks for catching that.
>
>> +       /*
>> +        * init.
>> +        */
> /* A one line comment should be like this, not 3 lines. */
Thanks for catching that.
>
>> +       if (params->offset == 0) {
>> +               ret = fpga_mgr_write_init_buf(mgr, info, fw->data, fw->size);
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +
>> +       /*
>> +        * Write the FPGA image to the FPGA.
>> +        */
> /* Write the FPGA image to the FPGA. */
>
>> +       mgr->state = FPGA_MGR_STATE_WRITE;
>> +       ret = mgr->mops->write(mgr, fw->data, fw->size);
>> +       if (ret) {
>> +               dev_err(dev, "Error while writing image data to FPGA\n");
>> +               mgr->state = FPGA_MGR_STATE_WRITE_ERR;
>> +               return ret;
>> +       }
>> +
>> +       if (fw->size < SZ_4K)
> Define a macro before the function and use it everywhere SZ_4K means
> the same thing here (3 occurrences), such as
>
> #define FW_CHUNK_SZ     SZ_4K
Okay.
>
>> +               ret = fpga_mgr_write_complete(mgr, info);
>> +
>> +       return ret;
>> +}
>> +
>> +/**
>> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga
>> + * @mgr:       fpga manager
>> + * @info:      fpga image specific information
>> + * @image_name:        name of image file on the firmware search path
>> + *
>> + * Streaming an FPGA image using the firmware class, then write out to the FPGA.
>> + * Update the state before each step to provide info on what step failed if
>> + * there is a failure.  This code assumes the caller got the mgr pointer
>> + * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not an error
>> + * code.
>> + *
>> + * Return: 0 on success, negative error code otherwise.
>> + */
>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>> +                            struct fpga_image_info *info,
>> +                            const char *image_name)
>> +{
>> +       int ret;
>> +       char *path = NULL;
>> +       void *buf;
>> +       size_t length = INT_MAX;
> I guess the firmware layer doesn't give us the size of the whole
> firmware, so length is used for the while() below, right?  Too bad
> since everybody using streaming firmware is going to be writing this
> same type of while statement.
Yes, with the non-streaming new driver_data_request_xx case, it will 
give the firmware structure with the size of image length.  But with the 
streaming case, we will not know the size of the image file until it 
reach to the end of the file.

>
>> +       struct device *dev = &mgr->dev;
>> +       struct fpga_mgr_streaming_priv_params params = {
> priv
>
>> +               .info = info,
>> +               .mgr = mgr,
>> +               .fw_size = 0,
>> +               .offset = 0,
>> +       };
>> +
>> +       const struct driver_data_req_params req_params = {
>> +               DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb, &params),
>> +               .reqs = DRIVER_DATA_REQ_NO_CACHE | DRIVER_DATA_REQ_STREAMING,
>> +               .alloc_buf_size = SZ_4K,
> FW_CHUNK_SZ or whatever name is appropriate.
>
>> +               .alloc_buf = &buf,
>> +               .img_offset = &params.offset,
>> +               .path = &path,
>> +       };
>> +
>> +       buf = kmalloc(SZ_4K, GFP_KERNEL);
>> +       if (!buf) {
>> +               dev_err(dev, "%s: kmalloc buf failed\n", __func__);
> This message isn't needed.
Got it
>
>> +               return -ENOMEM;
>> +       }
>> +
>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>> +       while (length > 0) {
> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
> && (length > 0));" since that's what it's really doing.
Yes, this is better, thanks.

>
>> +               ret = driver_data_request_sync(image_name, &req_params, dev);
>> +               if (ret) {
>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>> +                       return ret;
>> +               }
>> +
>> +               length -= params.fw_size;
>> +               params.offset += params.fw_size;
>> +               if (params.fw_size < SZ_4K)
>> +                       break;
>> +       }
>> +
>> +       kfree(buf);
> Please skip a line before return.
>
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
> Thanks for working on this!
>
> Alan
>
>> +
>>   /**
>>    * fpga_mgr_buf_load - load fpga from image in buffer
>>    * @mgr:       fpga manager
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index b4ac24c..083e091 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -143,6 +143,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>                             struct fpga_image_info *info,
>>                             const char *image_name);
>>
>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>> +                            struct fpga_image_info *info,
>> +                            const char *image_name);
>> +
>>   struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>
>>   struct fpga_manager *fpga_mgr_get(struct device *dev);
>> --
>> 2.7.4
>>

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Alan Tull]

On Mon, May 22, 2017 at 11:11 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
> hi Alan
>
>
> On 5/22/2017 4:09 PM, Alan Tull wrote:
>>
>> On Sat, May 20, 2017 at 1:49 AM,  <yi1.li@linux.intel.com> wrote:
>>
>> Hi Yi,
>>
>>> From: Yi Li <yi1.li@linux.intel.com>
>>>
>>> Since the FPGA image are getting bigger in size, this add an new API
>>> fpga_mgr_firmware_stream
>>
>> You could replace the guts of the current fpga_mgr_firmware_load()
>> with this new API (keep the old name).  Unless anyone can think of a
>> reason to keep my old memory hog version.  Assuming this all works :)
>>
>>> in FPGA manager, which will stream FPGA image in
>>> 4KB trunks.
>>
>> chunks?
>
> Oops.
>>
>>
>>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>>> ---
>>>   drivers/fpga/fpga-mgr.c       | 111
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/fpga/fpga-mgr.h |   4 ++
>>>   2 files changed, 115 insertions(+)
>>>
>>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>>> index 188ffef..420ee38 100644
>>> --- a/drivers/fpga/fpga-mgr.c
>>> +++ b/drivers/fpga/fpga-mgr.c
>>> @@ -27,6 +27,8 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/scatterlist.h>
>>>   #include <linux/highmem.h>
>>> +#include <linux/sizes.h>
>>> +#include <linux/driver_data.h>
>>>
>>>   static DEFINE_IDA(fpga_mgr_ida);
>>>   static struct class *fpga_mgr_class;
>>> @@ -196,6 +198,115 @@ static int fpga_mgr_buf_load_mapped(struct
>>> fpga_manager *mgr,
>>>          return fpga_mgr_write_complete(mgr, info);
>>>   }
>>>
>>> +struct fpga_mgr_streaming_priv_params {
>>
>> This name is boggling my mind. :-) Something can be private or it can
>> be params, but it seems like params aren't really private.  And later
>> it's referred to as 'params' although it's really private data.  Could
>> you rename and take off the _params?  That way when I'm reading
>> through this, I'll know that this is something that this code is
>> really passing to itself and I won't mistakenly think that this is a
>> struct that is being passed for the firmware code to consume.
>
>
> Okay, will rename the structure to fpga_mgr_streaming_context?

That sounds good.

>>
>>
>>> +       struct fpga_image_info *info;
>>> +       struct fpga_manager *mgr;
>>> +       loff_t offset;
>>> +       loff_t fw_size;
>>> +};
>>> +
>>
>> Please add a comment here explaining that this is the callback
>> function, called back from the firmware code for each 4K chunk of FPGA
>> image.
>
>
> Sure, will do.
>>>
>>> +static int fpga_mgr_streaming_fw_cb(void *context, const struct firmware
>>> *fw,
>>> +                                   int err)
>>> +{
>>> +       int ret = -EINVAL;
>>> +       struct fpga_mgr_streaming_priv_params *params =
>>
>> And please call this 'priv' or something with 'priv' in the name?
>
>
> Okay, struct fpga_mgr_streaming_context *streaming_context?

That's fine, especially since the firmware layer is calling it "context".

>>
>>
>>> +               (struct fpga_mgr_streaming_priv_params *)context;
>>> +       struct fpga_image_info *info = params->info;
>>> +       struct fpga_manager *mgr = params->mgr;
>>> +       struct device *dev = &mgr->dev;
>>> +
>>> +       params->fw_size = fw->size;
>>
>> Please skip a line here.
>
> Thanks for catching that.
>>
>>
>>> +       /*
>>> +        * init.
>>> +        */
>>
>> /* A one line comment should be like this, not 3 lines. */
>
> Thanks for catching that.
>>
>>
>>> +       if (params->offset == 0) {
>>> +               ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
>>> fw->size);
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>> +
>>> +       /*
>>> +        * Write the FPGA image to the FPGA.
>>> +        */
>>
>> /* Write the FPGA image to the FPGA. */
>>
>>> +       mgr->state = FPGA_MGR_STATE_WRITE;
>>> +       ret = mgr->mops->write(mgr, fw->data, fw->size);
>>> +       if (ret) {
>>> +               dev_err(dev, "Error while writing image data to FPGA\n");
>>> +               mgr->state = FPGA_MGR_STATE_WRITE_ERR;
>>> +               return ret;
>>> +       }
>>> +
>>> +       if (fw->size < SZ_4K)
>>
>> Define a macro before the function and use it everywhere SZ_4K means
>> the same thing here (3 occurrences), such as
>>
>> #define FW_CHUNK_SZ     SZ_4K
>
> Okay.
>>
>>
>>> +               ret = fpga_mgr_write_complete(mgr, info);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +/**
>>> + * fpga_mgr_firmware_stream - streaming firmware and load to fpga
>>> + * @mgr:       fpga manager
>>> + * @info:      fpga image specific information
>>> + * @image_name:        name of image file on the firmware search path
>>> + *
>>> + * Streaming an FPGA image using the firmware class, then write out to
>>> the FPGA.
>>> + * Update the state before each step to provide info on what step failed
>>> if
>>> + * there is a failure.  This code assumes the caller got the mgr pointer
>>> + * from of_fpga_mgr_get() or fpga_mgr_get() and checked that it is not
>>> an error
>>> + * code.
>>> + *
>>> + * Return: 0 on success, negative error code otherwise.
>>> + */
>>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>>> +                            struct fpga_image_info *info,
>>> +                            const char *image_name)
>>> +{
>>> +       int ret;
>>> +       char *path = NULL;
>>> +       void *buf;
>>> +       size_t length = INT_MAX;
>>
>> I guess the firmware layer doesn't give us the size of the whole
>> firmware, so length is used for the while() below, right?  Too bad
>> since everybody using streaming firmware is going to be writing this
>> same type of while statement.
>
> Yes, with the non-streaming new driver_data_request_xx case, it will give
> the firmware structure with the size of image length.  But with the
> streaming case, we will not know the size of the image file until it reach
> to the end of the file.
>
>>
>>> +       struct device *dev = &mgr->dev;
>>> +       struct fpga_mgr_streaming_priv_params params = {
>>
>> priv
>>
>>> +               .info = info,
>>> +               .mgr = mgr,
>>> +               .fw_size = 0,
>>> +               .offset = 0,
>>> +       };
>>> +
>>> +       const struct driver_data_req_params req_params = {
>>> +               DRIVER_DATA_DEFAULT_SYNC(fpga_mgr_streaming_fw_cb,
>>> &params),
>>> +               .reqs = DRIVER_DATA_REQ_NO_CACHE |
>>> DRIVER_DATA_REQ_STREAMING,
>>> +               .alloc_buf_size = SZ_4K,
>>
>> FW_CHUNK_SZ or whatever name is appropriate.
>>
>>> +               .alloc_buf = &buf,
>>> +               .img_offset = &params.offset,
>>> +               .path = &path,
>>> +       };
>>> +
>>> +       buf = kmalloc(SZ_4K, GFP_KERNEL);
>>> +       if (!buf) {
>>> +               dev_err(dev, "%s: kmalloc buf failed\n", __func__);
>>
>> This message isn't needed.
>
> Got it
>>
>>
>>> +               return -ENOMEM;
>>> +       }
>>> +
>>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>>> +       while (length > 0) {
>>
>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
>> && (length > 0));" since that's what it's really doing.
>
> Yes, this is better, thanks.
>
>>
>>> +               ret = driver_data_request_sync(image_name, &req_params,
>>> dev);
>>> +               if (ret) {
>>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;

Also need:  kfree(buf);

>>> +                       return ret;
>>> +               }
>>> +
>>> +               length -= params.fw_size;
>>> +               params.offset += params.fw_size;
>>> +               if (params.fw_size < SZ_4K)
>>> +                       break;
>>> +       }
>>> +
>>> +       kfree(buf);
>>
>> Please skip a line before return.
>>
>>> +       return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
>>
>> Thanks for working on this!
>>
>> Alan
>>
>>> +
>>>   /**
>>>    * fpga_mgr_buf_load - load fpga from image in buffer
>>>    * @mgr:       fpga manager
>>> diff --git a/include/linux/fpga/fpga-mgr.h
>>> b/include/linux/fpga/fpga-mgr.h
>>> index b4ac24c..083e091 100644
>>> --- a/include/linux/fpga/fpga-mgr.h
>>> +++ b/include/linux/fpga/fpga-mgr.h
>>> @@ -143,6 +143,10 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>>                             struct fpga_image_info *info,
>>>                             const char *image_name);
>>>
>>> +int fpga_mgr_firmware_stream(struct fpga_manager *mgr,
>>> +                            struct fpga_image_info *info,
>>> +                            const char *image_name);
>>> +
>>>   struct fpga_manager *of_fpga_mgr_get(struct device_node *node);
>>>
>>>   struct fpga_manager *fpga_mgr_get(struct device *dev);
>>> --
>>> 2.7.4
>>>
>

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Alan Tull]

On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote:

>>>> +
>>>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>>>> +       while (length > 0) {
>>>
>>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
>>> && (length > 0));" since that's what it's really doing.
>>
>> Yes, this is better, thanks.
>>
>>>
>>>> +               ret = driver_data_request_sync(image_name, &req_params,
>>>> dev);
>>>> +               if (ret) {
>>>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>>>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>
> Also need:  kfree(buf);
>
>>>> +                       return ret;

Or this could be a break instead of a return.

>>>> +               }
>>>> +
>>>> +               length -= params.fw_size;
>>>> +               params.offset += params.fw_size;
>>>> +               if (params.fw_size < SZ_4K)
>>>> +                       break;
>>>> +       }
>>>> +
>>>> +       kfree(buf);
>>>
>>> Please skip a line before return.
>>>
>>>> +       return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Li, Yi]

On 5/23/2017 10:25 AM, Alan Tull wrote:
> On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote:
>
>>>>> +
>>>>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>>>>> +       while (length > 0) {
>>>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
>>>> && (length > 0));" since that's what it's really doing.
>>> Yes, this is better, thanks.
>>>
>>>>> +               ret = driver_data_request_sync(image_name, &req_params,
>>>>> dev);
>>>>> +               if (ret) {
>>>>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>>>>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>> Also need:  kfree(buf);
>>
>>>>> +                       return ret;
> Or this could be a break instead of a return.

Thanks Alan, will fix it.
>>>>> +               }
>>>>> +
>>>>> +               length -= params.fw_size;
>>>>> +               params.offset += params.fw_size;
>>>>> +               if (params.fw_size < SZ_4K)
>>>>> +                       break;
>>>>> +       }
>>>>> +
>>>>> +       kfree(buf);
>>>> Please skip a line before return.
>>>>
>>>>> +       return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);

Re: [PATCHv2 3/3] fpga_mgr: Add streaming support through the new driver_data API

[Li, Yi]

On 5/23/2017 10:25 AM, Alan Tull wrote:
> On Tue, May 23, 2017 at 10:21 AM, Alan Tull <atull@kernel.org> wrote:
>
>>>>> +
>>>>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>>>>> +       while (length > 0) {
>>>> This could be "do { ... } while ((params.fw_size >= FIRMWARE_CHUNK_SZ)
>>>> && (length > 0));" since that's what it's really doing.
>>> Yes, this is better, thanks.
>>>
>>>>> +               ret = driver_data_request_sync(image_name, &req_params,
>>>>> dev);
>>>>> +               if (ret) {
>>>>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>>>>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>> Also need:  kfree(buf);
>>
>>>>> +                       return ret;
> Or this could be a break instead of a return.

thanks Alan, will fix it.

>
>>>>> +               }
>>>>> +
>>>>> +               length -= params.fw_size;
>>>>> +               params.offset += params.fw_size;
>>>>> +               if (params.fw_size < SZ_4K)
>>>>> +                       break;
>>>>> +       }
>>>>> +
>>>>> +       kfree(buf);
>>>> Please skip a line before return.
>>>>
>>>>> +       return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);