RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper

2012-03-30 Thread Marek Szyprowski
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

2012-03-30 Thread Subash Patel

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

2012-03-29 Thread Hiroshi Doyu
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

2012-03-29 Thread Marek Szyprowski
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

2012-03-29 Thread Krishna Reddy
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

2012-03-22 Thread Subash Patel

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

2012-03-20 Thread Subash Patel
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

2012-03-20 Thread KyongHo Cho
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

2012-03-07 Thread Marek Szyprowski
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

2012-03-07 Thread Marek Szyprowski
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

2012-03-06 Thread Krishna Reddy
  +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

2012-03-06 Thread Russell King - ARM Linux
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

2012-03-06 Thread Krishna Reddy
 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

2012-03-06 Thread Hiroshi Doyu
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

2012-03-06 Thread Hiroshi Doyu
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

2012-03-06 Thread Krishna Reddy
   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

2012-03-06 Thread Hiroshi Doyu
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

2012-03-05 Thread Marek Szyprowski
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

2012-03-02 Thread KyongHo Cho
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

2012-03-02 Thread Marek Szyprowski
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