linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND/PATCHv4 v4 0/3] request_firmware() into pre-allocated buffers
@ 2016-06-07 16:47 Stephen Boyd
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic Stephen Boyd
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stephen Boyd @ 2016-06-07 16:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Mimi Zohar, Andrew Morton, Mark Brown,
	Ming Lei, Vikram Mulukutla

Some systems are memory constrained but they need to load very
large firmwares. The firmware subsystem allows drivers to request
this firmware be loaded from the filesystem, but this requires
that the entire firmware be loaded into kernel memory first
before it's provided to the driver. This can lead to a situation
where we map the firmware twice, once to load the firmware into
kernel memory and once to copy the firmware into the final
resting place.

This design creates needless memory pressure and delays loading
because we have to copy from kernel memory to somewhere else.
This patch sets adds support to the request firmware API to load the
firmware directly into a pre-allocated buffer, skipping the intermediate
copying step and alleviating memory pressure during firmware loading.
The drawback is that we can't use the firmware caching feature because
the memory for the firmware cache is not managed by the firmware
layer.

Patches based on v4.7-rc1.

Changes since v3:
 * Rebased onto v4.7-rc1
 * Renamed enum per Mimi's comment

Changes since v2:
 * Dropped DMA API changes and moved to dma coherent APIs per Catalin's
   suggestions
 * Reworked to request firmware directly into any pre-allocated buffer
 * Split off cleanup patch into patch #1 to ease review
 * Added file reading enum for reading firmware directly into buffer

Changes since v1:
 * Rebased onto v4.6-rc1 (large conflicts due to movement of code from Mimi)
 * Added some CONFIG_HAS_DMA ifdefs around code that's using DMA ops

Stephen Boyd (2):
  firmware: Consolidate kmap/read/write logic
  firmware: Support loading into a pre-allocated buffer

Vikram Mulukutla (1):
  firmware: Provide infrastructure to make fw caching optional

 drivers/base/firmware_class.c | 183 +++++++++++++++++++++++++++++-------------
 fs/exec.c                     |   9 ++-
 include/linux/firmware.h      |   8 ++
 include/linux/fs.h            |   1 +
 4 files changed, 142 insertions(+), 59 deletions(-)

-- 
2.9.0-rc1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic
  2016-06-07 16:47 [RESEND/PATCHv4 v4 0/3] request_firmware() into pre-allocated buffers Stephen Boyd
@ 2016-06-07 16:47 ` Stephen Boyd
  2016-06-07 20:21   ` Andrew Morton
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional Stephen Boyd
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer Stephen Boyd
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2016-06-07 16:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Mimi Zohar, Andrew Morton, Mark Brown,
	Ming Lei, Vikram Mulukutla

We use similar structured code to read and write the kmapped
firmware pages. The only difference is read copies from the kmap
region and write copies to it. Consolidate this into one function
to reduce duplication.

Cc: Vikram Mulukutla <markivx@codeaurora.org>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 drivers/base/firmware_class.c | 57 ++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 773fc3099769..01d55723d82c 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -691,6 +691,29 @@ out:
 
 static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
 
+static void firmware_rw(struct firmware_buf *buf, char *buffer,
+			loff_t offset, size_t count, bool read)
+{
+	while (count) {
+		void *page_data;
+		int page_nr = offset >> PAGE_SHIFT;
+		int page_ofs = offset & (PAGE_SIZE-1);
+		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
+
+		page_data = kmap(buf->pages[page_nr]);
+
+		if (read)
+			memcpy(buffer, page_data + page_ofs, page_cnt);
+		else
+			memcpy(page_data + page_ofs, buffer, page_cnt);
+
+		kunmap(buf->pages[page_nr]);
+		buffer += page_cnt;
+		offset += page_cnt;
+		count -= page_cnt;
+	}
+}
+
 static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 				  struct bin_attribute *bin_attr,
 				  char *buffer, loff_t offset, size_t count)
@@ -715,21 +738,8 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	ret_count = count;
 
-	while (count) {
-		void *page_data;
-		int page_nr = offset >> PAGE_SHIFT;
-		int page_ofs = offset & (PAGE_SIZE-1);
-		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
-		page_data = kmap(buf->pages[page_nr]);
-
-		memcpy(buffer, page_data + page_ofs, page_cnt);
+	firmware_rw(buf, buffer, offset, count, true);
 
-		kunmap(buf->pages[page_nr]);
-		buffer += page_cnt;
-		offset += page_cnt;
-		count -= page_cnt;
-	}
 out:
 	mutex_unlock(&fw_lock);
 	return ret_count;
