[RFC 0/2] Add streaming API for firmware and FPGA manager

[RFC 0/2] Add streaming API for firmware and FPGA manager

[yi1.li]

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

As the FPGA hardware advances, the firmware image size grows (hundres
of MBs or more). It could be an issue for kernel to allocate a big
buffer to load the whole firmware file.
Here is an attempt to read the firmware file into a small buffer and
program the FPGA in a loop (or so call the streaming mode). It should not
be a performance hit for FPGA programing since the majority of time
spend is with the FPGA CvP/PR-IP interface. 

pseudo code in fpga manager:
while (size > 0) {
	ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
	ret = mgr->mops->write(mgr, fw->data, fw->size);
	offset += fw->size;
	size -= fw->size;
}

Thanks,
Yi

Yi Li (2):
  firmware class: Add stream_firmware API.
  fpga manager: Add fpga_mgr_firmware_stream API

 drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
 drivers/fpga/fpga-mgr.c       |  77 +++++++++++++++++++++++++
 include/linux/firmware.h      |   2 +
 include/linux/fpga/fpga-mgr.h |   4 ++
 4 files changed, 211 insertions(+)

-- 
2.7.4

[RFC 1/2] firmware class: Add stream_firmware API.

[yi1.li]

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

Add function to load firmware in multiple chucks instead of

loading the whole big firmware file at once.

Signed-off-by: Yi Li <yi1.li@linux.intel.com>
---
 drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/firmware.h      |   2 +
 2 files changed, 130 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ac350c5..44fddff 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 	return rc;
 }
 
+static int
+fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
+			size_t offset, size_t length)
+{
+	int i, len;
+	char *path;
+	int rc = 0;
+	struct file *file;
+
+	buf->size = 0;
+
+	path = __getname();
+	if (!path)
+		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",
+			       fw_path[i], buf->fw_id);
+		if (len >= PATH_MAX) {
+			rc = -ENAMETOOLONG;
+			break;
+		}
+
+		if (!path || !*path)
+			continue;
+
+		if (!buf->data) {
+			buf->data = vmalloc(length);
+			if (!buf->data) {
+				rc = -ENOMEM;
+				break;
+			}
+		}
+
+		file = filp_open(path, O_RDONLY, 0);
+		if (IS_ERR(file))
+			continue;
+
+		buf->size = kernel_read(file, offset, (char *) buf->data,
+					length);
+		fput(file);
+		break;
+	}
+
+	__putname(path);
+
+	if (rc)
+		dev_err(device, "loading %s failed with error %d\n",
+			 path, rc);
+	return rc;
+}
+
 /* firmware holds the ownership of pages */
 static void firmware_free_data(const struct firmware *fw)
 {
@@ -1267,6 +1323,78 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 }
 EXPORT_SYMBOL(request_firmware);
 
