[PATCH] dma_declare_coherent_memory wrong allocation

[PATCH] dma_declare_coherent_memory wrong allocation

[Guennadi Liakhovetski]

Hi

Either I've finally gone blind on this Friday 13th or... Looks like this 
almost 3 year old function has a bug. Patch below compile-tested... in a 
way.

Thanks
Guennadi
---
Guennadi Liakhovetski

dma_declare_coherent_memory() allocates a bitmap 1 bit per page, it 
calculates the bitmap size based on size of long, but allocates bytes...

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/i386/kernel/pci-dma.c b/arch/i386/kernel/pci-dma.c
index 3ebcea0..3d55226 100644
--- a/arch/i386/kernel/pci-dma.c
+++ b/arch/i386/kernel/pci-dma.c
@@ -77,7 +77,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 {
 	void __iomem *mem_base = NULL;
 	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int bitmap_size = DIV_ROUND_UP(pages, 8);
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[Andrew Morton]

On Fri, 13 Apr 2007 20:08:28 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> Hi
> 
> Either I've finally gone blind on this Friday 13th or... Looks like this 
> almost 3 year old function has a bug. Patch below compile-tested... in a 
> way.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 
> dma_declare_coherent_memory() allocates a bitmap 1 bit per page, it 
> calculates the bitmap size based on size of long, but allocates bytes...
> 
> Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>
> 
> diff --git a/arch/i386/kernel/pci-dma.c b/arch/i386/kernel/pci-dma.c
> index 3ebcea0..3d55226 100644
> --- a/arch/i386/kernel/pci-dma.c
> +++ b/arch/i386/kernel/pci-dma.c
> @@ -77,7 +77,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
>  {
>  	void __iomem *mem_base = NULL;
>  	int pages = size >> PAGE_SHIFT;
> -	int bitmap_size = (pages + 31)/32;
> +	int bitmap_size = DIV_ROUND_UP(pages, 8);
>  
>  	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
>  		goto out;

Looks that way to me, too.

It also hopes like hell that `size' was a multiple of PAGE_SIZE.

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[Guennadi Liakhovetski]

On Sat, 21 Apr 2007, Andrew Morton wrote:

> On Fri, 13 Apr 2007 20:08:28 +0200 (CEST) Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> 
> > Hi
> > 
> > Either I've finally gone blind on this Friday 13th or... Looks like this 
> > almost 3 year old function has a bug. Patch below compile-tested... in a 
> > way.

[patch skipped]

> Looks that way to me, too.

Then I'll dare suggest arch/cris - the second and last arch implementing 
this API - has the same problem. Maintainer added to cc:, patch below.

> It also hopes like hell that `size' was a multiple of PAGE_SIZE.

Well, this API seems to have a bigger problem: it is difficult to get to 
SoC/bus-local memory from several child devices. Even if I do implement 
that DMA_MEMORY_INCLUDES_CHILDREN flag, 1) you do not always have the 
desired device as your immediate parent, 2) you have to ref-count it. For 
example, I was trying to use SoC-local SRAM on PXA270 from the 
framebuffer, but fb-devices on PXA are either platform devices, or 
sometimes get other parents. So, looks like another way to get to the SoC 
node is needed...

Thanks
Guennadi
---
Guennadi Liakhovetski

Fix dma_declare_coherent_memory wrong allocation for cris/arch-v32
    