@@ -809,24 +819,9 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		goto out;
 
 	retval = count;
+	firmware_rw(buf, buffer, offset, count, false);
 
-	while (count) {
-		void *page_data;
-		int page_nr = offset >> PAGE_SHIFT;
-		int page_ofs = offset & (PAGE_SIZE - 1);
-		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
-		page_data = kmap(buf->pages[page_nr]);
-
-		memcpy(page_data + page_ofs, buffer, page_cnt);
-
-		kunmap(buf->pages[page_nr]);
-		buffer += page_cnt;
-		offset += page_cnt;
-		count -= page_cnt;
-	}
-
-	buf->size = max_t(size_t, offset, buf->size);
+	buf->size = max_t(size_t, offset + count, buf->size);
 out:
 	mutex_unlock(&fw_lock);
 	return retval;
-- 
2.9.0-rc1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional
  2016-06-07 16:47 [RESEND/PATCHv4 v4 0/3] request_firmware() into pre-allocated buffers Stephen Boyd
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic Stephen Boyd
@ 2016-06-07 16:47 ` Stephen Boyd
  2016-06-14  1:53   ` Luis R. Rodriguez
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer Stephen Boyd
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2016-06-07 16:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: Vikram Mulukutla, linux-arm-kernel, Mimi Zohar, Andrew Morton,
	Mark Brown, Ming Lei

From: Vikram Mulukutla <markivx@codeaurora.org>

Some low memory systems with complex peripherals cannot afford to
have the relatively large firmware images taking up valuable
memory during suspend and resume. Change the internal
implementation of firmware_class to disallow caching based on a
configurable option. In the near future, variants of
request_firmware will take advantage of this feature.

Signed-off-by: Vikram Mulukutla <markivx@codeaurora.org>
[stephen.boyd@linaro.org: Drop firmware_desc design and use flags]
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 drivers/base/firmware_class.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 01d55723d82c..45ed20cefa10 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -112,6 +112,7 @@ static inline long firmware_loading_timeout(void)
 #define FW_OPT_FALLBACK		0
 #endif
 #define FW_OPT_NO_WARN	(1U << 3)
