Messages in this thread | | | Subject | Re: [RFC 1/2] firmware class: Add stream_firmware API. | From | "Li, Yi" <> | Date | Mon, 27 Mar 2017 16:20:13 -0500 |
| |
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
| |