[PATCH] x86: ioremap ram check fix

[PATCH] x86: ioremap ram check fix

[Andres Salomon]

Hi Ingo,

bdd3cee2e4b7279457139058615ced6c2b41e7de (x86: ioremap(), extend check
to all RAM pages) breaks OLPC's ioremap call.  The ioremap that OLPC uses is:

        romsig = ioremap(0xffffffc0, 16);

The commit that breaks it is basically:

-       for (pfn = phys_addr >> PAGE_SHIFT; pfn < max_pfn_mapped &&
-            (pfn << PAGE_SHIFT) < last_addr; pfn++) {
+       for (pfn = phys_addr >> PAGE_SHIFT;
+                               (pfn << PAGE_SHIFT) < last_addr; pfn++) {
+

Previously, the 'pfn < max_pfn_mapped' check would've caused us to not
enter the loop.  Removing that check means we loop infinitely.  The
reason for that is because pfn is 0xfffff, and last_addr is 0xffffffcf.
The remaining check that is used to exit the loop is not sufficient;
when pfn<<PAGE_SHIFT is 0xfffff000, that is less than 0xffffffcf; when
we increment pfn and it overflows (pfn == 0x100000), pfn<<PAGE_SHIFT
ends up being 0.  That, of course, is less than last_addr.  In effect,
pfn<<PAGE_SHIFT is never lower than last_addr.

The simple fix for this is to limit the last_addr check to the PAGE_MASK;
a patch is below.




Signed-off-by: Andres Salomon <dilinger@debian.org>
---
 arch/x86/mm/ioremap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 804de18..fb960f5 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -149,7 +149,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
 	 * Don't allow anybody to remap normal RAM that we're using..
 	 */
 	for (pfn = phys_addr >> PAGE_SHIFT;
-				(pfn << PAGE_SHIFT) < last_addr; pfn++) {
+				(pfn << PAGE_SHIFT) < (last_addr & PAGE_MASK);
+				pfn++) {
 
 		int is_ram = page_is_ram(pfn);
 
-- 
1.5.5

Re: [PATCH] x86: ioremap ram check fix

[Vegard Nossum]

On Wed, Apr 30, 2008 at 5:30 PM, Andres Salomon <dilinger@queued.net> wrote:
> Hi Ingo,
>
>  bdd3cee2e4b7279457139058615ced6c2b41e7de (x86: ioremap(), extend check
>  to all RAM pages) breaks OLPC's ioremap call.  The ioremap that OLPC uses is:
>
>         romsig = ioremap(0xffffffc0, 16);
>
>  The commit that breaks it is basically:
>
>  -       for (pfn = phys_addr >> PAGE_SHIFT; pfn < max_pfn_mapped &&
>  -            (pfn << PAGE_SHIFT) < last_addr; pfn++) {
>  +       for (pfn = phys_addr >> PAGE_SHIFT;
>  +                               (pfn << PAGE_SHIFT) < last_addr; pfn++) {
>  +
>
>  Previously, the 'pfn < max_pfn_mapped' check would've caused us to not
>  enter the loop.  Removing that check means we loop infinitely.  The
>  reason for that is because pfn is 0xfffff, and last_addr is 0xffffffcf.
>  The remaining check that is used to exit the loop is not sufficient;
>  when pfn<<PAGE_SHIFT is 0xfffff000, that is less than 0xffffffcf; when
>  we increment pfn and it overflows (pfn == 0x100000), pfn<<PAGE_SHIFT
>  ends up being 0.  That, of course, is less than last_addr.  In effect,
>  pfn<<PAGE_SHIFT is never lower than last_addr.
>
>  The simple fix for this is to limit the last_addr check to the PAGE_MASK;
>  a patch is below.
>
>
>
>
>  Signed-off-by: Andres Salomon <dilinger@debian.org>
>  ---
>   arch/x86/mm/ioremap.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
>  diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>  index 804de18..fb960f5 100644
>  --- a/arch/x86/mm/ioremap.c
>  +++ b/arch/x86/mm/ioremap.c
>  @@ -149,7 +149,8 @@ static void __iomem *__ioremap_caller(resource_size_t phys_addr,
>          * Don't allow anybody to remap normal RAM that we're using..
>          */
>         for (pfn = phys_addr >> PAGE_SHIFT;
>  -                               (pfn << PAGE_SHIFT) < last_addr; pfn++) {
>  +                               (pfn << PAGE_SHIFT) < (last_addr & PAGE_MASK);
>  +                               pfn++) {
>
>                 int is_ram = page_is_ram(pfn);
>

This fixes two 150-second solid freezes during bootup on a P4 I have
as well. Acked!

Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH] x86: ioremap ram check fix

[Ingo Molnar]

* Andres Salomon <dilinger@queued.net> wrote:

> Hi Ingo,
> 
> bdd3cee2e4b7279457139058615ced6c2b41e7de (x86: ioremap(), extend check 
> to all RAM pages) breaks OLPC's ioremap call.  The ioremap that OLPC 
> uses is:

that breakage has been reported independently as well and the revert has 
been pushed to Linus yesterday - it's not upstream yet. See this commit 
in x86.git:

 commit 1b8104a0ec138de829bb351f6597d534c7c134dc
 Author: Ingo Molnar <mingo@elte.hu>
 Date:   Tue Apr 29 12:04:51 2008 +0200

    revert: "x86: ioremap(), extend check to all RAM pages"

    Vegard Nossum reported a large (150 seconds) boot delay during bootup,
    and bisected it to "x86: ioremap(), extend check to all RAM pages"
    (commit bdd3cee2e4b). Revert this commit for now.

    Bisected-by: Vegard Nossum <vegard.nossum@gmail.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] x86: ioremap ram check fix

[Andres Salomon]

On Wed, 30 Apr 2008 20:10:15 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andres Salomon <dilinger@queued.net> wrote:
> 
> > Hi Ingo,
> > 
> > bdd3cee2e4b7279457139058615ced6c2b41e7de (x86: ioremap(), extend check 
> > to all RAM pages) breaks OLPC's ioremap call.  The ioremap that OLPC 
> > uses is:
> 
> that breakage has been reported independently as well and the revert has 
> been pushed to Linus yesterday - it's not upstream yet. See this commit 
> in x86.git:
> 

Sure, either way.  FWIW, Vegard reported that the PAGE_MASK change made
the 150s delay disappear.



>  commit 1b8104a0ec138de829bb351f6597d534c7c134dc
>  Author: Ingo Molnar <mingo@elte.hu>
>  Date:   Tue Apr 29 12:04:51 2008 +0200
> 
>     revert: "x86: ioremap(), extend check to all RAM pages"
> 
>     Vegard Nossum reported a large (150 seconds) boot delay during bootup,
>     and bisected it to "x86: ioremap(), extend check to all RAM pages"
>     (commit bdd3cee2e4b). Revert this commit for now.
> 
>     Bisected-by: Vegard Nossum <vegard.nossum@gmail.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 	Ingo

Re: [PATCH] x86: ioremap ram check fix

[Ingo Molnar]

* Andres Salomon <dilinger@queued.net> wrote:

> > that breakage has been reported independently as well and the revert 
> > has been pushed to Linus yesterday - it's not upstream yet. See this 
> > commit in x86.git:
> 
> Sure, either way.  FWIW, Vegard reported that the PAGE_MASK change 
> made the 150s delay disappear.

sorry, i forgot to mention that your fix is the right one [it actually 
fixes the problem instead of reverting the change] so i've queued that 
up too :-) Thanks for fixing this!

	Ingo