diff mbox

[V2] ARM: add missing __FINIT to head-common.S to match __INIT

Message ID 1372875174-26812-1-git-send-email-swarren@wwwdotorg.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stephen Warren July 3, 2013, 6:12 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

Macro __INIT is used to place various code in head-common.S into the init
section. This should be matched by a closing __FINIT at the end of the
code destined for the init section. Add this missing macro.

This historically caused no problem, because macro __CPUINIT was used at
the exact location where __FINIT was missing, which then placed following
code into the cpuinit section. However, with commit 22f0a2736 "init.h:
remove __cpuinit sections from the kernel" applied, __CPUINIT becomes a
no-op, thus leaving all this code in the init section, rather than the
regular text section. This caused issues such as secondary CPU boot
failures or crashes.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
v2: Moved __FINIT after lookup_processor_type, to correctly match the
location of __CPUINIT.

Russell, I assume I should add this to the ARM patch tracker.

This version is based on v3.10.
---
 arch/arm/kernel/head-common.S | 2 ++
 1 file changed, 2 insertions(+)

Comments

Russell King - ARM Linux July 4, 2013, 8:35 a.m. UTC | #1 
On Wed, Jul 03, 2013 at 12:12:54PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Macro __INIT is used to place various code in head-common.S into the init
> section. This should be matched by a closing __FINIT at the end of the
> code destined for the init section. Add this missing macro.
> 
> This historically caused no problem, because macro __CPUINIT was used at
> the exact location where __FINIT was missing, which then placed following
> code into the cpuinit section. However, with commit 22f0a2736 "init.h:
> remove __cpuinit sections from the kernel" applied, __CPUINIT becomes a
> no-op, thus leaving all this code in the init section, rather than the
> regular text section. This caused issues such as secondary CPU boot
> failures or crashes.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
> v2: Moved __FINIT after lookup_processor_type, to correctly match the
> location of __CPUINIT.
> 
> Russell, I assume I should add this to the ARM patch tracker.

See the discussion I've had with Paul on this.  I disagree that adding
__FINIT "solves" the problem.  It just _changes_ the possibilities for
getting it wrong to a different set of possibilities.

__FINIT just swaps the current section with the one which was last in
use, which might be a fair way back up the file.  The problem is what
happens if someone changes the section before the __INIT without
checking the following code.

If we want to mark the end of the .init.text section, then __FINIT is
a good way to do that, but follow it with an explicit statement about
what section you want the following code to be in to ensure that it
is placed into the intended section irrespective of what future changes
occur.
diff mbox

Patch

diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 5b391a6..1fb887d 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -133,6 +133,8 @@  ENTRY(lookup_processor_type)
 	ldmfd	sp!, {r4 - r6, r9, pc}
 ENDPROC(lookup_processor_type)
 
+	__FINIT
+
 /*
  * Read processor ID register (CP#15, CR0), and look up in the linker-built
  * supported processor list.  Note that we can't use the absolute addresses