[PATCH] ARM: move body of head-common.S back to text section

[Paul Gortmaker]

[Re: [PATCH] ARM: move body of head-common.S back to text section] On 02/07/2013 (Tue 20:44) Stephen Warren wrote:

> On 07/02/2013 05:22 PM, Stephen Boyd wrote:
> > On 07/02, Stephen Warren wrote:
> >> From: Stephen Warren <swarren at nvidia.com>
> >>
> >> Commit 281ecb7 "arm: delete __cpuinit/__CPUINIT usage from all ARM

We shouldn't reference this commit since (a) the SHA is from linux-next
and hence is transient, and (b) this commit really isn't the one that
triggers the breakage.  It is the commit where we dummy out the macros
that will reveal the missing __FINIT.

So, assuming Linus takes my pull request here...

https://lkml.org/lkml/2013/7/2/405

...for phase one of the __cpuinit purge, then the SHA you want to
blame for triggering this hidden bug into life is:

22f0a2736 "init.h: remove __cpuinit sections from the kernel" at:

http://git.kernel.org/cgit/linux/kernel/git/paulg/linux.git/log/?h=cpuinit-delete

After you update the commit log (and make the change below), it probably
makes sense for this to go in via one of the ARM trees, since if I take
it, it will go in at the very end of the merge window with the phase two
commits.  If it goes in via an arm tree, the regression window within
3.10 --> 3.11-rc1 will be minimized. (That and it is a pre-existing bug).

> >> users" removed a __CPUINIT from head-common.S. However, the code
> >> immediately before the removed tag was marked __INIT, and was missing
> >> a match __FINIT. This caused code after the removed __CPUINIT to end
> >> up in the init section rather than the main text section as desired.
> >> This caused issues such as secondary CPU boot failures or crashes.
> >> Add the missing __FINIT to solve this.
> >>
> >> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> >> ---
> >>  arch/arm/kernel/head-common.S | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
> >> index 529ce99..a9dc804 100644
> >> --- a/arch/arm/kernel/head-common.S
> >> +++ b/arch/arm/kernel/head-common.S
> >> @@ -122,6 +122,8 @@ __mmap_switched_data:
> >>  	.long	init_thread_union + THREAD_START_SP @ sp
> >>  	.size	__mmap_switched_data, . - __mmap_switched_data
> >>  
> >> +	__FINIT
> >> +
> >>  /*
> >>   * This provides a C-API version of __lookup_processor_type
> >>   */
> > 
> > Shouldn't this come after lookup_processor_type()? Otherwise you
> > just removed that function from the init section?
> 
> I think this is correct. Prior to "arm: delete __cpuinit/__CPUINIT usage
> from all ARM users", lookup_processor_type() was in the cpuinit section.

No, it was __lookup_processor_type that was in the cpuinit section.
The non-underscore (C-API) version was __INIT, and if you look at the
one and only calling function in C, it too is tagged __init.  So it
appears to me that was a thought out and deliberate choice.

> After that commit, it moved to the init section and hence got dumped
> soon after boot. The change above moves lookup_processor_type() back
> into the regular text section, which I believe was the intent of the
> __cpuinit removal patch.

Given the above, actually I think StephenB is right here.  The __FINIT
can go exactly where the removed __CPUINIT was (plus or minus a blank
line...)

Thanks for testing, reporting and then tracking down where it was once
you knew what it was that you were looking for (and to Russell for the
quick and accurate diagnosis.)  You can add an Acked-by from me to the
updated patch if you want.

As an aside, I'm now thinking any __INIT that implicitly rely on EOF for
closure are nasty traps waiting to happen and it might be worthwhile to
audit and explicitly __FINIT them before someone appends to the file...

Paul.
--