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 >>>> >>>> >>>> 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 >>>>> --- >>>>> 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 > *names_cachep ; > #define __getname () > kmem_cache_alloc > (names_cachep > , GFP_KERNEL > ) > > names_cachep is created in fs/dcaches.c vfs_caches_init function with object > size equal to PATH_MAX. > names_cachep = > kmem_cache_create > (/"names_cache"/, > PATH_MAX , > 0,SLAB_HWCACHE_ALIGN > |SLAB_PANIC > , 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 >>> > >