+static int
+_stream_firmware(const struct firmware **firmware_p, const char *name,
+		  struct device *device, void *buf, size_t size,
+		  unsigned int opt_flags, size_t offset, size_t length)
+{
+	int ret;
+	struct firmware *fw = NULL;
+	struct firmware_buf *fbuf;
+
+	if ((!firmware_p) || (!name || name[0] == '\0')) {
+		dev_err(device, "invalid firmware pointer or file name\n");
+		return -EINVAL;
+	}
+
+	if (!*firmware_p) {
+		ret = _request_firmware_prepare(&fw, name, device, buf, size);
+		if (ret <= 0) {
+			dev_err(device, "%s: _request_firmware_prepare failed %d\n",
+				__func__, ret);
+		}
+	} else {
+		fw = (struct firmware *) *firmware_p;
+	}
+
+	fbuf = (struct firmware_buf *) fw->priv;
+	ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
+	fw->size = fbuf->size;
+	fw->data = fbuf->data;
+	*firmware_p = fw;
+
+	if (ret)
+		dev_err(device, "streaming with error %d\n", ret);
+	return ret;
+}
+
+/**
+ * stream_firmware: - send firmware request and wait for it
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ * @offset: offset of the file to read from
+ * @length: length in bytes to read
+ *
+ *      @firmware_p will be used to return a firmware image by the name
+ *      of @name for device @device.
+ *
+ *      Should be called from user context where sleeping is allowed.
+ *
+ *      @name will be used as $FIRMWARE in the uevent environment and
+ *      should be distinctive enough not to be confused with any other
+ *      firmware image for this or any other device.
+ *
+ *	Caller must hold the reference count of @device.
+ *
+ *	The function can be called safely inside device's suspend and
+ *	resume callback.
+ **/
+int
+stream_firmware(const struct firmware **firmware_p, const char *name,
+		 struct device *device, size_t offset, size_t length)
+{
+	size_t ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _stream_firmware(firmware_p, name, device, NULL, 0,
+				FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL(stream_firmware);
+
 /**
  * request_firmware_direct: - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0c..accd7f6 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -41,6 +41,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
+int stream_firmware(const struct firmware **fw, const char *name,
+		    struct device *device, size_t offset, size_t length);
 int request_firmware_nowait(
 	struct module *module, bool uevent,
 	const char *name, struct device *device, gfp_t gfp, void *context,
-- 
2.7.4

[RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API

[yi1.li]

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

Add fpga_mgr_firmware_stream API, which can load and program firmware

in trucks to FPGA instead of the whole file.

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

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 3206a53..bb55b80 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -27,10 +27,15 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/highmem.h>
+#include <linux/sizes.h>
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
 
+static int streamsize = SZ_4K;
+module_param(streamsize, int, 0664);
+MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");
+
 /*
  * Call the low level driver's write_init function.  This will do the
  * device-specific things to get the FPGA into the state where it is ready to
@@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
 
+/**
+ * fpga_mgr_firmware_load - request firmware and load to fpga
+ * @mgr:	fpga manager
+ * @info:	fpga image specific information
+ * @image_name:	name of image file on the firmware search path
+ *
+ * Request 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)
+{
+	struct device *dev = &mgr->dev;
+	const struct firmware *fw = NULL;
+	int ret;
+	size_t size = INT_MAX, offset = 0;
+	bool start_flag = 1;
+
+	dev_info(dev, "writing %s to %s with buffer size %d\n",
+			image_name, mgr->name, streamsize);
+
+	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+
+	while (size > 0) {
+		ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
+		if (ret) {
+			dev_err(dev, "Error reading firmware %d\n", ret);
+			mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+			break;
+		}
+
+		/*
+		 * init.
+		 */
+		if (start_flag) {
+			ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
+						      fw->size);
+			start_flag = 0;
+			if (ret)
+				break;
+		}
+
+		/*
+		 * 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;
+			break;
+		}
+
+		offset += fw->size;
+		size -= fw->size;
+		if (fw->size < streamsize)
+			break;
+	}
+
+	ret = fpga_mgr_write_complete(mgr, info);
+
+	release_firmware(fw);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
+
 int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
 {
 	if (info->firmware_name)
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 0f5072c..a25362e 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -151,6 +151,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);
+
 int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
 
 int fpga_mgr_lock(struct fpga_manager *mgr);
-- 
2.7.4

Re: [RFC 0/2] Add streaming API for firmware and FPGA manager

[matthew.gerlach]

On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:

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


Hi Yi,

This functionality is extremely helpful. I am working with a
firmware image of about 90 MBs, and even using scatter-gather instead of a 
continguous piece of memory is a lot of memory.

Matthew Gerlach

>
> As the FPGA hardware advances, the firmware image size grows (hundres
> of MBs or more). It could be an issue for kernel to allocate a big
> buffer to load the whole firmware file.
> Here is an attempt to read the firmware file into a small buffer and
> program the FPGA in a loop (or so call the streaming mode). It should not
> be a performance hit for FPGA programing since the majority of time
> spend is with the FPGA CvP/PR-IP interface.
>
> pseudo code in fpga manager:
> while (size > 0) {
> 	ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
> 	ret = mgr->mops->write(mgr, fw->data, fw->size);
> 	offset += fw->size;
> 	size -= fw->size;
> }
>
> Thanks,
> Yi
>
> Yi Li (2):
>  firmware class: Add stream_firmware API.
>  fpga manager: Add fpga_mgr_firmware_stream API
>
> drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
> drivers/fpga/fpga-mgr.c       |  77 +++++++++++++++++++++++++
> include/linux/firmware.h      |   2 +
> include/linux/fpga/fpga-mgr.h |   4 ++
> 4 files changed, 211 insertions(+)
>
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[matthew.gerlach]

On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:

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


Hi Yi,

Just one question below.

Matthew Gerlach


> Add function to load firmware in multiple chucks instead of
>
> loading the whole big firmware file at once.
>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
> drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/firmware.h      |   2 +
> 2 files changed, 130 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c5..44fddff 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
> 	return rc;
> }
>
> +static int
> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +			size_t offset, size_t length)
> +{
> +	int i, len;
> +	char *path;
> +	int rc = 0;
> +	struct file *file;
> +
> +	buf->size = 0;
> +
> +	path = __getname();
> +	if (!path)
> +		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",
> +			       fw_path[i], buf->fw_id);

I'm probably being paranoid, but is it safe to assume the length of the 
buffer returned by __getname() is at least PATH_MAX?  It seems like
the length should be pagesize.

> +		if (len >= PATH_MAX) {
> +			rc = -ENAMETOOLONG;
> +			break;
> +		}
> +
> +		if (!path || !*path)
> +			continue;
> +
> +		if (!buf->data) {
> +			buf->data = vmalloc(length);
> +			if (!buf->data) {
> +				rc = -ENOMEM;
> +				break;
> +			}
> +		}
> +
> +		file = filp_open(path, O_RDONLY, 0);
> +		if (IS_ERR(file))
> +			continue;
> +
> +		buf->size = kernel_read(file, offset, (char *) buf->data,
> +					length);
> +		fput(file);
> +		break;
> +	}
> +
> +	__putname(path);
> +
> +	if (rc)
> +		dev_err(device, "loading %s failed with error %d\n",
> +			 path, rc);
> +	return rc;
> +}
> +
> /* firmware holds the ownership of pages */
> static void firmware_free_data(const struct firmware *fw)
> {
> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> }
> EXPORT_SYMBOL(request_firmware);
>
> +static int
> +_stream_firmware(const struct firmware **firmware_p, const char *name,
> +		  struct device *device, void *buf, size_t size,
> +		  unsigned int opt_flags, size_t offset, size_t length)
> +{
> +	int ret;
> +	struct firmware *fw = NULL;
> +	struct firmware_buf *fbuf;
> +
> +	if ((!firmware_p) || (!name || name[0] == '\0')) {
> +		dev_err(device, "invalid firmware pointer or file name\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!*firmware_p) {
> +		ret = _request_firmware_prepare(&fw, name, device, buf, size);
> +		if (ret <= 0) {
> +			dev_err(device, "%s: _request_firmware_prepare failed %d\n",
> +				__func__, ret);
> +		}
> +	} else {
> +		fw = (struct firmware *) *firmware_p;
> +	}
> +
> +	fbuf = (struct firmware_buf *) fw->priv;
> +	ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
> +	fw->size = fbuf->size;
> +	fw->data = fbuf->data;
> +	*firmware_p = fw;
> +
> +	if (ret)
> +		dev_err(device, "streaming with error %d\n", ret);
> +	return ret;
> +}
> +
> +/**
> + * stream_firmware: - send firmware request and wait for it
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + * @offset: offset of the file to read from
> + * @length: length in bytes to read
> + *
> + *      @firmware_p will be used to return a firmware image by the name
> + *      of @name for device @device.
> + *
> + *      Should be called from user context where sleeping is allowed.
> + *
> + *      @name will be used as $FIRMWARE in the uevent environment and
> + *      should be distinctive enough not to be confused with any other
> + *      firmware image for this or any other device.
> + *
> + *	Caller must hold the reference count of @device.
> + *
> + *	The function can be called safely inside device's suspend and
> + *	resume callback.
> + **/
> +int
> +stream_firmware(const struct firmware **firmware_p, const char *name,
> +		 struct device *device, size_t offset, size_t length)
> +{
> +	size_t ret;
> +
> +	/* Need to pin this module until return */
> +	__module_get(THIS_MODULE);
> +	ret = _stream_firmware(firmware_p, name, device, NULL, 0,
> +				FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
> +	module_put(THIS_MODULE);
> +	return ret;
> +}
> +EXPORT_SYMBOL(stream_firmware);
> +
> /**
>  * request_firmware_direct: - load firmware directly without usermode helper
>  * @firmware_p: pointer to firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0c..accd7f6 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -41,6 +41,8 @@ struct builtin_fw {
> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
> int request_firmware(const struct firmware **fw, const char *name,
> 		     struct device *device);
> +int stream_firmware(const struct firmware **fw, const char *name,
> +		    struct device *device, size_t offset, size_t length);
> int request_firmware_nowait(
> 	struct module *module, bool uevent,
> 	const char *name, struct device *device, gfp_t gfp, void *context,
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Li, Yi]

Hi Matthew


On 3/10/2017 11:44 AM, matthew.gerlach@linux.intel.com wrote:
>
>
> On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:
>
>> From: Yi Li <yi1.li@linux.intel.com>
>
>
> Hi Yi,
>
> Just one question below.
>
> Matthew Gerlach
>
>
>> Add function to load firmware in multiple chucks instead of
>>
>> loading the whole big firmware file at once.
>>
>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>> ---
>> drivers/base/firmware_class.c | 128 
>> ++++++++++++++++++++++++++++++++++++++++++
>> include/linux/firmware.h      |   2 +
>> 2 files changed, 130 insertions(+)
>>
>> diff --git a/drivers/base/firmware_class.c 
>> b/drivers/base/firmware_class.c
>> index ac350c5..44fddff 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device 
>> *device, struct firmware_buf *buf)
>>     return rc;
>> }
>>
>> +static int
>> +fw_stream_filesystem_firmware(struct device *device, struct 
>> firmware_buf *buf,
>> +            size_t offset, size_t length)
>> +{
>> +    int i, len;
>> +    char *path;
>> +    int rc = 0;
>> +    struct file *file;
>> +
>> +    buf->size = 0;
>> +
>> +    path = __getname();
>> +    if (!path)
>> +        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",
>> +                   fw_path[i], buf->fw_id);
>
> I'm probably being paranoid, but is it safe to assume the length of 
> the buffer returned by __getname() is at least PATH_MAX?  It seems like
> the length should be pagesize.

The size should be the maximum number of char of the string be produced, 
not the input size.
According to 
https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
/The|snprintf|function is similar to|sprintf|, except that 
thesizeargument specifies the maximum number of characters to produce. 
The trailing null character is counted towards this limit, so you should 
allocate at leastsizecharacters for the strings. Ifsizeis zero, nothing, 
not even the null byte, shall be written andsmay be a null pointer.
The return value is the number of characters which would be generated 
for the given input, excluding the trailing null. If this value is 
greater than or equal tosize, not all characters from the result have 
been stored ins

>
>> +        if (len >= PATH_MAX) {
>> +            rc = -ENAMETOOLONG;
>> +            break;
>> +        }
>> +
>> +        if (!path || !*path)
>> +            continue;
>> +
>> +        if (!buf->data) {
>> +            buf->data = vmalloc(length);
>> +            if (!buf->data) {
>> +                rc = -ENOMEM;
>> +                break;
>> +            }
>> +        }
>> +
>> +        file = filp_open(path, O_RDONLY, 0);
>> +        if (IS_ERR(file))
>> +            continue;
>> +
>> +        buf->size = kernel_read(file, offset, (char *) buf->data,
>> +                    length);
>> +        fput(file);
>> +        break;
>> +    }
>> +
>> +    __putname(path);
>> +
>> +    if (rc)
>> +        dev_err(device, "loading %s failed with error %d\n",
>> +             path, rc);
>> +    return rc;
>> +}
>> +
>> /* firmware holds the ownership of pages */
>> static void firmware_free_data(const struct firmware *fw)
>> {
>> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware 
>> **firmware_p, const char *name,
>> }
>> EXPORT_SYMBOL(request_firmware);
>>
>> +static int
>> +_stream_firmware(const struct firmware **firmware_p, const char *name,
>> +          struct device *device, void *buf, size_t size,
>> +          unsigned int opt_flags, size_t offset, size_t length)
>> +{
>> +    int ret;
>> +    struct firmware *fw = NULL;
>> +    struct firmware_buf *fbuf;
>> +
>> +    if ((!firmware_p) || (!name || name[0] == '\0')) {
>> +        dev_err(device, "invalid firmware pointer or file name\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!*firmware_p) {
>> +        ret = _request_firmware_prepare(&fw, name, device, buf, size);
>> +        if (ret <= 0) {
>> +            dev_err(device, "%s: _request_firmware_prepare failed 
>> %d\n",
>> +                __func__, ret);
>> +        }
>> +    } else {
>> +        fw = (struct firmware *) *firmware_p;
>> +    }
>> +
>> +    fbuf = (struct firmware_buf *) fw->priv;
>> +    ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
>> +    fw->size = fbuf->size;
>> +    fw->data = fbuf->data;
>> +    *firmware_p = fw;
>> +
>> +    if (ret)
>> +        dev_err(device, "streaming with error %d\n", ret);
>> +    return ret;
>> +}
>> +
>> +/**
>> + * stream_firmware: - send firmware request and wait for it
>> + * @firmware_p: pointer to firmware image
>> + * @name: name of firmware file
>> + * @device: device for which firmware is being loaded
>> + * @offset: offset of the file to read from
>> + * @length: length in bytes to read
>> + *
>> + *      @firmware_p will be used to return a firmware image by the name
>> + *      of @name for device @device.
>> + *
>> + *      Should be called from user context where sleeping is allowed.
>> + *
>> + *      @name will be used as $FIRMWARE in the uevent environment and
>> + *      should be distinctive enough not to be confused with any other
>> + *      firmware image for this or any other device.
>> + *
>> + *    Caller must hold the reference count of @device.
>> + *
>> + *    The function can be called safely inside device's suspend and
>> + *    resume callback.
>> + **/
>> +int
>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>> +         struct device *device, size_t offset, size_t length)
>> +{
>> +    size_t ret;
>> +
>> +    /* Need to pin this module until return */
>> +    __module_get(THIS_MODULE);
>> +    ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>> +                FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
>> +    module_put(THIS_MODULE);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL(stream_firmware);
>> +
>> /**
>>  * request_firmware_direct: - load firmware directly without usermode 
>> helper
>>  * @firmware_p: pointer to firmware image
>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>> index b1f9f0c..accd7f6 100644
>> --- a/include/linux/firmware.h
>> +++ b/include/linux/firmware.h
>> @@ -41,6 +41,8 @@ struct builtin_fw {
>> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
>> defined(MODULE))
>> int request_firmware(const struct firmware **fw, const char *name,
>>              struct device *device);
>> +int stream_firmware(const struct firmware **fw, const char *name,
>> +            struct device *device, size_t offset, size_t length);
>> int request_firmware_nowait(
>>     struct module *module, bool uevent,
>>     const char *name, struct device *device, gfp_t gfp, void *context,
>> -- 
>> 2.7.4
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>

Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API

[Alan Tull]

On Thu, Mar 9, 2017 at 6:18 PM,  <yi1.li@linux.intel.com> wrote:

Hi Yi,

Thanks for your RFC.  I believe this functionality is badly needed.

I had a few comments about the chunk size and some nits about comments below...

> From: Yi Li <yi1.li@linux.intel.com>
>
> Add fpga_mgr_firmware_stream API, which can load and program firmware
>
> in trucks to FPGA instead of the whole file.
>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/fpga/fpga-mgr.c       | 77 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h |  4 +++
>  2 files changed, 81 insertions(+)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 3206a53..bb55b80 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -27,10 +27,15 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include <linux/sizes.h>
>
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
>
> +static int streamsize = SZ_4K;
> +module_param(streamsize, int, 0664);
> +MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");

I think we could fix the chunk size at PAGE_SIZE unless someone can
state a  reason for needing something different.  Below I have one
reason why we will sometimes need > PAGE_SIZE.

> +
>  /*
>   * Call the low level driver's write_init function.  This will do the
>   * device-specific things to get the FPGA into the state where it is ready to
> @@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>
> +/**
> + * fpga_mgr_firmware_load - request firmware and load to fpga
> + * @mgr:       fpga manager
> + * @info:      fpga image specific information
> + * @image_name:        name of image file on the firmware search path
> + *
> + * Request 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)

No need for both fpga_mgr_firmware_load and fpga_mgr_firmware_stream.
Just replace the old fpga_mgr_firmware_load with this function.

> +{
> +       struct device *dev = &mgr->dev;
> +       const struct firmware *fw = NULL;
> +       int ret;
> +       size_t size = INT_MAX, offset = 0;
> +       bool start_flag = 1;
> +
> +       dev_info(dev, "writing %s to %s with buffer size %d\n",
> +                       image_name, mgr->name, streamsize);
> +
> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> +       while (size > 0) {
> +               ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
> +               if (ret) {
> +                       dev_err(dev, "Error reading firmware %d\n", ret);
> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> +                       break;
> +               }
> +
> +               /*
> +                * init.
> +                */

We don't really need this comment.

> +               if (start_flag) {
> +                       ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
> +                                                     fw->size);

When the fpga manager is registered, one of the ops is
initial_header_size, which specifies how much buffer the write_init
needs.  So this call could fail if initial_header_size < streamsize.

I suggest that you make the streamsize equal to the smallest multiple
of PAGE_SIZE that is >= initial_header_size.

> +                       start_flag = 0;
> +                       if (ret)
> +                               break;
> +               }
> +
> +               /*
> +                * Write the FPGA image to the FPGA.
> +                */

Or this comment.

> +               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;
> +                       break;
> +               }
> +
> +               offset += fw->size;
> +               size -= fw->size;
> +               if (fw->size < streamsize)
> +                       break;
> +       }
> +
> +       ret = fpga_mgr_write_complete(mgr, info);
> +
> +       release_firmware(fw);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
> +
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>  {
>         if (info->firmware_name)
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 0f5072c..a25362e 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -151,6 +151,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);

Don't need both fpga_mgr_firmware_load and fpga_mgr_firmware_stream,
so no need to change the header.

> +
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>
>  int fpga_mgr_lock(struct fpga_manager *mgr);
> --
> 2.7.4
>

Re: [RFC 2/2] fpga manager: Add fpga_mgr_firmware_stream API

[Li, Yi]

Thanks Alan for the comments.


On 3/13/2017 1:00 PM, Alan Tull wrote:
> On Thu, Mar 9, 2017 at 6:18 PM,  <yi1.li@linux.intel.com> wrote:
>
> Hi Yi,
>
> Thanks for your RFC.  I believe this functionality is badly needed.
>
> I had a few comments about the chunk size and some nits about comments below...
>
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Add fpga_mgr_firmware_stream API, which can load and program firmware
>>
>> in trucks to FPGA instead of the whole file.
>>
>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>> ---
>>   drivers/fpga/fpga-mgr.c       | 77 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/fpga/fpga-mgr.h |  4 +++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 3206a53..bb55b80 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -27,10 +27,15 @@
>>   #include <linux/slab.h>
>>   #include <linux/scatterlist.h>
>>   #include <linux/highmem.h>
>> +#include <linux/sizes.h>
>>
>>   static DEFINE_IDA(fpga_mgr_ida);
>>   static struct class *fpga_mgr_class;
>>
>> +static int streamsize = SZ_4K;
>> +module_param(streamsize, int, 0664);
>> +MODULE_PARM_DESC(streamsize, "buffer size for firmware streaming");
> I think we could fix the chunk size at PAGE_SIZE unless someone can
> state a  reason for needing something different.  Below I have one
> reason why we will sometimes need > PAGE_SIZE.
>
>> +
>>   /*
>>    * Call the low level driver's write_init function.  This will do the
>>    * device-specific things to get the FPGA into the state where it is ready to
>> @@ -309,6 +314,78 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>>   }
>>   EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>>
>> +/**
>> + * fpga_mgr_firmware_load - request firmware and load to fpga
>> + * @mgr:       fpga manager
>> + * @info:      fpga image specific information
>> + * @image_name:        name of image file on the firmware search path
>> + *
>> + * Request 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)
> No need for both fpga_mgr_firmware_load and fpga_mgr_firmware_stream.
> Just replace the old fpga_mgr_firmware_load with this function.

Sure, will do.

>
>> +{
>> +       struct device *dev = &mgr->dev;
>> +       const struct firmware *fw = NULL;
>> +       int ret;
>> +       size_t size = INT_MAX, offset = 0;
>> +       bool start_flag = 1;
>> +
>> +       dev_info(dev, "writing %s to %s with buffer size %d\n",
>> +                       image_name, mgr->name, streamsize);
>> +
>> +       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
>> +
>> +       while (size > 0) {
>> +               ret = stream_firmware(&fw, image_name, dev, offset, streamsize);
>> +               if (ret) {
>> +                       dev_err(dev, "Error reading firmware %d\n", ret);
>> +                       mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
>> +                       break;
>> +               }
>> +
>> +               /*
>> +                * init.
>> +                */
> We don't really need this comment.
>
>> +               if (start_flag) {
>> +                       ret = fpga_mgr_write_init_buf(mgr, info, fw->data,
>> +                                                     fw->size);
> When the fpga manager is registered, one of the ops is
> initial_header_size, which specifies how much buffer the write_init
> needs.  So this call could fail if initial_header_size < streamsize.
>
> I suggest that you make the streamsize equal to the smallest multiple
> of PAGE_SIZE that is >= initial_header_size.

Agree, we can fix the streamsize to something bigger than 
initial_header_size for the bitstream file. Another reason to have the 
variable streamsize is for the performance, although with Altera Cyclone 
V FPGA CvP I did not see much difference when increasing the buffer 
size, since the bottleneck is with programming through PCIe and the time 
spent with firmware file reading is negligible.

>
>> +                       start_flag = 0;
>> +                       if (ret)
>> +                               break;
>> +               }
>> +
>> +               /*
>> +                * Write the FPGA image to the FPGA.
>> +                */
> Or this comment.
>
>> +               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;
>> +                       break;
>> +               }
>> +
>> +               offset += fw->size;
>> +               size -= fw->size;
>> +               if (fw->size < streamsize)
>> +                       break;
>> +       }
>> +
>> +       ret = fpga_mgr_write_complete(mgr, info);
>> +
>> +       release_firmware(fw);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_stream);
>> +
>>   int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
>>   {
>>          if (info->firmware_name)
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 0f5072c..a25362e 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -151,6 +151,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);
> Don't need both fpga_mgr_firmware_load and fpga_mgr_firmware_stream,
> so no need to change the header.
>
>> +
>>   int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info);
>>
>>   int fpga_mgr_lock(struct fpga_manager *mgr);
>> --
>> 2.7.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 8052 bytes --]



