linux-kernel - Re: [PATCH] dma_declare_coherent_memory: push ioremap() up to caller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20101223235434.GA20587@n2100.arm.linux.org.uk>
Date:	Thu, 23 Dec 2010 23:54:34 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Janusz Krzysztofik <jkrzyszt@....icnet.pl>
Cc:	linux-arch@...r.kernel.org, Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Arnd Bergmann <arnd@...db.de>,
	Dan Williams <dan.j.williams@...el.com>,
	linux-sh@...r.kernel.org, Paul Mundt <lethal@...ux-sh.org>,
	Sascha Hauer <kernel@...gutronix.de>,
	linux-usb@...r.kernel.org,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	linux-media@...r.kernel.org,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	linux-scsi@...r.kernel.org,
	"James E.J. Bottomley" <James.Bottomley@...e.de>,
	Catalin Marinas <catalin.marinas@....com>
Subject: Re: [PATCH] dma_declare_coherent_memory: push ioremap() up to
	caller

On Fri, Dec 24, 2010 at 12:20:32AM +0100, Janusz Krzysztofik wrote:
> The patch tries to implement a solution suggested by Russell King, 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-December/035264.html. 
> It is expected to solve video buffer allocation issues for at least a 
> few soc_camera I/O memory less host interface drivers, designed around 
> the videobuf_dma_contig layer, which allocates video buffers using 
> dma_alloc_coherent().
> 
> Created against linux-2.6.37-rc5.
> 
> Tested on ARM OMAP1 based Amstrad Delta with a WIP OMAP1 camera patch, 
> patterned upon two mach-mx3 machine types which already try to use the 
> dma_declare_coherent_memory() method for reserving a region of system 
> RAM preallocated with another dma_alloc_coherent(). Compile tested for 
> all modified files except arch/sh/drivers/pci/fixups-dreamcast.c.
> 
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@....icnet.pl>
> ---
> I intended to quote Russell in my commit message and even asked him for 
> his permission, but since he didn't respond, I decided to include a link 
> to his original message only.

There's no problem quoting messages which were sent to public mailing
lists, especially when there's a record of what was said in public
archives too.

I think this is definitely a step forward.

> --- linux-2.6.37-rc5/arch/arm/mach-mx3/mach-pcm037.c.orig	2010-12-09 23:07:34.000000000 +0100
> +++ linux-2.6.37-rc5/arch/arm/mach-mx3/mach-pcm037.c	2010-12-23 18:32:24.000000000 +0100
> @@ -431,7 +431,7 @@ static int __init pcm037_camera_alloc_dm
>  	memset(buf, 0, buf_size);
>  
>  	dma = dma_declare_coherent_memory(&mx3_camera.dev,
> -					dma_handle, dma_handle, buf_size,
> +					buf, dma_handle, buf_size,
>  					DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE);
>  
>  	/* The way we call dma_declare_coherent_memory only a malloc can fail */
> --- linux-2.6.37-rc5/arch/arm/mach-mx3/mach-mx31moboard.c.orig	2010-12-09 23:07:34.000000000 +0100
> +++ linux-2.6.37-rc5/arch/arm/mach-mx3/mach-mx31moboard.c	2010-12-23 18:32:24.000000000 +0100
> @@ -486,7 +486,7 @@ static int __init mx31moboard_cam_alloc_
>  	memset(buf, 0, buf_size);
>  
>  	dma = dma_declare_coherent_memory(&mx3_camera.dev,
> -					dma_handle, dma_handle, buf_size,
> +					buf, dma_handle, buf_size,
>  					DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE);
>  
>  	/* The way we call dma_declare_coherent_memory only a malloc can fail */

A side note for the mx3 folk: although it's not specified in DMA-API,
memory allocated from dma_alloc_coherent() on ARM is already memset
to zero by the allocation function.

> --- linux-2.6.37-rc5/drivers/base/dma-coherent.c.orig	2010-12-09 23:08:03.000000000 +0100
> +++ linux-2.6.37-rc5/drivers/base/dma-coherent.c	2010-12-23 18:32:24.000000000 +0100
> @@ -14,10 +14,9 @@ struct dma_coherent_mem {
>  	unsigned long	*bitmap;
>  };
>  
> -int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
> +int dma_declare_coherent_memory(struct device *dev, void *virt_addr,
>  				dma_addr_t device_addr, size_t size, int flags)
>  {
> -	void __iomem *mem_base = NULL;
>  	int pages = size >> PAGE_SHIFT;
>  	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
>  
> @@ -30,10 +29,6 @@ int dma_declare_coherent_memory(struct d
>  
>  	/* FIXME: this routine just ignores DMA_MEMORY_INCLUDES_CHILDREN */
>  
> -	mem_base = ioremap(bus_addr, size);
> -	if (!mem_base)
> -		goto out;
> -
>  	dev->dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL);
>  	if (!dev->dma_mem)
>  		goto out;
> @@ -41,7 +36,7 @@ int dma_declare_coherent_memory(struct d
>  	if (!dev->dma_mem->bitmap)
>  		goto free1_out;
>  
> -	dev->dma_mem->virt_base = mem_base;
> +	dev->dma_mem->virt_base = virt_addr;

I didn't see anything changing the dev->dma_mem->virt_base type to
drop the __iomem attribute (which I suspect shouldn't be there - we're
returning it via a void pointer anyway, so I think the whole coherent
part of the DMA API should be __iomem-less.

It also pushes the sparse address space warnings to the right place
IMHO too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/