linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
@ 2011-03-08 12:46 David Cohen
  2011-03-08 12:46 ` [PATCH 1/3] omap: iovmm: disallow mapping NULL address David Cohen
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: David Cohen @ 2011-03-08 12:46 UTC (permalink / raw)
  To: Hiroshi.DOYU
  Cc: linux-omap, fernando.lugo, linux-media, laurent.pinchart,
	sakari.ailus, David Cohen

IOVMM driver checks input 'da == 0' when mapping address to determine whether
user wants fixed 'da' or not. At the same time, it doesn't disallow address
0x0 to be used, what creates an ambiguous situation. This patch set moves
fixed 'da' check to the input flags.
It also fixes da_start value for ISP IOMMU instance.

David Cohen (2):
  omap3: change ISP's IOMMU da_start address
  omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON
    flags

Michael Jones (1):
  omap: iovmm: disallow mapping NULL address

 arch/arm/mach-omap2/omap-iommu.c |    2 +-
 arch/arm/plat-omap/iovmm.c       |   28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 11 deletions(-)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/3] omap: iovmm: disallow mapping NULL address
  2011-03-08 12:46 [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' David Cohen
@ 2011-03-08 12:46 ` David Cohen
  2011-03-08 18:06   ` Guzman Lugo, Fernando
  2011-03-08 12:46 ` [PATCH 2/3] omap3: change ISP's IOMMU da_start address David Cohen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 12:46 UTC (permalink / raw)
  To: Hiroshi.DOYU
  Cc: linux-omap, fernando.lugo, linux-media, laurent.pinchart,
	sakari.ailus, Michael Jones

From: Michael Jones <michael.jones@matrix-vision.de>

commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0, which would then not get unmapped.
Disallow this again.  And spell variable 'alignment' correctly.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..11c9b76 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 					   size_t bytes, u32 flags)
 {
 	struct iovm_struct *new, *tmp;
-	u32 start, prev_end, alignement;
+	u32 start, prev_end, alignment;
 
 	if (!obj || !bytes)
 		return ERR_PTR(-EINVAL);
 
 	start = da;
-	alignement = PAGE_SIZE;
+	alignment = PAGE_SIZE;
 
 	if (flags & IOVMF_DA_ANON) {
-		start = obj->da_start;
+		/* Don't map address 0 */
+		if (obj->da_start)
+			start = obj->da_start;
+		else
+			start = obj->da_start + alignment;
 
 		if (flags & IOVMF_LINEAR)
-			alignement = iopgsz_max(bytes);
-		start = roundup(start, alignement);
+			alignment = iopgsz_max(bytes);
+		start = roundup(start, alignment);
 	} else if (start < obj->da_start || start > obj->da_end ||
 					obj->da_end - start < bytes) {
 		return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 			goto found;
 
 		if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
-			start = roundup(tmp->da_end + 1, alignement);
+			start = roundup(tmp->da_end + 1, alignment);
 
 		prev_end = tmp->da_end;
 	}
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/3] omap3: change ISP's IOMMU da_start address
  2011-03-08 12:46 [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' David Cohen
  2011-03-08 12:46 ` [PATCH 1/3] omap: iovmm: disallow mapping NULL address David Cohen
@ 2011-03-08 12:46 ` David Cohen
  2011-03-08 14:06   ` Sakari Ailus
  2011-03-08 12:46 ` [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags David Cohen
  2011-03-08 12:49 ` [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' Hiroshi DOYU
  3 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 12:46 UTC (permalink / raw)
  To: Hiroshi.DOYU
  Cc: linux-omap, fernando.lugo, linux-media, laurent.pinchart,
	sakari.ailus, David Cohen

ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.

Signed-off-by: David Cohen <dacohen@gmail.com>
---
 arch/arm/mach-omap2/omap-iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
 			.name = "isp",
 			.nr_tlb_entries = 8,
 			.clk_name = "cam_ick",
-			.da_start = 0x0,
+			.da_start = 0x1000,
 			.da_end = 0xFFFFF000,
 		},
 	},
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 12:46 [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' David Cohen
  2011-03-08 12:46 ` [PATCH 1/3] omap: iovmm: disallow mapping NULL address David Cohen
  2011-03-08 12:46 ` [PATCH 2/3] omap3: change ISP's IOMMU da_start address David Cohen
@ 2011-03-08 12:46 ` David Cohen
  2011-03-08 17:59   ` Guzman Lugo, Fernando
  2011-03-08 12:49 ` [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' Hiroshi DOYU
  3 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 12:46 UTC (permalink / raw)
  To: Hiroshi.DOYU
  Cc: linux-omap, fernando.lugo, linux-media, laurent.pinchart,
	sakari.ailus, David Cohen

Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED

It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user. IOVMF_DA_ANON will be automatically set if
IOVMF_DA_FIXED isn't set.

Signed-off-by: David Cohen <dacohen@gmail.com>
---
 arch/arm/plat-omap/iovmm.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 11c9b76..dde9cb0 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_DISCONT;
 	flags |= IOVMF_MMIO;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+	if (~flags & IOVMF_DA_FIXED)
+		flags |= IOVMF_DA_ANON;
 
 	da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
@@ -713,7 +714,8 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_DISCONT;
 	flags |= IOVMF_ALLOC;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+	if (~flags & IOVMF_DA_FIXED)
+		flags |= IOVMF_DA_ANON;
 
 	sgt = sgtable_alloc(bytes, flags, da, 0);
 	if (IS_ERR(sgt)) {
@@ -803,7 +805,8 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_LINEAR;
 	flags |= IOVMF_MMIO;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+	if (~flags & IOVMF_DA_FIXED)
+		flags |= IOVMF_DA_ANON;
 
 	da = __iommu_kmap(obj, da, pa, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
@@ -862,7 +865,8 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_LINEAR;
 	flags |= IOVMF_ALLOC;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
+	if (~flags & IOVMF_DA_FIXED)
+		flags |= IOVMF_DA_ANON;
 
 	da = __iommu_kmap(obj, da, pa, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
  2011-03-08 12:46 [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' David Cohen
                   ` (2 preceding siblings ...)
  2011-03-08 12:46 ` [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags David Cohen
@ 2011-03-08 12:49 ` Hiroshi DOYU
  3 siblings, 0 replies; 18+ messages in thread
From: Hiroshi DOYU @ 2011-03-08 12:49 UTC (permalink / raw)
  To: dacohen, tony
  Cc: linux-omap, fernando.lugo, linux-media, laurent.pinchart,
	sakari.ailus

From: ext David Cohen <dacohen@gmail.com>
Subject: [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'
Date: Tue, 8 Mar 2011 14:46:02 +0200

> IOVMM driver checks input 'da == 0' when mapping address to determine whether
> user wants fixed 'da' or not. At the same time, it doesn't disallow address
> 0x0 to be used, what creates an ambiguous situation. This patch set moves
> fixed 'da' check to the input flags.
> It also fixes da_start value for ISP IOMMU instance.
> 
> David Cohen (2):
>   omap3: change ISP's IOMMU da_start address
>   omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON
>     flags
> 
> Michael Jones (1):
>   omap: iovmm: disallow mapping NULL address
> 
>  arch/arm/mach-omap2/omap-iommu.c |    2 +-
>  arch/arm/plat-omap/iovmm.c       |   28 ++++++++++++++++++----------
>  2 files changed, 19 insertions(+), 11 deletions(-)

Based on the previous discussion, those look ok for me.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] omap3: change ISP's IOMMU da_start address
  2011-03-08 12:46 ` [PATCH 2/3] omap3: change ISP's IOMMU da_start address David Cohen
@ 2011-03-08 14:06   ` Sakari Ailus
  2011-03-08 17:15     ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: Sakari Ailus @ 2011-03-08 14:06 UTC (permalink / raw)
  To: David Cohen
  Cc: Hiroshi.DOYU, linux-omap, fernando.lugo, linux-media,
	laurent.pinchart

David Cohen wrote:
> ISP doesn't consider 0x0 as a valid address, so it should explicitly
> exclude first page from allowed 'da' range.
> 
> Signed-off-by: David Cohen <dacohen@gmail.com>
> ---
>  arch/arm/mach-omap2/omap-iommu.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 3fc5dc7..3bea489 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>  			.name = "isp",
>  			.nr_tlb_entries = 8,
>  			.clk_name = "cam_ick",
> -			.da_start = 0x0,
> +			.da_start = 0x1000,

The NULL address is still valid for the MMU. Can the IOVMF_DA_ANON
mapping be specified by the API to be always non-NULL?

This way it would be possible to combine IOVMF_DA_FIXED and
IOVMF_DA_ANON mappings in the same IOMMU while still being able to rely
that IOVMF_DA_ANON mappings would always be non-NULL.

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/3] omap3: change ISP's IOMMU da_start address
  2011-03-08 14:06   ` Sakari Ailus
@ 2011-03-08 17:15     ` David Cohen
  0 siblings, 0 replies; 18+ messages in thread
From: David Cohen @ 2011-03-08 17:15 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Hiroshi.DOYU, linux-omap, fernando.lugo, linux-media,
	laurent.pinchart

Hi Sakari,

On Tue, Mar 8, 2011 at 4:06 PM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> David Cohen wrote:
>> ISP doesn't consider 0x0 as a valid address, so it should explicitly
>> exclude first page from allowed 'da' range.
>>
>> Signed-off-by: David Cohen <dacohen@gmail.com>
>> ---
>>  arch/arm/mach-omap2/omap-iommu.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
>> index 3fc5dc7..3bea489 100644
>> --- a/arch/arm/mach-omap2/omap-iommu.c
>> +++ b/arch/arm/mach-omap2/omap-iommu.c
>> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>>                       .name = "isp",
>>                       .nr_tlb_entries = 8,
>>                       .clk_name = "cam_ick",
>> -                     .da_start = 0x0,
>> +                     .da_start = 0x1000,
>
> The NULL address is still valid for the MMU. Can the IOVMF_DA_ANON
> mapping be specified by the API to be always non-NULL?

The patch 1/3 does that :)

>
> This way it would be possible to combine IOVMF_DA_FIXED and
> IOVMF_DA_ANON mappings in the same IOMMU while still being able to rely
> that IOVMF_DA_ANON mappings would always be non-NULL.

Indeed the IOMMU driver supports it. The flag is set by iovm area and
each IOMMU instance supports a mix of mappings with fixed da or not.
By changing the OMAP3 "isp" IOMMU pdata's da_start to 0x1000, we're
explicitly saying that iovm areas with fixed 'da' or not will never
map first page to OMAP3 ISP driver, what is the behavior it expects.
Without this patch, thanks to patch 1/3 the IOMMU driver will not map
address 0x0 to OMAP3 ISP anyway. But then ISP will be passing wrong
information that it would be fine to map first page. :)

Kind regards,

David Cohen

>
> Regards,
>
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 12:46 ` [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags David Cohen
@ 2011-03-08 17:59   ` Guzman Lugo, Fernando
  2011-03-08 18:09     ` Hiroshi DOYU
  0 siblings, 1 reply; 18+ messages in thread
From: Guzman Lugo, Fernando @ 2011-03-08 17:59 UTC (permalink / raw)
  To: David Cohen
  Cc: Hiroshi.DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus

On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
> to input 'da' address when mapping memory:
> da == 0: IOVMF_DA_ANON
> da != 0: IOVMF_DA_FIXED
>
> It prevents IOMMU to map first page with fixed 'da'. To avoid such
> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
> come from the user. IOVMF_DA_ANON will be automatically set if
> IOVMF_DA_FIXED isn't set.
>
> Signed-off-by: David Cohen <dacohen@gmail.com>
> ---
>  arch/arm/plat-omap/iovmm.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 11c9b76..dde9cb0 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_DISCONT;
>        flags |= IOVMF_MMIO;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> +       if (~flags & IOVMF_DA_FIXED)
> +               flags |= IOVMF_DA_ANON;

could we use only one? both are mutual exclusive, what happen if flag
is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
IOVMF_DA_ANON.

Regards,
Fernando.

>
>        da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> @@ -713,7 +714,8 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_DISCONT;
>        flags |= IOVMF_ALLOC;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> +       if (~flags & IOVMF_DA_FIXED)
> +               flags |= IOVMF_DA_ANON;
>
>        sgt = sgtable_alloc(bytes, flags, da, 0);
>        if (IS_ERR(sgt)) {
> @@ -803,7 +805,8 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_LINEAR;
>        flags |= IOVMF_MMIO;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> +       if (~flags & IOVMF_DA_FIXED)
> +               flags |= IOVMF_DA_ANON;
>
>        da = __iommu_kmap(obj, da, pa, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> @@ -862,7 +865,8 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_LINEAR;
>        flags |= IOVMF_ALLOC;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
> +       if (~flags & IOVMF_DA_FIXED)
> +               flags |= IOVMF_DA_ANON;
>
>        da = __iommu_kmap(obj, da, pa, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> --
> 1.7.0.4
>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
  2011-03-08 12:46 ` [PATCH 1/3] omap: iovmm: disallow mapping NULL address David Cohen
@ 2011-03-08 18:06   ` Guzman Lugo, Fernando
  2011-03-08 19:06     ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: Guzman Lugo, Fernando @ 2011-03-08 18:06 UTC (permalink / raw)
  To: David Cohen
  Cc: Hiroshi.DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus, Michael Jones

On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
> From: Michael Jones <michael.jones@matrix-vision.de>
>
> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
> the NULL address if da_start==0, which would then not get unmapped.
> Disallow this again.  And spell variable 'alignment' correctly.
>
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..11c9b76 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>                                           size_t bytes, u32 flags)
>  {
>        struct iovm_struct *new, *tmp;
> -       u32 start, prev_end, alignement;
> +       u32 start, prev_end, alignment;
>
>        if (!obj || !bytes)
>                return ERR_PTR(-EINVAL);
>
>        start = da;
> -       alignement = PAGE_SIZE;
> +       alignment = PAGE_SIZE;
>
>        if (flags & IOVMF_DA_ANON) {
> -               start = obj->da_start;
> +               /* Don't map address 0 */
> +               if (obj->da_start)
> +                       start = obj->da_start;
> +               else
> +                       start = obj->da_start + alignment;

else part obj->da_star is always 0, so why not?
start = alignment;

so, why I understand, it now it is possible mapp address 0x0 only if
IOVMF_DA_ANON is not set, right? maybe that would be mention in the
patch.

Regards,
Fernando.

>
>                if (flags & IOVMF_LINEAR)
> -                       alignement = iopgsz_max(bytes);
> -               start = roundup(start, alignement);
> +                       alignment = iopgsz_max(bytes);
> +               start = roundup(start, alignment);
>        } else if (start < obj->da_start || start > obj->da_end ||
>                                        obj->da_end - start < bytes) {
>                return ERR_PTR(-EINVAL);
> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>                        goto found;
>
>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
> -                       start = roundup(tmp->da_end + 1, alignement);
> +                       start = roundup(tmp->da_end + 1, alignment);
>
>                prev_end = tmp->da_end;
>        }
> --
> 1.7.0.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 17:59   ` Guzman Lugo, Fernando
@ 2011-03-08 18:09     ` Hiroshi DOYU
  2011-03-08 18:53       ` Guzman Lugo, Fernando
  0 siblings, 1 reply; 18+ messages in thread
From: Hiroshi DOYU @ 2011-03-08 18:09 UTC (permalink / raw)
  To: fernando.lugo
  Cc: dacohen, linux-omap, linux-media, laurent.pinchart, sakari.ailus

From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
Date: Tue, 8 Mar 2011 11:59:43 -0600

> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
>> to input 'da' address when mapping memory:
>> da == 0: IOVMF_DA_ANON
>> da != 0: IOVMF_DA_FIXED
>>
>> It prevents IOMMU to map first page with fixed 'da'. To avoid such
>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
>> come from the user. IOVMF_DA_ANON will be automatically set if
>> IOVMF_DA_FIXED isn't set.
>>
>> Signed-off-by: David Cohen <dacohen@gmail.com>
>> ---
>>  arch/arm/plat-omap/iovmm.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 11c9b76..dde9cb0 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>>        flags &= IOVMF_HW_MASK;
>>        flags |= IOVMF_DISCONT;
>>        flags |= IOVMF_MMIO;
>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>> +       if (~flags & IOVMF_DA_FIXED)
>> +               flags |= IOVMF_DA_ANON;
> 
> could we use only one? both are mutual exclusive, what happen if flag
> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
> IOVMF_DA_ANON.

Then, what about introducing some MACRO? Better names?

#define set_iovmf_da_anon(flags)
#define set_iovmf_da_fix(flags)
#define set_iovmf_mmio(flags)
......
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 18:09     ` Hiroshi DOYU
@ 2011-03-08 18:53       ` Guzman Lugo, Fernando
  2011-03-08 18:57         ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: Guzman Lugo, Fernando @ 2011-03-08 18:53 UTC (permalink / raw)
  To: Hiroshi DOYU
  Cc: dacohen, linux-omap, linux-media, laurent.pinchart, sakari.ailus

On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
> Date: Tue, 8 Mar 2011 11:59:43 -0600
>
>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
>>> to input 'da' address when mapping memory:
>>> da == 0: IOVMF_DA_ANON
>>> da != 0: IOVMF_DA_FIXED
>>>
>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such
>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
>>> come from the user. IOVMF_DA_ANON will be automatically set if
>>> IOVMF_DA_FIXED isn't set.
>>>
>>> Signed-off-by: David Cohen <dacohen@gmail.com>
>>> ---
>>>  arch/arm/plat-omap/iovmm.c |   12 ++++++++----
>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>> index 11c9b76..dde9cb0 100644
>>> --- a/arch/arm/plat-omap/iovmm.c
>>> +++ b/arch/arm/plat-omap/iovmm.c
>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>>>        flags &= IOVMF_HW_MASK;
>>>        flags |= IOVMF_DISCONT;
>>>        flags |= IOVMF_MMIO;
>>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>>> +       if (~flags & IOVMF_DA_FIXED)
>>> +               flags |= IOVMF_DA_ANON;
>>
>> could we use only one? both are mutual exclusive, what happen if flag
>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
>> IOVMF_DA_ANON.
>
> Then, what about introducing some MACRO? Better names?
>
> #define set_iovmf_da_anon(flags)
> #define set_iovmf_da_fix(flags)
> #define set_iovmf_mmio(flags)

will they be used by the users?

I think people are more used to use

iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON);

than

set_iovmf_da_anon(flags)
set_iovmf_mmio(flags)
iommu_vmap(obj, da, sgt, flags);

I don't have problem with the change, but I think how it is now is ok,
just that we don't we two bits to handle anon/fixed da, it can be
managed it only 1 bit (one flag), or is there a issue?

Regards,
Fernando.
> ......
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 18:53       ` Guzman Lugo, Fernando
@ 2011-03-08 18:57         ` David Cohen
  2011-03-08 19:33           ` Guzman Lugo, Fernando
  0 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 18:57 UTC (permalink / raw)
  To: Guzman Lugo, Fernando, Hiroshi DOYU
  Cc: linux-omap, linux-media, laurent.pinchart, sakari.ailus

Hi Hiroshi, Fernando,

On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
>> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
>> Date: Tue, 8 Mar 2011 11:59:43 -0600
>>
>>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
>>>> to input 'da' address when mapping memory:
>>>> da == 0: IOVMF_DA_ANON
>>>> da != 0: IOVMF_DA_FIXED
>>>>
>>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such
>>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
>>>> come from the user. IOVMF_DA_ANON will be automatically set if
>>>> IOVMF_DA_FIXED isn't set.
>>>>
>>>> Signed-off-by: David Cohen <dacohen@gmail.com>
>>>> ---
>>>>  arch/arm/plat-omap/iovmm.c |   12 ++++++++----
>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>>> index 11c9b76..dde9cb0 100644
>>>> --- a/arch/arm/plat-omap/iovmm.c
>>>> +++ b/arch/arm/plat-omap/iovmm.c
>>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>>>>        flags &= IOVMF_HW_MASK;
>>>>        flags |= IOVMF_DISCONT;
>>>>        flags |= IOVMF_MMIO;
>>>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>>>> +       if (~flags & IOVMF_DA_FIXED)
>>>> +               flags |= IOVMF_DA_ANON;
>>>
>>> could we use only one? both are mutual exclusive, what happen if flag
>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
>>> IOVMF_DA_ANON.
>>
>> Then, what about introducing some MACRO? Better names?
>>
>> #define set_iovmf_da_anon(flags)
>> #define set_iovmf_da_fix(flags)
>> #define set_iovmf_mmio(flags)
>
> will they be used by the users?
>
> I think people are more used to use
>
> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON);

I'd be happier with this approach, instead of the macros. :)
It's intuitive and very common on kernel.

>
> than
>
> set_iovmf_da_anon(flags)
> set_iovmf_mmio(flags)
> iommu_vmap(obj, da, sgt, flags);
>
> I don't have problem with the change, but I think how it is now is ok,
> just that we don't we two bits to handle anon/fixed da, it can be
> managed it only 1 bit (one flag), or is there a issue?

We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only.
I can resend my patch if we agree it's OK.

Regards,

David

>
> Regards,
> Fernando.
>> ......
>>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
  2011-03-08 18:06   ` Guzman Lugo, Fernando
@ 2011-03-08 19:06     ` David Cohen
  2011-03-08 19:53       ` Guzman Lugo, Fernando
  0 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 19:06 UTC (permalink / raw)
  To: Guzman Lugo, Fernando
  Cc: Hiroshi.DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus, Michael Jones

On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>> From: Michael Jones <michael.jones@matrix-vision.de>
>>
>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> the NULL address if da_start==0, which would then not get unmapped.
>> Disallow this again.  And spell variable 'alignment' correctly.
>>
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>> ---
>>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>>  1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 6dc1296..11c9b76 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>                                           size_t bytes, u32 flags)
>>  {
>>        struct iovm_struct *new, *tmp;
>> -       u32 start, prev_end, alignement;
>> +       u32 start, prev_end, alignment;
>>
>>        if (!obj || !bytes)
>>                return ERR_PTR(-EINVAL);
>>
>>        start = da;
>> -       alignement = PAGE_SIZE;
>> +       alignment = PAGE_SIZE;
>>
>>        if (flags & IOVMF_DA_ANON) {
>> -               start = obj->da_start;
>> +               /* Don't map address 0 */
>> +               if (obj->da_start)
>> +                       start = obj->da_start;
>> +               else
>> +                       start = obj->da_start + alignment;
>
> else part obj->da_star is always 0, so why not?
> start = alignment;

The patch is not from me, but I can fix it if Michael agrees.

>
> so, why I understand, it now it is possible mapp address 0x0 only if
> IOVMF_DA_ANON is not set, right? maybe that would be mention in the
> patch.

Indeed address 0x0 was never meant to be mapped. The mentioned commit
on the patch's description did that without noticing. But the new
change is documented in the code by the comment "Don't map address 0"
and it's also mentioned on the patch description. Is it OK for you?

Regards,

David Cohen

>
> Regards,
> Fernando.
>
>>
>>                if (flags & IOVMF_LINEAR)
>> -                       alignement = iopgsz_max(bytes);
>> -               start = roundup(start, alignement);
>> +                       alignment = iopgsz_max(bytes);
>> +               start = roundup(start, alignment);
>>        } else if (start < obj->da_start || start > obj->da_end ||
>>                                        obj->da_end - start < bytes) {
>>                return ERR_PTR(-EINVAL);
>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>                        goto found;
>>
>>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>> -                       start = roundup(tmp->da_end + 1, alignement);
>> +                       start = roundup(tmp->da_end + 1, alignment);
>>
>>                prev_end = tmp->da_end;
>>        }
>> --
>> 1.7.0.4
>>
>>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 18:57         ` David Cohen
@ 2011-03-08 19:33           ` Guzman Lugo, Fernando
  2011-03-08 19:46             ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: Guzman Lugo, Fernando @ 2011-03-08 19:33 UTC (permalink / raw)
  To: David Cohen
  Cc: Hiroshi DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus

On Tue, Mar 8, 2011 at 12:57 PM, David Cohen <dacohen@gmail.com> wrote:
> Hi Hiroshi, Fernando,
>
> On Tue, Mar 8, 2011 at 8:53 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Tue, Mar 8, 2011 at 12:09 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
>>> From: "ext Guzman Lugo, Fernando" <fernando.lugo@ti.com>
>>> Subject: Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
>>> Date: Tue, 8 Mar 2011 11:59:43 -0600
>>>
>>>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>>>>> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
>>>>> to input 'da' address when mapping memory:
>>>>> da == 0: IOVMF_DA_ANON
>>>>> da != 0: IOVMF_DA_FIXED
>>>>>
>>>>> It prevents IOMMU to map first page with fixed 'da'. To avoid such
>>>>> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
>>>>> come from the user. IOVMF_DA_ANON will be automatically set if
>>>>> IOVMF_DA_FIXED isn't set.
>>>>>
>>>>> Signed-off-by: David Cohen <dacohen@gmail.com>
>>>>> ---
>>>>>  arch/arm/plat-omap/iovmm.c |   12 ++++++++----
>>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>>>> index 11c9b76..dde9cb0 100644
>>>>> --- a/arch/arm/plat-omap/iovmm.c
>>>>> +++ b/arch/arm/plat-omap/iovmm.c
>>>>> @@ -654,7 +654,8 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>>>>>        flags &= IOVMF_HW_MASK;
>>>>>        flags |= IOVMF_DISCONT;
>>>>>        flags |= IOVMF_MMIO;
>>>>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>>>>> +       if (~flags & IOVMF_DA_FIXED)
>>>>> +               flags |= IOVMF_DA_ANON;
>>>>
>>>> could we use only one? both are mutual exclusive, what happen if flag
>>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
>>>> IOVMF_DA_ANON.
>>>
>>> Then, what about introducing some MACRO? Better names?
>>>
>>> #define set_iovmf_da_anon(flags)
>>> #define set_iovmf_da_fix(flags)
>>> #define set_iovmf_mmio(flags)
>>
>> will they be used by the users?
>>
>> I think people are more used to use
>>
>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON);
>
> I'd be happier with this approach, instead of the macros. :)
> It's intuitive and very common on kernel.
>
>>
>> than
>>
>> set_iovmf_da_anon(flags)
>> set_iovmf_mmio(flags)
>> iommu_vmap(obj, da, sgt, flags);
>>
>> I don't have problem with the change, but I think how it is now is ok,
>> just that we don't we two bits to handle anon/fixed da, it can be
>> managed it only 1 bit (one flag), or is there a issue?
>
> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only.
> I can resend my patch if we agree it's OK.

sounds perfect to me.

Regards,
Fernando.

>
> Regards,
>
> David
>
>>
>> Regards,
>> Fernando.
>>> ......
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 19:33           ` Guzman Lugo, Fernando
@ 2011-03-08 19:46             ` David Cohen
  2011-03-08 19:51               ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: David Cohen @ 2011-03-08 19:46 UTC (permalink / raw)
  To: Guzman Lugo, Fernando
  Cc: Hiroshi DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus

[snip]

>>>>>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>>>>>> +       if (~flags & IOVMF_DA_FIXED)
>>>>>> +               flags |= IOVMF_DA_ANON;
>>>>>
>>>>> could we use only one? both are mutual exclusive, what happen if flag
>>>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
>>>>> IOVMF_DA_ANON.
>>>>
>>>> Then, what about introducing some MACRO? Better names?
>>>>
>>>> #define set_iovmf_da_anon(flags)
>>>> #define set_iovmf_da_fix(flags)
>>>> #define set_iovmf_mmio(flags)
>>>
>>> will they be used by the users?
>>>
>>> I think people are more used to use
>>>
>>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON);
>>
>> I'd be happier with this approach, instead of the macros. :)
>> It's intuitive and very common on kernel.
>>
>>>
>>> than
>>>
>>> set_iovmf_da_anon(flags)
>>> set_iovmf_mmio(flags)
>>> iommu_vmap(obj, da, sgt, flags);
>>>
>>> I don't have problem with the change, but I think how it is now is ok,
>>> just that we don't we two bits to handle anon/fixed da, it can be
>>> managed it only 1 bit (one flag), or is there a issue?
>>
>> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only.
>> I can resend my patch if we agree it's OK.
>
> sounds perfect to me.

Not sure indeed if this change fits to this same patch. Looks like a
4th patch sounds better.

Br,

David Cohen
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags
  2011-03-08 19:46             ` David Cohen
@ 2011-03-08 19:51               ` David Cohen
  0 siblings, 0 replies; 18+ messages in thread
From: David Cohen @ 2011-03-08 19:51 UTC (permalink / raw)
  To: Guzman Lugo, Fernando
  Cc: Hiroshi DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus

On Tue, Mar 8, 2011 at 9:46 PM, David Cohen <dacohen@gmail.com> wrote:
> [snip]
>
>>>>>>> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>>>>>>> +       if (~flags & IOVMF_DA_FIXED)
>>>>>>> +               flags |= IOVMF_DA_ANON;
>>>>>>
>>>>>> could we use only one? both are mutual exclusive, what happen if flag
>>>>>> is IOVMF_DA_FIXED | IOVMF_DA_ANON? so, I suggest to get rid of
>>>>>> IOVMF_DA_ANON.
>>>>>
>>>>> Then, what about introducing some MACRO? Better names?
>>>>>
>>>>> #define set_iovmf_da_anon(flags)
>>>>> #define set_iovmf_da_fix(flags)
>>>>> #define set_iovmf_mmio(flags)
>>>>
>>>> will they be used by the users?
>>>>
>>>> I think people are more used to use
>>>>
>>>> iommu_vmap(obj, da, sgt, IOVMF_MMIO | IOVMF_DA_ANON);
>>>
>>> I'd be happier with this approach, instead of the macros. :)
>>> It's intuitive and very common on kernel.
>>>
>>>>
>>>> than
>>>>
>>>> set_iovmf_da_anon(flags)
>>>> set_iovmf_mmio(flags)
>>>> iommu_vmap(obj, da, sgt, flags);
>>>>
>>>> I don't have problem with the change, but I think how it is now is ok,
>>>> just that we don't we two bits to handle anon/fixed da, it can be
>>>> managed it only 1 bit (one flag), or is there a issue?
>>>
>>> We can exclude IOVMF_DA_ANON and stick with IOVMF_DA_FIXED only.
>>> I can resend my patch if we agree it's OK.
>>
>> sounds perfect to me.
>
> Not sure indeed if this change fits to this same patch. Looks like a
> 4th patch sounds better.

Indeed not. :)
A new set is coming soon.

Br,

David

>
> Br,
>
> David Cohen
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
  2011-03-08 19:06     ` David Cohen
@ 2011-03-08 19:53       ` Guzman Lugo, Fernando
  2011-03-08 20:07         ` David Cohen
  0 siblings, 1 reply; 18+ messages in thread
From: Guzman Lugo, Fernando @ 2011-03-08 19:53 UTC (permalink / raw)
  To: David Cohen
  Cc: Hiroshi.DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus, Michael Jones

On Tue, Mar 8, 2011 at 1:06 PM, David Cohen <dacohen@gmail.com> wrote:
> On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>
>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>> the NULL address if da_start==0, which would then not get unmapped.
>>> Disallow this again.  And spell variable 'alignment' correctly.
>>>
>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>>> ---
>>>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>>>  1 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>> index 6dc1296..11c9b76 100644
>>> --- a/arch/arm/plat-omap/iovmm.c
>>> +++ b/arch/arm/plat-omap/iovmm.c
>>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>>                                           size_t bytes, u32 flags)
>>>  {
>>>        struct iovm_struct *new, *tmp;
>>> -       u32 start, prev_end, alignement;
>>> +       u32 start, prev_end, alignment;
>>>
>>>        if (!obj || !bytes)
>>>                return ERR_PTR(-EINVAL);
>>>
>>>        start = da;
>>> -       alignement = PAGE_SIZE;
>>> +       alignment = PAGE_SIZE;
>>>
>>>        if (flags & IOVMF_DA_ANON) {
>>> -               start = obj->da_start;
>>> +               /* Don't map address 0 */
>>> +               if (obj->da_start)
>>> +                       start = obj->da_start;
>>> +               else
>>> +                       start = obj->da_start + alignment;
>>
>> else part obj->da_star is always 0, so why not?
>> start = alignment;
>
> The patch is not from me, but I can fix it if Michael agrees.
>
>>
>> so, why I understand, it now it is possible mapp address 0x0 only if
>> IOVMF_DA_ANON is not set, right? maybe that would be mention in the
>> patch.
>
> Indeed address 0x0 was never meant to be mapped. The mentioned commit
> on the patch's description did that without noticing. But the new
> change is documented in the code by the comment "Don't map address 0"

yeah, but it only applies when "flags & IOVMF_DA_ANON", So if I use
IOVMF_DA_FIXED and da_start == 0, I will be able to map some area
which starts from address 0x0, right?  if so, that looks good to me
and the description should mention that if is disallowing mapping
address 0x0 when IOVMF_DA_ANON is used.

Regards,
Fernando.

> and it's also mentioned on the patch description. Is it OK for you?
>
> Regards,
>
> David Cohen
>
>>
>> Regards,
>> Fernando.
>>
>>>
>>>                if (flags & IOVMF_LINEAR)
>>> -                       alignement = iopgsz_max(bytes);
>>> -               start = roundup(start, alignement);
>>> +                       alignment = iopgsz_max(bytes);
>>> +               start = roundup(start, alignment);
>>>        } else if (start < obj->da_start || start > obj->da_end ||
>>>                                        obj->da_end - start < bytes) {
>>>                return ERR_PTR(-EINVAL);
>>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>>                        goto found;
>>>
>>>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>>> -                       start = roundup(tmp->da_end + 1, alignement);
>>> +                       start = roundup(tmp->da_end + 1, alignment);
>>>
>>>                prev_end = tmp->da_end;
>>>        }
>>> --
>>> 1.7.0.4
>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/3] omap: iovmm: disallow mapping NULL address
  2011-03-08 19:53       ` Guzman Lugo, Fernando
@ 2011-03-08 20:07         ` David Cohen
  0 siblings, 0 replies; 18+ messages in thread
From: David Cohen @ 2011-03-08 20:07 UTC (permalink / raw)
  To: Guzman Lugo, Fernando
  Cc: Hiroshi.DOYU, linux-omap, linux-media, laurent.pinchart,
	sakari.ailus, Michael Jones

On Tue, Mar 8, 2011 at 9:53 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Tue, Mar 8, 2011 at 1:06 PM, David Cohen <dacohen@gmail.com> wrote:
>> On Tue, Mar 8, 2011 at 8:06 PM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> On Tue, Mar 8, 2011 at 6:46 AM, David Cohen <dacohen@gmail.com> wrote:
>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>>
>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>> the NULL address if da_start==0, which would then not get unmapped.
>>>> Disallow this again.  And spell variable 'alignment' correctly.
>>>>
>>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>>>> ---
>>>>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>>>>  1 files changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>>> index 6dc1296..11c9b76 100644
>>>> --- a/arch/arm/plat-omap/iovmm.c
>>>> +++ b/arch/arm/plat-omap/iovmm.c
>>>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>>>                                           size_t bytes, u32 flags)
>>>>  {
>>>>        struct iovm_struct *new, *tmp;
>>>> -       u32 start, prev_end, alignement;
>>>> +       u32 start, prev_end, alignment;
>>>>
>>>>        if (!obj || !bytes)
>>>>                return ERR_PTR(-EINVAL);
>>>>
>>>>        start = da;
>>>> -       alignement = PAGE_SIZE;
>>>> +       alignment = PAGE_SIZE;
>>>>
>>>>        if (flags & IOVMF_DA_ANON) {
>>>> -               start = obj->da_start;
>>>> +               /* Don't map address 0 */
>>>> +               if (obj->da_start)
>>>> +                       start = obj->da_start;
>>>> +               else
>>>> +                       start = obj->da_start + alignment;
>>>
>>> else part obj->da_star is always 0, so why not?
>>> start = alignment;
>>
>> The patch is not from me, but I can fix it if Michael agrees.
>>
>>>
>>> so, why I understand, it now it is possible mapp address 0x0 only if
>>> IOVMF_DA_ANON is not set, right? maybe that would be mention in the
>>> patch.
>>
>> Indeed address 0x0 was never meant to be mapped. The mentioned commit
>> on the patch's description did that without noticing. But the new
>> change is documented in the code by the comment "Don't map address 0"
>
> yeah, but it only applies when "flags & IOVMF_DA_ANON", So if I use
> IOVMF_DA_FIXED and da_start == 0, I will be able to map some area
> which starts from address 0x0, right?  if so, that looks good to me
> and the description should mention that if is disallowing mapping
> address 0x0 when IOVMF_DA_ANON is used.

Yes, that's the case. I will update the comment. I hope Michael does
not complain about the changes. :)

Br,

David

>
> Regards,
> Fernando.
>
>> and it's also mentioned on the patch description. Is it OK for you?
>>
>> Regards,
>>
>> David Cohen
>>
>>>
>>> Regards,
>>> Fernando.
>>>
>>>>
>>>>                if (flags & IOVMF_LINEAR)
>>>> -                       alignement = iopgsz_max(bytes);
>>>> -               start = roundup(start, alignement);
>>>> +                       alignment = iopgsz_max(bytes);
>>>> +               start = roundup(start, alignment);
>>>>        } else if (start < obj->da_start || start > obj->da_end ||
>>>>                                        obj->da_end - start < bytes) {
>>>>                return ERR_PTR(-EINVAL);
>>>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>>>                        goto found;
>>>>
>>>>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>>>> -                       start = roundup(tmp->da_end + 1, alignement);
>>>> +                       start = roundup(tmp->da_end + 1, alignment);
>>>>
>>>>                prev_end = tmp->da_end;
>>>>        }
>>>> --
>>>> 1.7.0.4
>>>>
>>>>
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2011-03-08 20:07 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-08 12:46 [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' David Cohen
2011-03-08 12:46 ` [PATCH 1/3] omap: iovmm: disallow mapping NULL address David Cohen
2011-03-08 18:06   ` Guzman Lugo, Fernando
2011-03-08 19:06     ` David Cohen
2011-03-08 19:53       ` Guzman Lugo, Fernando
2011-03-08 20:07         ` David Cohen
2011-03-08 12:46 ` [PATCH 2/3] omap3: change ISP's IOMMU da_start address David Cohen
2011-03-08 14:06   ` Sakari Ailus
2011-03-08 17:15     ` David Cohen
2011-03-08 12:46 ` [PATCH 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED/IOVMF_DA_ANON flags David Cohen
2011-03-08 17:59   ` Guzman Lugo, Fernando
2011-03-08 18:09     ` Hiroshi DOYU
2011-03-08 18:53       ` Guzman Lugo, Fernando
2011-03-08 18:57         ` David Cohen
2011-03-08 19:33           ` Guzman Lugo, Fernando
2011-03-08 19:46             ` David Cohen
2011-03-08 19:51               ` David Cohen
2011-03-08 12:49 ` [PATCH 0/3] omap: iovmm: Fix IOVMM check for fixed 'da' Hiroshi DOYU

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).