On Fri, 10 Mar 2017, Li, Yi wrote:

> Hi Matthew
>
Hi Yi,


>
> On 3/10/2017 11:44 AM, matthew.gerlach@linux.intel.com wrote:
>> 
>> 
>> On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:
>> 
>>> From: Yi Li <yi1.li@linux.intel.com>
>> 
>> 
>> Hi Yi,
>> 
>> Just one question below.
>> 
>> Matthew Gerlach
>> 
>> 
>>> Add function to load firmware in multiple chucks instead of
>>> 
>>> loading the whole big firmware file at once.
>>> 
>>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>>> ---
>>> drivers/base/firmware_class.c | 128 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/firmware.h      |   2 +
>>> 2 files changed, 130 insertions(+)
>>> 
>>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>>> index ac350c5..44fddff 100644
>>> --- a/drivers/base/firmware_class.c
>>> +++ b/drivers/base/firmware_class.c
>>> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, 
>>> struct firmware_buf *buf)
>>>     return rc;
>>> }
>>> 
>>> +static int
>>> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf 
>>> *buf,
>>> +            size_t offset, size_t length)
>>> +{
>>> +    int i, len;
>>> +    char *path;
>>> +    int rc = 0;
>>> +    struct file *file;
>>> +
>>> +    buf->size = 0;
>>> +
>>> +    path = __getname();
>>> +    if (!path)
>>> +        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",
>>> +                   fw_path[i], buf->fw_id);
>> 
>> I'm probably being paranoid, but is it safe to assume the length of the 
>> buffer returned by __getname() is at least PATH_MAX?  It seems like
>> the length should be pagesize.
>
> The size should be the maximum number of char of the string be produced, not 
> the input size.
> According to 
> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
> /The|snprintf|function is similar to|sprintf|, except that thesizeargument 
> specifies the maximum number of characters to produce. The trailing null 
> character is counted towards this limit, so you should allocate at 
> leastsizecharacters for the strings. Ifsizeis zero, nothing, not even the 
> null byte, shall be written andsmay be a null pointer.
> The return value is the number of characters which would be generated for the 
> given input, excluding the trailing null. If this value is greater than or 
> equal tosize, not all characters from the result have been stored ins
>

