[PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Hiroshi DOYU]

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 arch/arm/mm/ioremap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index b81dbf9..9ad7614 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -161,7 +161,7 @@ static void unmap_area_sections(unsigned long virt, unsigned long size)
 			/*
 			 * Free the page table, if there was one.
 			 */
-			if ((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_TABLE)
+			if ((pmd_table(pmd))
 				pte_free_kernel(&init_mm, pmd_page_vaddr(pmd));
 		}
 
-- 
1.5.5.1.357.g1af8b

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Tony Lindgren]

* Hiroshi DOYU <Hiroshi.DOYU@nokia.com> [080902 22:37]:
> Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>

This should be sent to linux-arm-kernel list.

Tony

> ---
>  arch/arm/mm/ioremap.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index b81dbf9..9ad7614 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -161,7 +161,7 @@ static void unmap_area_sections(unsigned long virt, unsigned long size)
>  			/*
>  			 * Free the page table, if there was one.
>  			 */
> -			if ((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_TABLE)
> +			if ((pmd_table(pmd))
>  				pte_free_kernel(&init_mm, pmd_page_vaddr(pmd));
>  		}
>  
> -- 
> 1.5.5.1.357.g1af8b
> 
> --
> 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

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Russell King - ARM Linux]

On Fri, Sep 05, 2008 at 10:21:33AM -0700, Tony Lindgren wrote:
> * Hiroshi DOYU <Hiroshi.DOYU@nokia.com> [080902 22:37]:
> > Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> 
> This should be sent to linux-arm-kernel list.

Except, pmd_table() is an omap thing at the moment, so isn't relevant
there.

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Hiroshi DOYU]

Hi Russell,

From: "ext Russell King - ARM Linux" <linux@arm.linux.org.uk>
Subject: Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()
Date: Fri, 5 Sep 2008 23:45:30 +0100

> On Fri, Sep 05, 2008 at 10:21:33AM -0700, Tony Lindgren wrote:
> > * Hiroshi DOYU <Hiroshi.DOYU@nokia.com> [080902 22:37]:
> > > Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> > 
> > This should be sent to linux-arm-kernel list.
> 
> Except, pmd_table() is an omap thing at the moment, so isn't relevant
> there.

Right.
Would it be possible to have "pmd_table()" definition "in pgtable.h"?

>From 0374c2c3e873a43e26cb439b891b9e319b90f83c Mon Sep 17 00:00:00 2001
From: ext Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
Date: Wed, 3 Sep 2008 08:36:28 +0300
Subject: [PATCH 1/1] Add pmd_table() MACRO

Use pmd_table() MACRO for unmap_area_sections()

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 arch/arm/include/asm/pgtable.h |    1 +
 arch/arm/mm/ioremap.c          |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 8e21ef1..acfac1a 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -292,6 +292,7 @@ static inline pte_t pte_mkspecial(pte_t pte) { return pte; }
 #define pmd_none(pmd)		(!pmd_val(pmd))
 #define pmd_present(pmd)	(pmd_val(pmd))
 #define pmd_bad(pmd)		(pmd_val(pmd) & 2)
+#define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_TABLE)
 
 #define copy_pmd(pmdpd,pmdps)		\
 	do {				\
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index b81dbf9..7ab4301 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -161,7 +161,7 @@ static void unmap_area_sections(unsigned long virt, unsigned long size)
 			/*
 			 * Free the page table, if there was one.
 			 */
-			if ((pmd_val(pmd) & PMD_TYPE_MASK) == PMD_TYPE_TABLE)
+			if (pmd_table(pmd))
 				pte_free_kernel(&init_mm, pmd_page_vaddr(pmd));
 		}
 
-- 
1.5.5.1.357.g1af8b

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Russell King - ARM Linux]

On Sat, Sep 06, 2008 at 03:56:35AM +0300, Hiroshi DOYU wrote:
> Would it be possible to have "pmd_table()" definition "in pgtable.h"?

Well, the first question is... where has this come from, and why.

Looking at the omap tree, it's because of arch/arm/plat-omap/mmu.c,
which seems to be somewhat weird and complex, duplicating some of
what's going on elsewhere in the kernel.

(And with the changes I've just queued up for the next mainline merge
window, this file will probably break because of it's use of the kernel
page table macros.)

A more portable way to do that would be to split out the innards of
__arm_ioremap_pfn() into a separate function, and use that instead
of exmap_set_armmmu().  Then, replace exmap_clear_armmmu() with
unmap_area_sections() to tear down the ARM side of the mapping.

As far as 'mmu->twl_mm' goes, the only reason I can see for that
existing is as a way to use the kernel's page table accessors to
manipulate something that isn't used for as the CPU's page table,
which in turn leads to quite a number of nasty hacks in the code.
Really that wants to use its own accessors and macros, and twl_mm
eliminated.  A mm_struct isn't small.  And by doing so, you'll
avoid the bug and memory leak in omap_mmu_register().  kfree'ing
(!) a mm_struct onto a different slab cache from whence it came!

