Re: [PATCH] dma_declare_coherent_memory wrong allocation

[James Bottomley <James.Bottomley@SteelEye.com>]

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;