I am familiar with the functionality of snprintf versus sprintf.  In the
snprintf call above, you are saying that memory pointed to by the variable 
path, has at least PATH_MAX number of bytes.  My question is how can you 
know that the memory returned by __getname() has PATH_MAX number of bytes?


>> 
>>> +        if (len >= PATH_MAX) {
>>> +            rc = -ENAMETOOLONG;
>>> +            break;
>>> +        }
>>> +
>>> +        if (!path || !*path)
>>> +            continue;
>>> +
>>> +        if (!buf->data) {
>>> +            buf->data = vmalloc(length);
>>> +            if (!buf->data) {
>>> +                rc = -ENOMEM;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        file = filp_open(path, O_RDONLY, 0);
>>> +        if (IS_ERR(file))
>>> +            continue;
>>> +
>>> +        buf->size = kernel_read(file, offset, (char *) buf->data,
>>> +                    length);
>>> +        fput(file);
>>> +        break;
>>> +    }
>>> +
>>> +    __putname(path);
>>> +
>>> +    if (rc)
>>> +        dev_err(device, "loading %s failed with error %d\n",
>>> +             path, rc);
>>> +    return rc;
>>> +}
>>> +
>>> /* firmware holds the ownership of pages */
>>> static void firmware_free_data(const struct firmware *fw)
>>> {
>>> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware 
>>> **firmware_p, const char *name,
>>> }
>>> EXPORT_SYMBOL(request_firmware);
>>> 
>>> +static int
>>> +_stream_firmware(const struct firmware **firmware_p, const char *name,
>>> +          struct device *device, void *buf, size_t size,
>>> +          unsigned int opt_flags, size_t offset, size_t length)
>>> +{
>>> +    int ret;
>>> +    struct firmware *fw = NULL;
>>> +    struct firmware_buf *fbuf;
>>> +
>>> +    if ((!firmware_p) || (!name || name[0] == '\0')) {
>>> +        dev_err(device, "invalid firmware pointer or file name\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (!*firmware_p) {
>>> +        ret = _request_firmware_prepare(&fw, name, device, buf, size);
>>> +        if (ret <= 0) {
>>> +            dev_err(device, "%s: _request_firmware_prepare failed %d\n",
>>> +                __func__, ret);
>>> +        }
>>> +    } else {
>>> +        fw = (struct firmware *) *firmware_p;
>>> +    }
>>> +
>>> +    fbuf = (struct firmware_buf *) fw->priv;
>>> +    ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
>>> +    fw->size = fbuf->size;
>>> +    fw->data = fbuf->data;
>>> +    *firmware_p = fw;
>>> +
>>> +    if (ret)
>>> +        dev_err(device, "streaming with error %d\n", ret);
>>> +    return ret;
>>> +}
>>> +
>>> +/**
>>> + * stream_firmware: - send firmware request and wait for it
>>> + * @firmware_p: pointer to firmware image
>>> + * @name: name of firmware file
>>> + * @device: device for which firmware is being loaded
>>> + * @offset: offset of the file to read from
>>> + * @length: length in bytes to read
>>> + *
>>> + *      @firmware_p will be used to return a firmware image by the name
>>> + *      of @name for device @device.
>>> + *
>>> + *      Should be called from user context where sleeping is allowed.
>>> + *
>>> + *      @name will be used as $FIRMWARE in the uevent environment and
>>> + *      should be distinctive enough not to be confused with any other
>>> + *      firmware image for this or any other device.
>>> + *
>>> + *    Caller must hold the reference count of @device.
>>> + *
>>> + *    The function can be called safely inside device's suspend and
>>> + *    resume callback.
>>> + **/
>>> +int
>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>> +         struct device *device, size_t offset, size_t length)
>>> +{
>>> +    size_t ret;
>>> +
>>> +    /* Need to pin this module until return */
>>> +    __module_get(THIS_MODULE);
>>> +    ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>> +                FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
>>> +    module_put(THIS_MODULE);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL(stream_firmware);
>>> +
>>> /**
>>>  * request_firmware_direct: - load firmware directly without usermode 
>>> helper
>>>  * @firmware_p: pointer to firmware image
>>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>>> index b1f9f0c..accd7f6 100644
>>> --- a/include/linux/firmware.h
>>> +++ b/include/linux/firmware.h
>>> @@ -41,6 +41,8 @@ struct builtin_fw {
>>> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
>>> defined(MODULE))
>>> int request_firmware(const struct firmware **fw, const char *name,
>>>              struct device *device);
>>> +int stream_firmware(const struct firmware **fw, const char *name,
>>> +            struct device *device, size_t offset, size_t length);
>>> int request_firmware_nowait(
>>>     struct module *module, bool uevent,
>>>     const char *name, struct device *device, gfp_t gfp, void *context,
>>> -- 
>>> 2.7.4
>>> 
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Li, Yi]

hi Matthew,


On 3/13/2017 4:09 PM, matthew.gerlach@linux.intel.com wrote:
>
>
> On Fri, 10 Mar 2017, Li, Yi wrote:
>
>> Hi Matthew
>>
> Hi Yi,
>
>
>>
>> On 3/10/2017 11:44 AM, matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:
>>>
>>>> From: Yi Li <yi1.li@linux.intel.com>
>>>
>>>
>>> Hi Yi,
>>>
>>> Just one question below.
>>>
>>> Matthew Gerlach
>>>
>>>
>>>> Add function to load firmware in multiple chucks instead of
>>>>
>>>> loading the whole big firmware file at once.
>>>>
>>>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>>>> ---
>>>> drivers/base/firmware_class.c | 128 
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>> include/linux/firmware.h      |   2 +
>>>> 2 files changed, 130 insertions(+)
>>>>
>>>> diff --git a/drivers/base/firmware_class.c 
>>>> b/drivers/base/firmware_class.c
>>>> index ac350c5..44fddff 100644
>>>> --- a/drivers/base/firmware_class.c
>>>> +++ b/drivers/base/firmware_class.c
>>>> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device 
>>>> *device, struct firmware_buf *buf)
>>>>     return rc;
>>>> }
>>>>
>>>> +static int
>>>> +fw_stream_filesystem_firmware(struct device *device, struct 
>>>> firmware_buf *buf,
>>>> +            size_t offset, size_t length)
>>>> +{
>>>> +    int i, len;
>>>> +    char *path;
>>>> +    int rc = 0;
>>>> +    struct file *file;
>>>> +
>>>> +    buf->size = 0;
>>>> +
>>>> +    path = __getname();
>>>> +    if (!path)
>>>> +        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",
>>>> +                   fw_path[i], buf->fw_id);
>>>
>>> I'm probably being paranoid, but is it safe to assume the length of 
>>> the buffer returned by __getname() is at least PATH_MAX?  It seems like
>>> the length should be pagesize.
>>
>> The size should be the maximum number of char of the string be 
>> produced, not the input size.
>> According to 
>> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
>> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
>> /The|snprintf|function is similar to|sprintf|, except that 
>> thesizeargument specifies the maximum number of characters to 
>> produce. The trailing null character is counted towards this limit, 
>> so you should allocate at leastsizecharacters for the strings. 
>> Ifsizeis zero, nothing, not even the null byte, shall be written 
>> andsmay be a null pointer.
>> The return value is the number of characters which would be generated 
>> for the given input, excluding the trailing null. If this value is 
>> greater than or equal tosize, not all characters from the result have 
>> been stored ins
>>
>
> I am familiar with the functionality of snprintf versus sprintf. In the
> snprintf call above, you are saying that memory pointed to by the 
> variable path, has at least PATH_MAX number of bytes.  My question is 
> how can you know that the memory returned by __getname() has PATH_MAX 
> number of bytes?
>
>

Ah, now I understand the concerns.

The __getname() will allocate an buffer object from names_cachep
extern struct kmem_cache 
<http://lxr.free-electrons.com/ident?i=kmem_cache> *names_cachep 
<http://lxr.free-electrons.com/ident?i=names_cachep>;
#define __getname <http://lxr.free-electrons.com/ident?i=__getname>() 
kmem_cache_alloc 
<http://lxr.free-electrons.com/ident?i=kmem_cache_alloc>(names_cachep 
<http://lxr.free-electrons.com/ident?i=names_cachep>, GFP_KERNEL 
<http://lxr.free-electrons.com/ident?i=GFP_KERNEL>)

names_cachep is created in fs/dcaches.c vfs_caches_init function with 
object size equal to PATH_MAX.
names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep> = 
kmem_cache_create 
<http://lxr.free-electrons.com/ident?i=kmem_cache_create>(/"names_cache"/, 
PATH_MAX <http://lxr.free-electrons.com/ident?i=PATH_MAX>, 
0,SLAB_HWCACHE_ALIGN 
<http://lxr.free-electrons.com/ident?i=SLAB_HWCACHE_ALIGN>|SLAB_PANIC 
<http://lxr.free-electrons.com/ident?i=SLAB_PANIC>, NULL 
<http://lxr.free-electrons.com/ident?i=NULL>);

so __getname() should allocate buffer with size of PATH_MAX.

The code is borrowed from fw_get_filesystem_firmware function, which 
should be reviewed and safe to use. :)

Thanks,
Yi


>>>
>>>> +        if (len >= PATH_MAX) {
>>>> +            rc = -ENAMETOOLONG;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        if (!path || !*path)
>>>> +            continue;
>>>> +
>>>> +        if (!buf->data) {
>>>> +            buf->data = vmalloc(length);
>>>> +            if (!buf->data) {
>>>> +                rc = -ENOMEM;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        file = filp_open(path, O_RDONLY, 0);
>>>> +        if (IS_ERR(file))
>>>> +            continue;
>>>> +
>>>> +        buf->size = kernel_read(file, offset, (char *) buf->data,
>>>> +                    length);
>>>> +        fput(file);
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    __putname(path);
>>>> +
>>>> +    if (rc)
>>>> +        dev_err(device, "loading %s failed with error %d\n",
>>>> +             path, rc);
>>>> +    return rc;
>>>> +}
>>>> +
>>>> /* firmware holds the ownership of pages */
>>>> static void firmware_free_data(const struct firmware *fw)
>>>> {
>>>> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware 
>>>> **firmware_p, const char *name,
>>>> }
>>>> EXPORT_SYMBOL(request_firmware);
>>>>
>>>> +static int
>>>> +_stream_firmware(const struct firmware **firmware_p, const char 
>>>> *name,
>>>> +          struct device *device, void *buf, size_t size,
>>>> +          unsigned int opt_flags, size_t offset, size_t length)
>>>> +{
>>>> +    int ret;
>>>> +    struct firmware *fw = NULL;
>>>> +    struct firmware_buf *fbuf;
>>>> +
>>>> +    if ((!firmware_p) || (!name || name[0] == '\0')) {
>>>> +        dev_err(device, "invalid firmware pointer or file name\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    if (!*firmware_p) {
>>>> +        ret = _request_firmware_prepare(&fw, name, device, buf, 
>>>> size);
>>>> +        if (ret <= 0) {
>>>> +            dev_err(device, "%s: _request_firmware_prepare failed 
>>>> %d\n",
>>>> +                __func__, ret);
>>>> +        }
>>>> +    } else {
>>>> +        fw = (struct firmware *) *firmware_p;
>>>> +    }
>>>> +
>>>> +    fbuf = (struct firmware_buf *) fw->priv;
>>>> +    ret = fw_stream_filesystem_firmware(device, fbuf, offset, 
>>>> length);
>>>> +    fw->size = fbuf->size;
>>>> +    fw->data = fbuf->data;
>>>> +    *firmware_p = fw;
>>>> +
>>>> +    if (ret)
>>>> +        dev_err(device, "streaming with error %d\n", ret);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * stream_firmware: - send firmware request and wait for it
>>>> + * @firmware_p: pointer to firmware image
>>>> + * @name: name of firmware file
>>>> + * @device: device for which firmware is being loaded
>>>> + * @offset: offset of the file to read from
>>>> + * @length: length in bytes to read
>>>> + *
>>>> + *      @firmware_p will be used to return a firmware image by the 
>>>> name
>>>> + *      of @name for device @device.
>>>> + *
>>>> + *      Should be called from user context where sleeping is allowed.
>>>> + *
>>>> + *      @name will be used as $FIRMWARE in the uevent environment and
>>>> + *      should be distinctive enough not to be confused with any 
>>>> other
>>>> + *      firmware image for this or any other device.
>>>> + *
>>>> + *    Caller must hold the reference count of @device.
>>>> + *
>>>> + *    The function can be called safely inside device's suspend and
>>>> + *    resume callback.
>>>> + **/
>>>> +int
>>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>>> +         struct device *device, size_t offset, size_t length)
>>>> +{
>>>> +    size_t ret;
>>>> +
>>>> +    /* Need to pin this module until return */
>>>> +    __module_get(THIS_MODULE);
>>>> +    ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>>> +                FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
>>>> +    module_put(THIS_MODULE);
>>>> +    return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(stream_firmware);
>>>> +
>>>> /**
>>>>  * request_firmware_direct: - load firmware directly without 
>>>> usermode helper
>>>>  * @firmware_p: pointer to firmware image
>>>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>>>> index b1f9f0c..accd7f6 100644
>>>> --- a/include/linux/firmware.h
>>>> +++ b/include/linux/firmware.h
>>>> @@ -41,6 +41,8 @@ struct builtin_fw {
>>>> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) 
>>>> && defined(MODULE))
>>>> int request_firmware(const struct firmware **fw, const char *name,
>>>>              struct device *device);
>>>> +int stream_firmware(const struct firmware **fw, const char *name,
>>>> +            struct device *device, size_t offset, size_t length);
>>>> int request_firmware_nowait(
>>>>     struct module *module, bool uevent,
>>>>     const char *name, struct device *device, gfp_t gfp, void *context,
>>>> -- 
>>>> 2.7.4
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe 
>>>> linux-fpga" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 10063 bytes --]



On Tue, 14 Mar 2017, Li, Yi wrote:

> hi Matthew,
>
Hi Yi,

>
> On 3/13/2017 4:09 PM, matthew.gerlach@linux.intel.com wrote:
>> 
>> 
>> On Fri, 10 Mar 2017, Li, Yi wrote:
>> 
>>> Hi Matthew
>>> 
>> Hi Yi,
>> 
>> 
>>> 
>>> On 3/10/2017 11:44 AM, matthew.gerlach@linux.intel.com wrote:
>>>> 
>>>> 
>>>> On Thu, 9 Mar 2017, yi1.li@linux.intel.com wrote:
>>>> 
>>>>> From: Yi Li <yi1.li@linux.intel.com>
>>>> 
>>>> 
>>>> Hi Yi,
>>>> 
>>>> Just one question below.
>>>> 
>>>> Matthew Gerlach
>>>> 
>>>> 
>>>>> Add function to load firmware in multiple chucks instead of
>>>>> 
>>>>> loading the whole big firmware file at once.
>>>>> 
>>>>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>>>>> ---
>>>>> drivers/base/firmware_class.c | 128 
>>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>> include/linux/firmware.h      |   2 +
>>>>> 2 files changed, 130 insertions(+)
>>>>> 
>>>>> diff --git a/drivers/base/firmware_class.c 
>>>>> b/drivers/base/firmware_class.c
>>>>> index ac350c5..44fddff 100644
>>>>> --- a/drivers/base/firmware_class.c
>>>>> +++ b/drivers/base/firmware_class.c
>>>>> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, 
>>>>> struct firmware_buf *buf)
>>>>>     return rc;
>>>>> }
>>>>> 
>>>>> +static int
>>>>> +fw_stream_filesystem_firmware(struct device *device, struct 
>>>>> firmware_buf *buf,
>>>>> +            size_t offset, size_t length)
>>>>> +{
>>>>> +    int i, len;
>>>>> +    char *path;
>>>>> +    int rc = 0;
>>>>> +    struct file *file;
>>>>> +
>>>>> +    buf->size = 0;
>>>>> +
>>>>> +    path = __getname();
>>>>> +    if (!path)
>>>>> +        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",
>>>>> +                   fw_path[i], buf->fw_id);
>>>> 
>>>> I'm probably being paranoid, but is it safe to assume the length of the 
>>>> buffer returned by __getname() is at least PATH_MAX?  It seems like
>>>> the length should be pagesize.
>>> 
>>> The size should be the maximum number of char of the string be produced, 
>>> not the input size.
>>> According to 
>>> https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
>>> Function:/int/*snprintf*/(char *s, size_tsize, const char *template, …)
>>> /The|snprintf|function is similar to|sprintf|, except that thesizeargument 
>>> specifies the maximum number of characters to produce. The trailing null 
>>> character is counted towards this limit, so you should allocate at 
>>> leastsizecharacters for the strings. Ifsizeis zero, nothing, not even the 
>>> null byte, shall be written andsmay be a null pointer.
>>> The return value is the number of characters which would be generated for 
>>> the given input, excluding the trailing null. If this value is greater 
>>> than or equal tosize, not all characters from the result have been stored 
>>> ins
>>> 
>> 
>> I am familiar with the functionality of snprintf versus sprintf. In the
>> snprintf call above, you are saying that memory pointed to by the variable 
>> path, has at least PATH_MAX number of bytes.  My question is how can you 
>> know that the memory returned by __getname() has PATH_MAX number of bytes?
>> 
>> 
>
> Ah, now I understand the concerns.
>
> The __getname() will allocate an buffer object from names_cachep
> extern struct kmem_cache <http://lxr.free-electrons.com/ident?i=kmem_cache> 
> *names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep>;
> #define __getname <http://lxr.free-electrons.com/ident?i=__getname>() 
> kmem_cache_alloc 
> <http://lxr.free-electrons.com/ident?i=kmem_cache_alloc>(names_cachep 
> <http://lxr.free-electrons.com/ident?i=names_cachep>, GFP_KERNEL 
> <http://lxr.free-electrons.com/ident?i=GFP_KERNEL>)
>
> names_cachep is created in fs/dcaches.c vfs_caches_init function with object 
> size equal to PATH_MAX.
> names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep> = 
> kmem_cache_create 
> <http://lxr.free-electrons.com/ident?i=kmem_cache_create>(/"names_cache"/, 
> PATH_MAX <http://lxr.free-electrons.com/ident?i=PATH_MAX>, 
> 0,SLAB_HWCACHE_ALIGN 
> <http://lxr.free-electrons.com/ident?i=SLAB_HWCACHE_ALIGN>|SLAB_PANIC 
> <http://lxr.free-electrons.com/ident?i=SLAB_PANIC>, NULL 
> <http://lxr.free-electrons.com/ident?i=NULL>);
>
> so __getname() should allocate buffer with size of PATH_MAX.
>
> The code is borrowed from fw_get_filesystem_firmware function, which should 
> be reviewed and safe to use. :)

Thanks for following up. I am no longer paranoid.

Matthew Gerlach
>
> Thanks,
> Yi
>
>
>>>> 
>>>>> +        if (len >= PATH_MAX) {
>>>>> +            rc = -ENAMETOOLONG;
>>>>> +            break;
>>>>> +        }
>>>>> +
>>>>> +        if (!path || !*path)
>>>>> +            continue;
>>>>> +
>>>>> +        if (!buf->data) {
>>>>> +            buf->data = vmalloc(length);
>>>>> +            if (!buf->data) {
>>>>> +                rc = -ENOMEM;
>>>>> +                break;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        file = filp_open(path, O_RDONLY, 0);
>>>>> +        if (IS_ERR(file))
>>>>> +            continue;
>>>>> +
>>>>> +        buf->size = kernel_read(file, offset, (char *) buf->data,
>>>>> +                    length);
>>>>> +        fput(file);
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    __putname(path);
>>>>> +
>>>>> +    if (rc)
>>>>> +        dev_err(device, "loading %s failed with error %d\n",
>>>>> +             path, rc);
>>>>> +    return rc;
>>>>> +}
>>>>> +
>>>>> /* firmware holds the ownership of pages */
>>>>> static void firmware_free_data(const struct firmware *fw)
>>>>> {
>>>>> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware 
>>>>> **firmware_p, const char *name,
>>>>> }
>>>>> EXPORT_SYMBOL(request_firmware);
>>>>> 
>>>>> +static int
>>>>> +_stream_firmware(const struct firmware **firmware_p, const char *name,
>>>>> +          struct device *device, void *buf, size_t size,
>>>>> +          unsigned int opt_flags, size_t offset, size_t length)
>>>>> +{
>>>>> +    int ret;
>>>>> +    struct firmware *fw = NULL;
>>>>> +    struct firmware_buf *fbuf;
>>>>> +
>>>>> +    if ((!firmware_p) || (!name || name[0] == '\0')) {
>>>>> +        dev_err(device, "invalid firmware pointer or file name\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    if (!*firmware_p) {
>>>>> +        ret = _request_firmware_prepare(&fw, name, device, buf, size);
>>>>> +        if (ret <= 0) {
>>>>> +            dev_err(device, "%s: _request_firmware_prepare failed 
>>>>> %d\n",
>>>>> +                __func__, ret);
>>>>> +        }
>>>>> +    } else {
>>>>> +        fw = (struct firmware *) *firmware_p;
>>>>> +    }
>>>>> +
>>>>> +    fbuf = (struct firmware_buf *) fw->priv;
>>>>> +    ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
>>>>> +    fw->size = fbuf->size;
>>>>> +    fw->data = fbuf->data;
>>>>> +    *firmware_p = fw;
>>>>> +
>>>>> +    if (ret)
>>>>> +        dev_err(device, "streaming with error %d\n", ret);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * stream_firmware: - send firmware request and wait for it
>>>>> + * @firmware_p: pointer to firmware image
>>>>> + * @name: name of firmware file
>>>>> + * @device: device for which firmware is being loaded
>>>>> + * @offset: offset of the file to read from
>>>>> + * @length: length in bytes to read
>>>>> + *
>>>>> + *      @firmware_p will be used to return a firmware image by the name
>>>>> + *      of @name for device @device.
>>>>> + *
>>>>> + *      Should be called from user context where sleeping is allowed.
>>>>> + *
>>>>> + *      @name will be used as $FIRMWARE in the uevent environment and
>>>>> + *      should be distinctive enough not to be confused with any other
>>>>> + *      firmware image for this or any other device.
>>>>> + *
>>>>> + *    Caller must hold the reference count of @device.
>>>>> + *
>>>>> + *    The function can be called safely inside device's suspend and
>>>>> + *    resume callback.
>>>>> + **/
>>>>> +int
>>>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>>>> +         struct device *device, size_t offset, size_t length)
>>>>> +{
>>>>> +    size_t ret;
>>>>> +
>>>>> +    /* Need to pin this module until return */
>>>>> +    __module_get(THIS_MODULE);
>>>>> +    ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>>>> +                FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
>>>>> +    module_put(THIS_MODULE);
>>>>> +    return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL(stream_firmware);
>>>>> +
>>>>> /**
>>>>>  * request_firmware_direct: - load firmware directly without usermode 
>>>>> helper
>>>>>  * @firmware_p: pointer to firmware image
>>>>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>>>>> index b1f9f0c..accd7f6 100644
>>>>> --- a/include/linux/firmware.h
>>>>> +++ b/include/linux/firmware.h
>>>>> @@ -41,6 +41,8 @@ struct builtin_fw {
>>>>> #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
>>>>> defined(MODULE))
>>>>> int request_firmware(const struct firmware **fw, const char *name,
>>>>>              struct device *device);
>>>>> +int stream_firmware(const struct firmware **fw, const char *name,
>>>>> +            struct device *device, size_t offset, size_t length);
>>>>> int request_firmware_nowait(
>>>>>     struct module *module, bool uevent,
>>>>>     const char *name, struct device *device, gfp_t gfp, void *context,
>>>>> -- 
>>>>> 2.7.4
>>>>> 
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> 
>>> 
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 
>
>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Alan Tull]

On Thu, Mar 9, 2017 at 6:18 PM,  <yi1.li@linux.intel.com> wrote:

Hi Yi,

As FPGA image sizes are increasing, this change can be really helpful.
I have one comment below.

Alan Tull

> From: Yi Li <yi1.li@linux.intel.com>
>
> Add function to load firmware in multiple chucks instead of
>
> loading the whole big firmware file at once.
>
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware.h      |   2 +
>  2 files changed, 130 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c5..44fddff 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>         return rc;
>  }
>
> +static int
> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +                       size_t offset, size_t length)
> +{
> +       int i, len;
> +       char *path;
> +       int rc = 0;
> +       struct file *file;
> +
> +       buf->size = 0;
> +
> +       path = __getname();
> +       if (!path)
> +               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",
> +                              fw_path[i], buf->fw_id);
> +               if (len >= PATH_MAX) {
> +                       rc = -ENAMETOOLONG;
> +                       break;
> +               }
> +
> +               if (!path || !*path)
> +                       continue;
> +
> +               if (!buf->data) {
> +                       buf->data = vmalloc(length);
> +                       if (!buf->data) {
> +                               rc = -ENOMEM;
> +                               break;
> +                       }
> +               }

Your implementation is pretty straightforward.  My one complaint is
that this searches for the file on the firmware path each time we need
to get the next chunk of the firmware.  Could you change this to find
the file in the firmware path once and save that path somewhere?
Possibly added to struct firmware_buf since that's the priv.

> +
> +               file = filp_open(path, O_RDONLY, 0);
> +               if (IS_ERR(file))
> +                       continue;
> +
> +               buf->size = kernel_read(file, offset, (char *) buf->data,
> +                                       length);
> +               fput(file);
> +               break;
> +       }
> +
> +       __putname(path);
> +
> +       if (rc)
> +               dev_err(device, "loading %s failed with error %d\n",
> +                        path, rc);
> +       return rc;
> +}
> +
>  /* firmware holds the ownership of pages */
>  static void firmware_free_data(const struct firmware *fw)
>  {
> @@ -1267,6 +1323,78 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>  }
>  EXPORT_SYMBOL(request_firmware);
>
> +static int
> +_stream_firmware(const struct firmware **firmware_p, const char *name,
> +                 struct device *device, void *buf, size_t size,
> +                 unsigned int opt_flags, size_t offset, size_t length)
> +{
> +       int ret;
> +       struct firmware *fw = NULL;
> +       struct firmware_buf *fbuf;
> +
> +       if ((!firmware_p) || (!name || name[0] == '\0')) {
> +               dev_err(device, "invalid firmware pointer or file name\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!*firmware_p) {
> +               ret = _request_firmware_prepare(&fw, name, device, buf, size);
> +               if (ret <= 0) {
> +                       dev_err(device, "%s: _request_firmware_prepare failed %d\n",
> +                               __func__, ret);
> +               }
> +       } else {
> +               fw = (struct firmware *) *firmware_p;
> +       }
> +
> +       fbuf = (struct firmware_buf *) fw->priv;
> +       ret = fw_stream_filesystem_firmware(device, fbuf, offset, length);
> +       fw->size = fbuf->size;
> +       fw->data = fbuf->data;
> +       *firmware_p = fw;
> +
> +       if (ret)
> +               dev_err(device, "streaming with error %d\n", ret);
> +       return ret;
> +}
> +
> +/**
> + * stream_firmware: - send firmware request and wait for it
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded
> + * @offset: offset of the file to read from
> + * @length: length in bytes to read
> + *
> + *      @firmware_p will be used to return a firmware image by the name
> + *      of @name for device @device.
> + *
> + *      Should be called from user context where sleeping is allowed.
> + *
> + *      @name will be used as $FIRMWARE in the uevent environment and
> + *      should be distinctive enough not to be confused with any other
> + *      firmware image for this or any other device.
> + *
> + *     Caller must hold the reference count of @device.
> + *
> + *     The function can be called safely inside device's suspend and
> + *     resume callback.
> + **/
> +int
> +stream_firmware(const struct firmware **firmware_p, const char *name,
> +                struct device *device, size_t offset, size_t length)
> +{
> +       size_t ret;
> +
> +       /* Need to pin this module until return */
> +       __module_get(THIS_MODULE);
> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
> +       module_put(THIS_MODULE);
> +       return ret;
> +}
> +EXPORT_SYMBOL(stream_firmware);
> +
>  /**
>   * request_firmware_direct: - load firmware directly without usermode helper
>   * @firmware_p: pointer to firmware image
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0c..accd7f6 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -41,6 +41,8 @@ struct builtin_fw {
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>  int request_firmware(const struct firmware **fw, const char *name,
>                      struct device *device);
> +int stream_firmware(const struct firmware **fw, const char *name,
> +                   struct device *device, size_t offset, size_t length);
>  int request_firmware_nowait(
>         struct module *module, bool uevent,
>         const char *name, struct device *device, gfp_t gfp, void *context,
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Alan Tull]

On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:

>> +int
>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>> +                struct device *device, size_t offset, size_t length)
>> +{
>> +       size_t ret;
>> +
>> +       /* Need to pin this module until return */
>> +       __module_get(THIS_MODULE);
>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);

IIUC, here you are setting size == 0 and buf == NULL  to prevent
_request_firmware_prepare from attempting to load from built in
firmware.

So three of the parameters buf, size, and opt_flags are fixed and
don't need to be passed to _stream_firmware().

Alternatively, I wonder how hard it would be to code this so that the
streaming interface will fall back and successfully get the built in
or cached firmware if it exists and stream it out in PAGE_SIZE chunks.

Alan Tull

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Li, Yi]

Alan


On 3/20/2017 1:34 PM, Alan Tull wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com> wrote:
>
>>> +int
>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>> +                struct device *device, size_t offset, size_t length)
>>> +{
>>> +       size_t ret;
>>> +
>>> +       /* Need to pin this module until return */
>>> +       __module_get(THIS_MODULE);
>>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset, length);
> IIUC, here you are setting size == 0 and buf == NULL  to prevent
> _request_firmware_prepare from attempting to load from built in
> firmware.
>
> So three of the parameters buf, size, and opt_flags are fixed and
> don't need to be passed to _stream_firmware().

Sure.

> Alternatively, I wonder how hard it would be to code this so that the
> streaming interface will fall back and successfully get the built in
> or cached firmware if it exists and stream it out in PAGE_SIZE chunks.

That's an interesting idea, I will try it out and submit patch for 
review later. On another hand, if the kernel already cache the whole 
firmware image, why should we use streaming instead of regular 
request_firmware?

>
> Alan Tull
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Alan Tull]

On Wed, Mar 22, 2017 at 5:05 PM, Li, Yi <yi1.li@linux.intel.com> wrote:
> Alan
>
>
> On 3/20/2017 1:34 PM, Alan Tull wrote:
>>
>> On Mon, Mar 20, 2017 at 1:00 PM, Alan Tull <delicious.quinoa@gmail.com>
>> wrote:
>>
>>>> +int
>>>> +stream_firmware(const struct firmware **firmware_p, const char *name,
>>>> +                struct device *device, size_t offset, size_t length)
>>>> +{
>>>> +       size_t ret;
>>>> +
>>>> +       /* Need to pin this module until return */
>>>> +       __module_get(THIS_MODULE);
>>>> +       ret = _stream_firmware(firmware_p, name, device, NULL, 0,
>>>> +                               FW_OPT_UEVENT | FW_OPT_NO_WARN, offset,
>>>> length);
>>
>> IIUC, here you are setting size == 0 and buf == NULL  to prevent
>> _request_firmware_prepare from attempting to load from built in
>> firmware.
>>
>> So three of the parameters buf, size, and opt_flags are fixed and
>> don't need to be passed to _stream_firmware().
>
>
> Sure.
>
>> Alternatively, I wonder how hard it would be to code this so that the
>> streaming interface will fall back and successfully get the built in
>> or cached firmware if it exists and stream it out in PAGE_SIZE chunks.
>
>
> That's an interesting idea, I will try it out and submit patch for review
> later. On another hand, if the kernel already cache the whole firmware
> image, why should we use streaming instead of regular request_firmware?

The caller doesn't know whether the firmware is cached or not.  The
caller just wants the firmware if it exists, wherever it is.  If that can be
made automatic then the caller doesn't have to first attempt stream_firmware
and then if that fails fall back to calling request_firmware.

>
>>
>> Alan Tull
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Luis R. Rodriguez]

On Thu, Mar 09, 2017 at 06:18:09PM -0600, yi1.li@linux.intel.com wrote:
> From: Yi Li <yi1.li@linux.intel.com>
> 
> Add function to load firmware in multiple chucks instead of
> 
> loading the whole big firmware file at once.
> 
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/firmware.h      |   2 +
>  2 files changed, 130 insertions(+)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ac350c5..44fddff 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>  	return rc;
>  }
>  
> +static int
> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +			size_t offset, size_t length)
> +{
> +	int i, len;
> +	char *path;
> +	int rc = 0;
> +	struct file *file;
> +
> +	buf->size = 0;
> +
> +	path = __getname();
> +	if (!path)
> +		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",
> +			       fw_path[i], buf->fw_id);
> +		if (len >= PATH_MAX) {
> +			rc = -ENAMETOOLONG;
> +			break;
> +		}
> +
> +		if (!path || !*path)
> +			continue;
> +
> +		if (!buf->data) {
> +			buf->data = vmalloc(length);
> +			if (!buf->data) {
> +				rc = -ENOMEM;
> +				break;
> +			}
> +		}
> +
> +		file = filp_open(path, O_RDONLY, 0);
> +		if (IS_ERR(file))
> +			continue;
> +
> +		buf->size = kernel_read(file, offset, (char *) buf->data,
> +					length);
> +		fput(file);
> +		break;
> +	}
> +
> +	__putname(path);
> +
> +	if (rc)
> +		dev_err(device, "loading %s failed with error %d\n",
> +			 path, rc);
> +	return rc;
> +}