Which probably will mean that there ends up only being one place
where the construct which pmd_table() is replacing exists.

RE: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Woodruff, Richard]

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Russell King - ARM Linux

> Well, the first question is... where has this come from, and why.

In OMAPs, a few devices have a local IO MMU.  Their basic structure is similar to an ARM MMU, but less featured. IVA & Camera have one each on 3430/2430, a 2420 has those before mentioned and + 3rd in its video accelerator.

The usage in TI kernels has varied a bit.  Some times they are just load and lock TLB's, for others table walking can be enabled.  In all cases if a table walk miss happens it is fatal and the IP block device will have to be reset.

Paul Mundt had done the linux-omap tree version used with dspgateway.

Regards,
Richard W.

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Hiroshi DOYU]

Hi Richard,

From: "ext Woodruff, Richard" <r-woodruff2@ti.com>
Subject: RE: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()
Date: Sun, 7 Sep 2008 21:04:56 -0500

> 
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Russell King - ARM Linux
> 
> > Well, the first question is... where has this come from, and why.
> 
> In OMAPs, a few devices have a local IO MMU.  Their basic structure
> is similar to an ARM MMU, but less featured. IVA & Camera have one
> each on 3430/2430, a 2420 has those before mentioned and + 3rd in
> its video accelerator.
> 
> The usage in TI kernels has varied a bit.  Some times they are just
> load and lock TLB's, for others table walking can be enabled.  In
> all cases if a table walk miss happens it is fatal and the IP block
> device will have to be reset.

In *theory*, from this IOMMU hardware perspective, it's feasible to
load something on memory dynamically("on-demand loading") when a table
walk miss happens by notifying it to some memory manager through
its assigned interrupt. Practically this latency may be a problem in
some cases;)

   Hiroshi DOYU

RE: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Woodruff, Richard]

Hi,

> > The usage in TI kernels has varied a bit.  Some times they are just
> > load and lock TLB's, for others table walking can be enabled.  In
> > all cases if a table walk miss happens it is fatal and the IP block
> > device will have to be reset.
>
> In *theory*, from this IOMMU hardware perspective, it's feasible to
> load something on memory dynamically("on-demand loading") when a table
> walk miss happens by notifying it to some memory manager through
> its assigned interrupt. Practically this latency may be a problem in
> some cases;)

I've been told explicitly by a few people that a table miss is fatal and the block needs to be reset to recover state.

I've not personally dug into TRM and underlying functional specs to validate this.  It might be different per IP.  Extreme case some kind of checkpoint would work ;)

In some discussions some people have even taken in table walk time as a consideration if certain camera streaming cases will work.  So, your comment about latency seems on.

Regards,
Richard W.

Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()

[Hiroshi DOYU]

Hi Russell, thank you for reviewing.

From: "ext Russell King - ARM Linux" <linux@arm.linux.org.uk>
Subject: Re: [PATCH 1/1] Use pmd_table() MACRO for unmap_area_sections()
Date: Sun, 7 Sep 2008 20:36:20 +0100

> On Sat, Sep 06, 2008 at 03:56:35AM +0300, Hiroshi DOYU wrote:
> > Would it be possible to have "pmd_table()" definition "in pgtable.h"?
> 
> Well, the first question is... where has this come from, and why.
> 
> Looking at the omap tree, it's because of arch/arm/plat-omap/mmu.c,
> which seems to be somewhat weird and complex, duplicating some of
> what's going on elsewhere in the kernel.

I totally agree on this and this should be refactored anyway.

> (And with the changes I've just queued up for the next mainline merge
> window, this file will probably break because of it's use of the kernel
> page table macros.)

OK, I will drop it and see how mainline merge affects.

> A more portable way to do that would be to split out the innards of
> __arm_ioremap_pfn() into a separate function, and use that instead
> of exmap_set_armmmu().  Then, replace exmap_clear_armmmu() with
> unmap_area_sections() to tear down the ARM side of the mapping.

I got it. This code which keeps consistency with ARM MMU should be
replaced completedly by them.

> As far as 'mmu->twl_mm' goes, the only reason I can see for that
> existing is as a way to use the kernel's page table accessors to
> manipulate something that isn't used for as the CPU's page table,
> which in turn leads to quite a number of nasty hacks in the code.
> Really that wants to use its own accessors and macros, and twl_mm
> eliminated.  A mm_struct isn't small.  And by doing so, you'll
> avoid the bug and memory leak in omap_mmu_register().  kfree'ing
> (!) a mm_struct onto a different slab cache from whence it came!

Apart from the status of current "plat-omap/mmu.c", I cannot deny
completely the possibilty of applying kernel generic APIs to this
IOMMU driver in more cleaner manner because OMAP IOMMU is too
similar to the generic MMU handling and ARM MMU code are designed
quite flexible to afford the different versions of MMUs. I'm making a
reworking patches of this IOMMU. So if possible, please take a look at
them later.

  Hiroshi DOYU