[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Pawel Moll]

Since commit 576d2f2525612ecb5af029a76f21f22a3b82563d "ARM: add
generic ioremap optimization by reusing static mappings" ioremap()
is trying to reuse existing, static mapping when possible.

The condition checking boundaries of the requested and existing
mappings didn't take in-page offset into consideration though,
which lead to obscure and hard to debug problems when.

Cc: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 arch/arm/mm/ioremap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 80632e8..ba15937 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -225,7 +225,8 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
 			continue;
 		if (__phys_to_pfn(area->phys_addr) > pfn ||
-		    __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
+		    __pfn_to_phys(pfn) + offset + size-1 >
+		    area->phys_addr + area->size-1)
 			continue;
 		/* we can drop the lock here as we know *area is static */
 		read_unlock(&vmlist_lock);
-- 
1.7.5.4

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Nicolas Pitre]

On Wed, 25 Jan 2012, Pawel Moll wrote:

> Since commit 576d2f2525612ecb5af029a76f21f22a3b82563d "ARM: add
> generic ioremap optimization by reusing static mappings" ioremap()
> is trying to reuse existing, static mapping when possible.
> 
> The condition checking boundaries of the requested and existing
> mappings didn't take in-page offset into consideration though,
> which lead to obscure and hard to debug problems when.

... when what?

Good catch nevertheless.

> Cc: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Acked-by: Nicolas Pitre <nico@linaro.org>

> ---
>  arch/arm/mm/ioremap.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 80632e8..ba15937 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -225,7 +225,8 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
>  			continue;
>  		if (__phys_to_pfn(area->phys_addr) > pfn ||
> -		    __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
> +		    __pfn_to_phys(pfn) + offset + size-1 >
> +		    area->phys_addr + area->size-1)
>  			continue;
>  		/* we can drop the lock here as we know *area is static */
>  		read_unlock(&vmlist_lock);
> -- 
> 1.7.5.4
> 
> 

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Pawel Moll]

On Wed, 2012-01-25 at 18:37 +0000, Nicolas Pitre wrote:
> On Wed, 25 Jan 2012, Pawel Moll wrote:
> 
> > Since commit 576d2f2525612ecb5af029a76f21f22a3b82563d "ARM: add
> > generic ioremap optimization by reusing static mappings" ioremap()
> > is trying to reuse existing, static mapping when possible.
> > 
> > The condition checking boundaries of the requested and existing
> > mappings didn't take in-page offset into consideration though,
> > which lead to obscure and hard to debug problems when.
> 
> ... when what?

Apparently I was hungry and ate the second half of the sentence ;-)

... when requested mapping crosses end of the static one.

Cheers!

Pawe?

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Joachim Eastwood]

Hi all,

On Thu, Jan 26, 2012 at 11:35 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2012-01-25 at 18:37 +0000, Nicolas Pitre wrote:
>> On Wed, 25 Jan 2012, Pawel Moll wrote:
>>
>> > Since commit 576d2f2525612ecb5af029a76f21f22a3b82563d "ARM: add
>> > generic ioremap optimization by reusing static mappings" ioremap()
>> > is trying to reuse existing, static mapping when possible.
>> >
>> > The condition checking boundaries of the requested and existing
>> > mappings didn't take in-page offset into consideration though,
>> > which lead to obscure and hard to debug problems when.
>>
>> ... when what?
>
> Apparently I was hungry and ate the second half of the sentence ;-)
>
> ... when requested mapping crosses end of the static one.
>

"ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master

Breaks booting on my custom AT91RM9200 board.
There isn't any error messages or anything that indicates what goes
wrong it just stops after; Uncompressing Linux... done, booting the
kernel.

Reverting it makes my board boot again.

regards
Joachim Eastwood

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Russell King - ARM Linux]

On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
> 
> Breaks booting on my custom AT91RM9200 board.
> There isn't any error messages or anything that indicates what goes
> wrong it just stops after; Uncompressing Linux... done, booting the
> kernel.

Your best way of finding out what's going on is to enable:

DEBUG_KERNEL
DEBUG_LL
EARLY_PRINTK

and select the appropriate kernel low-level debugging port.  That should
show you what's going on.

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Joachim Eastwood]

On Sun, Jan 29, 2012 at 1:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
>> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
>> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
>>
>> Breaks booting on my custom AT91RM9200 board.
>> There isn't any error messages or anything that indicates what goes
>> wrong it just stops after; Uncompressing Linux... done, booting the
>> kernel.
>
> Your best way of finding out what's going on is to enable:
>
> DEBUG_KERNEL
> DEBUG_LL
> EARLY_PRINTK
>
> and select the appropriate kernel low-level debugging port. ?That should
> show you what's going on.

Sadly, it doesn't make a difference. Still no output.

grep -E '(DEBUG_KERNEL|DEBUG_LL|EARLY_PRINTK)' .config
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_LL=y
# CONFIG_DEBUG_LL_UART_NONE is not set
CONFIG_AT91_DEBUG_LL_DBGU0=y
CONFIG_EARLY_PRINTK=y

I have a call to ioremap in my custom board setup, but removing it
didn't make a difference either. So I guess other at91 boards should
be affected as well. My board is not that different from rn9200ek.

regards
Joachim Eastwood

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Russell King - ARM Linux]

On Sun, Jan 29, 2012 at 02:10:34PM +0100, Joachim Eastwood wrote:
> On Sun, Jan 29, 2012 at 1:11 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
> >> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
> >> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
> >>
> >> Breaks booting on my custom AT91RM9200 board.
> >> There isn't any error messages or anything that indicates what goes
> >> wrong it just stops after; Uncompressing Linux... done, booting the
> >> kernel.
> >
> > Your best way of finding out what's going on is to enable:
> >
> > DEBUG_KERNEL
> > DEBUG_LL
> > EARLY_PRINTK
> >
> > and select the appropriate kernel low-level debugging port. ?That should
> > show you what's going on.
> 
> Sadly, it doesn't make a difference. Still no output.

Sorry, I missed that you also need 'earlyprintk' or something like that
on the command line.  Alternatively, use the following patch, which IMHO
is a lot less error prone:

diff --git a/kernel/printk.c b/kernel/printk.c
index b51b156..611e495 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -875,7 +875,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
 	/* Emit the output into the temporary buffer */
 	printed_len += vscnprintf(printk_buf + printed_len,
 				  sizeof(printk_buf) - printed_len, fmt, args);
-
+{ extern void printascii(const char *); printascii(printk_buf); }
 	p = printk_buf;
 
 	/* Read log level and handle special printk prefix */

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Joachim Eastwood]

On Sun, Jan 29, 2012 at 2:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jan 29, 2012 at 02:10:34PM +0100, Joachim Eastwood wrote:
>> On Sun, Jan 29, 2012 at 1:11 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
>> >> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
>> >> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
>> >>
>> >> Breaks booting on my custom AT91RM9200 board.
>> >> There isn't any error messages or anything that indicates what goes
>> >> wrong it just stops after; Uncompressing Linux... done, booting the
>> >> kernel.
>> >
>> > Your best way of finding out what's going on is to enable:
>> >
>> > DEBUG_KERNEL
>> > DEBUG_LL
>> > EARLY_PRINTK
>> >
>> > and select the appropriate kernel low-level debugging port. ?That should
>> > show you what's going on.
>>
>> Sadly, it doesn't make a difference. Still no output.
>
> Sorry, I missed that you also need 'earlyprintk' or something like that
> on the command line. ?Alternatively, use the following patch, which IMHO
> is a lot less error prone:

<snip patch>

The patch did the job. Thanks.

Here is output:
Uncompressing Linux... done, booting the kernel.
<6>Booting Linux on physical CPU 0
<5>Linux version 3.3.0-rc1-mpa+ (subcon at archspace) (gcc version 4.3.3
(GCC) ) #179 PREEMPT Sun Jan 29 14:18:29 CET 2012
CPU: ARM920T [41129200] revision 0 (ARMv4T), cr=c0007177
CPU: VIVT data cache, VIVT instruction cache
Machine: Phontech MPA 1600
Memory policy: ECC disabled, Data cache writeback
<6>AT91: Detected soc type: at91rm9200
<6>AT91: Detected soc subtype: Unknown
<6>AT91: sram at 0x200000 of 0x4000 mapped at 0xfef74000
<7>On node 0 totalpages: 16384
<7>free_area_init_node: node 0, pgdat c02c4e3c, node_mem_map c02d6000
<7>  Normal zone: 128 pages used for memmap
<7>  Normal zone: 0 pages reserved
<7>  Normal zone: 16256 pages, LIFO batch:3
<6>AT91: filled in soc subtype: at91rm9200 PQFP
Clocks: CPU 179 MHz, master 59 MHz, main 18.432 MHz
<6>gpiochip_add: registered GPIOs 0 to 31 on device: pioA
<6>gpiochip_add: registered GPIOs 32 to 63 on device: pioB
<6>gpiochip_add: registered GPIOs 64 to 95 on device: pioC
<1>Unable to handle kernel paging request at virtual address fed73444
<1>pgd = c0004000
<1>[fed73444] *pgd=00000000
<0>Internal error: Oops: 805 [#1] PREEMPT
<d>Modules linked in:
CPU: 0    Not tainted  (3.3.0-rc1-mpa+ #179)
PC is at at91_set_A_periph+0x50/0x78
LR is at at91_register_uart+0x54/0x1e8
pc : [<c0010bf0>]    lr : [<c0293528>]    psr: 600000d3
sp : c02abf50  ip : fed73400  fp : c02abf5c
r10: 80000200  r9 : c0258fe4  r8 : c02bc594
r7 : c02a34b4  r6 : c02ae848  r5 : 00000000  r4 : 00000000
r3 : 00000060  r2 : 40000000  r1 : 00000000  r0 : 0000001e
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: c000717f  Table: 20004000  DAC: 00000017
<0>Process swapper (pid: 0, stack limit = 0xc02aa270)
<0>Stack: (0xc02abf50 to 0xc02ac000)
<0>bf40:                                     c02abf74 c02abf60 c0293528 c0010bb0
<0>bf60: c0356220 c02cc5a0 c02abf84 c02abf78 c0293b34 c02934e4 c02abfc4 c02abf88
<0>bf80: c02901a4 c0293b18 c0121c3c c0007177 41129200 c02a433c c02abfb4 00000001
<0>bfa0: c02ac010 c02a4b40 c02ae7dc 20004000 41129200 202a3480 c02abff4 c02abfc8
<0>bfc0: c028d624 c028fb4c 00000000 00000000 00000000 c02a473c c0007175 c02ac010
<0>bfe0: c02a4b40 c02ae7dc 00000000 c02abff8 20008040 c028d5c0 00000000 00000000
Backtrace:
[<c0010ba0>] (at91_set_A_periph+0x0/0x78) from [<c0293528>]
(at91_register_uart+0x54/0x1e8)
[<c02934d4>] (at91_register_uart+0x0/0x1e8) from [<c0293b34>]
(mpa1600_init_early+0x2c/0x4c)
 r5:c02cc5a0 r4:c0356220
[<c0293b08>] (mpa1600_init_early+0x0/0x4c) from [<c02901a4>]
(setup_arch+0x668/0x76c)
[<c028fb3c>] (setup_arch+0x0/0x76c) from [<c028d624>] (start_kernel+0x74/0x318)
[<c028d5b0>] (start_kernel+0x0/0x318) from [<20008040>] (0x20008040)
 r7:c02ae7dc r6:c02a4b40 r5:c02ac010 r4:c0007175
<0>Code: e1a02312 e3510000 13a03064 03a03060 (e58c2044)
<4>---[ end trace 1b75b31a2719ed1c ]---
<0>Kernel panic - not syncing: Attempted to kill the idle task!

regards
Joachim Eastwood

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Russell King - ARM Linux]

On Sun, Jan 29, 2012 at 02:22:17PM +0100, Joachim Eastwood wrote:
> On Sun, Jan 29, 2012 at 2:14 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Jan 29, 2012 at 02:10:34PM +0100, Joachim Eastwood wrote:
> >> On Sun, Jan 29, 2012 at 1:11 AM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >> > On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
> >> >> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
> >> >> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
> >> >>
> >> >> Breaks booting on my custom AT91RM9200 board.
> >> >> There isn't any error messages or anything that indicates what goes
> >> >> wrong it just stops after; Uncompressing Linux... done, booting the
> >> >> kernel.
> >> >
> >> > Your best way of finding out what's going on is to enable:
> >> >
> >> > DEBUG_KERNEL
> >> > DEBUG_LL
> >> > EARLY_PRINTK
> >> >
> >> > and select the appropriate kernel low-level debugging port. ?That should
> >> > show you what's going on.
> >>
> >> Sadly, it doesn't make a difference. Still no output.
> >
> > Sorry, I missed that you also need 'earlyprintk' or something like that
> > on the command line. ?Alternatively, use the following patch, which IMHO
> > is a lot less error prone:
> 
> <snip patch>
> 
> The patch did the job. Thanks.

Thanks - digging through the AT91 code I can't see how this has happened.
So, one more patch to add please:

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ba15937..d2514fb 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -217,6 +217,8 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	 * Try to reuse one of the static mapping whenever possible.
 	 */
 	read_lock(&vmlist_lock);
+printk("ioremap: pfn=%lx phys=%lx offset=%lx size=%lx\n",
+	pfn, __pfn_to_phys(pfn), offset, size);
 	for (area = vmlist; area; area = area->next) {
 		if (!size || (sizeof(phys_addr_t) == 4 && pfn >= 0x100000))
 			break;
@@ -224,6 +226,8 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 			continue;
 		if ((area->flags & VM_ARM_MTYPE_MASK) != VM_ARM_MTYPE(mtype))
 			continue;
+printk("ioremap: area %p: phys_addr=%lx pfn=%lx size=%lx\n", area,
+	area->phys_addr, __phys_to_pfn(area->phys_addr), area->size);
 		if (__phys_to_pfn(area->phys_addr) > pfn ||
 		    __pfn_to_phys(pfn) + offset + size-1 >
 		    area->phys_addr + area->size-1)
@@ -232,6 +236,7 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 		read_unlock(&vmlist_lock);
 		addr = (unsigned long)area->addr;
 		addr += __pfn_to_phys(pfn) - area->phys_addr;
+printk("ioremap: found: addr %p => %lx => %lx\n", area->addr, addr, offset + addr);
 		return (void __iomem *) (offset + addr);
 	}
 	read_unlock(&vmlist_lock);

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Joachim Eastwood]

