ARM: Fix memblock_reserve() to include stext

PATCH New Magnus Damm 4 participants 11 messages Jul 31, 2013 – Aug 2, 2013

Subsystems (2)

Arm (32-bit) SuperH

lore

Share & tools

lore

Magnus Damm

author

2013-07-31 05:58 UTC #0 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

From: Magnus Damm <[email protected]>

This fix for the ARM architecture will include stext in the memblock_reserve() call to make sure that the following symbols are not overwritten (from System.map):

c0008000 T _text
c0008000 T stext
...
c0008138 T secondary_startup
...
c0009000 T _stext

With this patch applied CPU Hotplug starts working again. Without this patch code in secondary_startup never gets reached as expected.

This issue started triggering on kernels later than v3.10 - perhaps a side effect of the CPU Hotplug init section rework - so this is a fix for v3.11-rc.

Tested on the sh73a0 KZM9G board with CPU Hotplug:

 # echo 0 > /sys/devices/system/cpu/cpu1/online
 CPU1: shutdown
 # echo 1 > /sys/devices/system/cpu/cpu1/online
 CPU1: Booted secondary processor

Signed-off-by Magnus Damm <[email protected]>

---

 arch/arm/mm/init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/init.c +1 -1

@@ -346,7 +346,7 @@ void __init arm_memblock_init(struct mem
 #ifdef CONFIG_XIP_KERNEL
 	memblock_reserve(__pa(_sdata), _end - _sdata);
 #else
-	memblock_reserve(__pa(_stext), _end - _stext);
+	memblock_reserve(__pa(_text), _end - _text);
 #endif
 #ifdef CONFIG_BLK_DEV_INITRD
 	if (phys_initrd_size &&

Russell King - ARM Linux

2013-07-31 09:43 UTC #1 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 13 lines · Magnus Damm

From: Magnus Damm <[email protected]>
This fix for the ARM architecture will include stext in the memblock_reserve() call to make sure that the following symbols are not overwritten (from System.map):
c0008000 T _text
c0008000 T stext
...
c0008138 T secondary_startup
...
c0009000 T _stext

Quoted 7 lines

With this patch applied CPU Hotplug starts working again. Without this patch code in secondary_startup never gets reached as expected.
This issue started triggering on kernels later than v3.10 - perhaps a side effect of the CPU Hotplug init section rework - so this is a fix for v3.11-rc.

This is not the right fix. The commit you previously pointed out (removal of CPU hotplug) probably means there's a missing annotation somewhere in the code.

And yes - the __CPUINIT just before ENTRY(secondary_startup) was removed rather than being replaced with a .text (in both head.S and head-nommu.S). Same goes for ENTRY(lookup_processor_type) in head-common.S. All these need to be replaced with .text to stop them falling into the head section.

Paul Gortmaker

2013-07-31 17:06 UTC #2 lore raw

PATCH 1/1 arm: add .text at __CPUINIT places to prevent section grandfathering

In commit 22f0a27367742f65130c0fb25ef00f7297e032c1 ("init.h: remove __cpuinit sections from the kernel") the macros like __CPUINIT were converted from .section commands to no-ops.

This caused CPU hotplug regressions because the code following the __CPUINIT lines was now grandfathered into whatever the previous section happened to be. In most files, __CPUINIT was at the top of the file, so there was no previous section.

However in the files changed here, there was a previous section, and so we'd inadvertently garbage collect CPU hotplug related code. This changeset inserts ".text" into the exact same spot that the __CPUINIT section markers were found prior to commit 8bd26e3a7e49af2697449bbcb7187a39dc85d672 ("arm: delete __cpuinit/__CPUINIT usage from all ARM users:) in the three affected files, as suggested by Russell.

Bisected-by Simon Horman <[email protected]>

Reported-by Magnus Damm <[email protected]>

Reported-by Russell King <[email protected]>

Signed-off-by Paul Gortmaker <[email protected]>

---

[NB: Untested - I've not got an SMP ARM handy at the moment]

 arch/arm/kernel/head-common.S | 1 +
 arch/arm/kernel/head-nommu.S  | 1 +
 arch/arm/kernel/head.S        | 1 +
 3 files changed, 3 insertions(+)

arch/arm/kernel/head-common.S +1 -0

index 47cd974..def853e 100644
@@ -149,6 +149,7 @@ ENDPROC(lookup_processor_type)
  *	r5 = proc_info pointer in physical address space
  *	r9 = cpuid (preserved)
  */
+	.text
 __lookup_processor_type:
 	adr	r3, __lookup_processor_type_data
 	ldmia	r3, {r4 - r6}

arch/arm/kernel/head-nommu.S +1 -0

index b361de1..14235ba 100644
@@ -87,6 +87,7 @@ ENTRY(stext)
 ENDPROC(stext)
 
 #ifdef CONFIG_SMP
+	.text
 ENTRY(secondary_startup)
 	/*
 	 * Common entry point for secondary CPUs.

arch/arm/kernel/head.S +1 -0

index 9cf6063..2c7cc1e 100644
@@ -343,6 +343,7 @@ __turn_mmu_on_loc:
 	.long	__turn_mmu_on_end
 
 #if defined(CONFIG_SMP)
+	.text
 ENTRY(secondary_startup)
 	/*
 	 * Common entry point for secondary CPUs.

Signature · 1 line

1.8.1.2

Russell King - ARM Linux

2013-07-31 17:16 UTC #3 lore raw

PATCH 1/1 arm: add .text at __CPUINIT places to prevent section grandfathering

Quoted 17 lines · Paul Gortmaker

In commit 22f0a27367742f65130c0fb25ef00f7297e032c1 ("init.h: remove __cpuinit sections from the kernel") the macros like __CPUINIT were converted from .section commands to no-ops.
This caused CPU hotplug regressions because the code following the __CPUINIT lines was now grandfathered into whatever the previous section happened to be. In most files, __CPUINIT was at the top of the file, so there was no previous section.
However in the files changed here, there was a previous section, and so we'd inadvertently garbage collect CPU hotplug related code. This changeset inserts ".text" into the exact same spot that the __CPUINIT section markers were found prior to commit 8bd26e3a7e49af2697449bbcb7187a39dc85d672 ("arm: delete __cpuinit/__CPUINIT usage from all ARM users:) in the three affected files, as suggested by Russell.

Quoted 4 lines

Bisected-by Simon Horman <[email protected]>
Reported-by Magnus Damm <[email protected]>
Reported-by Russell King <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>

I already have this in my git tree as of this morning, thanks anyway.

Magnus Damm

author

2013-07-31 19:34 UTC #4 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

In reply to Russell King - ARM Linux

Quoted 22 lines · Russell King - ARM Linux

Quoted 13 lines · Magnus Damm
From: Magnus Damm <[email protected]>
This fix for the ARM architecture will include stext in the memblock_reserve() call to make sure that the following symbols are not overwritten (from System.map):
c0008000 T _text
c0008000 T stext
...
c0008138 T secondary_startup
...
c0009000 T _stext
Quoted 7 lines
With this patch applied CPU Hotplug starts working again. Without this patch code in secondary_startup never gets reached as expected.
This issue started triggering on kernels later than v3.10 - perhaps a side effect of the CPU Hotplug init section rework - so this is a fix for v3.11-rc.

Quoted 8 lines

This is not the right fix. The commit you previously pointed out (removal of CPU hotplug) probably means there's a missing annotation somewhere in the code.
And yes - the __CPUINIT just before ENTRY(secondary_startup) was removed rather than being replaced with a .text (in both head.S and head-nommu.S). Same goes for ENTRY(lookup_processor_type) in head-common.S. All these need to be replaced with .text to stop them falling into the head section.

Thanks for your guidance, Russell. The right way to fix this now seems pretty clear to me.

Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.

Cheers,

/ magnus

Russell King - ARM Linux

2013-07-31 19:39 UTC #5 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 5 lines · Magnus Damm

Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.

I committed a fix for it this morning, so it should be in linux-next by tomorrow.

Simon Horman

2013-08-01 07:34 UTC #6 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 9 lines · Russell King - ARM Linux

Quoted 5 lines · Magnus Damm
Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.
I committed a fix for it this morning, so it should be in linux-next by tomorrow.

Hi Russell,

Magnus has some outstanding patches for Renesas SoC boards which I believe depend on this problem being resolved. Could you indicate when you expect this patch to be in a stable branch which I could use as a base for a branch that will subsequently be pulled into arm-soc.

Thanks.

Paul Gortmaker

2013-08-01 12:58 UTC #7 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 11 lines · Simon Horman

Quoted 9 lines · Russell King - ARM Linux
Quoted 5 lines · Magnus Damm
Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.
I committed a fix for it this morning, so it should be in linux-next by tomorrow.

Quoted 6 lines

Hi Russell,
Magnus has some outstanding patches for Renesas SoC boards which I believe depend on this problem being resolved. Could you indicate when you expect this patch to be in a stable branch which I could use as a base for a branch that will subsequently be pulled into arm-soc.

It will never be in a stable release, since the cpuinit removal was added in the 3.11-rc1 merge window content, and the fix will be in before 3.11 final hits the streets. If you want stable stuff, you should be using v3.10 at this point in time.

Paul.
--

Quoted 3 lines

Thanks.

Simon Horman

2013-08-02 01:32 UTC #8 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 19 lines · Paul Gortmaker

Quoted 11 lines · Simon Horman
Quoted 9 lines · Russell King - ARM Linux
Quoted 5 lines · Magnus Damm
Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.
I committed a fix for it this morning, so it should be in linux-next by tomorrow.
Quoted 6 lines
Hi Russell,
Magnus has some outstanding patches for Renesas SoC boards which I believe depend on this problem being resolved. Could you indicate when you expect this patch to be in a stable branch which I could use as a base for a branch that will subsequently be pulled into arm-soc.

Quoted 4 lines

It will never be in a stable release, since the cpuinit removal was added in the 3.11-rc1 merge window content, and the fix will be in before 3.11 final hits the streets. If you want stable stuff, you should be using v3.10 at this point in time.

Sorry for my loose use of the word stable.

I meant a branch that will be incorporated into a future mainline release.

Quoted 1 line

From your response above I assume that I can use v3.11-rcX for some values

of X.

Can I confirm that the commit I should be looking for is cd717a2a3db96af1e308d5bdb76c9d5f8154b4bd
("ARM: Add .text annotations where required after __CPUINIT removal")?

Magnus Damm

author

2013-08-02 03:24 UTC #9 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

In reply to Russell King - ARM Linux

Quoted 9 lines · Russell King - ARM Linux

Quoted 5 lines · Magnus Damm
Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.
I committed a fix for it this morning, so it should be in linux-next by tomorrow.

Thanks for writing a fix, Russell. I cherry picked the following commit from linux-arm:

cd717a2 ARM: Add .text annotations where required after __CPUINIT removal

Applying it on top of kernel.org renesas git tag renesas-devel-20130801v2 (v3.11-rc3 + v3.12 material) solves the problem for me. I just tested this on KZM9G hardware without and with the patch, so I can confirm that CPU Hotplug is now working again.

Tested-by Magnus Damm <[email protected]>

Cheers,

/ magnus

Russell King - ARM Linux

2013-08-02 09:26 UTC #10 lore raw

PATCH 1/1 ARM: Fix memblock_reserve() to include stext

Quoted 15 lines · Magnus Damm

Quoted 9 lines · Russell King - ARM Linux
Quoted 5 lines · Magnus Damm
Are you aware of any existing fix for this or anyone working on solving this issue? If not then I don't mind stepping up. I can however think of quite a few other things to do if this has been solved already though, so please let me know if you think I should focus on fixing this.
I committed a fix for it this morning, so it should be in linux-next by tomorrow.
Thanks for writing a fix, Russell. I cherry picked the following commit from linux-arm:

Quoted 8 lines

cd717a2 ARM: Add .text annotations where required after __CPUINIT removal
Applying it on top of kernel.org renesas git tag renesas-devel-20130801v2 (v3.11-rc3 + v3.12 material) solves the problem for me. I just tested this on KZM9G hardware without and with the patch, so I can confirm that CPU Hotplug is now working again.
Tested-by Magnus Damm <[email protected]>

It should've ended up in Linus' tree last night, but from the looks of how quiet things seem to be, I guess everyone's away on August vacations at the moment.