omap3isp cache error when unloading

omap3isp cache error when unloading

[Michael Jones]

Hi all,

I get a warning about a cache error with the following steps:

0. load omap3-isp
1. set up media broken media pipeline. (e.g. set different formats on
opposite ends of a link, as will be the case for using the lane shifter)
2. try to capture images.  isp_video_streamon() returns -EPIPE from the
failed isp_video_validate_pipeline() call.
3. unload omap3-isp module

then I get the following from kmem_cache_destroy():

slab error in kmem_cache_destroy(): cache `iovm_area_cache': Can't free all objects
[<c0040318>] (unwind_backtrace+0x0/0xec) from [<c00bfe14>] (kmem_cache_destroy+0x88/0xf4)
[<c00bfe14>] (kmem_cache_destroy+0x88/0xf4) from [<c00861f8>] (sys_delete_module+0x1c4/0x230)
[<c00861f8>] (sys_delete_module+0x1c4/0x230) from [<c003b680>] (ret_fast_syscall+0x0/0x30)

Then, when reloading the module:
SLAB: cache with size 32 has lost its name

Can somebody else confirm that they also observe this behavior?

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: omap3isp cache error when unloading

[Laurent Pinchart]

Hi Michael,

On Tuesday 01 March 2011 17:41:01 Michael Jones wrote:
> Hi all,
> 
> I get a warning about a cache error with the following steps:
> 
> 0. load omap3-isp
> 1. set up media broken media pipeline. (e.g. set different formats on
> opposite ends of a link, as will be the case for using the lane shifter)
> 2. try to capture images.  isp_video_streamon() returns -EPIPE from the
> failed isp_video_validate_pipeline() call.
> 3. unload omap3-isp module
> 
> then I get the following from kmem_cache_destroy():
> 
> slab error in kmem_cache_destroy(): cache `iovm_area_cache': Can't free all
> objects [<c0040318>] (unwind_backtrace+0x0/0xec) from [<c00bfe14>]
> (kmem_cache_destroy+0x88/0xf4) [<c00bfe14>] (kmem_cache_destroy+0x88/0xf4)
> from [<c00861f8>] (sys_delete_module+0x1c4/0x230) [<c00861f8>]
> (sys_delete_module+0x1c4/0x230) from [<c003b680>]
> (ret_fast_syscall+0x0/0x30)
> 
> Then, when reloading the module:
> SLAB: cache with size 32 has lost its name
> 
> Can somebody else confirm that they also observe this behavior?

I can't reproduce that (tried both 2.6.32 and 2.6.37). Could you give me some 
more details about your exact test procedure (such as how you configure the 
pipeline) ?

-- 
Regards,

Laurent Pinchart

Re: omap3isp cache error when unloading

[Michael Jones]

On 03/02/2011 08:18 PM, Laurent Pinchart wrote:
> Hi Michael,
> 
> On Tuesday 01 March 2011 17:41:01 Michael Jones wrote:
>> Hi all,
>>
>> I get a warning about a cache error with the following steps:
>>
>> 0. load omap3-isp
>> 1. set up media broken media pipeline. (e.g. set different formats on
>> opposite ends of a link, as will be the case for using the lane shifter)
>> 2. try to capture images.  isp_video_streamon() returns -EPIPE from the
>> failed isp_video_validate_pipeline() call.
>> 3. unload omap3-isp module
>>
>> then I get the following from kmem_cache_destroy():
>>
>> slab error in kmem_cache_destroy(): cache `iovm_area_cache': Can't free all
>> objects [<c0040318>] (unwind_backtrace+0x0/0xec) from [<c00bfe14>]
>> (kmem_cache_destroy+0x88/0xf4) [<c00bfe14>] (kmem_cache_destroy+0x88/0xf4)
>> from [<c00861f8>] (sys_delete_module+0x1c4/0x230) [<c00861f8>]
>> (sys_delete_module+0x1c4/0x230) from [<c003b680>]
>> (ret_fast_syscall+0x0/0x30)
>>
>> Then, when reloading the module:
>> SLAB: cache with size 32 has lost its name
>>
>> Can somebody else confirm that they also observe this behavior?
> 
> I can't reproduce that (tried both 2.6.32 and 2.6.37). Could you give me some 
> more details about your exact test procedure (such as how you configure the 
> pipeline) ?
> 

Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
It's actually not related to mis-configuring the ISP pipeline like I
thought at first- it also happens after I have successfully captured images.

I've since tracked down the problem, although I don't understand the
cache management well enough to be sure it's a proper fix, so hopefully
some new recipients on this can make suggestions/comments.

The patch below solves the problem, which modifies a commit by Fernando
Guzman Lugo from December.

-Michael

>From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
From: Michael Jones <michael.jones@matrix-vision.de>
Date: Thu, 3 Mar 2011 16:50:39 +0100
Subject: [PATCH] fix iovmm slab cache error on module unload

modify "OMAP: iommu: create new api to set valid da range"

This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
---
 arch/arm/plat-omap/iovmm.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..2fba6f1 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 	alignement = PAGE_SIZE;
 
 	if (flags & IOVMF_DA_ANON) {
-		start = obj->da_start;
+		/*
+		 * Reserve the first page for NULL
+		 */
+		start = obj->da_start + PAGE_SIZE;
 
 		if (flags & IOVMF_LINEAR)
 			alignement = iopgsz_max(bytes);
-- 
1.7.4.1



MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: omap3isp cache error when unloading

[Sakari Ailus]

Hi Michael,

Michael Jones wrote:
> On 03/02/2011 08:18 PM, Laurent Pinchart wrote:
>> Hi Michael,
>>
>> On Tuesday 01 March 2011 17:41:01 Michael Jones wrote:
>>> Hi all,
>>>
>>> I get a warning about a cache error with the following steps:
>>>
>>> 0. load omap3-isp
>>> 1. set up media broken media pipeline. (e.g. set different formats on
>>> opposite ends of a link, as will be the case for using the lane shifter)
>>> 2. try to capture images.  isp_video_streamon() returns -EPIPE from the
>>> failed isp_video_validate_pipeline() call.
>>> 3. unload omap3-isp module
>>>
>>> then I get the following from kmem_cache_destroy():
>>>
>>> slab error in kmem_cache_destroy(): cache `iovm_area_cache': Can't free all
>>> objects [<c0040318>] (unwind_backtrace+0x0/0xec) from [<c00bfe14>]
>>> (kmem_cache_destroy+0x88/0xf4) [<c00bfe14>] (kmem_cache_destroy+0x88/0xf4)
>>> from [<c00861f8>] (sys_delete_module+0x1c4/0x230) [<c00861f8>]
>>> (sys_delete_module+0x1c4/0x230) from [<c003b680>]
>>> (ret_fast_syscall+0x0/0x30)
>>>
>>> Then, when reloading the module:
>>> SLAB: cache with size 32 has lost its name
>>>
>>> Can somebody else confirm that they also observe this behavior?
>>
>> I can't reproduce that (tried both 2.6.32 and 2.6.37). Could you give me some 
>> more details about your exact test procedure (such as how you configure the 
>> pipeline) ?
>>
> 
> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
> based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
> It's actually not related to mis-configuring the ISP pipeline like I
> thought at first- it also happens after I have successfully captured images.
> 
> I've since tracked down the problem, although I don't understand the
> cache management well enough to be sure it's a proper fix, so hopefully
> some new recipients on this can make suggestions/comments.
> 
> The patch below solves the problem, which modifies a commit by Fernando
> Guzman Lugo from December.

