sections mismatches: How to mark new false positives?

sections mismatches: How to mark new false positives?

[Adrian Bunk]

I'm getting the following in the latest -git:

<--  snip  -->

...
WARNING: arch/x86/kernel/built-in.o(.exit.text+0x1db): Section mismatch in reference from the function msr_exit() to the variable .cpuinit.data:msr_class_cpu_notifier
...

<--  snip  -->

That's obviously a false positive (unregister_hotcpu_notifier() is a 
noop if CONFIG_HOTPLUG_CPU=n), but how can I silence the warning?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: sections mismatches: How to mark new false positives?

[Sam Ravnborg]

On Tue, Jan 29, 2008 at 07:02:55PM +0200, Adrian Bunk wrote:
> I'm getting the following in the latest -git:
> 
> <--  snip  -->
> 
> ...
> WARNING: arch/x86/kernel/built-in.o(.exit.text+0x1db): Section mismatch in reference from the function msr_exit() to the variable .cpuinit.data:msr_class_cpu_notifier
> ...
> 
> <--  snip  -->
> 
> That's obviously a false positive (unregister_hotcpu_notifier() is a 
> noop if CONFIG_HOTPLUG_CPU=n), but how can I silence the warning?

What is the purpose of __cpuinit?
It seems to be used for two purposes:

To annotate code that is used to initialize cpu's
  if CONFIG_HOTPLUG_CPU=n then discard it after
                               init has completed
  if CONFIG_HOTPLUG_CPU=y then keep it

To annotate all 'core' cpu hotplug related code
  if CONFIG_HOTPLUG_CPU=n then it is not used and
                               can safely be discarded
  if CONFIG_HOTPLUG_CPU=y then keep it

And the variable msr_class_cpu_notifier belongs to the last category.
So the root cause of all the __cpu* related section mismatch
warnings are the misuse of __cpuinit to mark all core functions.

How are we going to fix this?

I see a couple of possibilities:

1) annotate like hell to hide the misuse of __cpuinit
2) introduce __cpu to make cpu hotplug 'core' stuff
3) drop section mismatch checks for __cpu stuff


	Sam

Re: sections mismatches: How to mark new false positives?

[Adrian Bunk]

On Tue, Jan 29, 2008 at 07:13:06PM +0100, Sam Ravnborg wrote:
> On Tue, Jan 29, 2008 at 07:02:55PM +0200, Adrian Bunk wrote:
> > I'm getting the following in the latest -git:
> > 
> > <--  snip  -->
> > 
> > ...
> > WARNING: arch/x86/kernel/built-in.o(.exit.text+0x1db): Section mismatch in reference from the function msr_exit() to the variable .cpuinit.data:msr_class_cpu_notifier
> > ...
> > 
> > <--  snip  -->
> > 
> > That's obviously a false positive (unregister_hotcpu_notifier() is a 
> > noop if CONFIG_HOTPLUG_CPU=n), but how can I silence the warning?
> 
> What is the purpose of __cpuinit?
> It seems to be used for two purposes:
> 
> To annotate code that is used to initialize cpu's
>   if CONFIG_HOTPLUG_CPU=n then discard it after
>                                init has completed
>   if CONFIG_HOTPLUG_CPU=y then keep it
> 
> To annotate all 'core' cpu hotplug related code
>   if CONFIG_HOTPLUG_CPU=n then it is not used and
>                                can safely be discarded
>   if CONFIG_HOTPLUG_CPU=y then keep it
> 
> And the variable msr_class_cpu_notifier belongs to the last category.
> So the root cause of all the __cpu* related section mismatch
> warnings are the misuse of __cpuinit to mark all core functions.
> 
> How are we going to fix this?
> 
> I see a couple of possibilities:
> 
> 1) annotate like hell to hide the misuse of __cpuinit
> 2) introduce __cpu to make cpu hotplug 'core' stuff
> 3) drop section mismatch checks for __cpu stuff

My main point is not related to __cpu*

E.g. look at the following warnings:

WARNING: drivers/pci/built-in.o(.text+0xa385): Section mismatch in 
reference from the function cpci_configure_slot() to the function 
.devinit.text:pci_do_scan_bus()
WARNING: drivers/pci/built-in.o(.text+0x13052): Section mismatch in 
reference from the function cpqhp_configure_device() to the function 
.devinit.text:pci_do_scan_bus()
WARNING: drivers/pci/built-in.o(.text+0x1561c): Section mismatch in 
reference from the function ibm_configure_device() to the function 
.devinit.text:pci_do_scan_bus()
WARNING: drivers/pci/built-in.o(.text+0x26176): Section mismatch in 
reference from the function shpchp_configure_device() to the function 
.devinit.text:pci_do_scan_bus()

