Re: [PATCH v3 4/5] firmware: generalize reading file contents as a helper

[Luis R. Rodriguez]

Greg,

Due to Mimi's awesome work on generalizing a common VFS file read [0] as
described as possible in the commit log below this patch will be dropped in
preference for Mimi's VFS work to go in first.

Mimi,

since your generic VFS routine is pretty much identical to fw_read_file()
defined here you could make use of hunks #2 and #3 below. To make this more
legible it does depend on another patch which simplifies things. You can feel
free to pick that up as part of your series if you see it helps. If it doesn't
help that's fine too, I'll just submit later, but since you're digging in the
same way as I was I figured this could help.

I'll copy on you the other patch next.

[0] http//lkml.kernel.org/r/1453129886-20192-1-git-send-email-zohar@xxxxxxxxxxxxxxxxxx

  Luis

On Wed, Dec 23, 2015 at 01:34:56PM -0800, Luis R. Rodriguez wrote:
> From: David Howells <dhowells@xxxxxxxxxx>
> 
> We'll want to reuse this same code later in order to read
> two separate types of file contents. This generalizes
> fw_read_file_contents() for reading a file and rebrands it
> as fw_read_file(). This new caller is now generic: the path
> used can be arbitrary and the caller is also agnostic to the
> firmware_class code now, this begs the possibility of code
> re-use with other similar callers in the kernel. For instance
> in the future we may want to share a solution with:
> 
>     - firmware_class: fw_read_file()
>     - module: kernel_read()
>     - kexec: copy_file_fd()
> 
> While at it this also cleans up the exit paths on fw_read_file().
> 
> Reviewed-by: Josh Boyer <jwboyer@xxxxxxxxxxxxxxxxx>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> ---
>  drivers/base/firmware_class.c | 62 +++++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index e10a5349aa61..cd51a90ed012 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -291,34 +291,51 @@ static const char * const fw_path[] = {
>  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_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +/*
> + * Read the contents of a file.
> + */
> +static int fw_read_file(const char *path, void **_buf, size_t *_size)
>  {
> -	int size;
> +	struct file *file;
> +	size_t size;
>  	char *buf;
>  	int rc;
>  
> +	file = filp_open(path, O_RDONLY, 0);
> +	if (IS_ERR(file))
> +		return PTR_ERR(file);
> +
> +	rc = -EINVAL;
>  	if (!S_ISREG(file_inode(file)->i_mode))
> -		return -EINVAL;
> +		goto err_file;
>  	size = i_size_read(file_inode(file));
>  	if (size <= 0)
> -		return -EINVAL;
> +		goto err_file;
> +	rc = -ENOMEM;
>  	buf = vmalloc(size);
>  	if (!buf)
> -		return -ENOMEM;
> +		goto err_file;
> +
>  	rc = kernel_read(file, 0, buf, size);
> +	if (rc < 0)
> +		goto err_buf;
>  	if (rc != size) {
> -		if (rc > 0)
> -			rc = -EIO;
> -		goto fail;
> +		rc = -EIO;
> +		goto err_buf;
>  	}
> +
>  	rc = security_kernel_fw_from_file(file, buf, size);
>  	if (rc)
> -		goto fail;
> -	fw_buf->data = buf;
> -	fw_buf->size = size;
> +		goto err_buf;
> +
> +	*_buf = buf;
> +	*_size = size;
>  	return 0;
> -fail:
> +
> +err_buf:
>  	vfree(buf);
> +err_file:
> +	fput(file);
>  	return rc;
>  }
>  
> @@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
>  }
>  
>  static int fw_get_filesystem_firmware(struct device *device,
> -				       struct firmware_buf *buf)
> +				      struct firmware_buf *buf)
>  {
>  	int i, len;
>  	int rc = -ENOENT;
> -	char *path;
> +	char *path = NULL;
>  
>  	path = __getname();
>  	if (!path)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Try each possible firmware blob in turn till one doesn't produce
> +	 * ENOENT.
> +	 */
>  	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> -		struct file *file;
> -
>  		/* skip the unset customized path */
>  		if (!fw_path[i][0])
>  			continue;
> @@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device *device,
>  			break;
>  		}
>  
> -		file = filp_open(path, O_RDONLY, 0);
> -		if (IS_ERR(file))
> -			continue;
> -		rc = fw_read_file_contents(file, buf);
> -		fput(file);
> +		rc = fw_read_file(path, &buf->data, &buf->size);
>  		if (rc == 0) {
>  			dev_dbg(device, "system data: direct-loading firmware %s\n",
>  				buf->fw_id);
>  			fw_finish_direct_load(device, buf);
>  			goto out;
> -		} else
> +		} else if (rc != -ENOENT) {
>  			dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
>  				 path, rc);
> +			goto out;
> +		}
>  	}
>  out:
>  	__putname(path);
> -
>  	return rc;
>  }
>  
> -- 
> 2.6.2
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg