Solving section mismatches

Solving section mismatches

[Russell King - ARM Linux]

Sam,

Any idea how to solve this:

WARNING: arch/arm/kernel/built-in.o(.text+0x1ebc): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
The function cpu_idle() references a function in an exit section.
Often the function cpu_die() has valid usage outside the exit section
and the fix is to remove the __cpuexit annotation of cpu_die.

WARNING: arch/arm/kernel/built-in.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
The function __cpuexit cpu_die() references
a function __cpuinit secondary_start_kernel().
This is often seen when error handling in the exit function
uses functionality in the init path.
The fix is often to remove the __cpuinit annotation of
secondary_start_kernel() so it may be used outside an init section.

Logically, the annotations are correct - in the first case, cpu_die()
will only ever be called if hotplug CPU is enabled, since you can't
offline a CPU without hotplug CPU enabled.  In that case, the __cpuexit.*
sections are not discarded.

In the second case, this is platform code to restart the CPU, and
again the anotations are entirely correct - cpuexit code only exists
if hotplug CPU is enabled, in which case the cpuinit code must also
be kept around.  Therefore, it is entirely legal for cpuexit code to
call cpuinit code - unlike __init vs __exit or __devinit vs __devexit.

Therefore, I believe both of these warnings to be incorrect.

Solving section mismatches

[Sam Ravnborg]

On Sun, Sep 27, 2009 at 05:41:16PM +0100, Russell King - ARM Linux wrote:
> Sam,
> 
> Any idea how to solve this:
> 
> WARNING: arch/arm/kernel/built-in.o(.text+0x1ebc): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
> The function cpu_idle() references a function in an exit section.
> Often the function cpu_die() has valid usage outside the exit section
> and the fix is to remove the __cpuexit annotation of cpu_die.
> 
> WARNING: arch/arm/kernel/built-in.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> The function __cpuexit cpu_die() references
> a function __cpuinit secondary_start_kernel().
> This is often seen when error handling in the exit function
> uses functionality in the init path.
> The fix is often to remove the __cpuinit annotation of
> secondary_start_kernel() so it may be used outside an init section.
> 
> Logically, the annotations are correct - in the first case, cpu_die()
> will only ever be called if hotplug CPU is enabled, since you can't
> offline a CPU without hotplug CPU enabled.  In that case, the __cpuexit.*
> sections are not discarded.

The annotation of cpu_die() is wrong.
To be annotated __cpuexit the function shall:
- be used in exit context and only in exit context with HOTPLUG_CPU=n
- be used outside exit context with HOTPLUG_CPU=y

cpu_die() fails on the first condition because it is only used
if HOTPLUG_CPU=y.
The annotation is wrongly used as a replacement for an ifdef.
As cpu_die() is already inside ifdef CONFIG_HOTPLUG_CPU is should
be enough to just remove the annotation.

Like this (copy'n'paste so it does not apply)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index e0d3277..de4ef1c 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
  * of the other hotplug-cpu capable cores, so presumably coming
  * out of idle fixes this.
  */
-void __cpuexit cpu_die(void)
+void cpu_die(void)
 {
        unsigned int cpu = smp_processor_id();
 


I did not see any section mismatch with defconfig so I failed to
test if it made a difference.

> 
> In the second case, this is platform code to restart the CPU, and
> again the anotations are entirely correct - cpuexit code only exists
> if hotplug CPU is enabled, in which case the cpuinit code must also
> be kept around.  Therefore, it is entirely legal for cpuexit code to
> call cpuinit code - unlike __init vs __exit or __devinit vs __devexit.
> 
> Therefore, I believe both of these warnings to be incorrect.

Same reasoning as above.
And the patch ougth to solve this case too.

Please note that throughout the kernel there is a lot of places where
the __cpu* annotations has been misused.
I once suggested to drop the chek from modpost as I could not see
this being fixed - but alas no replies it never happened.

	Sam

Solving section mismatches

[Russell King - ARM Linux]

On Sun, Sep 27, 2009 at 08:27:07PM +0200, Sam Ravnborg wrote:
> On Sun, Sep 27, 2009 at 05:41:16PM +0100, Russell King - ARM Linux wrote:
> > Sam,
> > 
> > Any idea how to solve this:
> > 
> > WARNING: arch/arm/kernel/built-in.o(.text+0x1ebc): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
> > The function cpu_idle() references a function in an exit section.
> > Often the function cpu_die() has valid usage outside the exit section
> > and the fix is to remove the __cpuexit annotation of cpu_die.
> > 
> > WARNING: arch/arm/kernel/built-in.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> > The function __cpuexit cpu_die() references
> > a function __cpuinit secondary_start_kernel().
> > This is often seen when error handling in the exit function
> > uses functionality in the init path.
> > The fix is often to remove the __cpuinit annotation of
> > secondary_start_kernel() so it may be used outside an init section.
> > 
> > Logically, the annotations are correct - in the first case, cpu_die()
> > will only ever be called if hotplug CPU is enabled, since you can't
> > offline a CPU without hotplug CPU enabled.  In that case, the __cpuexit.*
> > sections are not discarded.
> 
> The annotation of cpu_die() is wrong.
> To be annotated __cpuexit the function shall:
> - be used in exit context and only in exit context with HOTPLUG_CPU=n
> - be used outside exit context with HOTPLUG_CPU=y
> 
> cpu_die() fails on the first condition because it is only used
> if HOTPLUG_CPU=y.
> The annotation is wrongly used as a replacement for an ifdef.
> As cpu_die() is already inside ifdef CONFIG_HOTPLUG_CPU is should
> be enough to just remove the annotation.
> 
> Like this (copy'n'paste so it does not apply)
> 
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index e0d3277..de4ef1c 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
>   * of the other hotplug-cpu capable cores, so presumably coming
>   * out of idle fixes this.
>   */
> -void __cpuexit cpu_die(void)
> +void cpu_die(void)
>  {
>         unsigned int cpu = smp_processor_id();

This is wrong.  cpu_die() does not need to exist if hotplug CPU is
disabled.  In that case, it should be discarded and this is precisely
what __cpuexit does.  The annotation is, therefore, correct.

Solving section mismatches

[Sam Ravnborg]

On Sun, Sep 27, 2009 at 07:39:04PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 27, 2009 at 08:27:07PM +0200, Sam Ravnborg wrote:
> > On Sun, Sep 27, 2009 at 05:41:16PM +0100, Russell King - ARM Linux wrote:
> > > Sam,
> > > 
> > > Any idea how to solve this:
> > > 
> > > WARNING: arch/arm/kernel/built-in.o(.text+0x1ebc): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
> > > The function cpu_idle() references a function in an exit section.
> > > Often the function cpu_die() has valid usage outside the exit section
> > > and the fix is to remove the __cpuexit annotation of cpu_die.
> > > 
> > > WARNING: arch/arm/kernel/built-in.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> > > The function __cpuexit cpu_die() references
> > > a function __cpuinit secondary_start_kernel().
> > > This is often seen when error handling in the exit function
> > > uses functionality in the init path.
> > > The fix is often to remove the __cpuinit annotation of
> > > secondary_start_kernel() so it may be used outside an init section.
> > > 
> > > Logically, the annotations are correct - in the first case, cpu_die()
> > > will only ever be called if hotplug CPU is enabled, since you can't
> > > offline a CPU without hotplug CPU enabled.  In that case, the __cpuexit.*
> > > sections are not discarded.
> > 
> > The annotation of cpu_die() is wrong.
> > To be annotated __cpuexit the function shall:
> > - be used in exit context and only in exit context with HOTPLUG_CPU=n
> > - be used outside exit context with HOTPLUG_CPU=y
> > 
> > cpu_die() fails on the first condition because it is only used
> > if HOTPLUG_CPU=y.
> > The annotation is wrongly used as a replacement for an ifdef.
> > As cpu_die() is already inside ifdef CONFIG_HOTPLUG_CPU is should
> > be enough to just remove the annotation.
> > 
> > Like this (copy'n'paste so it does not apply)
> > 
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index e0d3277..de4ef1c 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
> >   * of the other hotplug-cpu capable cores, so presumably coming
> >   * out of idle fixes this.
> >   */
> > -void __cpuexit cpu_die(void)
> > +void cpu_die(void)
> >  {
> >         unsigned int cpu = smp_processor_id();
> 
> This is wrong.  cpu_die() does not need to exist if hotplug CPU is
> disabled.  In that case, it should be discarded and this is precisely
> what __cpuexit does.  The annotation is, therefore, correct.

Solving section mismatches

[Russell King - ARM Linux]

On Sun, Sep 27, 2009 at 08:46:45PM +0200, Sam Ravnborg wrote:
> Please look at the above and realise that cpu_die() is only ever defined
> in case that HOTPLUG_CPU is defined. So there is nothing to discard if
> HOTPLUG_CPU equals to n.
> 
> And just to repeat myself....
> The only correct use of __cpu* annotation is for function/data that is
> used with or without HOTPLUG_CPU equals to y.
> Which is NOT the case for cpu_die().
> 
> The __cpu* annotation is not a replacement for ifdeffed out code that is
> not relevant for the non-HOTPLUG_CPU case.

Ok.  But that still doesn't solve:

WARNING: arch/arm/kernel/built-in.o(.text+0x6274): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
The function cpu_die() references
the function __cpuinit secondary_start_kernel().
This is often because cpu_die lacks a __cpuinit 
annotation or the annotation of secondary_start_kernel is wrong.

So are you saying secondary_start_kernel should not have __cpuinit?

That seems wrong, since we do want it to be discarded as __init in the
non-hotplug case, but we do want it kept around in the hotplug cpu
case.  I guess something like the following could be used instead:

#ifdef CONFIG_HOTPLUG_CPU
asmlinkage void secondary_start_kernel(void)
#else
asmlinkage void __init secondary_start_kernel(void)
#endif
{
...
}

but my understanding was that's what __cpuinit was supposed to be
doing for us in the first place.

Solving section mismatches

[Sam Ravnborg]

On Sun, Sep 27, 2009 at 08:09:53PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 27, 2009 at 08:46:45PM +0200, Sam Ravnborg wrote:
> > Please look at the above and realise that cpu_die() is only ever defined
> > in case that HOTPLUG_CPU is defined. So there is nothing to discard if
> > HOTPLUG_CPU equals to n.
> > 
> > And just to repeat myself....
> > The only correct use of __cpu* annotation is for function/data that is
> > used with or without HOTPLUG_CPU equals to y.
> > Which is NOT the case for cpu_die().
> > 
> > The __cpu* annotation is not a replacement for ifdeffed out code that is
> > not relevant for the non-HOTPLUG_CPU case.
> 
> Ok.  But that still doesn't solve:
> 
> WARNING: arch/arm/kernel/built-in.o(.text+0x6274): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> The function cpu_die() references
> the function __cpuinit secondary_start_kernel().
> This is often because cpu_die lacks a __cpuinit 
> annotation or the annotation of secondary_start_kernel is wrong.
> 
> So are you saying secondary_start_kernel should not have __cpuinit?

Lets try it.
- is secondary_start_kernel used only in init context with HOTPLUG_CPU=n - yes
- is secondary_start_kernel used outside init context with HOTPLUG_CPU=y - yes

So the __cpuinit annotation is correct.

cpu_idle is calling secondary_start_kernel() but cpu_idle() does not fulfill
the criteria for any of the __cpu* annotations.

So there is only two ways to tell modpost that this is correct.
Either rename it so it fullfill one of the whitelisted names
or explicitly tell modpost that this function may reference
__*init/__*exit annotated functions/code.

The latter is much preferred.
So the right fix is to annotate it with __ref. (see include/linux/init.h)

Had I looked closely last time I would have know this already - but alas I was
to quick to reply.

Please try this:
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index e0d3277..835188b 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
  * of the other hotplug-cpu capable cores, so presumably coming
  * out of idle fixes this.
  */
-void __cpuexit cpu_die(void)
+void __ref cpu_die(void)
 {
        unsigned int cpu = smp_processor_id();
 

You can add my:

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
or
Acked-by: Sam Ravnborg <sam@ravnborg.org>
as you prefer.

Assuming this works of course.

	Sam

Solving section mismatches

[Russell King - ARM Linux]

On Sun, Sep 27, 2009 at 09:47:47PM +0200, Sam Ravnborg wrote:
> Please try this:
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index e0d3277..835188b 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
>   * of the other hotplug-cpu capable cores, so presumably coming
>   * out of idle fixes this.
>   */
> -void __cpuexit cpu_die(void)
> +void __ref cpu_die(void)
>  {
>         unsigned int cpu = smp_processor_id();

That works - so can I get your ack for this entire patch please:

From: Russell King <rmk+kernel@arm.linux.org.uk>
Subject: [PATCH] ARM: Fix __cpuexit section mismatch warnings

Fix:

WARNING: vmlinux.o(.text+0x247c): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
The function cpu_idle() references a function in an exit section.
Often the function cpu_die() has valid usage outside the exit section
and the fix is to remove the __cpuexit annotation of cpu_die.

WARNING: vmlinux.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
The function __cpuexit cpu_die() references
a function __cpuinit secondary_start_kernel().
This is often seen when error handling in the exit function
uses functionality in the init path.
The fix is often to remove the __cpuinit annotation of
secondary_start_kernel() so it may be used outside an init section.

Sam says:
> The annotation of cpu_die() is wrong.
> To be annotated __cpuexit the function shall:
> - be used in exit context and only in exit context with HOTPLUG_CPU=n
> - be used outside exit context with HOTPLUG_CPU=y

So, this also means __cpu_disable(), __cpu_die() and twd_timer_stop() are
also wrong.  However, removing __cpuexit from cpu_die() creates:

WARNING: vmlinux.o(.text+0x6834): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
The function cpu_die() references
the function __cpuinit secondary_start_kernel().
This is often because cpu_die lacks a __cpuinit
annotation or the annotation of secondary_start_kernel is wrong.

so fix this using __ref.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/smp.c     |    6 +++---
 arch/arm/kernel/smp_twd.c |    4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 9d015ee..57162af 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -154,7 +154,7 @@ int __cpuinit __cpu_up(unsigned int cpu)
 /*
  * __cpu_disable runs on the processor to be shutdown.
  */
-int __cpuexit __cpu_disable(void)
+int __cpu_disable(void)
 {
 	unsigned int cpu = smp_processor_id();
 	struct task_struct *p;
@@ -201,7 +201,7 @@ int __cpuexit __cpu_disable(void)
  * called on the thread which is asking for a CPU to be shutdown -
  * waits until shutdown has completed, or it is timed out.
  */
-void __cpuexit __cpu_die(unsigned int cpu)
+void __cpu_die(unsigned int cpu)
 {
 	if (!platform_cpu_kill(cpu))
 		printk("CPU%u: unable to kill\n", cpu);
@@ -215,7 +215,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
  * of the other hotplug-cpu capable cores, so presumably coming
  * out of idle fixes this.
  */
-void __cpuexit cpu_die(void)
+void __ref cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
 
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index d8c88c6..a73a34d 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -166,10 +166,12 @@ void __cpuinit twd_timer_setup(struct clock_event_device *clk)
 	clockevents_register_device(clk);
 }
 
+#ifdef CONFIG_HOTPLUG_CPU
 /*
  * take a local timer down
  */
-void __cpuexit twd_timer_stop(void)
+void twd_timer_stop(void)
 {
 	__raw_writel(0, twd_base + TWD_TIMER_CONTROL);
 }
+#endif

Solving section mismatches

[Sam Ravnborg]

On Sun, Sep 27, 2009 at 09:15:07PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 27, 2009 at 09:47:47PM +0200, Sam Ravnborg wrote:
> > Please try this:
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index e0d3277..835188b 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -214,7 +214,7 @@ void __cpuexit __cpu_die(unsigned int cpu)
> >   * of the other hotplug-cpu capable cores, so presumably coming
> >   * out of idle fixes this.
> >   */
> > -void __cpuexit cpu_die(void)
> > +void __ref cpu_die(void)
> >  {
> >         unsigned int cpu = smp_processor_id();
> 
> That works - so can I get your ack for this entire patch please:
> 
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: [PATCH] ARM: Fix __cpuexit section mismatch warnings
> 
> Fix:
> 
> WARNING: vmlinux.o(.text+0x247c): Section mismatch in reference from the function cpu_idle() to the function .cpuexit.text:cpu_die()
> The function cpu_idle() references a function in an exit section.
> Often the function cpu_die() has valid usage outside the exit section
> and the fix is to remove the __cpuexit annotation of cpu_die.
> 
> WARNING: vmlinux.o(.cpuexit.text+0x3c): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> The function __cpuexit cpu_die() references
> a function __cpuinit secondary_start_kernel().
> This is often seen when error handling in the exit function
> uses functionality in the init path.
> The fix is often to remove the __cpuinit annotation of
> secondary_start_kernel() so it may be used outside an init section.
> 
> Sam says:
> > The annotation of cpu_die() is wrong.
> > To be annotated __cpuexit the function shall:
> > - be used in exit context and only in exit context with HOTPLUG_CPU=n
> > - be used outside exit context with HOTPLUG_CPU=y
> 
> So, this also means __cpu_disable(), __cpu_die() and twd_timer_stop() are
> also wrong.  However, removing __cpuexit from cpu_die() creates:
> 
> WARNING: vmlinux.o(.text+0x6834): Section mismatch in reference from the function cpu_die() to the function .cpuinit.text:secondary_start_kernel()
> The function cpu_die() references
> the function __cpuinit secondary_start_kernel().
> This is often because cpu_die lacks a __cpuinit
> annotation or the annotation of secondary_start_kernel is wrong.
> 
> so fix this using __ref.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Patch looks good!
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

Solving section mismatches

[Russell King - ARM Linux]

On Sun, Sep 27, 2009 at 10:22:51PM +0200, Sam Ravnborg wrote:
> Patch looks good!
> Acked-by: Sam Ravnborg <sam@ravnborg.org>

Thanks Sam.