Yet another API call to read files form the fs seems rather odd, are you sure
nothing can be done to re-purpose the existing call ?

> +
> +EXPORT_SYMBOL(stream_firmware);

New functionality should be EXPORT_SYMBOL_GPL().

> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index b1f9f0c..accd7f6 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -41,6 +41,8 @@ struct builtin_fw {
>  #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>  int request_firmware(const struct firmware **fw, const char *name,
>  		     struct device *device);
> +int stream_firmware(const struct firmware **fw, const char *name,
> +		    struct device *device, size_t offset, size_t length);

Have you looked at the rather new request_firmware_into_buf() ? I hated that
as it did not get any proper code *review* but its there now... If we can
leverage off of it to give us something useful for actual upstream drivers
rather than cruft outside of the kernel I'd be a bit happier.

Also, please note that I had been noting we keep extending the firmware API
with loose APIs, I've been consolidating a bit of this into a newer API which
provides a flexible API for us. Since this is not upstream I don't expect
you to work off of that, but I will Cc you on some updated patches which
will fold in this work, I am expecting that the API we will ultimately use for
this feature you are preoposing could be folded there.

So for now, please consider the review notes above, and we can later see how
we fold this into a new set of APIs which I hope to bake this week.

  Luis

Re: [RFC 1/2] firmware class: Add stream_firmware API.