Thanks for the patch.

It looks like this patch from Fernando, perhaps unintentionally, also
makes the first page mappable so the NULL address is also valid. The
NULL address isn't considered valid by the ISP driver which thus does
not iommu_vunmap() the NULL address.

Hiroshi, David, what do you think? I think the patch is correct so it
could be applied with description changed to mention it disallows
mapping the first page again.

Or is there a reason to allow mapping the first page automatically? I
personally don't see any.

Cheers,

> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
> From: Michael Jones <michael.jones@matrix-vision.de>
> Date: Thu, 3 Mar 2011 16:50:39 +0100
> Subject: [PATCH] fix iovmm slab cache error on module unload
> 
> modify "OMAP: iommu: create new api to set valid da range"
> 
> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
> ---
>  arch/arm/plat-omap/iovmm.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..2fba6f1 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>  	alignement = PAGE_SIZE;
>  
>  	if (flags & IOVMF_DA_ANON) {
> -		start = obj->da_start;
> +		/*
> +		 * Reserve the first page for NULL
> +		 */
> +		start = obj->da_start + PAGE_SIZE;
>  
>  		if (flags & IOVMF_LINEAR)
>  			alignement = iopgsz_max(bytes);


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

Re: omap3isp cache error when unloading

[Hiroshi DOYU]

From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
Subject: Re: omap3isp cache error when unloading
Date: Fri, 4 Mar 2011 09:38:22 +0200

> Hi Michael,
> 
> Michael Jones wrote:
>> On 03/02/2011 08:18 PM, Laurent Pinchart wrote:
>>> Hi Michael,
>>>
>>> On Tuesday 01 March 2011 17:41:01 Michael Jones wrote:
>>>> Hi all,
>>>>
>>>> I get a warning about a cache error with the following steps:
>>>>
>>>> 0. load omap3-isp
>>>> 1. set up media broken media pipeline. (e.g. set different formats on
>>>> opposite ends of a link, as will be the case for using the lane shifter)
>>>> 2. try to capture images.  isp_video_streamon() returns -EPIPE from the
>>>> failed isp_video_validate_pipeline() call.
>>>> 3. unload omap3-isp module
>>>>
>>>> then I get the following from kmem_cache_destroy():
>>>>
>>>> slab error in kmem_cache_destroy(): cache `iovm_area_cache': Can't free all
>>>> objects [<c0040318>] (unwind_backtrace+0x0/0xec) from [<c00bfe14>]
>>>> (kmem_cache_destroy+0x88/0xf4) [<c00bfe14>] (kmem_cache_destroy+0x88/0xf4)
>>>> from [<c00861f8>] (sys_delete_module+0x1c4/0x230) [<c00861f8>]
>>>> (sys_delete_module+0x1c4/0x230) from [<c003b680>]
>>>> (ret_fast_syscall+0x0/0x30)
>>>>
>>>> Then, when reloading the module:
>>>> SLAB: cache with size 32 has lost its name
>>>>
>>>> Can somebody else confirm that they also observe this behavior?
>>>
>>> I can't reproduce that (tried both 2.6.32 and 2.6.37). Could you give me some 
>>> more details about your exact test procedure (such as how you configure the 
>>> pipeline) ?
>>>
>> 
>> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
>> based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
>> It's actually not related to mis-configuring the ISP pipeline like I
>> thought at first- it also happens after I have successfully captured images.
>> 
>> I've since tracked down the problem, although I don't understand the
>> cache management well enough to be sure it's a proper fix, so hopefully
>> some new recipients on this can make suggestions/comments.
>> 
>> The patch below solves the problem, which modifies a commit by Fernando
>> Guzman Lugo from December.
> 
> Thanks for the patch.
> 
> It looks like this patch from Fernando, perhaps unintentionally, also
> makes the first page mappable so the NULL address is also valid. The
> NULL address isn't considered valid by the ISP driver which thus does
> not iommu_vunmap() the NULL address.
> 
> Hiroshi, David, what do you think? I think the patch is correct so it
> could be applied with description changed to mention it disallows
> mapping the first page again.
>
> Or is there a reason to allow mapping the first page automatically? I
> personally don't see any.

I think that was done "unintentionally". The invalid first page may be
quite reasonable generally.

Re: omap3isp cache error when unloading

[David Cohen]

Hi,

[snip]

> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
> based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
> It's actually not related to mis-configuring the ISP pipeline like I
> thought at first- it also happens after I have successfully captured images.
>
> I've since tracked down the problem, although I don't understand the
> cache management well enough to be sure it's a proper fix, so hopefully
> some new recipients on this can make suggestions/comments.
>
> The patch below solves the problem, which modifies a commit by Fernando
> Guzman Lugo from December.
>
> -Michael
>
> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
> From: Michael Jones <michael.jones@matrix-vision.de>
> Date: Thu, 3 Mar 2011 16:50:39 +0100
> Subject: [PATCH] fix iovmm slab cache error on module unload
>
> modify "OMAP: iommu: create new api to set valid da range"
>
> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
> ---
>  arch/arm/plat-omap/iovmm.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..2fba6f1 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>        alignement = PAGE_SIZE;
>
>        if (flags & IOVMF_DA_ANON) {
> -               start = obj->da_start;
> +               /*
> +                * Reserve the first page for NULL
> +                */
> +               start = obj->da_start + PAGE_SIZE;

IMO if obj->da_start != 0, no need to add PAGE_SIZE. Otherwise, it
does make sense to correct wrong obj->da_start == 0. Another thing is
this piece of code is using alignement (alignment) variable instead of
PAGE_SIZE (which is the same value).

Br,

David

Re: omap3isp cache error when unloading

[Michael Jones]

Hi David,

On 03/04/2011 02:12 PM, David Cohen wrote:
> Hi,
> 
> [snip]
> 
>> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
>> based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
>> It's actually not related to mis-configuring the ISP pipeline like I
>> thought at first- it also happens after I have successfully captured images.
>>
>> I've since tracked down the problem, although I don't understand the
>> cache management well enough to be sure it's a proper fix, so hopefully
>> some new recipients on this can make suggestions/comments.
>>
>> The patch below solves the problem, which modifies a commit by Fernando
>> Guzman Lugo from December.
>>
>> -Michael
>>
>> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
>> From: Michael Jones <michael.jones@matrix-vision.de>
>> Date: Thu, 3 Mar 2011 16:50:39 +0100
>> Subject: [PATCH] fix iovmm slab cache error on module unload
>>
>> modify "OMAP: iommu: create new api to set valid da range"
>>
>> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
>> ---
>>  arch/arm/plat-omap/iovmm.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 6dc1296..2fba6f1 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>        alignement = PAGE_SIZE;
>>
>>        if (flags & IOVMF_DA_ANON) {
>> -               start = obj->da_start;
>> +               /*
>> +                * Reserve the first page for NULL
>> +                */
>> +               start = obj->da_start + PAGE_SIZE;
> 
> IMO if obj->da_start != 0, no need to add PAGE_SIZE. Otherwise, it
> does make sense to correct wrong obj->da_start == 0. Another thing is
> this piece of code is using alignement (alignment) variable instead of
> PAGE_SIZE (which is the same value).
> 
> Br,
> 
> David

I believe the following patch addresses your comments.  I couldn't
resist also fixing the misspelling of alignment when I was using the
variable in my patch.

-Michael

>From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
From: Michael Jones <michael.jones@matrix-vision.de>
Date: Fri, 4 Mar 2011 15:09:48 +0100
Subject: [PATCH] omap: iovmm: disallow mapping NULL address

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.4.1



MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: omap3isp cache error when unloading

[David Cohen]

On Fri, Mar 4, 2011 at 4:39 PM, Michael Jones
<michael.jones@matrix-vision.de> wrote:
> Hi David,

Hi Michael,

>
> On 03/04/2011 02:12 PM, David Cohen wrote:
>> Hi,
>>
>> [snip]
>>
>>> Sorry, I should've mentioned: I'm using your media-0005-omap3isp branch
>>> based on 2.6.38-rc5.  I didn't have the problem with 2.6.37, either.
>>> It's actually not related to mis-configuring the ISP pipeline like I
>>> thought at first- it also happens after I have successfully captured images.
>>>
>>> I've since tracked down the problem, although I don't understand the
>>> cache management well enough to be sure it's a proper fix, so hopefully
>>> some new recipients on this can make suggestions/comments.
>>>
>>> The patch below solves the problem, which modifies a commit by Fernando
>>> Guzman Lugo from December.
>>>
>>> -Michael
>>>
>>> From db35fb8edca2a4f8fd37197d77fd58676cb1dcac Mon Sep 17 00:00:00 2001
>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>> Date: Thu, 3 Mar 2011 16:50:39 +0100
>>> Subject: [PATCH] fix iovmm slab cache error on module unload
>>>
>>> modify "OMAP: iommu: create new api to set valid da range"
>>>
>>> This modifies commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb.
>>> ---
>>>  arch/arm/plat-omap/iovmm.c |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>>> index 6dc1296..2fba6f1 100644
>>> --- a/arch/arm/plat-omap/iovmm.c
>>> +++ b/arch/arm/plat-omap/iovmm.c
>>> @@ -280,7 +280,10 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>>        alignement = PAGE_SIZE;
>>>
>>>        if (flags & IOVMF_DA_ANON) {
>>> -               start = obj->da_start;
>>> +               /*
>>> +                * Reserve the first page for NULL
>>> +                */
>>> +               start = obj->da_start + PAGE_SIZE;
>>
>> IMO if obj->da_start != 0, no need to add PAGE_SIZE. Otherwise, it
>> does make sense to correct wrong obj->da_start == 0. Another thing is
>> this piece of code is using alignement (alignment) variable instead of
>> PAGE_SIZE (which is the same value).
>>
>> Br,
>>
>> David
>
> I believe the following patch addresses your comments.  I couldn't
> resist also fixing the misspelling of alignment when I was using the
> variable in my patch.
>
> -Michael
>
> From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
> From: Michael Jones <michael.jones@matrix-vision.de>
> Date: Fri, 4 Mar 2011 15:09:48 +0100
> Subject: [PATCH] omap: iovmm: disallow mapping NULL address
>
> 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;

It seems to be fine for me now. Let's see what Hiroshi says.

Regards,

David

>
>                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.4.1
>
>
>
> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
> Registergericht: Amtsgericht Stuttgart, HRB 271090
> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>

Re: omap3isp cache error when unloading

[David Cohen]

[snip]

>> From 2712f2fd087ca782e964c912c7f1973e7d84f2b7 Mon Sep 17 00:00:00 2001
>> From: Michael Jones <michael.jones@matrix-vision.de>
>> Date: Fri, 4 Mar 2011 15:09:48 +0100
>> Subject: [PATCH] omap: iovmm: disallow mapping NULL address
>>
>> 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;
>
> It seems to be fine for me now. Let's see what Hiroshi says.

Sorry, I'm afraid I changed my mind after take a look into the driver. :)
Try to correct obj->da_start in the functions iommu_set_da_range() and
omap_iommu_probe(). That should be the correct way. Your patch doesn't
fix this situation when IOVMF_DA_ANON isn't set.
After obj->da_start is correctly set, your current patch is non longer required.

Regards,

David

>
> Regards,
>
> David
>
>>
>>                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.4.1
>>
>>
>>
>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
>> Registergericht: Amtsgericht Stuttgart, HRB 271090
>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>>
>

[PATCH] omap: iommu: disallow mapping NULL address

[Michael Jones]

>From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
From: Michael Jones <michael.jones@matrix-vision.de>
Date: Mon, 7 Mar 2011 13:36:15 +0100
Subject: [PATCH] omap: iommu: disallow mapping NULL address

commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
the NULL address if da_start==0.  Force da_start to exclude the
first page.

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

diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
index 5990ea6..dcb5513 100644
--- a/arch/arm/plat-omap/iommu.c
+++ b/arch/arm/plat-omap/iommu.c
@@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
 	if (end < start || !PAGE_ALIGN(start | end))
 		return -EINVAL;
 
-	obj->da_start = start;
+	obj->da_start = max(start, (u32)PAGE_SIZE);
 	obj->da_end = end;
 
 	return 0;
@@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
 	obj->name = pdata->name;
 	obj->dev = &pdev->dev;
 	obj->ctx = (void *)obj + sizeof(*obj);
-	obj->da_start = pdata->da_start;
+
+	/* reserve the first page for NULL */
+	obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE);
 	obj->da_end = pdata->da_end;
 
 	mutex_init(&obj->iommu_lock);
-- 
1.7.4.1


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Guzman Lugo, Fernando]

On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
<michael.jones@matrix-vision.de> wrote:
> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
> From: Michael Jones <michael.jones@matrix-vision.de>
> Date: Mon, 7 Mar 2011 13:36:15 +0100
> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>
> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
> the NULL address if da_start==0.  Force da_start to exclude the
> first page.

what about devices that uses page 0? ipu after reset always starts
from 0x00000000 how could we map that address??

Regards,
Fernando.

>
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  arch/arm/plat-omap/iommu.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
> index 5990ea6..dcb5513 100644
> --- a/arch/arm/plat-omap/iommu.c
> +++ b/arch/arm/plat-omap/iommu.c
> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
>        if (end < start || !PAGE_ALIGN(start | end))
>                return -EINVAL;
>
> -       obj->da_start = start;
> +       obj->da_start = max(start, (u32)PAGE_SIZE);
>        obj->da_end = end;
>
>        return 0;
> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>        obj->name = pdata->name;
>        obj->dev = &pdev->dev;
>        obj->ctx = (void *)obj + sizeof(*obj);
> -       obj->da_start = pdata->da_start;
> +
> +       /* reserve the first page for NULL */
> +       obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE);
>        obj->da_end = pdata->da_end;
>
>        mutex_init(&obj->iommu_lock);
> --
> 1.7.4.1
>
>
> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
> Registergericht: Amtsgericht Stuttgart, HRB 271090
> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[David Cohen]

On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
> <michael.jones@matrix-vision.de> wrote:
>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>> From: Michael Jones <michael.jones@matrix-vision.de>
>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>
>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> the NULL address if da_start==0.  Force da_start to exclude the
>> first page.
>
> what about devices that uses page 0? ipu after reset always starts
> from 0x00000000 how could we map that address??

from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?

Br,

David

>
> Regards,
> Fernando.
>
>>
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>> ---
>>  arch/arm/plat-omap/iommu.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>> index 5990ea6..dcb5513 100644
>> --- a/arch/arm/plat-omap/iommu.c
>> +++ b/arch/arm/plat-omap/iommu.c
>> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
>>        if (end < start || !PAGE_ALIGN(start | end))
>>                return -EINVAL;
>>
>> -       obj->da_start = start;
>> +       obj->da_start = max(start, (u32)PAGE_SIZE);
>>        obj->da_end = end;
>>
>>        return 0;
>> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>>        obj->name = pdata->name;
>>        obj->dev = &pdev->dev;
>>        obj->ctx = (void *)obj + sizeof(*obj);
>> -       obj->da_start = pdata->da_start;
>> +
>> +       /* reserve the first page for NULL */
>> +       obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE);
>>        obj->da_end = pdata->da_end;
>>
>>        mutex_init(&obj->iommu_lock);
>> --
>> 1.7.4.1
>>
>>
>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
>> Registergericht: Amtsgericht Stuttgart, HRB 271090
>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>>
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Guzman Lugo, Fernando]

On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>> <michael.jones@matrix-vision.de> wrote:
>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>
>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>> the NULL address if da_start==0.  Force da_start to exclude the
>>> first page.
>>
>> what about devices that uses page 0? ipu after reset always starts
>> from 0x00000000 how could we map that address??
>
> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?

unlike DSP that you can load a register with the addres the DSP will
boot, IPU core always starts from address 0x00000000, so if you take
IPU out of reset it will try to access address 0x0 if not map it,
there will be a mmu fault.

Regards,
Fernando.

>
> Br,
>
> David
>
>>
>> Regards,
>> Fernando.
>>
>>>
>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>>> ---
>>>  arch/arm/plat-omap/iommu.c |    6 ++++--
>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>>> index 5990ea6..dcb5513 100644
>>> --- a/arch/arm/plat-omap/iommu.c
>>> +++ b/arch/arm/plat-omap/iommu.c
>>> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
>>>        if (end < start || !PAGE_ALIGN(start | end))
>>>                return -EINVAL;
>>>
>>> -       obj->da_start = start;
>>> +       obj->da_start = max(start, (u32)PAGE_SIZE);
>>>        obj->da_end = end;
>>>
>>>        return 0;
>>> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>>>        obj->name = pdata->name;
>>>        obj->dev = &pdev->dev;
>>>        obj->ctx = (void *)obj + sizeof(*obj);
>>> -       obj->da_start = pdata->da_start;
>>> +
>>> +       /* reserve the first page for NULL */
>>> +       obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE);
>>>        obj->da_end = pdata->da_end;
>>>
>>>        mutex_init(&obj->iommu_lock);
>>> --
>>> 1.7.4.1
>>>
>>>
>>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
>>> Registergericht: Amtsgericht Stuttgart, HRB 271090
>>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>>>
>>
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[David Cohen]

On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>> <michael.jones@matrix-vision.de> wrote:
>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>
>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>> first page.
>>>
>>> what about devices that uses page 0? ipu after reset always starts
>>> from 0x00000000 how could we map that address??
>>
>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
>
> unlike DSP that you can load a register with the addres the DSP will
> boot, IPU core always starts from address 0x00000000, so if you take
> IPU out of reset it will try to access address 0x0 if not map it,
> there will be a mmu fault.

Hm. Looks like the iommu should not restrict any da. The valid da
range should rely only on pdata.
Michael, what about just update ISP's da_start on omap-iommu.c file?
Set it to 0x1000.

Hiroshi, any opinion?

Br,

David

>
> Regards,
> Fernando.
>
>>
>> Br,
>>
>> David
>>
>>>
>>> Regards,
>>> Fernando.
>>>
>>>>
>>>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>>>> ---
>>>>  arch/arm/plat-omap/iommu.c |    6 ++++--
>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c
>>>> index 5990ea6..dcb5513 100644
>>>> --- a/arch/arm/plat-omap/iommu.c
>>>> +++ b/arch/arm/plat-omap/iommu.c
>>>> @@ -850,7 +850,7 @@ int iommu_set_da_range(struct iommu *obj, u32 start, u32 end)
>>>>        if (end < start || !PAGE_ALIGN(start | end))
>>>>                return -EINVAL;
>>>>
>>>> -       obj->da_start = start;
>>>> +       obj->da_start = max(start, (u32)PAGE_SIZE);
>>>>        obj->da_end = end;
>>>>
>>>>        return 0;
>>>> @@ -950,7 +950,9 @@ static int __devinit omap_iommu_probe(struct platform_device *pdev)
>>>>        obj->name = pdata->name;
>>>>        obj->dev = &pdev->dev;
>>>>        obj->ctx = (void *)obj + sizeof(*obj);
>>>> -       obj->da_start = pdata->da_start;
>>>> +
>>>> +       /* reserve the first page for NULL */
>>>> +       obj->da_start = max(pdata->da_start, (u32)PAGE_SIZE);
>>>>        obj->da_end = pdata->da_end;
>>>>
>>>>        mutex_init(&obj->iommu_lock);
>>>> --
>>>> 1.7.4.1
>>>>
>>>>
>>>> MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
>>>> Registergericht: Amtsgericht Stuttgart, HRB 271090
>>>> Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner
>>>>
>>>
>>
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Laurent Pinchart]

Hi David,

On Monday 07 March 2011 20:41:21 David Cohen wrote:
> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
> >>>> From: Michael Jones <michael.jones@matrix-vision.de>
> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
> >>>> 
> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
> >>>> the NULL address if da_start==0.  Force da_start to exclude the
> >>>> first page.
> >>> 
> >>> what about devices that uses page 0? ipu after reset always starts
> >>> from 0x00000000 how could we map that address??
> >> 
> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
> >> want it?
> > 
> > unlike DSP that you can load a register with the addres the DSP will
> > boot, IPU core always starts from address 0x00000000, so if you take
> > IPU out of reset it will try to access address 0x0 if not map it,
> > there will be a mmu fault.
> 
> Hm. Looks like the iommu should not restrict any da. The valid da
> range should rely only on pdata.
> Michael, what about just update ISP's da_start on omap-iommu.c file?
> Set it to 0x1000.

What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as 
an invalid/freed pointer ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap: iommu: disallow mapping NULL address

[David Cohen]

On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi David,

Hi Laurent,

>
> On Monday 07 March 2011 20:41:21 David Cohen wrote:
>> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
>> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
>> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
>> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
>> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>> >>>> From: Michael Jones <michael.jones@matrix-vision.de>
>> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>> >>>>
>> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> >>>> the NULL address if da_start==0.  Force da_start to exclude the
>> >>>> first page.
>> >>>
>> >>> what about devices that uses page 0? ipu after reset always starts
>> >>> from 0x00000000 how could we map that address??
>> >>
>> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
>> >> want it?
>> >
>> > unlike DSP that you can load a register with the addres the DSP will
>> > boot, IPU core always starts from address 0x00000000, so if you take
>> > IPU out of reset it will try to access address 0x0 if not map it,
>> > there will be a mmu fault.
>>
>> Hm. Looks like the iommu should not restrict any da. The valid da
>> range should rely only on pdata.
>> Michael, what about just update ISP's da_start on omap-iommu.c file?
>> Set it to 0x1000.
>
> What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as
> an invalid/freed pointer ?

I wouldn't be comfortable to use 0 (or NULL) value as valid address on
ISP driver. The 'da' range (da_start and da_end) is defined per VM and
specified as platform data. IMO, to set da_start = 0x1000 seems to be
a correct approach for ISP as it's the only client for its IOMMU
instance.

Regards,

David

>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Hiroshi DOYU]

From: ext David Cohen <dacohen@gmail.com>
Subject: Re: [PATCH] omap: iommu: disallow mapping NULL address
Date: Mon, 7 Mar 2011 21:41:21 +0200

> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando
> <fernando.lugo@ti.com> wrote:
>> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>>> <michael.jones@matrix-vision.de> wrote:
>>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>>
>>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>>> first page.
>>>>
>>>> what about devices that uses page 0? ipu after reset always starts
>>>> from 0x00000000 how could we map that address??
>>>
>>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
>>
>> unlike DSP that you can load a register with the addres the DSP will
>> boot, IPU core always starts from address 0x00000000, so if you take
>> IPU out of reset it will try to access address 0x0 if not map it,
>> there will be a mmu fault.
> 
> Hm. Looks like the iommu should not restrict any da. The valid da
> range should rely only on pdata.
> Michael, what about just update ISP's da_start on omap-iommu.c file?
> Set it to 0x1000.
> 
> Hiroshi, any opinion?

We have assumed that 'da == 0' is NULL so far. According to Fernando's
explanation, 'da == 0' should be allowed in iovmm layer by default.

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Hiroshi DOYU]

From: ext David Cohen <dacohen@gmail.com>
Subject: Re: [PATCH] omap: iommu: disallow mapping NULL address
Date: Mon, 7 Mar 2011 23:35:31 +0200

> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi David,
> 
> Hi Laurent,
> 
>>
>> On Monday 07 March 2011 20:41:21 David Cohen wrote:
>>> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
>>> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
>>> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
>>> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
>>> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>> >>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>> >>>>
>>> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>> >>>> the NULL address if da_start==0.  Force da_start to exclude the
>>> >>>> first page.
>>> >>>
>>> >>> what about devices that uses page 0? ipu after reset always starts
>>> >>> from 0x00000000 how could we map that address??
>>> >>
>>> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
>>> >> want it?
>>> >
>>> > unlike DSP that you can load a register with the addres the DSP will
>>> > boot, IPU core always starts from address 0x00000000, so if you take
>>> > IPU out of reset it will try to access address 0x0 if not map it,
>>> > there will be a mmu fault.
>>>
>>> Hm. Looks like the iommu should not restrict any da. The valid da
>>> range should rely only on pdata.
>>> Michael, what about just update ISP's da_start on omap-iommu.c file?
>>> Set it to 0x1000.
>>
>> What about patching the OMAP3 ISP driver to use a non-zero value (maybe -1) as
>> an invalid/freed pointer ?
> 
> I wouldn't be comfortable to use 0 (or NULL) value as valid address on
> ISP driver. The 'da' range (da_start and da_end) is defined per VM and
> specified as platform data. IMO, to set da_start = 0x1000 seems to be
> a correct approach for ISP as it's the only client for its IOMMU
> instance.

Sounds reasonable to me too. Considering 'da == 0' as invalid can be
reasonably acceptable intuitively in most cases, and just let it
allowed theoretically.

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Sakari Ailus]

Guzman Lugo, Fernando wrote:
> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>> <fernando.lugo@ti.com> wrote:
>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>> <michael.jones@matrix-vision.de> wrote:
>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>
>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>> first page.
>>>
>>> what about devices that uses page 0? ipu after reset always starts
>>> from 0x00000000 how could we map that address??
>>
>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
> 
> unlike DSP that you can load a register with the addres the DSP will
> boot, IPU core always starts from address 0x00000000, so if you take
> IPU out of reset it will try to access address 0x0 if not map it,
> there will be a mmu fault.

I think the driver for IPU (what is it, btw.?) must map the NULL address
explicitly. It cannot rely on automatic allocation of the NULL address
by the iommu even if it was the first allocation.

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

Re: [PATCH] omap: iommu: disallow mapping NULL address

[David Cohen]

Hi Fernando,

On Tue, Mar 8, 2011 at 11:13 AM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Guzman Lugo, Fernando wrote:
>> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>>> <michael.jones@matrix-vision.de> wrote:
>>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>>
>>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>>> first page.
>>>>
>>>> what about devices that uses page 0? ipu after reset always starts
>>>> from 0x00000000 how could we map that address??
>>>
>>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
>>
>> unlike DSP that you can load a register with the addres the DSP will
>> boot, IPU core always starts from address 0x00000000, so if you take
>> IPU out of reset it will try to access address 0x0 if not map it,
>> there will be a mmu fault.
>
> I think the driver for IPU (what is it, btw.?) must map the NULL address
> explicitly. It cannot rely on automatic allocation of the NULL address
> by the iommu even if it was the first allocation.

That's an interesting question. My first thought was "it's not
automatic allocation", because it seems you know the specific 'da' IPU
needs. But then, looking into the driver's API, the automatic
allocation is defined whether the argument da == 0 (automatic
allocation) or da != 0 (fixed da). So, by default, the IOMMU driver
does not see da == 0 as valid address for fixed da. Then, why only
automatic allocation should use such address? My second point is: if
you're using automatic allocation, you *cannot* rely on specific da to
be used, as it would be up to IOMMU driver to choose. So, doesn't
matter the option, your driver seems to be wrong, unless I'm missing
something. If you were using fixed da passing da = 0, you were just
being lucky that it was the first request and automatic allocation
returned da == 0.
IMO either first page is not allowed at all or OMAP's IOMMU API should
change the way it checks if it's fixed da or not.

Kind regards,

David

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

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Guzman Lugo, Fernando]

On Tue, Mar 8, 2011 at 3:13 AM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> Guzman Lugo, Fernando wrote:
>> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>>> <fernando.lugo@ti.com> wrote:
>>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>>> <michael.jones@matrix-vision.de> wrote:
>>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>>
>>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>>> first page.
>>>>
>>>> what about devices that uses page 0? ipu after reset always starts
>>>> from 0x00000000 how could we map that address??
>>>
>>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
>>
>> unlike DSP that you can load a register with the addres the DSP will
>> boot, IPU core always starts from address 0x00000000, so if you take
>> IPU out of reset it will try to access address 0x0 if not map it,
>> there will be a mmu fault.
>
> I think the driver for IPU (what is it, btw.?) must map the NULL address
> explicitly. It cannot rely on automatic allocation of the NULL address
> by the iommu even if it was the first allocation.

IPU = imaging processor unit (Cortex-M3 on omap4).

yeah, we should rely on that, so we will need to pass IOVMF_DA_FIXED
flag, what ideally always be success because it is the first map after
getting iommu handle. In this moment it is mapped direcctly using
iommu.c and other layer upon that, but it would be nice be able to use
iovmm in the future.

Regards,
Fernando.


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

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Guzman Lugo, Fernando]

On Tue, Mar 8, 2011 at 3:55 AM, David Cohen <dacohen@gmail.com> wrote:
> Hi Fernando,
>
> On Tue, Mar 8, 2011 at 11:13 AM, Sakari Ailus
> <sakari.ailus@maxwell.research.nokia.com> wrote:
>> Guzman Lugo, Fernando wrote:
>>> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen <dacohen@gmail.com> wrote:
>>>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando
>>>> <fernando.lugo@ti.com> wrote:
>>>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones
>>>>> <michael.jones@matrix-vision.de> wrote:
>>>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00 2001
>>>>>> From: Michael Jones <michael.jones@matrix-vision.de>
>>>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>>>
>>>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>>>> first page.
>>>>>
>>>>> what about devices that uses page 0? ipu after reset always starts
>>>>> from 0x00000000 how could we map that address??
>>>>
>>>> from 0x0? The driver sees da == 0 as error. May I ask you why do you want it?
>>>
>>> unlike DSP that you can load a register with the addres the DSP will
>>> boot, IPU core always starts from address 0x00000000, so if you take
>>> IPU out of reset it will try to access address 0x0 if not map it,
>>> there will be a mmu fault.
>>
>> I think the driver for IPU (what is it, btw.?) must map the NULL address
>> explicitly. It cannot rely on automatic allocation of the NULL address
>> by the iommu even if it was the first allocation.
>
> That's an interesting question. My first thought was "it's not
> automatic allocation", because it seems you know the specific 'da' IPU
> needs. But then, looking into the driver's API, the automatic
> allocation is defined whether the argument da == 0 (automatic
> allocation) or da != 0 (fixed da). So, by default, the IOMMU driver
> does not see da == 0 as valid address for fixed da. Then, why only
> automatic allocation should use such address? My second point is: if
> you're using automatic allocation, you *cannot* rely on specific da to
> be used, as it would be up to IOMMU driver to choose. So, doesn't
> matter the option, your driver seems to be wrong, unless I'm missing
> something. If you were using fixed da passing da = 0, you were just
> being lucky that it was the first request and automatic allocation
> returned da == 0.

yes, the driver is wrong, it should use only flag IOVMF_DA_ANON to get
an automatic address. it has to be changed too.

Regards,
Fernando.

> IMO either first page is not allowed at all or OMAP's IOMMU API should
> change the way it checks if it's fixed da or not.
>
> Kind regards,
>
> David
>
>>
>> --
>> Sakari Ailus
>> sakari.ailus@maxwell.research.nokia.com
>>
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Laurent Pinchart]

Hi David,

On Monday 07 March 2011 22:35:31 David Cohen wrote:
> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote:
> > On Monday 07 March 2011 20:41:21 David Cohen wrote:
> >> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
> >> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
> >> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
> >> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
> >> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00
> >> >>>> 2001 From: Michael Jones <michael.jones@matrix-vision.de>
> >> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
> >> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
> >> >>>> 
> >> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
> >> >>>> the NULL address if da_start==0.  Force da_start to exclude the
> >> >>>> first page.
> >> >>> 
> >> >>> what about devices that uses page 0? ipu after reset always starts
> >> >>> from 0x00000000 how could we map that address??
> >> >> 
> >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
> >> >> want it?
> >> > 
> >> > unlike DSP that you can load a register with the addres the DSP will
> >> > boot, IPU core always starts from address 0x00000000, so if you take
> >> > IPU out of reset it will try to access address 0x0 if not map it,
> >> > there will be a mmu fault.
> >> 
> >> Hm. Looks like the iommu should not restrict any da. The valid da
> >> range should rely only on pdata.
> >> Michael, what about just update ISP's da_start on omap-iommu.c file?
> >> Set it to 0x1000.
> > 
> > What about patching the OMAP3 ISP driver to use a non-zero value (maybe
> > -1) as an invalid/freed pointer ?
> 
> I wouldn't be comfortable to use 0 (or NULL) value as valid address on
> ISP driver.

Why not ? The IOMMUs can use 0x00000000 as a valid address. Whether we allow 
it or not is a software architecture decision, not influenced by the IOMMU 
hardware. As some peripherals (namely IPU) require mapping memory to 
0x00000000, the IOMMU layer must support it and not treat 0x00000000 
specially. All da == 0 checks to aim at catching invalid address values must 
be removed, both from the IOMMU API and the IOMMU internals.

> The 'da' range (da_start and da_end) is defined per VM and specified as
> platform data. IMO, to set da_start = 0x1000 seems to be> a correct approach
> for ISP as it's the only client for its IOMMU instance.

We can do that, and then use 0 as an invalid pointer in the ISP driver. As the 
IOMMU API will use another value (what about 0xffffffff, as for the userspace 
mmap() call ?) to mean "invalid pointer", it might be better to use the same 
value in the ISP driver.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Guzman Lugo, Fernando]

On Tue, Mar 8, 2011 at 2:31 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi David,
>
> On Monday 07 March 2011 22:35:31 David Cohen wrote:
>> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote:
>> > On Monday 07 March 2011 20:41:21 David Cohen wrote:
>> >> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
>> >> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
>> >> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
>> >> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
>> >> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00
>> >> >>>> 2001 From: Michael Jones <michael.jones@matrix-vision.de>
>> >> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>> >> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>> >> >>>>
>> >> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> >> >>>> the NULL address if da_start==0.  Force da_start to exclude the
>> >> >>>> first page.
>> >> >>>
>> >> >>> what about devices that uses page 0? ipu after reset always starts
>> >> >>> from 0x00000000 how could we map that address??
>> >> >>
>> >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
>> >> >> want it?
>> >> >
>> >> > unlike DSP that you can load a register with the addres the DSP will
>> >> > boot, IPU core always starts from address 0x00000000, so if you take
>> >> > IPU out of reset it will try to access address 0x0 if not map it,
>> >> > there will be a mmu fault.
>> >>
>> >> Hm. Looks like the iommu should not restrict any da. The valid da
>> >> range should rely only on pdata.
>> >> Michael, what about just update ISP's da_start on omap-iommu.c file?
>> >> Set it to 0x1000.
>> >
>> > What about patching the OMAP3 ISP driver to use a non-zero value (maybe
>> > -1) as an invalid/freed pointer ?
>>
>> I wouldn't be comfortable to use 0 (or NULL) value as valid address on
>> ISP driver.
>
> Why not ? The IOMMUs can use 0x00000000 as a valid address. Whether we allow
> it or not is a software architecture decision, not influenced by the IOMMU
> hardware. As some peripherals (namely IPU) require mapping memory to
> 0x00000000, the IOMMU layer must support it and not treat 0x00000000
> specially. All da == 0 checks to aim at catching invalid address values must
> be removed, both from the IOMMU API and the IOMMU internals.

Yes, I completely agree with this approach.

Regards,
Fernando.

>
>> The 'da' range (da_start and da_end) is defined per VM and specified as
>> platform data. IMO, to set da_start = 0x1000 seems to be> a correct approach
>> for ISP as it's the only client for its IOMMU instance.
>
> We can do that, and then use 0 as an invalid pointer in the ISP driver. As the
> IOMMU API will use another value (what about 0xffffffff, as for the userspace
> mmap() call ?) to mean "invalid pointer", it might be better to use the same
> value in the ISP driver.
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[David Cohen]

On Tue, Mar 8, 2011 at 10:31 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi David,
>
> On Monday 07 March 2011 22:35:31 David Cohen wrote:
>> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote:
>> > On Monday 07 March 2011 20:41:21 David Cohen wrote:
>> >> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
>> >> > On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
>> >> >> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
>> >> >>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
>> >> >>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00
>> >> >>>> 2001 From: Michael Jones <michael.jones@matrix-vision.de>
>> >> >>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>> >> >>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>> >> >>>>
>> >> >>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>> >> >>>> the NULL address if da_start==0.  Force da_start to exclude the
>> >> >>>> first page.
>> >> >>>
>> >> >>> what about devices that uses page 0? ipu after reset always starts
>> >> >>> from 0x00000000 how could we map that address??
>> >> >>
>> >> >> from 0x0? The driver sees da == 0 as error. May I ask you why do you
>> >> >> want it?
>> >> >
>> >> > unlike DSP that you can load a register with the addres the DSP will
>> >> > boot, IPU core always starts from address 0x00000000, so if you take
>> >> > IPU out of reset it will try to access address 0x0 if not map it,
>> >> > there will be a mmu fault.
>> >>
>> >> Hm. Looks like the iommu should not restrict any da. The valid da
>> >> range should rely only on pdata.
>> >> Michael, what about just update ISP's da_start on omap-iommu.c file?
>> >> Set it to 0x1000.
>> >
>> > What about patching the OMAP3 ISP driver to use a non-zero value (maybe
>> > -1) as an invalid/freed pointer ?
>>
>> I wouldn't be comfortable to use 0 (or NULL) value as valid address on
>> ISP driver.
>
> Why not ? The IOMMUs can use 0x00000000 as a valid address. Whether we allow
> it or not is a software architecture decision, not influenced by the IOMMU
> hardware. As some peripherals (namely IPU) require mapping memory to
> 0x00000000, the IOMMU layer must support it and not treat 0x00000000
> specially. All da == 0 checks to aim at catching invalid address values must
> be removed, both from the IOMMU API and the IOMMU internals.

Yes, it can use and IOMMU should not treat is specially. That's the
aim of my patch:
[PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
I'm not advocating to not allow 0x0, but to not use it when user is
not requesting fixed da. In many sw architecture decisions 0x0 address
is a special case. To avoid any misuse, IOMMU will not use it unless
it's requested. If user is not requesting fixed 'da', it's not a
problem to not give 0x0 anyway. IMO that's the safer option for all
cases.

>
>> The 'da' range (da_start and da_end) is defined per VM and specified as
>> platform data. IMO, to set da_start = 0x1000 seems to be> a correct approach
>> for ISP as it's the only client for its IOMMU instance.
>
> We can do that, and then use 0 as an invalid pointer in the ISP driver. As the
> IOMMU API will use another value (what about 0xffffffff, as for the userspace
> mmap() call ?) to mean "invalid pointer", it might be better to use the same
> value in the ISP driver.

That can be done, of course. But the main point is in OMAP3 ISP all
initial register values to read/write from/to memory are 0x0. It means
sometimes we can catch bugs more easily by not mapping that address.
So, IMO, OMAP3 ISP should not allow to map first page. But that's a
special case for this driver only.

Br,

David

>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH] omap: iommu: disallow mapping NULL address

[Sakari Ailus]

David Cohen wrote:
> On Tue, Mar 8, 2011 at 10:31 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi David,
>>
>> On Monday 07 March 2011 22:35:31 David Cohen wrote:
>>> On Mon, Mar 7, 2011 at 11:19 PM, Laurent Pinchart wrote:
>>>> On Monday 07 March 2011 20:41:21 David Cohen wrote:
>>>>> On Mon, Mar 7, 2011 at 9:25 PM, Guzman Lugo, Fernando wrote:
>>>>>> On Mon, Mar 7, 2011 at 1:19 PM, David Cohen wrote:
>>>>>>> On Mon, Mar 7, 2011 at 9:17 PM, Guzman Lugo, Fernando wrote:
>>>>>>>> On Mon, Mar 7, 2011 at 7:10 AM, Michael Jones wrote:
>>>>>>>>> From e7dbe4c4b64eb114f9b0804d6af3a3ca0e78acc8 Mon Sep 17 00:00:00
>>>>>>>>> 2001 From: Michael Jones <michael.jones@matrix-vision.de>
>>>>>>>>> Date: Mon, 7 Mar 2011 13:36:15 +0100
>>>>>>>>> Subject: [PATCH] omap: iommu: disallow mapping NULL address
>>>>>>>>>
>>>>>>>>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping
>>>>>>>>> the NULL address if da_start==0.  Force da_start to exclude the
>>>>>>>>> first page.
>>>>>>>>
>>>>>>>> what about devices that uses page 0? ipu after reset always starts
>>>>>>>> from 0x00000000 how could we map that address??
>>>>>>>
>>>>>>> from 0x0? The driver sees da == 0 as error. May I ask you why do you
>>>>>>> want it?
>>>>>>
>>>>>> unlike DSP that you can load a register with the addres the DSP will
>>>>>> boot, IPU core always starts from address 0x00000000, so if you take
>>>>>> IPU out of reset it will try to access address 0x0 if not map it,
>>>>>> there will be a mmu fault.
>>>>>
>>>>> Hm. Looks like the iommu should not restrict any da. The valid da
>>>>> range should rely only on pdata.
>>>>> Michael, what about just update ISP's da_start on omap-iommu.c file?
>>>>> Set it to 0x1000.
>>>>
>>>> What about patching the OMAP3 ISP driver to use a non-zero value (maybe
>>>> -1) as an invalid/freed pointer ?
>>>
>>> I wouldn't be comfortable to use 0 (or NULL) value as valid address on
>>> ISP driver.
>>
>> Why not ? The IOMMUs can use 0x00000000 as a valid address. Whether we allow
>> it or not is a software architecture decision, not influenced by the IOMMU
>> hardware. As some peripherals (namely IPU) require mapping memory to
>> 0x00000000, the IOMMU layer must support it and not treat 0x00000000
>> specially. All da == 0 checks to aim at catching invalid address values must
>> be removed, both from the IOMMU API and the IOMMU internals.
> 
> Yes, it can use and IOMMU should not treat is specially. That's the
> aim of my patch:
> [PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag
> I'm not advocating to not allow 0x0, but to not use it when user is
> not requesting fixed da. In many sw architecture decisions 0x0 address
> is a special case. To avoid any misuse, IOMMU will not use it unless
> it's requested. If user is not requesting fixed 'da', it's not a
> problem to not give 0x0 anyway. IMO that's the safer option for all
> cases.

I agree.

>>> The 'da' range (da_start and da_end) is defined per VM and specified as
>>> platform data. IMO, to set da_start = 0x1000 seems to be> a correct approach
>>> for ISP as it's the only client for its IOMMU instance.
>>
>> We can do that, and then use 0 as an invalid pointer in the ISP driver. As the
>> IOMMU API will use another value (what about 0xffffffff, as for the userspace
>> mmap() call ?) to mean "invalid pointer", it might be better to use the same
>> value in the ISP driver.
> 
> That can be done, of course. But the main point is in OMAP3 ISP all
> initial register values to read/write from/to memory are 0x0. It means
> sometimes we can catch bugs more easily by not mapping that address.
> So, IMO, OMAP3 ISP should not allow to map first page. But that's a
> special case for this driver only.

I beg to disagree. The ISP isn't so special. The hardware registers
(including DMA destination registers) typically are NULL after reset and
NULL is used by drivers to mark a nonexistent object, for example a
video buffer.

There's a reason why the first page isn't mapped in the system MMU either.

Regards,

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