+#define FW_OPT_NOCACHE	(1U << 4)
 
 struct firmware_cache {
 	/* firmware_buf instance will be added into the below list */
@@ -1065,14 +1066,16 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 	 * should be fixed in devres or driver core.
 	 */
 	/* don't cache firmware handled without uevent */
-	if (device && (opt_flags & FW_OPT_UEVENT))
+	if (device && (opt_flags & FW_OPT_UEVENT) &&
+	    !(opt_flags & FW_OPT_NOCACHE))
 		fw_add_devm_name(device, buf->fw_id);
 
 	/*
 	 * After caching firmware image is started, let it piggyback
 	 * on request firmware.
 	 */
-	if (buf->fwc->state == FW_LOADER_START_CACHE) {
+	if (!(opt_flags & FW_OPT_NOCACHE) &&
+	    buf->fwc->state == FW_LOADER_START_CACHE) {
 		if (fw_cache_piggyback_on_request(buf->fw_id))
 			kref_get(&buf->ref);
 	}
-- 
2.9.0-rc1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer
  2016-06-07 16:47 [RESEND/PATCHv4 v4 0/3] request_firmware() into pre-allocated buffers Stephen Boyd
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic Stephen Boyd
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional Stephen Boyd
@ 2016-06-07 16:47 ` Stephen Boyd
  2016-06-12  9:28   ` kbuild test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2016-06-07 16:47 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Mimi Zohar, Andrew Morton, Mark Brown,
	Ming Lei, Vikram Mulukutla

Some systems are memory constrained but they need to load very
large firmwares. The firmware subsystem allows drivers to request
this firmware be loaded from the filesystem, but this requires
that the entire firmware be loaded into kernel memory first
before it's provided to the driver. This can lead to a situation
where we map the firmware twice, once to load the firmware into
kernel memory and once to copy the firmware into the final
resting place.

This creates needless memory pressure and delays loading because
we have to copy from kernel memory to somewhere else. Let's add a
request_firmware_into_buf() API that allows drivers to request
firmware be loaded directly into a pre-allocated buffer. This
skips the intermediate step of allocating a buffer in kernel
memory to hold the firmware image while it's read from the
filesystem. It also requires that drivers know how much memory
they'll require before requesting the firmware and negates any
benefits of firmware caching because the firmware layer doesn't
manage the buffer lifetime.

For a 16MB buffer, about half the time is spent performing a
memcpy from the buffer to the final resting place. I see loading
times go from 0.081171 seconds to 0.047696 seconds after applying
this patch. Plus the vmalloc pressure is reduced.

This is based on a patch from Vikram Mulukutla on
codeaurora.org[1].

[1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Vikram Mulukutla <markivx@codeaurora.org>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---
 drivers/base/firmware_class.c | 125 +++++++++++++++++++++++++++++++++---------
 fs/exec.c                     |   9 ++-
 include/linux/firmware.h      |   8 +++
 include/linux/fs.h            |   1 +
 4 files changed, 114 insertions(+), 29 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 45ed20cefa10..22d1760a4278 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -46,7 +46,8 @@ MODULE_LICENSE("GPL");
 extern struct builtin_fw __start_builtin_fw[];
 extern struct builtin_fw __end_builtin_fw[];
 
-static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
+static bool fw_get_builtin_firmware(struct firmware *fw, const char *name,
+				    void *buf, size_t size)
 {
 	struct builtin_fw *b_fw;
 
@@ -54,6 +55,9 @@ static bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
 		if (strcmp(name, b_fw->name) == 0) {
 			fw->size = b_fw->size;
 			fw->data = b_fw->data;
+
+			if (buf && fw->size <= size)
+				memcpy(buf, fw->data, fw->size);
 			return true;
 		}
 	}
@@ -74,7 +78,9 @@ static bool fw_is_builtin_firmware(const struct firmware *fw)
 
 #else /* Module case - no builtin firmware support */
 
-static inline bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
+static inline bool fw_get_builtin_firmware(struct firmware *fw,
+					   const char *name, void *buf,
+					   size_t size)
 {
 	return false;
 }
@@ -144,6 +150,7 @@ struct firmware_buf {
 	unsigned long status;
 	void *data;
 	size_t size;
+	size_t allocated_size;
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	bool is_paged_buf;
 	bool need_uevent;
@@ -179,7 +186,8 @@ static DEFINE_MUTEX(fw_lock);
 static struct firmware_cache fw_cache;
 
 static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
-					      struct firmware_cache *fwc)
+					      struct firmware_cache *fwc,
+					      void *dbuf, size_t size)
 {
 	struct firmware_buf *buf;
 
@@ -195,6 +203,8 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name,
 
 	kref_init(&buf->ref);
 	buf->fwc = fwc;
+	buf->data = dbuf;
+	buf->allocated_size = size;
 	init_completion(&buf->completion);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&buf->pending_list);
@@ -218,7 +228,8 @@ static struct firmware_buf *__fw_lookup_buf(const char *fw_name)
 
 static int fw_lookup_and_allocate_buf(const char *fw_name,
 				      struct firmware_cache *fwc,
-				      struct firmware_buf **buf)
+				      struct firmware_buf **buf, void *dbuf,
+				      size_t size)
 {
 	struct firmware_buf *tmp;
 
@@ -230,7 +241,7 @@ static int fw_lookup_and_allocate_buf(const char *fw_name,
 		*buf = tmp;
 		return 1;
 	}
-	tmp = __allocate_fw_buf(fw_name, fwc);
+	tmp = __allocate_fw_buf(fw_name, fwc, dbuf, size);
 	if (tmp)
 		list_add(&tmp->list, &fwc->head);
 	spin_unlock(&fwc->lock);
@@ -262,6 +273,7 @@ static void __fw_free_buf(struct kref *ref)
 		vfree(buf->pages);
 	} else
 #endif
+	if (!buf->allocated_size)
 		vfree(buf->data);
 	kfree_const(buf->fw_id);
 	kfree(buf);
@@ -302,13 +314,21 @@ static void fw_finish_direct_load(struct device *device,
 	mutex_unlock(&fw_lock);
 }
 
-static int fw_get_filesystem_firmware(struct device *device,
-				       struct firmware_buf *buf)
+static int
+fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
 	loff_t size;
 	int i, len;
 	int rc = -ENOENT;
 	char *path;
+	enum kernel_read_file_id id = READING_FIRMWARE;
+	size_t msize = INT_MAX;
+
+	/* Already populated data member means we're loading into a buffer */
+	if (buf->data) {
+		id = READING_FIRMWARE_PREALLOC_BUFFER;
+		msize = buf->allocated_size;
+	}
 
 	path = __getname();
 	if (!path)
@@ -327,8 +347,8 @@ static int fw_get_filesystem_firmware(struct device *device,
 		}
 
 		buf->size = 0;
-		rc = kernel_read_file_from_path(path, &buf->data, &size,
-						INT_MAX, READING_FIRMWARE);
+		rc = kernel_read_file_from_path(path, &buf->data, &size, msize,
+						id);
 		if (rc) {
 			if (rc == -ENOENT)
 				dev_dbg(device, "loading %s failed with error %d\n",
@@ -692,6 +712,15 @@ out:
 
 static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
 
+static void firmware_rw_buf(struct firmware_buf *buf, char *buffer,
+			   loff_t offset, size_t count, bool read)
+{
+	if (read)
+		memcpy(buffer, buf->data + offset, count);
+	else
+		memcpy(buf->data + offset, buffer, count);
+}
+
 static void firmware_rw(struct firmware_buf *buf, char *buffer,
 			loff_t offset, size_t count, bool read)
 {
@@ -739,7 +768,10 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
 
 	ret_count = count;
 
-	firmware_rw(buf, buffer, offset, count, true);
+	if (buf->data)
+		firmware_rw_buf(buf, buffer, offset, count, true);
+	else
+		firmware_rw(buf, buffer, offset, count, true);
 
 out:
 	mutex_unlock(&fw_lock);
@@ -815,12 +847,21 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
 		goto out;
 	}
 
-	retval = fw_realloc_buffer(fw_priv, offset + count);
-	if (retval)
-		goto out;
+	if (buf->data) {
+		if (offset + count > buf->allocated_size) {
+			retval = -ENOMEM;
+			goto out;
+		}
+		firmware_rw_buf(buf, buffer, offset, count, false);
+		retval = count;
+	} else {
+		retval = fw_realloc_buffer(fw_priv, offset + count);
+		if (retval)
+			goto out;
 
-	retval = count;
-	firmware_rw(buf, buffer, offset, count, false);
+		retval = count;
+		firmware_rw(buf, buffer, offset, count, false);
+	}
 
 	buf->size = max_t(size_t, offset + count, buf->size);
 out:
@@ -890,7 +931,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	struct firmware_buf *buf = fw_priv->buf;
 
 	/* fall back on userspace loading */
-	buf->is_paged_buf = true;
+	if (!buf->data)
+		buf->is_paged_buf = true;
 
 	dev_set_uevent_suppress(f_dev, true);
 
@@ -925,7 +967,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 
 	if (is_fw_load_aborted(buf))
 		retval = -EAGAIN;
-	else if (!buf->data)
+	else if (buf->is_paged_buf && !buf->data)
 		retval = -ENOMEM;
 
 	device_del(f_dev);
@@ -1008,7 +1050,7 @@ static int sync_cached_firmware_buf(struct firmware_buf *buf)
  */
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
-			  struct device *device)
+			  struct device *device, void *dbuf, size_t size)
 {
 	struct firmware *firmware;
 	struct firmware_buf *buf;
@@ -1021,12 +1063,12 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 		return -ENOMEM;
 	}
 
-	if (fw_get_builtin_firmware(firmware, name)) {
+	if (fw_get_builtin_firmware(firmware, name, dbuf, size)) {
 		dev_dbg(device, "using built-in %s\n", name);
 		return 0; /* assigned */
 	}
 
-	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf);
+	ret = fw_lookup_and_allocate_buf(name, &fw_cache, &buf, dbuf, size);
 
 	/*
 	 * bind with 'buf' now to avoid warning in failure path
@@ -1089,7 +1131,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
 /* called from request_firmware() and request_firmware_work_func() */
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		  struct device *device, unsigned int opt_flags)
+		  struct device *device, void *buf, size_t size,
+		  unsigned int opt_flags)
 {
 	struct firmware *fw = NULL;
 	long timeout;
@@ -1103,7 +1146,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
-	ret = _request_firmware_prepare(&fw, name, device);
+	ret = _request_firmware_prepare(&fw, name, device, buf, size);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
@@ -1182,7 +1225,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device,
+	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_FALLBACK);
 	module_put(THIS_MODULE);
 	return ret;
