From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera Date: Mon, 13 Dec 2010 16:29:47 +0000 Message-ID: <20101213162947.GA11730@n2100.arm.linux.org.uk> References: <201012051929.07220.jkrzyszt@tis.icnet.pl> <201012101159.21845.jkrzyszt@tis.icnet.pl> <201012101203.09441.jkrzyszt@tis.icnet.pl> <20101210170356.GA28472@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from caramon.arm.linux.org.uk ([78.32.30.218]:48821 "EHLO caramon.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758017Ab0LMQak (ORCPT ); Mon, 13 Dec 2010 11:30:40 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Catalin Marinas Cc: Janusz Krzysztofik , Tony Lindgren , "linux-omap@vger.kernel.org" , Guennadi Liakhovetski , linux-arm-kernel@lists.infradead.org, Linux Media Mailing List On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote: > On 10 December 2010 17:03, Russell King - ARM Linux > wrote: > > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote: > >> =A0void __init omap1_camera_init(void *info) > >> =A0{ > >> =A0 =A0 =A0 struct platform_device *dev =3D &omap1_camera_device; > >> + =A0 =A0 dma_addr_t paddr =3D omap1_camera_phys_mempool_base; > >> + =A0 =A0 dma_addr_t size =3D omap1_camera_phys_mempool_size; > >> =A0 =A0 =A0 int ret; > >> > >> =A0 =A0 =A0 dev->dev.platform_data =3D info; > >> > >> + =A0 =A0 if (paddr) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 if (dma_declare_coherent_memory(&dev->de= v, paddr, paddr, size, > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DMA_MEMO= RY_MAP | DMA_MEMORY_EXCLUSIVE)) > > > > Although this works, you're ending up with SDRAM being mapped via > > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being > > mapped as if it were a device. >=20 > BTW, does the generic dma_declare_coherent_memory() does the correct > thing in using ioremap()? I argue it doesn't, as I said above. It means we map SDRAM as device memory, and as I understand the way interconnects work, it's entirely possible that this may not result in the SDRAM being accessible. > Maybe some other function that takes a > pgprot_t would be better (ioremap_page_range) and could pass somethin= g > like pgprot_noncached (though ideally a pgprot_dmacoherent). Or just > an architecturally-defined function. This API was designed in the ARMv5 days when things were a lot simpler. What we now have is rather messy though, and really needs sorting out before we get too many users of it. (Arguably it should never have bee= n accepted in its current state.) We have a rule that the returned value of ioremap() is not to be used as a pointer which can be dereferenced by anything except driver code. Yet this is exactly what dma_declare_coherent_memory() does: struct dma_coherent_mem { void *virt_base; =2E.. }; int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr= , dma_addr_t device_addr, size_t size, in= t flags) { void __iomem *mem_base =3D NULL; mem_base =3D ioremap(bus_addr, size); dev->dma_mem->virt_base =3D mem_base; } int dma_alloc_from_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle, void **r= et) { *ret =3D mem->virt_base + (pageno << PAGE_SHIFT); } and drivers expect the returned value from dma_alloc_coherent() to be dereferenceable. Note that this also fails sparse checking. It will fail on any architecture where the return value from ioremap() is not a virtual address. So, not only does this fail the kernel's own rules, but as we already k= now, it fails the architecture's restrictions with multiple mappings of memo= ry when used with SDRAM, and it also maps main memory as a device. I wond= er how many more things this broken API needs to do wrong before it's curr= ent implementation is consigned to the circular filing cabinet. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@arm.linux.org.uk (Russell King - ARM Linux) Date: Mon, 13 Dec 2010 16:29:47 +0000 Subject: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera In-Reply-To: References: <201012051929.07220.jkrzyszt@tis.icnet.pl> <201012101159.21845.jkrzyszt@tis.icnet.pl> <201012101203.09441.jkrzyszt@tis.icnet.pl> <20101210170356.GA28472@n2100.arm.linux.org.uk> Message-ID: <20101213162947.GA11730@n2100.arm.linux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote: > On 10 December 2010 17:03, Russell King - ARM Linux > wrote: > > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote: > >> ?void __init omap1_camera_init(void *info) > >> ?{ > >> ? ? ? struct platform_device *dev = &omap1_camera_device; > >> + ? ? dma_addr_t paddr = omap1_camera_phys_mempool_base; > >> + ? ? dma_addr_t size = omap1_camera_phys_mempool_size; > >> ? ? ? int ret; > >> > >> ? ? ? dev->dev.platform_data = info; > >> > >> + ? ? if (paddr) { > >> + ? ? ? ? ? ? if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size, > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE)) > > > > Although this works, you're ending up with SDRAM being mapped via > > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being > > mapped as if it were a device. > > BTW, does the generic dma_declare_coherent_memory() does the correct > thing in using ioremap()? I argue it doesn't, as I said above. It means we map SDRAM as device memory, and as I understand the way interconnects work, it's entirely possible that this may not result in the SDRAM being accessible. > Maybe some other function that takes a > pgprot_t would be better (ioremap_page_range) and could pass something > like pgprot_noncached (though ideally a pgprot_dmacoherent). Or just > an architecturally-defined function. This API was designed in the ARMv5 days when things were a lot simpler. What we now have is rather messy though, and really needs sorting out before we get too many users of it. (Arguably it should never have been accepted in its current state.) We have a rule that the returned value of ioremap() is not to be used as a pointer which can be dereferenced by anything except driver code. Yet this is exactly what dma_declare_coherent_memory() does: struct dma_coherent_mem { void *virt_base; ... }; 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; mem_base = ioremap(bus_addr, size); dev->dma_mem->virt_base = mem_base; } int dma_alloc_from_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle, void **ret) { *ret = mem->virt_base + (pageno << PAGE_SHIFT); } and drivers expect the returned value from dma_alloc_coherent() to be dereferenceable. Note that this also fails sparse checking. It will fail on any architecture where the return value from ioremap() is not a virtual address. So, not only does this fail the kernel's own rules, but as we already know, it fails the architecture's restrictions with multiple mappings of memory when used with SDRAM, and it also maps main memory as a device. I wonder how many more things this broken API needs to do wrong before it's current implementation is consigned to the circular filing cabinet. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from caramon.arm.linux.org.uk ([78.32.30.218]:48824 "EHLO caramon.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758017Ab0LMQaw (ORCPT ); Mon, 13 Dec 2010 11:30:52 -0500 Date: Mon, 13 Dec 2010 16:29:47 +0000 From: Russell King - ARM Linux To: Catalin Marinas Cc: Janusz Krzysztofik , Tony Lindgren , "linux-omap@vger.kernel.org" , Guennadi Liakhovetski , linux-arm-kernel@lists.infradead.org, Linux Media Mailing List Subject: Re: [RESEND] [PATCH 1/2] OMAP1: allow reserving memory for camera Message-ID: <20101213162947.GA11730@n2100.arm.linux.org.uk> References: <201012051929.07220.jkrzyszt@tis.icnet.pl> <201012101159.21845.jkrzyszt@tis.icnet.pl> <201012101203.09441.jkrzyszt@tis.icnet.pl> <20101210170356.GA28472@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: List-ID: Sender: Mauro Carvalho Chehab On Mon, Dec 13, 2010 at 03:52:20PM +0000, Catalin Marinas wrote: > On 10 December 2010 17:03, Russell King - ARM Linux > wrote: > > On Fri, Dec 10, 2010 at 12:03:07PM +0100, Janusz Krzysztofik wrote: > >>  void __init omap1_camera_init(void *info) > >>  { > >>       struct platform_device *dev = &omap1_camera_device; > >> +     dma_addr_t paddr = omap1_camera_phys_mempool_base; > >> +     dma_addr_t size = omap1_camera_phys_mempool_size; > >>       int ret; > >> > >>       dev->dev.platform_data = info; > >> > >> +     if (paddr) { > >> +             if (dma_declare_coherent_memory(&dev->dev, paddr, paddr, size, > >> +                             DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE)) > > > > Although this works, you're ending up with SDRAM being mapped via > > ioremap, which uses MT_DEVICE - so what is SDRAM ends up being > > mapped as if it were a device. > > BTW, does the generic dma_declare_coherent_memory() does the correct > thing in using ioremap()? I argue it doesn't, as I said above. It means we map SDRAM as device memory, and as I understand the way interconnects work, it's entirely possible that this may not result in the SDRAM being accessible. > Maybe some other function that takes a > pgprot_t would be better (ioremap_page_range) and could pass something > like pgprot_noncached (though ideally a pgprot_dmacoherent). Or just > an architecturally-defined function. This API was designed in the ARMv5 days when things were a lot simpler. What we now have is rather messy though, and really needs sorting out before we get too many users of it. (Arguably it should never have been accepted in its current state.) We have a rule that the returned value of ioremap() is not to be used as a pointer which can be dereferenced by anything except driver code. Yet this is exactly what dma_declare_coherent_memory() does: struct dma_coherent_mem { void *virt_base; ... }; 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; mem_base = ioremap(bus_addr, size); dev->dma_mem->virt_base = mem_base; } int dma_alloc_from_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle, void **ret) { *ret = mem->virt_base + (pageno << PAGE_SHIFT); } and drivers expect the returned value from dma_alloc_coherent() to be dereferenceable. Note that this also fails sparse checking. It will fail on any architecture where the return value from ioremap() is not a virtual address. So, not only does this fail the kernel's own rules, but as we already know, it fails the architecture's restrictions with multiple mappings of memory when used with SDRAM, and it also maps main memory as a device. I wonder how many more things this broken API needs to do wrong before it's current implementation is consigned to the circular filing cabinet.