drivers: dma-coherent: free memory when failed to init DMA memory pool
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)
drivers/base/dma-coherent.c +1 -0
@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) &mem) != DMA_MEMORY_MAP) {pr_err ("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n" , &rmem->base, (unsigned long )rmem->size /SZ_1M ); +kfree (mem);return -ENODEV ; } rmem->priv = mem;
Signature · 1 line
1.9.1
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
Quoted 13 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)drivers/base/dma-coherent.c +0
-0
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) · line 289
Quoted 4 lines
drivers/base/dma-coherent.c +1
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) &mem) != DMA_MEMORY_MAP) {pr_err ("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n" , &rmem->base, (unsigned long )rmem->size /SZ_1M ); +kfree (mem);
mem == NULL at this point. If dma_init_coherent_memory doesn?t return DMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of it happening.
Quoted 6 lines
drivers/base/dma-coherent.c +0
-0return -ENODEV ; } rmem->priv = mem;Signature · 1 line
1.9.1
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, ??? ?mina86? ?????? (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 24 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory pooldrivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem
Quoted 13 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem*rmem, struct device *dev)
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem · line 287
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory pool
Quoted 8 lines
at %pa, size %ld MiB\n",
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) · line 289
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of it happening.
Kfree() will check mem NULL condition, so no need to add check in here.
Quoted 12 lines
Quoted 6 lines
drivers/base/dma-coherent.c +0
-1return -ENODEV ; } rmem->priv = mem; --1.9.1
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, ??? ?mina86? ?????? (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
Quoted 33 lines · Duan Andy
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 24 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory pooldrivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem
Quoted 13 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem*rmem, struct device *dev)
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem · line 287
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory poolQuoted 7 lines
at %pa, size %ld MiB\n",
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) · line 289
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of it happening.
Quoted 2 lines
Kfree()will check mem NULL condition, so no need to add check in here.
Sure, but besides the point. mem is always NULL so kfree(mem) is a no-op so why add it? There is no memory leak possible. What is this patch fixing?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/base/dma-coherent.c +0
-0return -ENODEV ; } rmem->priv = mem;
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, ??? ?mina86? ?????? (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
In reply to Duan AndyQuoted 26 lines · Duan Andy
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 25 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory pooldrivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem
Quoted 13 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem*rmem, struct device *dev)
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem · line 287
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory poolat %pa, size %ld MiB\n",
Quoted 8 lines
Quoted 7 lines
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) · line 289
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of it happening.
Kfree()will check mem NULL condition, so no need to add check in here.
That isn't what the reviewer was stating.
However, had the reviewer looked at the existing code, then he may have stated a different opinion. :)
When you look at the existing code, there are three possible return values from dma_init_coherent_memory():
- zero, which means failure, and the mem pointer is left alone. In the above code, we know that it's guaranteed to be NULL, as we won't get into the if() unless it was.
- DMA_MEMORY_IO, which would mean that mem points at a kmalloc()d chunk of memory, and this case is treated as failure because we want memory.
- DMA_MEMORY_MAP, which also means that mem points at a kmalloc()d chunk of memory, which we treat as success.
So, in the case of DMA_MEMORY_IO, we have a failure case where the kmalloc'd memory is dropped on the floor - aka a memory leak. In the failure paths, there are two known values for mem: NULL, or validly pointing at kmalloc()'d memory which would be leaked. So, adding a kfree() here is certainly correct.
However, the above explanation needs to go into the commit message to justify the change.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
Quoted 34 lines · Russell King - ARM Linux
Quoted 33 lines · Duan Andy
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 32 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory pooldrivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem
Quoted 13 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes memmory leak.
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem*rmem, struct device *dev)
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem · line 287
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory poolat %pa, size %ld MiB\n",
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem · line 289
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of it happening.
Quoted 6 lines
Quoted 1 line
Kfree()will check mem NULL condition, so no need to add check in here.That isn't what the reviewer was stating.
However, had the reviewer looked at the existing code, then he may have stated a different opinion. :)
I?m not sure that I would. Here?s code I?m looking at:
static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addr,
size_t size, int flags,
struct dma_coherent_mem **mem)
{
struct dma_coherent_mem *dma_mem = NULL;
void __iomem *mem_base = NULL;
int pages = size >> PAGE_SHIFT;
int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
goto out;
if (!size)
goto out;
mem_base = ioremap(phys_addr, size);
if (!mem_base)
goto out;
dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL);
if (!dma_mem)
goto out;
dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
if (!dma_mem->bitmap)
goto out;
dma_mem->virt_base = mem_base;
dma_mem->device_base = device_addr;
dma_mem->pfn_base = PFN_DOWN(phys_addr);
dma_mem->size = pages;
dma_mem->flags = flags;
spin_lock_init(&dma_mem->spinlock);
/**************************************************************/
/* Here's the only place where mem is set to non-NULL value. */
/**************************************************************/
*mem = dma_mem;
if (flags & DMA_MEMORY_MAP)
/******************************************************/
/* Here's where function returns. */
/******************************************************/
return DMA_MEMORY_MAP;
return DMA_MEMORY_IO;out:
kfree(dma_mem); if (mem_base) iounmap(mem_base); return 0; }
static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)
{
struct dma_coherent_mem *mem = rmem->priv;
if (!mem &&
/**********************************************************/
/* Note that flags & DMA_MEMORY_MAP is non-zero. */
/**********************************************************/
dma_init_coherent_memory(rmem->base, rmem->base, rmem->size,
DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE,
&mem) != DMA_MEMORY_MAP) {
pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n",
&rmem->base, (unsigned long)rmem->size / SZ_1M);
return -ENODEV;
}
rmem->priv = mem;
dma_assign_coherent_memory(dev, mem);
return 0;
}Quoted 9 lines
When you look at the existing code, there are three possible return values from
dma_init_coherent_memory():- zero, which means failure, and the mem pointer is left alone. In the above code, we know that it's guaranteed to be NULL, as we won't get into the
if()unless it was.
-DMA_MEMORY_IO, which would mean that mem points at a kmalloc()d chunk of memory, and this case is treated as failure because we want memory.
No. This never happens in the code that is being patched. flags & DMA_MEMORY_MAP is non-zero thus dma_init_coherent_memory returns DMA_MEMORY_MAP.
Quoted 4 lines
-
DMA_MEMORY_MAP, which also means that mem points at a kmalloc()d chunk of memory, which we treat as success.So, in the case of
DMA_MEMORY_IO,
Which never happens.
Quoted 2 lines
we have a failure case where the kmalloc'd memory is dropped on the floor - aka a memory leak.
To me it seems that a better patch would be the following (not tested in any capacity):
Quoted 1 line
From
49cece0c0a052022022776f2555d26308deba959Mon Sep 17 00:00:00 2001
From: Michal Nazarewicz <[email protected]>
Date: Thu, 3 Dec 2015 17:15:07 +0100
Subject: [PATCH] drivers: dma-coherent: simplify dma_init_coherent_memory
return value
Since only dma_declare_coherent_memory cares about dma_init_coherent_memory returning part of flags as it return value, move the condition to the former and simplify the latter. This in turn makes rmem_dma_device_init less confusing.
--- drivers/base/dma-coherent.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
\o/ negative delta.
drivers/base/dma-coherent.c +11 -17
@@ -17,9 +17,9 @@ struct dma_coherent_mem {spinlock_t spinlock; }; -static int dma_init_coherent_memory (phys_addr_t phys_addr,dma_addr_t device_addr, -size_t size,int flags, -struct dma_coherent_mem **mem) +static bool dma_init_coherent_memory ( +phys_addr_t phys_addr,dma_addr_t device_addr,size_t size,int flags, +struct dma_coherent_mem **mem) {struct dma_coherent_mem *dma_mem =NULL ;void __iomem *mem_base =NULL ;
@@ -50,17 +50,13 @@ static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addspin_lock_init (&dma_mem->spinlock); *mem = dma_mem; - -if (flags &DMA_MEMORY_MAP ) -return DMA_MEMORY_MAP ; - -return DMA_MEMORY_IO ; +return true; out:kfree (dma_mem);if (mem_base)iounmap (mem_base); -return 0 ; +return false; }static void dma_release_coherent_memory (struct dma_coherent_mem *mem)
@@ -88,15 +84,13 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags) { struct dma_coherent_mem *mem; -int ret; - ret =dma_init_coherent_memory (phys_addr, device_addr, size, flags, - &mem); -if (ret ==0 ) +if (!dma_init_coherent_memory (phys_addr, device_addr, size, flags, + &mem))return 0 ;if (dma_assign_coherent_memory (dev, mem) ==0 ) -return ret; +return flags &DMA_MEMORY_MAP ?DMA_MEMORY_MAP :DMA_MEMORY_IO ;dma_release_coherent_memory (mem);return 0 ;
@@ -281,9 +275,9 @@ static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)struct dma_coherent_mem *mem = rmem->priv;if (!mem && -dma_init_coherent_memory (rmem->base, rmem->base, rmem->size, -DMA_MEMORY_MAP |DMA_MEMORY_EXCLUSIVE , - &mem) !=DMA_MEMORY_MAP ) { + !dma_init_coherent_memory (rmem->base, rmem->base, rmem->size, +DMA_MEMORY_MAP |DMA_MEMORY_EXCLUSIVE , + &mem)) {pr_err ("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n" , &rmem->base, (unsigned long )rmem->size /SZ_1M );return -ENODEV ;
Signature · 7 lines
2.6.0.rc2.230.g3dd15c0 -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, ??? ?mina86? ?????? (o o) ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
From: Mike Nazarewicz <[email protected]> Sent: Friday, December 04, 2015 12:23 AM
Quoted 145 lines
To: Russell King - ARM Linux; Duan Fugang-B38611
Cc: torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com; iamjoonsoo.kim at lge.com; arnd at arndb.de; linux- arm-kernel at lists.infradead.org
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory poolQuoted 10 lines · Russell King - ARM Linux
Quoted 9 lines · Duan Andy
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 7 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory poolQuoted 3 lines
Quoted 3 lines
Quoted 3 lines · Fugang Duan
Quoted 2 lines
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes
memmory leak.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 11 lines
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)(quoted diff) +0
-0
b/drivers/base/dma-coherent.cindex 55b8398..beb6bbe 100644drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(structQuoted 1 line
reserved_mem*rmem, struct device *dev)
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory poolQuoted 7 lines
Quoted 7 lines
Quoted 7 lines
at %pa, size %ld MiB\n",
drivers/base/dma-coherent.c
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of ithappening.
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
Kfree()will check mem NULL condition, so no need to add check in here.That isn't what the reviewer was stating.
However, had the reviewer looked at the existing code, then he may have stated a different opinion. :)
I?m not sure that I would. Here?s code I?m looking at:
static int dma_init_coherent_memory(phys_addr_t
phys_addr,dma_addr_tdevice_addr,
size_tsize, int flags, structdma_coherent_mem**mem){ struct dma_coherent_mem *dma_mem = NULL; void __iomem *mem_base = NULL; int pages = size >> PAGE_SHIFT; int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0) goto out; if (!size) goto out; mem_base = ioremap(phys_addr, size); if (!mem_base) goto out; dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL); if (!dma_mem) goto out; dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL); if (!dma_mem->bitmap) goto out; dma_mem->virt_base = mem_base; dma_mem->device_base = device_addr; dma_mem->pfn_base = PFN_DOWN(phys_addr); dma_mem->size = pages; dma_mem->flags = flags; spin_lock_init(&dma_mem->spinlock); /**************************************************************/ /* Here's the only place where mem is set to non-NULL value. */ /**************************************************************/ *mem = dma_mem; if (flags & DMA_MEMORY_MAP) /******************************************************/ /* Here's where function returns. */ /******************************************************/ return DMA_MEMORY_MAP; return DMA_MEMORY_IO;out:
kfree(dma_mem); if (mem_base) iounmap(mem_base); return 0; }static int rmem_dma_device_init(struct
reserved_mem*rmem, struct device*dev) { struct dma_coherent_mem *mem = rmem->priv; if (!mem && /**********************************************************/ /* Note that flags & DMA_MEMORY_MAP is non-zero. */ /**********************************************************/ dma_init_coherent_memory(rmem->base, rmem->base, rmem->size, DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE, &mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n", &rmem->base, (unsigned long)rmem->size / SZ_1M); return -ENODEV; } rmem->priv = mem; dma_assign_coherent_memory(dev, mem); return 0; }Quoted 9 lines
When you look at the existing code, there are three possible return values from
dma_init_coherent_memory():- zero, which means failure, and the mem pointer is left alone. In the above code, we know that it's guaranteed to be NULL, as we won't get into the
if()unless it was.
-DMA_MEMORY_IO, which would mean that mem points at a kmalloc()d chunk of memory, and this case is treated as failure because we want memory.No. This never happens in the code that is being patched. flags &
DMA_MEMORY_MAPis non-zero thusdma_init_coherent_memoryreturnsDMA_MEMORY_MAP.
Quoted 7 lines
Quoted 4 lines
-
DMA_MEMORY_MAP, which also means that mem points at a kmalloc()d chunk of memory, which we treat as success.So, in the case of
DMA_MEMORY_IO,Which never happens.
I agree with your explain. For current code, dma_init_coherent_memory(x,x,x, DMA_MEMORY_MAP|x, x) bring DMA_MEMORY_MAP flag, so return value DMA_MEMORY_IO never happens.
Sorry for my mistake, pls drop the patch. Thanks Michal and Russell's detail comments and review.
[snip]
PATCH 1/1 drivers: dma-coherent: free memory when failed to init DMA memory pool
In reply to Russell King - ARM LinuxFrom: Russell King - ARM Linux <[email protected]> Sent: Thursday, December 03, 2015 11:40 PM
Quoted 65 lines
To: Duan Fugang-B38611
Cc: Michal Nazarewicz; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com; iamjoonsoo.kim at lge.com; arnd at arndb.de; linux-arm- kernel at lists.infradead.org
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory poolQuoted 12 lines · Duan Andy
From: Mike Nazarewicz <[email protected]> Sent: Thursday, December 03, 2015 8:51 PM
Quoted 10 lines
To: Duan Fugang-B38611; torvalds at linux-foundation.org; gregkh at linuxfoundation.org; m.szyprowski at samsung.com
Cc: linux-arm-kernel at lists.infradead.org; arnd at arndb.de; iamjoonsoo.kim at lge.com; Duan Fugang-B38611
Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory poolQuoted 2 lines · Fugang Duan
Free dma coherent memory when it failed to init DMA memory pool after calling .dma_init_coherent_memory(), otherwise it causes
memmory leak.
Quoted 15 lines
Quoted 15 lines
Quoted 11 lines
Signed-off-by Fugang Duan <[email protected]>--- drivers/base/dma-coherent.c | 1 + 1 file changed, 1 insertion(+)(quoted diff) +0
-0
b/drivers/base/dma-coherent.cindex 55b8398..beb6bbe 100644drivers/base/dma-coherent.c +0
-0@@ -286,6 +286,7 @@ static int rmem_dma_device_init(structQuoted 1 line
reserved_mem*rmem, struct device *dev)
Quoted 2 lines
drivers/base/dma-coherent.c +0
-0&mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory poolQuoted 7 lines
Quoted 7 lines
at %pa, size %ld MiB\n",
drivers/base/dma-coherent.c static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) · line 289
Quoted 2 lines
drivers/base/dma-coherent.c +1
-0&rmem->base, (unsigned long)rmem->size / SZ_1M); +kfree (mem);mem == NULL at this point. If
dma_init_coherent_memorydoesn?t returnDMA_MEMORY_MAP, mem pointer is not assigned any value. If you think there is a memory leak, please demonstrate a path of ithappening.
Quoted 2 lines
Quoted 1 line
Kfree()will check mem NULL condition, so no need to add check in here.That isn't what the reviewer was stating.
However, had the reviewer looked at the existing code, then he may have stated a different opinion. :)
When you look at the existing code, there are three possible return values from
dma_init_coherent_memory():- zero, which means failure, and the mem pointer is left alone. In the above code, we know that it's guaranteed to be NULL, as we won't get into the
if()unless it was.
-DMA_MEMORY_IO, which would mean that mem points at a kmalloc()d chunk of memory, and this case is treated as failure because we want memory.
-DMA_MEMORY_MAP, which also means that mem points at a kmalloc()d chunk of memory, which we treat as success.
Quoted 9 lines
So, in the case of
DMA_MEMORY_IO, we have a failure case where the kmalloc'd memory is dropped on the floor - aka a memory leak. In the failure paths, there are two known values for mem: NULL, or validly pointing at kmalloc()'d memory which would be leaked. So, adding akfree()here is certainly correct.However, the above explanation needs to go into the commit message to justify the change.
Thanks Russell's detail explain.
As Michal's comment in next mail, return DMA_MEMORY_IO never happens. So no need to free the mem.