@@ -1206,13 +1249,43 @@ int request_firmware_direct(const struct firmware **firmware_p,
 	int ret;
 
 	__module_get(THIS_MODULE);
-	ret = _request_firmware(firmware_p, name, device,
+	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN);
 	module_put(THIS_MODULE);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded and DMA region allocated
+ * @buf: address of buffer to load firmware into
+ * @size: size of buffer
+ *
+ * This function works pretty much like request_firmware(), but it doesn't
+ * allocate a buffer to hold the firmware data. Instead, the firmware
+ * is loaded directly into the buffer pointed to by @buf and the @firmware_p
+ * data member is pointed at @buf.
+ *
+ * This function doesn't cache firmware either.
+ */
+int
+request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
+			  struct device *device, void *buf, size_t size)
+{
+	int ret;
+
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, buf, size,
+				FW_OPT_UEVENT | FW_OPT_FALLBACK |
+				FW_OPT_NOCACHE);
+	module_put(THIS_MODULE);
+	return ret;
+}
+EXPORT_SYMBOL(request_firmware_into_buf);
+
 /**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
@@ -1245,7 +1318,7 @@ static void request_firmware_work_func(struct work_struct *work)
 
 	fw_work = container_of(work, struct firmware_work, work);
 
-	_request_firmware(&fw, fw_work->name, fw_work->device,
+	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
 			  fw_work->opt_flags);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
@@ -1378,7 +1451,7 @@ static int uncache_firmware(const char *fw_name)
 
 	pr_debug("%s: %s\n", __func__, fw_name);
 
-	if (fw_get_builtin_firmware(&fw, fw_name))
+	if (fw_get_builtin_firmware(&fw, fw_name, NULL, 0))
 		return 0;
 
 	buf = fw_lookup_buf(fw_name);
diff --git a/fs/exec.c b/fs/exec.c
index 887c1c955df8..433edea54253 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -866,7 +866,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 		goto out;
 	}
 
-	*buf = vmalloc(i_size);
+	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
+		*buf = vmalloc(i_size);
 	if (!*buf) {
 		ret = -ENOMEM;
 		goto out;
@@ -897,8 +898,10 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 
 out_free:
 	if (ret < 0) {
-		vfree(*buf);
-		*buf = NULL;
+		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+			vfree(*buf);
+			*buf = NULL;
+		}
 	}
 
 out:
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e75b5c..bdc24ee92823 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
+int request_firmware_into_buf(const struct firmware **firmware_p,
+	const char *name, struct device *device, void *buf, size_t size);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -75,5 +77,11 @@ static inline int request_firmware_direct(const struct firmware **fw,
 	return -EINVAL;
 }
 
+static inline int request_firmware_into_buf(const struct firmware **firmware_p,
+	const char *name, struct device *device, void *buf, size_t size);
+{
+	return -EINVAL;
+}
+
 #endif
 #endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index dd288148a6b1..c4f3a26614a7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2631,6 +2631,7 @@ extern int do_pipe_flags(int *, int);
 #define __kernel_read_file_id(id) \
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
+	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
-- 
2.9.0-rc1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic Stephen Boyd
@ 2016-06-07 20:21   ` Andrew Morton
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2016-06-07 20:21 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-kernel, linux-arm-kernel, Mimi Zohar, Mark Brown, Ming Lei,
	Vikram Mulukutla

On Tue,  7 Jun 2016 09:47:39 -0700 Stephen Boyd <stephen.boyd@linaro.org> wrote:

> We use similar structured code to read and write the kmapped
> firmware pages. The only difference is read copies from the kmap
> region and write copies to it. Consolidate this into one function
> to reduce duplication.
> 
> ...
>
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -691,6 +691,29 @@ out:
>  
>  static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
>  
> +static void firmware_rw(struct firmware_buf *buf, char *buffer,
> +			loff_t offset, size_t count, bool read)
> +{
> +	while (count) {
> +		void *page_data;
> +		int page_nr = offset >> PAGE_SHIFT;
> +		int page_ofs = offset & (PAGE_SIZE-1);
> +		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> +
> +		page_data = kmap(buf->pages[page_nr]);

fwiw, we could switch the code to kmap_atomic() here.  It is faster.

> +		if (read)
> +			memcpy(buffer, page_data + page_ofs, page_cnt);
> +		else
> +			memcpy(page_data + page_ofs, buffer, page_cnt);
> +
> +		kunmap(buf->pages[page_nr]);
> +		buffer += page_cnt;
> +		offset += page_cnt;
> +		count -= page_cnt;
> +	}
> +}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer Stephen Boyd
@ 2016-06-12  9:28   ` kbuild test robot
  0 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2016-06-12  9:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: kbuild-all, linux-kernel, linux-arm-kernel, Mimi Zohar,
	Andrew Morton, Mark Brown, Ming Lei, Vikram Mulukutla

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

Hi,

[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on v4.7-rc2]
[cannot apply to next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Stephen-Boyd/firmware-Consolidate-kmap-read-write-logic/20160607-164005
config: x86_64-randconfig-v0-06121632 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from drivers/video/fbdev/metronomefb.c:33:0:
>> include/linux/firmware.h:82:1: error: expected identifier or '(' before '{' token
    {
    ^
   include/linux/firmware.h:80:19: warning: 'request_firmware_into_buf' declared 'static' but never defined [-Wunused-function]
    static inline int request_firmware_into_buf(const struct firmware **firmware_p,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +82 include/linux/firmware.h

    76	{
    77		return -EINVAL;
    78	}
    79	
    80	static inline int request_firmware_into_buf(const struct firmware **firmware_p,
    81		const char *name, struct device *device, void *buf, size_t size);
  > 82	{
    83		return -EINVAL;
    84	}
    85	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23402 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional
  2016-06-07 16:47 ` [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional Stephen Boyd
@ 2016-06-14  1:53   ` Luis R. Rodriguez
  0 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2016-06-14  1:53 UTC (permalink / raw)
  To: Stephen Boyd, David Woodhouse, Andy Lutomirski, Takashi Iwai, Kees Cook
  Cc: linux-kernel, Ming Lei, Vikram Mulukutla, Mark Brown,
	Andrew Morton, Mimi Zohar, linux-arm-kernel, Julia Lawall,
	Greg Kroah-Hartman, Stephen Rothwell

On Tue, Jun 7, 2016 at 9:47 AM, Stephen Boyd <stephen.boyd@linaro.org> wrote:
> From: Vikram Mulukutla <markivx@codeaurora.org>
>
> Some low memory systems with complex peripherals cannot afford to
> have the relatively large firmware images taking up valuable
> memory during suspend and resume. Change the internal
> implementation of firmware_class to disallow caching based on a
> configurable option. In the near future, variants of
> request_firmware will take advantage of this feature.
>
> Signed-off-by: Vikram Mulukutla <markivx@codeaurora.org>
> [stephen.boyd@linaro.org: Drop firmware_desc design and use flags]
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
> ---
>  drivers/base/firmware_class.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 01d55723d82c..45ed20cefa10 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -112,6 +112,7 @@ static inline long firmware_loading_timeout(void)
>  #define FW_OPT_FALLBACK                0
>  #endif
>  #define FW_OPT_NO_WARN (1U << 3)
> +#define FW_OPT_NOCACHE (1U << 4)

6 days late -- but even if I wasn't Cc'd I caught this on linux-next
-- but please folks

NACK,

We should not be adding *yet another flag* and *yet another new API*
for a mild tweak to the API. The firmware API is already messed up
enough as it is, enough is enough. I'll post v4 of the new flexible
API which after feedback from v3 series [0] should put this to an end.
That should enable you to add your optional needs via a descriptor
without any other collateral evolutins. I had been waiting to push out
v4 until me and Julia completed some SmPL conversion to help drivers
easily convert over but seeing this patch got merged and I think this
can't wait any more, the SmPL grooviness will have to wait a bit and
some simple manual transformation examples will be provided.

[0] http://lkml.kernel.org/r/1450906497-24179-6-git-send-email-mcgrof@do-not-panic.com

  Luis

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-06-14  1:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 16:47 [RESEND/PATCHv4 v4 0/3] request_firmware() into pre-allocated buffers Stephen Boyd
2016-06-07 16:47 ` [RESEND/PATCHv4 v4 1/3] firmware: Consolidate kmap/read/write logic Stephen Boyd
2016-06-07 20:21   ` Andrew Morton
2016-06-07 16:47 ` [RESEND/PATCHv4 v4 2/3] firmware: Provide infrastructure to make fw caching optional Stephen Boyd
2016-06-14  1:53   ` Luis R. Rodriguez
2016-06-07 16:47 ` [RESEND/PATCHv4 v4 3/3] firmware: Support loading into a pre-allocated buffer Stephen Boyd
2016-06-12  9:28   ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).