lkml.org 
[lkml]   [2010]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: request_firmware API exhaust memory
From
Date
On Tue, 2010-04-27 at 14:05 +0200, Kay Sievers wrote:
>
> The patch I posted makes the issue go away. It's still not the right
> fix, because the pages are only get freed when the device id cleaned
> up, not on calling release_firmware. But it should illustrate the
> underlying issue, and that there is no leaked memory anymore.
>
> > I think this needs some more review.
>
> If David does not fix it, it probably just needs to be reverted. And
> instead of implementing our own "memory management", we should rather
> add a vrealloc(), and the firmware loader should use that.

The whole point was to avoid the vrealloc(). We really don't want to be
screwing with page tables, globally, for each page written from
userspace.

This untested patch attempts to put the page array into the 'struct
firmware' so that we can free it from release_firmware().

It would actually be nice if we could make that the _primary_ method of
returning data to drivers, and we could ditch the vmap() requirement
altogether... drivers which really need it to be virtually contiguous
can depend on CONFIG_MMU and do the vmap() for themselves.

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..cc9a79b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -162,8 +162,13 @@ static ssize_t firmware_loading_store(struct device *dev,
mutex_unlock(&fw_lock);
break;
}
- vfree(fw_priv->fw->data);
+ vunmap(fw_priv->fw->data);
fw_priv->fw->data = NULL;
+ if (fw_priv->fw->pages) {
+ for (i = 0; i < PFN_UP(fw_priv->fw->size); i++)
+ __free_page(fw_priv->fw->pages[i]);
+ kfree(fw_priv->fw->pages);
+ }
for (i = 0; i < fw_priv->nr_pages; i++)
__free_page(fw_priv->pages[i]);
kfree(fw_priv->pages);
@@ -176,7 +181,7 @@ static ssize_t firmware_loading_store(struct device *dev,
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 +189,9 @@ 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);
@@ -571,6 +578,7 @@ void
release_firmware(const struct firmware *fw)
{
struct builtin_fw *builtin;
+ int i;

if (fw) {
for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
@@ -578,7 +586,12 @@ release_firmware(const struct firmware *fw)
if (fw->data == builtin->data)
goto free_fw;
}
- vfree(fw->data);
+ vunmap(fw->data);
+ if (fw->pages) {
+ for (i = 0; i < PFN_UP(fw->size); i++)
+ __free_page(fw->pages[i]);
+ kfree(fw->pages);
+ }
free_fw:
kfree(fw);
}

--
dwmw2



\
 
 \ /
  Last update: 2010-04-27 14:45    [W:0.064 / U:2.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site