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

["Luis R. Rodriguez" <mcgrof@kernel.org>]

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