[Li, Yi]

hi Luis,


On 3/27/2017 2:36 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 09, 2017 at 06:18:09PM -0600, yi1.li@linux.intel.com wrote:
>> From: Yi Li <yi1.li@linux.intel.com>
>>
>> Add function to load firmware in multiple chucks instead of
>>
>> loading the whole big firmware file at once.
>>
>> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
>> ---
>>   drivers/base/firmware_class.c | 128 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/firmware.h      |   2 +
>>   2 files changed, 130 insertions(+)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index ac350c5..44fddff 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -436,6 +436,62 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
>>   	return rc;
>>   }
>>   
>> +static int
>> +fw_stream_filesystem_firmware(struct device *device, struct firmware_buf *buf,
>> +			size_t offset, size_t length)
>> +{
>> +	int i, len;
>> +	char *path;
>> +	int rc = 0;
>> +	struct file *file;
>> +
>> +	buf->size = 0;
>> +
>> +	path = __getname();
>> +	if (!path)
>> +		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",
>> +			       fw_path[i], buf->fw_id);
>> +		if (len >= PATH_MAX) {
>> +			rc = -ENAMETOOLONG;
>> +			break;
>> +		}
>> +
>> +		if (!path || !*path)
>> +			continue;
>> +
>> +		if (!buf->data) {
>> +			buf->data = vmalloc(length);
>> +			if (!buf->data) {
>> +				rc = -ENOMEM;
>> +				break;
>> +			}
>> +		}
>> +
>> +		file = filp_open(path, O_RDONLY, 0);
>> +		if (IS_ERR(file))
>> +			continue;
>> +
>> +		buf->size = kernel_read(file, offset, (char *) buf->data,
>> +					length);
>> +		fput(file);
>> +		break;
>> +	}
>> +
>> +	__putname(path);
>> +
>> +	if (rc)
>> +		dev_err(device, "loading %s failed with error %d\n",
>> +			 path, rc);
>> +	return rc;
>> +}
> Yet another API call to read files form the fs seems rather odd, are you sure
> nothing can be done to re-purpose the existing call ?

