RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Friday, March 30, 2012 4:24 AM Krishna Reddy wrote: Hi, I have found a bug in arm_iommu_map_sg(). +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, +enum dma_data_direction dir, struct dma_attrs *attrs) { + struct scatterlist *s = sg, *dma = sg, *start = sg; + int i, count = 0; + unsigned int offset = s-offset; + unsigned int size = s-offset + s-length; + unsigned int max = dma_get_max_seg_size(dev); + + for (i = 1; i nents; i++) { + s-dma_address = ARM_DMA_ERROR; + s-dma_length = 0; + + s = sg_next(s); With above code, the last sg element's dma_length is not getting set to zero. This causing additional incorrect unmapping during arm_iommu_unmap_sg call and leading to random crashes. The order of above three lines should be as follows. s = sg_next(s); s-dma_address = ARM_DMA_ERROR; s-dma_length = 0; You are right, the order of those lines must be reversed. In all my test codes the scatter list was initially cleared, so I missed this typical off-by-one error. Thanks for spotting it! Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hi KyongHo, On 03/22/2012 07:29 PM, Subash Patel wrote: Hi KyongHo, On 03/21/2012 05:26 AM, KyongHo Cho wrote: On Tue, Mar 20, 2012 at 10:50 PM, Subash Patelsubas...@gmail.com wrote: Sorry for digging this very late. But as part of integrating dma_map v7 sysmmu v12 on 3.3-rc5, I am facing below issue: a) By un-selecting IOMMU in menu config, I am able to allocate memory in vb2-dma-contig b) When I enable SYSMMU support for the IP's, I am receiving below fault: Unhandled fault: external abort on non-linefetch (0x818) at 0xb6f55000 I think this has something to do with the access to the SYSMMU registers for writing the page table. Has anyone of you faced this issue while testing these(dma_map+iommu) patches on kernel mentioned above? This must be something related to recent changes, as I didn't have issues with these patches on 3.2 kernel. 0xb6f55000 is not an address of SYSMMU register if your kernel starts at 0xc000. Can you tell me any detailed information or situation? I hate to say this, but I am not able to catch the fault location even with JTAG. Once the fault comes, the debugger looses all control over. I think now possible method is reproduction at your end :) Thanks to you, Issue is now figured out. This was due to generic Power Domain code added recently. SYSMMU registers were not getting enabled due to this. I reverted the PD changes from the machine to architecture specific, and voila, SYSMMU is back into action using the dma-mapping-v7. I will have to see how the same would behave when the complete PD changes comes to mainline from maintainers for-next in future. Regards, KyongHo. Regards, Subash Regards, Subash -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hi Marek, On Wed, 29 Feb 2012 16:04:22 +0100 Marek Szyprowski m.szyprow...@samsung.com wrote: This patch add a complete implementation of DMA-mapping API for devices that have IOMMU support. All DMA-mapping calls are supported. This patch contains some of the code kindly provided by Krishna Reddy vdu...@nvidia.com and Andrzej Pietrasiewicz andrze...@samsung.com Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- arch/arm/Kconfig |8 + arch/arm/include/asm/device.h|3 + arch/arm/include/asm/dma-iommu.h | 34 ++ arch/arm/mm/dma-mapping.c| 726 +- arch/arm/mm/vmregion.h |2 +- 5 files changed, 758 insertions(+), 15 deletions(-) create mode 100644 arch/arm/include/asm/dma-iommu.h snip +/* + * Map a part of the scatter-gather list into contiguous io address space + */ +static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, + size_t size, dma_addr_t *handle, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev-archdata.mapping; + dma_addr_t iova, iova_base; + int ret = 0; + unsigned int count; + struct scatterlist *s; + + size = PAGE_ALIGN(size); + *handle = ARM_DMA_ERROR; + + iova_base = iova = __alloc_iova(mapping, size); + if (iova == ARM_DMA_ERROR) + return -ENOMEM; + + for (count = 0, s = sg; count (size PAGE_SHIFT); s = sg_next(s)) + { + phys_addr_t phys = page_to_phys(sg_page(s)); + unsigned int len = PAGE_ALIGN(s-offset + s-length); + + if (!arch_is_coherent()) + __dma_page_cpu_to_dev(sg_page(s), s-offset, s-length, dir); + + ret = iommu_map(mapping-domain, iova, phys, len, 0); + if (ret 0) + goto fail; + count += len PAGE_SHIFT; + iova += len; + } + *handle = iova_base; + + return 0; +fail: + iommu_unmap(mapping-domain, iova_base, count * PAGE_SIZE); + __free_iova(mapping, iova_base, size); + return ret; +} Do we need to set dma_address as below? From e8bcc3cdac5375b5d7f5ac5b3f716a11c1008f38 Mon Sep 17 00:00:00 2001 From: Hiroshi DOYU hd...@nvidia.com Date: Thu, 29 Mar 2012 09:59:04 +0300 Subject: [PATCH 1/1] ARM: dma-mapping: Fix dma_address in sglist s-dma_address wasn't set at mapping. Signed-off-by: Hiroshi DOYU hd...@nvidia.com --- arch/arm/mm/dma-mapping.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 3347c37..11a9d65 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -,6 +,8 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, ret = iommu_map(mapping-domain, iova, phys, len, 0); if (ret 0) goto fail; + s-dma_address = iova; + count += len PAGE_SHIFT; iova += len; } -- 1.7.5.4 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Thursday, March 29, 2012 9:19 AM Hiroshi Doyu wrote: On Wed, 29 Feb 2012 16:04:22 +0100 Marek Szyprowski m.szyprow...@samsung.com wrote: This patch add a complete implementation of DMA-mapping API for devices that have IOMMU support. All DMA-mapping calls are supported. This patch contains some of the code kindly provided by Krishna Reddy vdu...@nvidia.com and Andrzej Pietrasiewicz andrze...@samsung.com Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- arch/arm/Kconfig |8 + arch/arm/include/asm/device.h|3 + arch/arm/include/asm/dma-iommu.h | 34 ++ arch/arm/mm/dma-mapping.c| 726 +- arch/arm/mm/vmregion.h |2 +- 5 files changed, 758 insertions(+), 15 deletions(-) create mode 100644 arch/arm/include/asm/dma-iommu.h snip +/* + * Map a part of the scatter-gather list into contiguous io address space + */ +static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, + size_t size, dma_addr_t *handle, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev-archdata.mapping; + dma_addr_t iova, iova_base; + int ret = 0; + unsigned int count; + struct scatterlist *s; + + size = PAGE_ALIGN(size); + *handle = ARM_DMA_ERROR; + + iova_base = iova = __alloc_iova(mapping, size); + if (iova == ARM_DMA_ERROR) + return -ENOMEM; + + for (count = 0, s = sg; count (size PAGE_SHIFT); s = sg_next(s)) + { + phys_addr_t phys = page_to_phys(sg_page(s)); + unsigned int len = PAGE_ALIGN(s-offset + s-length); + + if (!arch_is_coherent()) + __dma_page_cpu_to_dev(sg_page(s), s-offset, s-length, dir); + + ret = iommu_map(mapping-domain, iova, phys, len, 0); + if (ret 0) + goto fail; + count += len PAGE_SHIFT; + iova += len; + } + *handle = iova_base; + + return 0; +fail: + iommu_unmap(mapping-domain, iova_base, count * PAGE_SIZE); + __free_iova(mapping, iova_base, size); + return ret; +} Do we need to set dma_address as below? Nope, this one is not correct. Please check the arm_iommu_map_sg() function. It calls __map_sg_chunk() and gives it dma-dma_address as one of the arguments, so the dma address is correctly stored in the scatter list. Please note that scatterlist is really non-trivial structure and information about physical memory pages/chunks is disjoint from the information about dma address space chunks, although both are stored on the same list. In arm_iommu_map_sg() 's' pointer is used for physical memory chunks and 'dma' pointer is used for dma address space chunks. The number of dma address space chunks (returned by arm_iommu_map_sq) can be lower than the number of physical memory chunks (given as nents argument). In case of IOMMU you usually construct the scatter list in such a way, that you get only one dma address chunk (so in fact you get a buffer mapped contiguously in virtual io address space). From e8bcc3cdac5375b5d7f5ac5b3f716a11c1008f38 Mon Sep 17 00:00:00 2001 From: Hiroshi DOYU hd...@nvidia.com Date: Thu, 29 Mar 2012 09:59:04 +0300 Subject: [PATCH 1/1] ARM: dma-mapping: Fix dma_address in sglist s-dma_address wasn't set at mapping. Signed-off-by: Hiroshi DOYU hd...@nvidia.com --- arch/arm/mm/dma-mapping.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 3347c37..11a9d65 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -,6 +,8 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, ret = iommu_map(mapping-domain, iova, phys, len, 0); if (ret 0) goto fail; + s-dma_address = iova; + count += len PAGE_SHIFT; iova += len; } The above patch is not needed at all. Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hi, I have found a bug in arm_iommu_map_sg(). +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir, struct dma_attrs *attrs) { + struct scatterlist *s = sg, *dma = sg, *start = sg; + int i, count = 0; + unsigned int offset = s-offset; + unsigned int size = s-offset + s-length; + unsigned int max = dma_get_max_seg_size(dev); + + for (i = 1; i nents; i++) { + s-dma_address = ARM_DMA_ERROR; + s-dma_length = 0; + + s = sg_next(s); With above code, the last sg element's dma_length is not getting set to zero. This causing additional incorrect unmapping during arm_iommu_unmap_sg call and leading to random crashes. The order of above three lines should be as follows. s = sg_next(s); s-dma_address = ARM_DMA_ERROR; s-dma_length = 0; -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hi KyongHo, On 03/21/2012 05:26 AM, KyongHo Cho wrote: On Tue, Mar 20, 2012 at 10:50 PM, Subash Patelsubas...@gmail.com wrote: Sorry for digging this very late. But as part of integrating dma_map v7 sysmmu v12 on 3.3-rc5, I am facing below issue: a) By un-selecting IOMMU in menu config, I am able to allocate memory in vb2-dma-contig b) When I enable SYSMMU support for the IP's, I am receiving below fault: Unhandled fault: external abort on non-linefetch (0x818) at 0xb6f55000 I think this has something to do with the access to the SYSMMU registers for writing the page table. Has anyone of you faced this issue while testing these(dma_map+iommu) patches on kernel mentioned above? This must be something related to recent changes, as I didn't have issues with these patches on 3.2 kernel. 0xb6f55000 is not an address of SYSMMU register if your kernel starts at 0xc000. Can you tell me any detailed information or situation? I hate to say this, but I am not able to catch the fault location even with JTAG. Once the fault comes, the debugger looses all control over. I think now possible method is reproduction at your end :) Regards, KyongHo. Regards, Subash -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Sorry for digging this very late. But as part of integrating dma_map v7 sysmmu v12 on 3.3-rc5, I am facing below issue: a) By un-selecting IOMMU in menu config, I am able to allocate memory in vb2-dma-contig b) When I enable SYSMMU support for the IP's, I am receiving below fault: Unhandled fault: external abort on non-linefetch (0x818) at 0xb6f55000 I think this has something to do with the access to the SYSMMU registers for writing the page table. Has anyone of you faced this issue while testing these(dma_map+iommu) patches on kernel mentioned above? This must be something related to recent changes, as I didn't have issues with these patches on 3.2 kernel. Regards, Subash On 03/02/2012 01:35 PM, KyongHo Cho wrote: On Thu, Mar 1, 2012 at 12:04 AM, Marek Szyprowski m.szyprow...@samsung.com wrote: +/** + * arm_iommu_map_sg - map a set of SG buffers for streaming mode DMA + * @dev: valid struct device pointer + * @sg: list of buffers + * @nents: number of buffers to map + * @dir: DMA transfer direction + * + * Map a set of buffers described by scatterlist in streaming mode for DMA. + * The scatter gather list elements are merged together (if possible) and + * tagged with the appropriate dma address and length. They are obtained via + * sg_dma_{address,length}. + */ +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, +enum dma_data_direction dir, struct dma_attrs *attrs) +{ + struct scatterlist *s = sg, *dma = sg, *start = sg; + int i, count = 0; + unsigned int offset = s-offset; + unsigned int size = s-offset + s-length; + unsigned int max = dma_get_max_seg_size(dev); + + for (i = 1; i nents; i++) { + s-dma_address = ARM_DMA_ERROR; + s-dma_length = 0; + + s = sg_next(s); + + if (s-offset || (size ~PAGE_MASK) || size + s-length max) { + if (__map_sg_chunk(dev, start, size,dma-dma_address, + dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + size = offset = s-offset; + start = s; + dma = sg_next(dma); + count += 1; + } + size += s-length; + } + if (__map_sg_chunk(dev, start, size,dma-dma_address, dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + return count+1; + +bad_mapping: + for_each_sg(sg, s, count, i) + __iommu_remove_mapping(dev, sg_dma_address(s), sg_dma_len(s)); + return 0; +} + This looks that the given sg list specifies the list of physical memory chunks and the list of IO virtual memory chunks at the same time after calling arm_dma_map_sg(). It can happen that dma_address and dma_length of a sg entry does not correspond to physical memory information of the sg entry. I think it is beneficial for handling IO virtual memory. However, I worry about any other problems caused by a single sg entry contains information from 2 different context. Regards, Cho KyongHo. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
On Tue, Mar 20, 2012 at 10:50 PM, Subash Patel subas...@gmail.com wrote: Sorry for digging this very late. But as part of integrating dma_map v7 sysmmu v12 on 3.3-rc5, I am facing below issue: a) By un-selecting IOMMU in menu config, I am able to allocate memory in vb2-dma-contig b) When I enable SYSMMU support for the IP's, I am receiving below fault: Unhandled fault: external abort on non-linefetch (0x818) at 0xb6f55000 I think this has something to do with the access to the SYSMMU registers for writing the page table. Has anyone of you faced this issue while testing these(dma_map+iommu) patches on kernel mentioned above? This must be something related to recent changes, as I didn't have issues with these patches on 3.2 kernel. 0xb6f55000 is not an address of SYSMMU register if your kernel starts at 0xc000. Can you tell me any detailed information or situation? Regards, KyongHo. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Wednesday, March 07, 2012 12:22 AM Russell King - ARM Linux wrote: On Wed, Feb 29, 2012 at 04:04:22PM +0100, Marek Szyprowski wrote: +static int arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + struct dma_attrs *attrs) +{ + struct arm_vmregion *c; + + vma-vm_page_prot = __get_dma_pgprot(attrs, vma-vm_page_prot); + c = arm_vmregion_find(consistent_head, (unsigned long)cpu_addr); What protects this against other insertions/removals from the list? arm_vmregion_* functions have their own spinlock. (snipped) + if (c) { + struct page **pages = c-priv; + __dma_free_remap(cpu_addr, size); + __iommu_remove_mapping(dev, handle, size); + __iommu_free_buffer(dev, pages, size); + } +} + +/* + * Map a part of the scatter-gather list into contiguous io address space + */ +static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, + size_t size, dma_addr_t *handle, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev-archdata.mapping; + dma_addr_t iova, iova_base; + int ret = 0; + unsigned int count; + struct scatterlist *s; + + size = PAGE_ALIGN(size); + *handle = ARM_DMA_ERROR; + + iova_base = iova = __alloc_iova(mapping, size); + if (iova == ARM_DMA_ERROR) + return -ENOMEM; + + for (count = 0, s = sg; count (size PAGE_SHIFT); s = sg_next(s)) + { + phys_addr_t phys = page_to_phys(sg_page(s)); + unsigned int len = PAGE_ALIGN(s-offset + s-length); + + if (!arch_is_coherent()) + __dma_page_cpu_to_dev(sg_page(s), s-offset, s-length, dir); + + ret = iommu_map(mapping-domain, iova, phys, len, 0); Dealing with phys addresses on one part and pages + offset + length in a different part doesn't look like a good idea. Why can't there be some consistency? Well, I have no idea how to be more consistent here. scatter-lists operates on pages + offsets + length parameters. iommu api operates on the whole pages, but they are referred with physical address. Right now I cannot change any of it, at least not it the near future. Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Wednesday, March 07, 2012 8:16 AM Hiroshi Doyu wrote: From: Hiroshi DOYU hd...@nvidia.com Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Wed, 07 Mar 2012 08:37:06 +0200 (EET) Message-ID: 20120307.083706.2087121294965856946.hd...@nvidia.com From: Hiroshi DOYU hd...@nvidia.com Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Wed, 07 Mar 2012 08:09:52 +0200 (EET) Message-ID: 20120307.080952.2152478004740487196.hd...@nvidia.com From: Krishna Reddy vdu...@nvidia.com Subject: RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Tue, 6 Mar 2012 23:48:42 +0100 Message-ID: 401e54ce964cd94bae1eb4a729c7087e3797011...@hqmail04.nvidia.com +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); The count calculation doesn't seem correct. order is log2 number and size PAGE_SHIFT is number of pages. If size is passed as 64*4096(256KB) and order is 6(allocation granularity is 2^6 pages=256KB), just 1 bit is enough to manage allocations. So it should be 4 bytes or one long. Good catch! But the calculation gives count = 64 - 6 = 58 and Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. order isn't the order of size passed, which is minimal *page* allocation order which client decides whatever, just in case. It should be as follows. unsigned int count = 1 get_order(size) - order; To be precise, as below? unsigned int count = 1 (get_order(size) - order); This could be: From fd40740ef4bc4a3924fe1188ea6dd785be0fe859 Mon Sep 17 00:00:00 2001 From: Hiroshi DOYU hd...@nvidia.com Date: Wed, 7 Mar 2012 08:14:38 +0200 Subject: [PATCH 1/1] dma-mapping: Fix count calculation of iova space Fix count calculation of iova space. Pointed by Krishna Reddy vdu...@nvidia.com Signed-off-by: Hiroshi DOYU hd...@nvidia.com --- arch/arm/mm/dma-mapping.c | 11 +-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 6c2f104..56f0af5 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1483,11 +1483,18 @@ struct dma_iommu_mapping * arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, int order) { - unsigned int count = (size PAGE_SHIFT) - order; - unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); + unsigned int n, count; + unsigned int bitmap_size; struct dma_iommu_mapping *mapping; int err = -ENOMEM; + n = get_order(size); + if (n order) + return ERR_PTR(-EINVAL); + + count = 1 (n - order); + bitmap_size = BITS_TO_LONGS(count) * sizeof(long); + mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL); if (!mapping) goto err; Thanks again for finding another bug. I thought that I've checked that code more than twice, but it looks that I've missed something again. IMHO the size of virtual memory area doesn't need to be aligned to the power of two, so I will simplify it to the following code: unsigned int count = size (PAGE_SHIFT + order); unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); if (!count) return ERR_PTR(-EINVAL); ... Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
+struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); The count calculation doesn't seem correct. order is log2 number and size PAGE_SHIFT is number of pages. If size is passed as 64*4096(256KB) and order is 6(allocation granularity is 2^6 pages=256KB), just 1 bit is enough to manage allocations. So it should be 4 bytes or one long. But the calculation gives count = 64 - 6 = 58 and Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. It should be as follows. unsigned int count = 1 get_order(size) - order; unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long) * BITS_PER_BYTE; -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
On Wed, Feb 29, 2012 at 04:04:22PM +0100, Marek Szyprowski wrote: +static int arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + struct dma_attrs *attrs) +{ + struct arm_vmregion *c; + + vma-vm_page_prot = __get_dma_pgprot(attrs, vma-vm_page_prot); + c = arm_vmregion_find(consistent_head, (unsigned long)cpu_addr); What protects this against other insertions/removals from the list? + + if (c) { + struct page **pages = c-priv; + + unsigned long uaddr = vma-vm_start; + unsigned long usize = vma-vm_end - vma-vm_start; + int i = 0; + + do { + int ret; + + ret = vm_insert_page(vma, uaddr, pages[i++]); + if (ret) { + pr_err(Remapping memory, error: %d\n, ret); + return ret; + } + + uaddr += PAGE_SIZE; + usize -= PAGE_SIZE; + } while (usize 0); + } + return 0; +} + +/* + * free a page as defined by the above mapping. + * Must not be called with IRQs disabled. + */ +void arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr, + dma_addr_t handle, struct dma_attrs *attrs) +{ + struct arm_vmregion *c; + size = PAGE_ALIGN(size); + + c = arm_vmregion_find(consistent_head, (unsigned long)cpu_addr); What protects this against other insertions/removals from the list? + if (c) { + struct page **pages = c-priv; + __dma_free_remap(cpu_addr, size); + __iommu_remove_mapping(dev, handle, size); + __iommu_free_buffer(dev, pages, size); + } +} + +/* + * Map a part of the scatter-gather list into contiguous io address space + */ +static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, + size_t size, dma_addr_t *handle, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev-archdata.mapping; + dma_addr_t iova, iova_base; + int ret = 0; + unsigned int count; + struct scatterlist *s; + + size = PAGE_ALIGN(size); + *handle = ARM_DMA_ERROR; + + iova_base = iova = __alloc_iova(mapping, size); + if (iova == ARM_DMA_ERROR) + return -ENOMEM; + + for (count = 0, s = sg; count (size PAGE_SHIFT); s = sg_next(s)) + { + phys_addr_t phys = page_to_phys(sg_page(s)); + unsigned int len = PAGE_ALIGN(s-offset + s-length); + + if (!arch_is_coherent()) + __dma_page_cpu_to_dev(sg_page(s), s-offset, s-length, dir); + + ret = iommu_map(mapping-domain, iova, phys, len, 0); Dealing with phys addresses on one part and pages + offset + length in a different part doesn't look like a good idea. Why can't there be some consistency? -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
On Wed, Feb 29, 2012 at 04:04:22PM +0100, Marek Szyprowski wrote: +static int arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma, + void *cpu_addr, dma_addr_t dma_addr, size_t size, + struct dma_attrs *attrs) +{ + struct arm_vmregion *c; + + vma-vm_page_prot = __get_dma_pgprot(attrs, vma- vm_page_prot); + c = arm_vmregion_find(consistent_head, (unsigned long)cpu_addr); What protects this against other insertions/removals from the list? arm_vmregion_find uses a spin_lock internally before accessing consistent_head. So, it is protected. -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
From: Krishna Reddy vdu...@nvidia.com Subject: RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Tue, 6 Mar 2012 23:48:42 +0100 Message-ID: 401e54ce964cd94bae1eb4a729c7087e3797011...@hqmail04.nvidia.com +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); The count calculation doesn't seem correct. order is log2 number and size PAGE_SHIFT is number of pages. If size is passed as 64*4096(256KB) and order is 6(allocation granularity is 2^6 pages=256KB), just 1 bit is enough to manage allocations. So it should be 4 bytes or one long. Good catch! But the calculation gives count = 64 - 6 = 58 and Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. order isn't the order of size passed, which is minimal *page* allocation order which client decides whatever, just in case. It should be as follows. unsigned int count = 1 get_order(size) - order; unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long) * BITS_PER_BYTE; -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
From: Hiroshi DOYU hd...@nvidia.com Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Wed, 07 Mar 2012 08:09:52 +0200 (EET) Message-ID: 20120307.080952.2152478004740487196.hd...@nvidia.com From: Krishna Reddy vdu...@nvidia.com Subject: RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Tue, 6 Mar 2012 23:48:42 +0100 Message-ID: 401e54ce964cd94bae1eb4a729c7087e3797011...@hqmail04.nvidia.com +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); The count calculation doesn't seem correct. order is log2 number and size PAGE_SHIFT is number of pages. If size is passed as 64*4096(256KB) and order is 6(allocation granularity is 2^6 pages=256KB), just 1 bit is enough to manage allocations. So it should be 4 bytes or one long. Good catch! But the calculation gives count = 64 - 6 = 58 and Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. order isn't the order of size passed, which is minimal *page* allocation order which client decides whatever, just in case. It should be as follows. unsigned int count = 1 get_order(size) - order; To be precise, as below? unsigned int count = 1 (get_order(size) - order); unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long) * BITS_PER_BYTE; -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
It should be as follows. unsigned int count = 1 get_order(size) - order; To be precise, as below? unsigned int count = 1 (get_order(size) - order); Minus has more precedence than left shift. 1 get_order(size) - order; is equivalent to 1 (get_order(size) - order); -KR --nvpublic -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
From: Hiroshi DOYU hd...@nvidia.com Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Wed, 07 Mar 2012 08:37:06 +0200 (EET) Message-ID: 20120307.083706.2087121294965856946.hd...@nvidia.com From: Hiroshi DOYU hd...@nvidia.com Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Wed, 07 Mar 2012 08:09:52 +0200 (EET) Message-ID: 20120307.080952.2152478004740487196.hd...@nvidia.com From: Krishna Reddy vdu...@nvidia.com Subject: RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper Date: Tue, 6 Mar 2012 23:48:42 +0100 Message-ID: 401e54ce964cd94bae1eb4a729c7087e3797011...@hqmail04.nvidia.com +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); The count calculation doesn't seem correct. order is log2 number and size PAGE_SHIFT is number of pages. If size is passed as 64*4096(256KB) and order is 6(allocation granularity is 2^6 pages=256KB), just 1 bit is enough to manage allocations. So it should be 4 bytes or one long. Good catch! But the calculation gives count = 64 - 6 = 58 and Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. order isn't the order of size passed, which is minimal *page* allocation order which client decides whatever, just in case. It should be as follows. unsigned int count = 1 get_order(size) - order; To be precise, as below? unsigned int count = 1 (get_order(size) - order); This could be: From fd40740ef4bc4a3924fe1188ea6dd785be0fe859 Mon Sep 17 00:00:00 2001 From: Hiroshi DOYU hd...@nvidia.com Date: Wed, 7 Mar 2012 08:14:38 +0200 Subject: [PATCH 1/1] dma-mapping: Fix count calculation of iova space Fix count calculation of iova space. Pointed by Krishna Reddy vdu...@nvidia.com Signed-off-by: Hiroshi DOYU hd...@nvidia.com --- arch/arm/mm/dma-mapping.c | 11 +-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 6c2f104..56f0af5 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1483,11 +1483,18 @@ struct dma_iommu_mapping * arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, int order) { - unsigned int count = (size PAGE_SHIFT) - order; - unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); + unsigned int n, count; + unsigned int bitmap_size; struct dma_iommu_mapping *mapping; int err = -ENOMEM; + n = get_order(size); + if (n order) + return ERR_PTR(-EINVAL); + + count = 1 (n - order); + bitmap_size = BITS_TO_LONGS(count) * sizeof(long); + mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL); if (!mapping) goto err; -- 1.7.5.4 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Monday, March 05, 2012 12:47 PM Hiroshi Doyu wrote: On Wed, 29 Feb 2012 16:04:22 +0100 Marek Szyprowski m.szyprow...@samsung.com wrote: This patch add a complete implementation of DMA-mapping API for devices that have IOMMU support. All DMA-mapping calls are supported. This patch contains some of the code kindly provided by Krishna Reddy vdu...@nvidia.com and Andrzej Pietrasiewicz andrze...@samsung.com Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com --- (snipped) +/** + * arm_iommu_create_mapping + * @bus: pointer to the bus holding the client device (for IOMMU calls) + * @base: start address of the valid IO address space + * @size: size of the valid IO address space + * @order: accuracy of the IO addresses allocations + * + * Creates a mapping structure which holds information about used/unused + * IO address ranges, which is required to perform memory allocation and + * mapping with IOMMU aware functions. + * + * The client device need to be attached to the mapping with + * arm_iommu_attach_device function. + */ +struct dma_iommu_mapping * +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, +int order) +{ + unsigned int count = (size PAGE_SHIFT) - order; + unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); + struct dma_iommu_mapping *mapping; + int err = -ENOMEM; + + mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL); + if (!mapping) + goto err; + + mapping-bitmap = kzalloc(bitmap_size, GFP_KERNEL); + if (!mapping-bitmap) + goto err2; + + mapping-base = base; + mapping-bits = bitmap_size; Shouldn't the above be as below? From 093c77ac6f19899679f2f2447a9d2c684eab7b2e Mon Sep 17 00:00:00 2001 From: Hiroshi DOYU hd...@nvidia.com Date: Mon, 5 Mar 2012 13:04:38 +0200 Subject: [PATCH 1/1] dma-mapping: Fix mapping-bits size Amount of bits should be mutiplied by BITS_PER_BITE. Signed-off-by: Hiroshi DOYU hd...@nvidia.com --- arch/arm/mm/dma-mapping.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index e55f425..5ec7747 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1495,7 +1495,7 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, goto err2; mapping-base = base; - mapping-bits = bitmap_size; + mapping-bits = BITS_PER_BYTE * bitmap_size; mapping-order = order; spin_lock_init(mapping-lock); You are right, thanks for spotting this issue! Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
On Thu, Mar 1, 2012 at 12:04 AM, Marek Szyprowski m.szyprow...@samsung.com wrote: +/** + * arm_iommu_map_sg - map a set of SG buffers for streaming mode DMA + * @dev: valid struct device pointer + * @sg: list of buffers + * @nents: number of buffers to map + * @dir: DMA transfer direction + * + * Map a set of buffers described by scatterlist in streaming mode for DMA. + * The scatter gather list elements are merged together (if possible) and + * tagged with the appropriate dma address and length. They are obtained via + * sg_dma_{address,length}. + */ +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir, struct dma_attrs *attrs) +{ + struct scatterlist *s = sg, *dma = sg, *start = sg; + int i, count = 0; + unsigned int offset = s-offset; + unsigned int size = s-offset + s-length; + unsigned int max = dma_get_max_seg_size(dev); + + for (i = 1; i nents; i++) { + s-dma_address = ARM_DMA_ERROR; + s-dma_length = 0; + + s = sg_next(s); + + if (s-offset || (size ~PAGE_MASK) || size + s-length max) { + if (__map_sg_chunk(dev, start, size, dma-dma_address, + dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + size = offset = s-offset; + start = s; + dma = sg_next(dma); + count += 1; + } + size += s-length; + } + if (__map_sg_chunk(dev, start, size, dma-dma_address, dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + return count+1; + +bad_mapping: + for_each_sg(sg, s, count, i) + __iommu_remove_mapping(dev, sg_dma_address(s), sg_dma_len(s)); + return 0; +} + This looks that the given sg list specifies the list of physical memory chunks and the list of IO virtual memory chunks at the same time after calling arm_dma_map_sg(). It can happen that dma_address and dma_length of a sg entry does not correspond to physical memory information of the sg entry. I think it is beneficial for handling IO virtual memory. However, I worry about any other problems caused by a single sg entry contains information from 2 different context. Regards, Cho KyongHo. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper
Hello, On Friday, March 02, 2012 9:06 AM KyongHo Cho wrote: On Thu, Mar 1, 2012 at 12:04 AM, Marek Szyprowski m.szyprow...@samsung.com wrote: +/** + * arm_iommu_map_sg - map a set of SG buffers for streaming mode DMA + * @dev: valid struct device pointer + * @sg: list of buffers + * @nents: number of buffers to map + * @dir: DMA transfer direction + * + * Map a set of buffers described by scatterlist in streaming mode for DMA. + * The scatter gather list elements are merged together (if possible) and + * tagged with the appropriate dma address and length. They are obtained via + * sg_dma_{address,length}. + */ +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir, struct dma_attrs *attrs) +{ + struct scatterlist *s = sg, *dma = sg, *start = sg; + int i, count = 0; + unsigned int offset = s-offset; + unsigned int size = s-offset + s-length; + unsigned int max = dma_get_max_seg_size(dev); + + for (i = 1; i nents; i++) { + s-dma_address = ARM_DMA_ERROR; + s-dma_length = 0; + + s = sg_next(s); + + if (s-offset || (size ~PAGE_MASK) || size + s-length max) { + if (__map_sg_chunk(dev, start, size, dma-dma_address, + dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + size = offset = s-offset; + start = s; + dma = sg_next(dma); + count += 1; + } + size += s-length; + } + if (__map_sg_chunk(dev, start, size, dma-dma_address, dir) 0) + goto bad_mapping; + + dma-dma_address += offset; + dma-dma_length = size - offset; + + return count+1; + +bad_mapping: + for_each_sg(sg, s, count, i) + __iommu_remove_mapping(dev, sg_dma_address(s), sg_dma_len(s)); + return 0; +} + This looks that the given sg list specifies the list of physical memory chunks and the list of IO virtual memory chunks at the same time after calling arm_dma_map_sg(). It can happen that dma_address and dma_length of a sg entry does not correspond to physical memory information of the sg entry. Right, that's how it is designed. If fact sg entries describes 2 independent lists - one for physical memory chunks and one for virtual memory chunks. It might happen that the whole scattered physical memory can be mapped into contiguous virtual memory chunk, what result in only one element describing the io dma addresses. Here is the respective paragraph from Documentation/DMA-API-HOWTO.txt (lines 511-517): 'The implementation is free to merge several consecutive sglist entries into one (e.g. if DMA mapping is done with PAGE_SIZE granularity, any consecutive sglist entries can be merged into one provided the first one ends and the second one starts on a page boundary - in fact this is a huge advantage for cards which either cannot do scatter-gather or have very limited number of scatter-gather entries) and returns the actual number of sg entries it mapped them to. On failure 0 is returned.' I think it is beneficial for handling IO virtual memory. However, I worry about any other problems caused by a single sg entry contains information from 2 different context. What do you mean by the 'context'. DMA mapping assumes that a single call to dma_map_sg maps a single memory buffer. Best regards -- Marek Szyprowski Samsung Poland RD Center -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html