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@xxxxxxxxxxxxxxx wrote:
> > From: Yi Li <yi1.li@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html