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

From: matthew . gerlach
Date: Tue Mar 14 2017 - 12:55:50 EST



On Tue, 14 Mar 2017, Li, Yi wrote:

hi Matthew,

Hi Yi,


On 3/13/2017 4:09 PM, matthew.gerlach@xxxxxxxxxxxxxxx wrote:


On Fri, 10 Mar 2017, Li, Yi wrote:

Hi Matthew

Hi Yi,



On 3/10/2017 11:44 AM, matthew.gerlach@xxxxxxxxxxxxxxx wrote:


On Thu, 9 Mar 2017, yi1.li@xxxxxxxxxxxxxxx wrote:

From: Yi Li <yi1.li@xxxxxxxxxxxxxxx>


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 <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);

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 <http://lxr.free-electrons.com/ident?i=kmem_cache> *names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep>;
#define __getname <http://lxr.free-electrons.com/ident?i=__getname>() kmem_cache_alloc <http://lxr.free-electrons.com/ident?i=kmem_cache_alloc>(names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep>, GFP_KERNEL <http://lxr.free-electrons.com/ident?i=GFP_KERNEL>)

names_cachep is created in fs/dcaches.c vfs_caches_init function with object size equal to PATH_MAX.
names_cachep <http://lxr.free-electrons.com/ident?i=names_cachep> = kmem_cache_create <http://lxr.free-electrons.com/ident?i=kmem_cache_create>(/"names_cache"/, PATH_MAX <http://lxr.free-electrons.com/ident?i=PATH_MAX>, 0,SLAB_HWCACHE_ALIGN <http://lxr.free-electrons.com/ident?i=SLAB_HWCACHE_ALIGN>|SLAB_PANIC <http://lxr.free-electrons.com/ident?i=SLAB_PANIC>, NULL <http://lxr.free-electrons.com/ident?i=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@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html