Agree, I also hated to duplicate the read files from fs, what we are 
missing in existing API is an "offset", which will enable loading the 
firmware in pieces.

>
>> +
>> +EXPORT_SYMBOL(stream_firmware);
> New functionality should be EXPORT_SYMBOL_GPL().

Got it.

>
>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>> index b1f9f0c..accd7f6 100644
>> --- a/include/linux/firmware.h
>> +++ b/include/linux/firmware.h
>> @@ -41,6 +41,8 @@ struct builtin_fw {
>>   #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE))
>>   int request_firmware(const struct firmware **fw, const char *name,
>>   		     struct device *device);
>> +int stream_firmware(const struct firmware **fw, const char *name,
>> +		    struct device *device, size_t offset, size_t length);
> Have you looked at the rather new request_firmware_into_buf() ? I hated that
> as it did not get any proper code *review* but its there now... If we can
> leverage off of it to give us something useful for actual upstream drivers
> rather than cruft outside of the kernel I'd be a bit happier.

Yes, I looked request_firmware_into_buf API, it still load the whole 
firmware image into a big memory buffer (but allocated outside of 
firmware class).

>
> Also, please note that I had been noting we keep extending the firmware API
> with loose APIs, I've been consolidating a bit of this into a newer API which
> provides a flexible API for us. Since this is not upstream I don't expect
> you to work off of that, but I will Cc you on some updated patches which
> will fold in this work, I am expecting that the API we will ultimately use for
> this feature you are preoposing could be folded there.
>
> So for now, please consider the review notes above, and we can later see how
> we fold this into a new set of APIs which I hope to bake this week.

Cool, copy me on the updated patches. I am also working on the patch 
based on Alan Tull's comments. Pardon me if I am slow this 2 weeks due 
to family emergency.

>
>    Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html