On Sun, Jan 29, 2012 at 3:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jan 29, 2012 at 02:22:17PM +0100, Joachim Eastwood wrote:
>> On Sun, Jan 29, 2012 at 2:14 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Sun, Jan 29, 2012 at 02:10:34PM +0100, Joachim Eastwood wrote:
>> >> On Sun, Jan 29, 2012 at 1:11 AM, Russell King - ARM Linux
>> >> <linux@arm.linux.org.uk> wrote:
>> >> > On Sat, Jan 28, 2012 at 11:55:19PM +0100, Joachim Eastwood wrote:
>> >> >> "ARM: 7304/1: ioremap: fix boundary check when reusing static mapping"
>> >> >> Commit: 3c424f359898aff48c3d5bed608ac706f8a528c3 in Linus master
>> >> >>
>> >> >> Breaks booting on my custom AT91RM9200 board.
>> >> >> There isn't any error messages or anything that indicates what goes
>> >> >> wrong it just stops after; Uncompressing Linux... done, booting the
>> >> >> kernel.
>> >> >
>> >> > Your best way of finding out what's going on is to enable:
>> >> >
>> >> > DEBUG_KERNEL
>> >> > DEBUG_LL
>> >> > EARLY_PRINTK
>> >> >
>> >> > and select the appropriate kernel low-level debugging port. ?That should
>> >> > show you what's going on.
>> >>
>> >> Sadly, it doesn't make a difference. Still no output.
>> >
>> > Sorry, I missed that you also need 'earlyprintk' or something like that
>> > on the command line. ?Alternatively, use the following patch, which IMHO
>> > is a lot less error prone:
>>
>> <snip patch>
>>
>> The patch did the job. Thanks.
>
> Thanks - digging through the AT91 code I can't see how this has happened.
> So, one more patch to add please:

Sure.

<snip patch>

