* [PATCH v2] firmware_class: fix memory leak - free allocated pages
@ 2010-04-29 18:26 Tomas Winkler
2010-04-30 16:01 ` Greg KH
0 siblings, 1 reply; 2+ messages in thread
From: Tomas Winkler @ 2010-04-29 18:26 UTC (permalink / raw)
To: linux-kernel, David Woodhouse, Kay Sievers, Greg Kroah-Hartman
Cc: David Woodhouse, Johannes Berg, Greg Kroah-Hartman, Ming Lei,
Catalin Marinas, Kay Sievers, Tomas Winkler
From: David Woodhouse <dwmw2@infradead.org>
fix memory leak introduced by the patch 6e03a201bbe:
firmware: speed up request_firmware()
1. vfree won't release pages there were allocated explicitly and mapped
using vmap. The memory has to be vunmap-ed and the pages needs
to be freed explicitly
2. page array is moved into the 'struct
firmware' so that we can free it from release_firmware()
and not only in fw_dev_release()
The fix doesn't break the firmware load speed.
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Ming Lei <tom.leiming@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: fix authorship of the patch
drivers/base/firmware_class.c | 28 ++++++++++++++++++++++------
include/linux/firmware.h | 1 +
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..8200adc 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -130,6 +130,20 @@ static ssize_t firmware_loading_show(struct device *dev,
return sprintf(buf, "%d\n", loading);
}
+static void firmware_free_data(struct firmware *fw)
+{
+ int i;
+ vunmap(fw->data);
+ fw->data = NULL;
+ if (fw->pages) {
+ for (i = 0; i < PFN_UP(fw->size); i++)
+ __free_page(fw->pages[i]);
+ kfree(fw->pages);
+ }
+ fw->pages = NULL;
+ fw->size = 0;
+}
+
/* Some architectures don't have PAGE_KERNEL_RO */
#ifndef PAGE_KERNEL_RO
#define PAGE_KERNEL_RO PAGE_KERNEL
@@ -162,21 +176,20 @@ static ssize_t firmware_loading_store(struct device *dev,
mutex_unlock(&fw_lock);
break;
}
- vfree(fw_priv->fw->data);
- fw_priv->fw->data = NULL;
+ firmware_free_data(fw_priv->fw);
+ /* If the pages are not owned by 'struct firmware' */
for (i = 0; i < fw_priv->nr_pages; i++)
__free_page(fw_priv->pages[i]);
kfree(fw_priv->pages);
fw_priv->pages = NULL;
fw_priv->page_array_size = 0;
fw_priv->nr_pages = 0;
- fw_priv->fw->size = 0;
set_bit(FW_STATUS_LOADING, &fw_priv->status);
mutex_unlock(&fw_lock);
break;
case 0:
if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
- vfree(fw_priv->fw->data);
+ vunmap(fw_priv->fw->data);
fw_priv->fw->data = vmap(fw_priv->pages,
fw_priv->nr_pages,
0, PAGE_KERNEL_RO);
@@ -184,7 +197,10 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: vmap() failed\n", __func__);
goto err;
}
- /* Pages will be freed by vfree() */
+ /* Pages are now owned by 'struct firmware' */
+ fw_priv->fw->pages = fw_priv->pages;
+ fw_priv->pages = NULL;
+
fw_priv->page_array_size = 0;
fw_priv->nr_pages = 0;
complete(&fw_priv->completion);
@@ -578,7 +594,7 @@ release_firmware(const struct firmware *fw)
if (fw->data == builtin->data)
goto free_fw;
}
- vfree(fw->data);
+ firmware_free_data(fw);
free_fw:
kfree(fw);
}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 043811f..53d1e6c 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -12,6 +12,7 @@
struct firmware {
size_t size;
const u8 *data;
+ struct page **pages;
};
struct device;
--
1.6.6.1
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: firmware_class: fix memory leak - free allocated pages
2010-04-29 18:26 [PATCH v2] firmware_class: fix memory leak - free allocated pages Tomas Winkler
@ 2010-04-30 16:01 ` Greg KH
0 siblings, 0 replies; 2+ messages in thread
From: Greg KH @ 2010-04-30 16:01 UTC (permalink / raw)
To: David Woodhouse
Cc: linux-kernel, Kay Sievers, Greg Kroah-Hartman, Johannes Berg,
Ming Lei, Catalin Marinas, Tomas Winkler
On Thu, Apr 29, 2010 at 09:26:17PM +0300, David Woodhouse wrote:
>
> From: David Woodhouse <dwmw2@infradead.org>
>
> fix memory leak introduced by the patch 6e03a201bbe:
> firmware: speed up request_firmware()
>
> 1. vfree won't release pages there were allocated explicitly and mapped
> using vmap. The memory has to be vunmap-ed and the pages needs
> to be freed explicitly
>
> 2. page array is moved into the 'struct
> firmware' so that we can free it from release_firmware()
> and not only in fw_dev_release()
>
> The fix doesn't break the firmware load speed.
But it does create a ton of compiler warnings due the const * change. I
had to fix up the patch to get the firmware.c file to build cleanly, but
that then causes a bunch of warnings all over the tree.
So can someone please fix the patch up properly so it doesn't have these
problems and resend it to me?
thanks,
greg k-h
>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Ming Lei <tom.leiming@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
> ---
> drivers/base/firmware_class.c | 30 +++++++++++++++++++++++-------
> include/linux/firmware.h | 5 +++--
> 2 files changed, 26 insertions(+), 9 deletions(-)
>
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -130,6 +130,20 @@ static ssize_t firmware_loading_show(str
> return sprintf(buf, "%d\n", loading);
> }
>
> +static void firmware_free_data(struct firmware *fw)
> +{
> + int i;
> + vunmap(fw->data);
> + fw->data = NULL;
> + if (fw->pages) {
> + for (i = 0; i < PFN_UP(fw->size); i++)
> + __free_page(fw->pages[i]);
> + kfree(fw->pages);
> + }
> + fw->pages = NULL;
> + fw->size = 0;
> +}
> +
> /* Some architectures don't have PAGE_KERNEL_RO */
> #ifndef PAGE_KERNEL_RO
> #define PAGE_KERNEL_RO PAGE_KERNEL
> @@ -162,21 +176,20 @@ static ssize_t firmware_loading_store(st
> mutex_unlock(&fw_lock);
> break;
> }
> - vfree(fw_priv->fw->data);
> - fw_priv->fw->data = NULL;
> + firmware_free_data(fw_priv->fw);
> + /* If the pages are not owned by 'struct firmware' */
> for (i = 0; i < fw_priv->nr_pages; i++)
> __free_page(fw_priv->pages[i]);
> kfree(fw_priv->pages);
> fw_priv->pages = NULL;
> fw_priv->page_array_size = 0;
> fw_priv->nr_pages = 0;
> - fw_priv->fw->size = 0;
> set_bit(FW_STATUS_LOADING, &fw_priv->status);
> mutex_unlock(&fw_lock);
> break;
> case 0:
> if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
> - vfree(fw_priv->fw->data);
> + vunmap(fw_priv->fw->data);
> fw_priv->fw->data = vmap(fw_priv->pages,
> fw_priv->nr_pages,
> 0, PAGE_KERNEL_RO);
> @@ -184,7 +197,10 @@ static ssize_t firmware_loading_store(st
> dev_err(dev, "%s: vmap() failed\n", __func__);
> goto err;
> }
> - /* Pages will be freed by vfree() */
> + /* Pages are now owned by 'struct firmware' */
> + fw_priv->fw->pages = fw_priv->pages;
> + fw_priv->pages = NULL;
> +
> fw_priv->page_array_size = 0;
> fw_priv->nr_pages = 0;
> complete(&fw_priv->completion);
> @@ -568,7 +584,7 @@ request_firmware(const struct firmware *
> * @fw: firmware resource to release
> **/
> void
> -release_firmware(const struct firmware *fw)
> +release_firmware(struct firmware *fw)
> {
> struct builtin_fw *builtin;
>
> @@ -578,7 +594,7 @@ release_firmware(const struct firmware *
> if (fw->data == builtin->data)
> goto free_fw;
> }
> - vfree(fw->data);
> + firmware_free_data(fw);
> free_fw:
> kfree(fw);
> }
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -12,6 +12,7 @@
> struct firmware {
> size_t size;
> const u8 *data;
> + struct page **pages;
> };
>
> struct device;
> @@ -42,7 +43,7 @@ int request_firmware_nowait(
> const char *name, struct device *device, gfp_t gfp, void *context,
> void (*cont)(const struct firmware *fw, void *context));
>
> -void release_firmware(const struct firmware *fw);
> +void release_firmware(struct firmware *fw);
> #else
> static inline int request_firmware(const struct firmware **fw,
> const char *name,
> @@ -58,7 +59,7 @@ static inline int request_firmware_nowai
> return -EINVAL;
> }
>
> -static inline void release_firmware(const struct firmware *fw)
> +static inline void release_firmware(struct firmware *fw)
> {
> }
> #endif
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-04-30 19:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-29 18:26 [PATCH v2] firmware_class: fix memory leak - free allocated pages Tomas Winkler
2010-04-30 16:01 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.