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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