The code seems to be 100% correct, but how can I silence the warnings 
without needlessly bloating the kernel in the CONFIG_HOTPLUG=n case?

> 	Sam

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: sections mismatches: How to mark new false positives?

[Sam Ravnborg]

[Greg added to cc: as I guess he is most familiar with this code base]
> 
> WARNING: drivers/pci/built-in.o(.text+0xa385): Section mismatch in 
> reference from the function cpci_configure_slot() to the function 
> .devinit.text:pci_do_scan_bus()
> WARNING: drivers/pci/built-in.o(.text+0x13052): Section mismatch in 
> reference from the function cpqhp_configure_device() to the function 
> .devinit.text:pci_do_scan_bus()
> WARNING: drivers/pci/built-in.o(.text+0x1561c): Section mismatch in 
> reference from the function ibm_configure_device() to the function 
> .devinit.text:pci_do_scan_bus()
> WARNING: drivers/pci/built-in.o(.text+0x26176): Section mismatch in 
> reference from the function shpchp_configure_device() to the function 
> .devinit.text:pci_do_scan_bus()
> 
> The code seems to be 100% correct, but how can I silence the warnings 
> without needlessly bloating the kernel in the CONFIG_HOTPLUG=n case?

The recommended way to silence warnings is to use annotation.
Use __ref for functions that may reference any __*init and __*exit
code/data.
Use __refdata for variables, and __refconst for const varibales.

Examples:

static int __ref foo(int bar)
{
	if (use_an_init_var)
		call_an_init_function();
}

The __ref annotation will place the code of the function in a section
named .ref.text and modpost will not check relocation records from
.ref.text to anything.

Likewise for variables:
int my_init_ref_variable[] __refdata = { ...};


But that said I took a closer look at the last warning.
We have pci_do_scan_bus() which is annotated __devinit.
It is a 4 line function that calls pci_scan_child_bus() which
is not annotated and calls pci_bus_add_devices() which
is not annotated.

pci_do_scan_bus() has sole users in drivers/pci/hotplug
and none of the users in drivers/pci/hotplug looks like
functions used during init time.

So it really looks like the __devinit annotation
in this case has been used to say that this is
code that is only relevant for hotplug use.
But the code is NOT restricted to be used in
the init phase and thus the __devinit annotation is *WRONG*.

The right fix is to restrict the code to be used
solely when HOTPLUG is enabled and no annotation can
help us here.

I would suggest the following fix.

Now it is obvious that this is HOTPLUG only code
and no false annotation that this is only used
in the init phase but the init code needs
to be preserved in the hotplug case.

So exactly the same issue as I previously described for
the __cpuinit misuse.

PS. I sneaked in a delete of a forward declaration. That
fix should maybe be factored out.

	Sam

diff --git a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c
index 2b5352a..e0a00bf 100644
--- a/drivers/pci/hotplug.c
+++ b/drivers/pci/hotplug.c
@@ -35,3 +35,17 @@ int pci_uevent(struct device *dev, struct kobj_uevent_env *env)
 		return -ENOMEM;
 	return 0;
 }
+
+unsigned int pci_do_scan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+
+	max = pci_scan_child_bus(bus);
+
+	/*
+	 * Make the discovered devices available.
+	 */
+	pci_bus_add_devices(bus);
+
+	return max;
+}
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5fd5852..3a99065 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -471,8 +471,6 @@ static void pci_fixup_parent_subordinate_busnr(struct pci_bus *child, int max)
 	}
 }
 
-unsigned int pci_scan_child_bus(struct pci_bus *bus);
-
 /*
  * If it's a bridge, configure it and scan the bus behind it.
  * For CardBus bridges, we don't scan behind as the devices will
@@ -1050,20 +1048,6 @@ unsigned int pci_scan_child_bus(struct pci_bus *bus)
 	return max;
 }
 
-unsigned int __devinit pci_do_scan_bus(struct pci_bus *bus)
-{
-	unsigned int max;
-
-	max = pci_scan_child_bus(bus);
-
-	/*
-	 * Make the discovered devices available.
-	 */
-	pci_bus_add_devices(bus);
-
-	return max;
-}
-
 struct pci_bus * pci_create_bus(struct device *parent,
 		int bus, struct pci_ops *ops, void *sysdata)
 {

Re: sections mismatches: How to mark new false positives?

[Sam Ravnborg]

> 
> I would suggest the following fix.

Looked a bit closer.
We have more warnings in this area and I see
code where we previously remove the __devinit
annotation.

It looks like we should take all the HOTPLUG only
functions from probe.c and move them elsewhere
(hotplug.c or probe_hotplug.c) and then
only build this file if HOTPLUG is enabled.

This seems like a straightforward solution.
If no-one beats me I will look into that in the weekend.

	Sam