arch/cris/arch-v32 seems to have copy-pasted the bug from arch/i386 fixed 
with an earlier patch - allocate bitmap in bytes, not words.
    
Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/cris/arch-v32/drivers/pci/dma.c b/arch/cris/arch-v32/drivers/pci/dma.c
index 70d3bf0..d634347 100644
--- a/arch/cris/arch-v32/drivers/pci/dma.c
+++ b/arch/cris/arch-v32/drivers/pci/dma.c
@@ -76,7 +76,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 {
 	void __iomem *mem_base;
 	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int bitmap_size = DIV_ROUND_UP(pages, 8);
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[James Bottomley]

On Fri, 2007-04-13 at 20:08 +0200, Guennadi Liakhovetski wrote:
> Either I've finally gone blind on this Friday 13th or... Looks like this 
> almost 3 year old function has a bug. Patch below compile-tested... in a 
> way.

No, it's a longstanding bug in the x86 implementation, thanks for
finding it.

> -	int bitmap_size = (pages + 31)/32;
> +	int bitmap_size = DIV_ROUND_UP(pages, 8);

This isn't quite right.  Bitmaps are arrays of longs, not arrays of
bytes.  The bug is forgetting that kmalloc() takes bytes ...

How about

int bitmap_size = DIV_ROUNDUP(pages, 32) * 4;

?

James

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[Guennadi Liakhovetski]

On Sun, 22 Apr 2007, James Bottomley wrote:

> On Fri, 2007-04-13 at 20:08 +0200, Guennadi Liakhovetski wrote:
> > -	int bitmap_size = (pages + 31)/32;
> > +	int bitmap_size = DIV_ROUND_UP(pages, 8);
> 
> This isn't quite right.  Bitmaps are arrays of longs, not arrays of
> bytes.  The bug is forgetting that kmalloc() takes bytes ...
> 
> How about
> 
> int bitmap_size = DIV_ROUNDUP(pages, 32) * 4;

Right, thinko. How about using his:

+	int pages = DIV_ROUND_UP(size, PAGE_SIZE);
+	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;

to also allow for size not an integer number of pages as Andrew noticed? 
This could be done in 2 patches:

---
Introduce BYTES_PER_LONG and remove local definitions. fbsys 
compile-tested, amifb untested (framebuffer maintainer cc'ed)...

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

--- a/include/linux/types.h	2007-04-15 22:07:52.000000000 +0200
+++ b/include/linux/types.h	2007-04-22 22:51:17.000000000 +0200
@@ -9,6 +9,7 @@
 	unsigned long name[BITS_TO_LONGS(bits)]
 
 #define BITS_PER_BYTE 8
+#define BYTES_PER_LONG (BITS_PER_LONG / BITS_PER_BYTE)
 #endif
 
 #include <linux/posix_types.h>
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index 40c80c8..6bb68b8 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -19,6 +19,7 @@
 #include <linux/fb.h>
 #include <linux/console.h>
 #include <linux/module.h>
+#include <linux/types.h>
 
 #define FB_SYSFS_FLAG_ATTR 1
 
@@ -37,7 +38,6 @@
  */
 struct fb_info *framebuffer_alloc(size_t size, struct device *dev)
 {
-#define BYTES_PER_LONG (BITS_PER_LONG/8)
 #define PADDING (BYTES_PER_LONG - (sizeof(struct fb_info) % BYTES_PER_LONG))
 	int fb_info_size = sizeof(struct fb_info);
 	struct fb_info *info;
diff --git a/drivers/video/amifb.c b/drivers/video/amifb.c
index 1a849b8..a53d5a2 100644
--- a/drivers/video/amifb.c
+++ b/drivers/video/amifb.c
@@ -51,6 +51,8 @@
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/ioport.h>
+#include <linux/types.h>
+#include <linux/log2.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>
@@ -1350,15 +1352,7 @@ static int amifb_pan_display(struct fb_var_screeninfo *var,
 }
 
 
-#if BITS_PER_LONG == 32
-#define BYTES_PER_LONG	4
-#define SHIFT_PER_LONG	5
-#elif BITS_PER_LONG == 64
-#define BYTES_PER_LONG	8
-#define SHIFT_PER_LONG	6
-#else
-#define Please update me
-#endif
+#define SHIFT_PER_LONG	ilog2(BITS_PER_LONG)
 
 
     /*

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[Guennadi Liakhovetski]

On Mon, 23 Apr 2007, Guennadi Liakhovetski wrote:

> to also allow for size not an integer number of pages as Andrew noticed? 
> This could be done in 2 patches:

patch 2:

---
Fix bitmap allocation and size non-multiple of PAGE_SIZE in 
dma_declare_coherent_memory implementations. i386 compile-tested.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/i386/kernel/pci-dma.c b/arch/i386/kernel/pci-dma.c
index 3ebcea0..0a9317a 100644
--- a/arch/i386/kernel/pci-dma.c
+++ b/arch/i386/kernel/pci-dma.c
@@ -76,8 +76,8 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 				dma_addr_t device_addr, size_t size, int flags)
 {
 	void __iomem *mem_base = NULL;
-	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int pages = DIV_ROUND_UP(size, PAGE_SIZE);
+	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;
diff --git a/arch/cris/arch-v32/drivers/pci/dma.c b/arch/cris/arch-v32/drivers/pci/dma.c
index d634347..99f760e 100644
--- a/arch/cris/arch-v32/drivers/pci/dma.c
+++ b/arch/cris/arch-v32/drivers/pci/dma.c
@@ -75,8 +75,8 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 				dma_addr_t device_addr, size_t size, int flags)
 {
 	void __iomem *mem_base;
-	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int pages = DIV_ROUND_UP(size, PAGE_SIZE);
+	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[James Bottomley]

On Mon, 2007-04-23 at 00:45 +0200, Guennadi Liakhovetski wrote:
> Right, thinko. How about using his:
> 
> +	int pages = DIV_ROUND_UP(size, PAGE_SIZE);

Actually, no ... this has to be size >> PAGE_SHIFT.  The reason being
that the  allocator is designed to allocate pages out of a device memory
buffer.  If the size isn't a multiple of page size, we have to round
down (we can't allocate the last page if the memory we have is only part
of a page).

I suppose if you want to catch the unlikely nutcase where size <
PAGE_SIZE you could

if (unlikely(pages == 0))
	goto out;

> +	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;

This is fine, except the BYTES_PER_LONG.  Traditionally, we do this with
sizeof(long).  The only reason to have a #define for it is if it has to
be used in a macro (the compiler does sizeof() not the preprocessor).

How about just a simple

int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);

?

Then you don't need the first patch defining BYTES_PER_LONG.

> to also allow for size not an integer number of pages as Andrew noticed? 
> This could be done in 2 patches:

James

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[Guennadi Liakhovetski]

On Mon, 23 Apr 2007, James Bottomley wrote:

> On Mon, 2007-04-23 at 00:45 +0200, Guennadi Liakhovetski wrote:
> > Right, thinko. How about using his:
> > 
> > +	int pages = DIV_ROUND_UP(size, PAGE_SIZE);
> 
> Actually, no ... this has to be size >> PAGE_SHIFT.  The reason being
> that the  allocator is designed to allocate pages out of a device memory
> buffer.  If the size isn't a multiple of page size, we have to round
> down (we can't allocate the last page if the memory we have is only part
> of a page).
> 
> I suppose if you want to catch the unlikely nutcase where size <
> PAGE_SIZE you could
> 
> if (unlikely(pages == 0))
> 	goto out;
> 
> > +	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;
> 
> This is fine, except the BYTES_PER_LONG.  Traditionally, we do this with
> sizeof(long).  The only reason to have a #define for it is if it has to
> be used in a macro (the compiler does sizeof() not the preprocessor).
> 
> How about just a simple
> 
> int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);

Ok, version 3, small, and hopefully, beautiful:

dma_declare_coherent_memory() allocates a bitmap 1 bit per page, it 
calculates the bitmap size based on size of long, but allocates bytes...
Thanks to James Bottomley for clarifications and corrections.

Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

diff --git a/arch/cris/arch-v32/drivers/pci/dma.c b/arch/cris/arch-v32/drivers/pci/dma.c
index 70d3bf0..832fc63 100644
--- a/arch/cris/arch-v32/drivers/pci/dma.c
+++ b/arch/cris/arch-v32/drivers/pci/dma.c
@@ -76,7 +76,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 {
 	void __iomem *mem_base;
 	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;
diff --git a/arch/i386/kernel/pci-dma.c b/arch/i386/kernel/pci-dma.c
index 3ebcea0..30b754f 100644
--- a/arch/i386/kernel/pci-dma.c
+++ b/arch/i386/kernel/pci-dma.c
@@ -77,7 +77,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 {
 	void __iomem *mem_base = NULL;
 	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = (pages + 31)/32;
+	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;

Re: [PATCH] dma_declare_coherent_memory wrong allocation

[James Bottomley]

On Mon, 2007-04-23 at 22:45 +0200, Guennadi Liakhovetski wrote:
> On Mon, 23 Apr 2007, James Bottomley wrote:
> 
> > On Mon, 2007-04-23 at 00:45 +0200, Guennadi Liakhovetski wrote:
> > > Right, thinko. How about using his:
> > > 
> > > +	int pages = DIV_ROUND_UP(size, PAGE_SIZE);
> > 
> > Actually, no ... this has to be size >> PAGE_SHIFT.  The reason being
> > that the  allocator is designed to allocate pages out of a device memory
> > buffer.  If the size isn't a multiple of page size, we have to round
> > down (we can't allocate the last page if the memory we have is only part
> > of a page).
> > 
> > I suppose if you want to catch the unlikely nutcase where size <
> > PAGE_SIZE you could
> > 
> > if (unlikely(pages == 0))
> > 	goto out;
> > 
> > > +	int bitmap_size = BITS_TO_LONGS(pages) * BYTES_PER_LONG;
> > 
> > This is fine, except the BYTES_PER_LONG.  Traditionally, we do this with
> > sizeof(long).  The only reason to have a #define for it is if it has to
> > be used in a macro (the compiler does sizeof() not the preprocessor).
> > 
> > How about just a simple
> > 
> > int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
> 
> Ok, version 3, small, and hopefully, beautiful:
> 
> dma_declare_coherent_memory() allocates a bitmap 1 bit per page, it 
> calculates the bitmap size based on size of long, but allocates bytes...
> Thanks to James Bottomley for clarifications and corrections.
> 
> Signed-off-by: G. Liakhovetski <g.liakhovetski@gmx.de>

Looks perfect to me.

Acked-by: James Bottomley <James.Bottomley@SteelEye.com>

> diff --git a/arch/cris/arch-v32/drivers/pci/dma.c b/arch/cris/arch-v32/drivers/pci/dma.c
> index 70d3bf0..832fc63 100644
> --- a/arch/cris/arch-v32/drivers/pci/dma.c
> +++ b/arch/cris/arch-v32/drivers/pci/dma.c
> @@ -76,7 +76,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
>  {
>  	void __iomem *mem_base;
>  	int pages = size >> PAGE_SHIFT;
> -	int bitmap_size = (pages + 31)/32;
> +	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
>  
>  	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
>  		goto out;
> diff --git a/arch/i386/kernel/pci-dma.c b/arch/i386/kernel/pci-dma.c
> index 3ebcea0..30b754f 100644
> --- a/arch/i386/kernel/pci-dma.c
> +++ b/arch/i386/kernel/pci-dma.c
> @@ -77,7 +77,7 @@ int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
>  {
>  	void __iomem *mem_base = NULL;
>  	int pages = size >> PAGE_SHIFT;
> -	int bitmap_size = (pages + 31)/32;
> +	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
>  
>  	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
>  		goto out;