Output w/patch:
<6>Booting Linux on physical CPU 0
<5>Linux version 3.3.0-rc1-mpa+ (subcon at archspace) (gcc version 4.3.3
(GCC) ) #180 PREEMPT Sun Jan 29 15:29:41 CET 2012
CPU: ARM920T [41129200] revision 0 (ARMv4T), cr=c0007177
CPU: VIVT data cache, VIVT instruction cache
Machine: Phontech MPA 1600
Memory policy: ECC disabled, Data cache writeback
<6>AT91: Detected soc type: at91rm9200
<6>AT91: Detected soc subtype: Unknown
<6>AT91: sram at 0x200000 of 0x4000 mapped at 0xfef74000
<7>On node 0 totalpages: 16384
<7>free_area_init_node: node 0, pgdat c02c515c, node_mem_map c02d6000
<7>  Normal zone: 128 pages used for memmap
<7>  Normal zone: 0 pages reserved
<7>  Normal zone: 16256 pages, LIFO batch:3
<6>AT91: filled in soc subtype: at91rm9200 PQFP
Clocks: CPU 179 MHz, master 59 MHz, main 18.432 MHz
<1>ioremap: pfn=fffff phys=fffff000 offset=400 size=1000
<1>ioremap: area c3ffdfc0: phys_addr=200000 pfn=200 size=4000
<1>ioremap: found: addr fef74000 => fed73000 => fed73400
<6>gpiochip_add: registered GPIOs 0 to 31 on device: pioA
<1>ioremap: pfn=fffff phys=fffff000 offset=600 size=1000
<1>ioremap: area c3ffdfc0: phys_addr=200000 pfn=200 size=4000
<1>ioremap: found: addr fef74000 => fed73000 => fed73600
<6>gpiochip_add: registered GPIOs 32 to 63 on device: pioB
<1>ioremap: pfn=fffff phys=fffff000 offset=800 size=1000
<1>ioremap: area c3ffdfc0: phys_addr=200000 pfn=200 size=4000
<1>ioremap: found: addr fef74000 => fed73000 => fed73800
<6>gpiochip_add: registered GPIOs 64 to 95 on device: pioC
<1>Unable to handle kernel paging request at virtual address fed73444
<1>pgd = c0004000
<1>[fed73444] *pgd=00000000
<0>Internal error: Oops: 805 [#1] PREEMPT
<d>Modules linked in:
CPU: 0    Not tainted  (3.3.0-rc1-mpa+ #180)
PC is at at91_set_A_periph+0x50/0x78
LR is at at91_register_uart+0x54/0x1e8
pc : [<c0010c50>]    lr : [<c0293528>]    psr: 600000d3
sp : c02abf50  ip : fed73400  fp : c02abf5c
r10: 80000200  r9 : c0258fe4  r8 : c02bc8b4
r7 : c02a3570  r6 : c02ae848  r5 : 00000000  r4 : 00000000
r3 : 00000060  r2 : 40000000  r1 : 00000000  r0 : 0000001e
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: c000717f  Table: 20004000  DAC: 00000017
<0>Process swapper (pid: 0, stack limit = 0xc02aa270)
<0>Stack: (0xc02abf50 to 0xc02ac000)
<0>bf40:                                     c02abf74 c02abf60 c0293528 c0010c10
<0>bf60: c0356220 c02cc8c0 c02abf84 c02abf78 c0293bf0 c02934e4 c02abfc4 c02abf88
<0>bf80: c02901a4 c0293bd4 c0121c9c c0007177 41129200 c02a43f4 c02abfb4 00000001
<0>bfa0: c02ac010 c02a4bf8 c02ae7dc 20004000 41129200 202a353c c02abff4 c02abfc8
<0>bfc0: c028d624 c028fb4c 00000000 00000000 00000000 c02a47f4 c0007175 c02ac010
<0>bfe0: c02a4bf8 c02ae7dc 00000000 c02abff8 20008040 c028d5c0 00000000 00000000
Backtrace:
[<c0010c00>] (at91_set_A_periph+0x0/0x78) from [<c0293528>]
(at91_register_uart+0x54/0x1e8)
[<c02934d4>] (at91_register_uart+0x0/0x1e8) from [<c0293bf0>]
(mpa1600_init_early+0x2c/0x4c)
 r5:c02cc8c0 r4:c0356220
[<c0293bc4>] (mpa1600_init_early+0x0/0x4c) from [<c02901a4>]
(setup_arch+0x668/0x76c)
[<c028fb3c>] (setup_arch+0x0/0x76c) from [<c028d624>] (start_kernel+0x74/0x318)
[<c028d5b0>] (start_kernel+0x0/0x318) from [<20008040>] (0x20008040)
 r7:c02ae7dc r6:c02a4bf8 r5:c02ac010 r4:c0007175
<0>Code: e1a02312 e3510000 13a03064 03a03060 (e58c2044)
<4>---[ end trace 1b75b31a2719ed1c ]---
<0>Kernel panic - not syncing: Attempted to kill the idle task!


regards
Joachim Eastwood

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Russell King - ARM Linux]

On Sun, Jan 29, 2012 at 03:33:35PM +0100, Joachim Eastwood wrote:
> <1>ioremap: pfn=fffff phys=fffff000 offset=400 size=1000
> <1>ioremap: area c3ffdfc0: phys_addr=200000 pfn=200 size=4000
> <1>ioremap: found: addr fef74000 => fed73000 => fed73400

Ok, first thing is that patch 7304/1 is utter crap.  The reason is simple -
'size' here:

                if (__phys_to_pfn(area->phys_addr) > pfn ||
                    __pfn_to_phys(pfn) + offset + size-1 >
                    area->phys_addr + area->size-1)
                        continue;

_already_ contains 'offset', as we've only _just_ done this a few lines
above the loop:

        size = PAGE_ALIGN(offset + size);

So, 'size' includes the requested offset *already*.  So, Nicolas' original:

             if (__phys_to_pfn(area->phys_addr) > pfn ||
                 __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
                     continue;

was correct in the first place.  In any case, let's see what's going on
here:

                if (__phys_to_pfn(area->phys_addr) > pfn ||
                    __pfn_to_phys(pfn) + offset + size-1 >
                    area->phys_addr + area->size-1)
                        continue;

we have:

area->phys_addr = 0x00200000, __phys_to_pfn(area->phys_addr) = 0x00200
area->size = 0x4000, pfn = 0xfffff, offset = 0x400, size = 0x1000

So, this if condition becomes:

		if (0x00200 > 0xfffff ||
		    0xfffff000 + 0x400 + 0x1000-1 > 0x00200000 + 0x4000-1)
and therefore:
		if (0x00200 > 0xfffff ||
		    0x000003ff > 0x00203fff)

So, the if condition fails, and so we _believe_ that the SRAM mapping fits
our request.  Clearly that's totally bogus and crap.

Unfortunately, Pawel doesn't say what the problem he was trying to fix was,
all we have to go on is this totally crap commit comment:

    The condition checking boundaries of the requested and existing
    mappings didn't take in-page offset into consideration though,
    which lead to obscure and hard to debug problems when requested
    mapping crossed end of the static one.

In any case, I don't think it matters, and I'm going to schedule a revert
of that crap commit.  So, Joachim, well spotted.

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Nicolas Pitre]

On Sun, 29 Jan 2012, Russell King - ARM Linux wrote:

> On Sun, Jan 29, 2012 at 03:33:35PM +0100, Joachim Eastwood wrote:
> > <1>ioremap: pfn=fffff phys=fffff000 offset=400 size=1000
> > <1>ioremap: area c3ffdfc0: phys_addr=200000 pfn=200 size=4000
> > <1>ioremap: found: addr fef74000 => fed73000 => fed73400
> 
> Ok, first thing is that patch 7304/1 is utter crap.  The reason is simple -
> 'size' here:
> 
>                 if (__phys_to_pfn(area->phys_addr) > pfn ||
>                     __pfn_to_phys(pfn) + offset + size-1 >
>                     area->phys_addr + area->size-1)
>                         continue;
> 
> _already_ contains 'offset', as we've only _just_ done this a few lines
> above the loop:
> 
>         size = PAGE_ALIGN(offset + size);
> 
> So, 'size' includes the requested offset *already*.

Bah, indeed.  OK, I feel better now for not having made this basic 
mistake in the first place, but not any better overall since I didn't 
review Pawel's patch thoroughly enough on the basis that his "fix" made 
things work for him.

> In any case, I don't think it matters, and I'm going to schedule a revert
> of that crap commit.  So, Joachim, well spotted.

For the revert:

Acked-by: Nicolas Pitre <nico@linaro.org>


Nicolas

[PATCH] ARM: ioremap: fix boundary check when reusing static mapping

[Pawel Moll]

On Sun, 2012-01-29 at 14:55 +0000, Russell King - ARM Linux wrote:
> So, 'size' includes the requested offset *already*.  So, Nicolas' original:
> 
>              if (__phys_to_pfn(area->phys_addr) > pfn ||
>                  __pfn_to_phys(pfn) + size-1 > area->phys_addr + area->size-1)
>                      continue;
> 
> was correct in the first place.  In any case, let's see what's going on
> here:

Uh. Sincere apologies - I was too happy to see my problem "fixed" and I
rushed things :-/ Lesson learned - I shall not post patches 5 minutes
after creating them and 5 minutes before leaving work.

> Unfortunately, Pawel doesn't say what the problem he was trying to fix was,
> all we have to go on is this totally crap commit comment:

After rebasing my DT4VE patches on top of 3.3-rc1 I seemed not to get
any interrupts from peripherals. Bisect pointed to Nico's patch and
commenting out the loop checking existing mappings made things work, so
did enlarging the static mapping that is created for MPCore peripherals.
Also some data from printk (I'll have to get them back to the code to
see were I made the mistake) made me think that the condition gets a
false positive. Back to debugging then.

> In any case, I don't think it matters, and I'm going to schedule a revert
> of that crap commit.  So, Joachim, well spotted.

I'm really sorry about the hassle.

Pawe?