Re: [PATCH] x86: fix improper .init-type section references

From: Satyam Sharma
Date: Tue Jun 12 2007 - 08:09:21 EST


Hi Jan,

On 6/12/07, Satyam Sharma <satyam.sharma@xxxxxxxxx> wrote:
On 6/12/07, Jan Beulich <jbeulich@xxxxxxxxxx> wrote:
> [...]
> @@ -448,7 +448,7 @@ static void __cpuinit cache_shared_cpu_m
> -static void __cpuinit cache_remove_shared_cpu_map(unsigned int cpu, int index)
> +static void __cpuexit cache_remove_shared_cpu_map(unsigned int cpu, int index)

You might've done this because the caller of
cache_remove_shared_cpu_map() is cache_remove_dev()
which is __cpuexit. However, cache_remove_dev() itself is called
from cacheinfo_cpu_callback() which is ... __cpuinit, because it's
caller cache_sysfs_init() is *definitely* __cpuinit. What a mess ...

device_initcall(cache_sysfs_init):

__cpuinit cache_sysfs_init
-> __cpuinit cacheinfo_cpu_callback <======== PROBLEM
-> __cpuexit cache_remove_dev <======== PROBLEM
-> __cpuinit cache_remove_shared_cpu_map

Where cacheinfo_cpu_callback() itself is the callback for the
cacheinfo_cpu_notifier notifier, which is itself marked __cpuinitdata.

I suspect the __cpuexit-marking of cache_remove_dev() here is totally
bogus. When !CONFIG_HOTPLUG_CPU, __cpuexit == __exit, and
__exit simply means that the function can be discarded and ignored
from being linked in for non-modular (built-in) build of that particular
module. I'm not sure how the code in question here can ever be built
as a module, so cache_remove_dev shouldn't be a __cpuexit in the
first place. But not marking it anything would take us back to the
modpost warnings. So ... something needs to be done about the
logic in this whole place ...

I did some more code inspection and found:

The cacheinfo_cpu_notifier itself is called out from _cpu_down() which
is ... yep, *not* __cpuinit (and obviously *cannot* be so). _cpu_down()
is called from cpu_down() which is (thankfully!) protected inside
#ifdef CONFIG_HOTPLUG_CPU, so we've /just about/ escaped trouble
so far, but another of it's callers is disable_nonboot_cpus() which does
*not* depend on HOTPLUG_CPU, but is #ifdef'ed inside SUSPEND_SMP
(gargh!) instead, and ... is _not_ __cpuinit either (obviously, again).

Wait, SUSPEND_SMP again depends on HOTPLUG_CPU, so there
are no issues here in marking cache_remove_dev() __cpuexit
(which effectively simply becomes #ifdef HOTPLUG_CPU) as all
callsites that call out the notifier with CPU_DEAD/FROZEN are also
going to be disabled. [ Ok, looks like using __cpuexit as this kind of
pseudo-#ifdef HOTPLUG_CPU is a standard practice? ]

But modpost will still complain (bogus warning) about calling __exit
from __init (when HOTPLUG_CPU=n) for cacheinfo_cpu_callback()
calling cache_remove_dev(), no?

So I'd guess all the various notifier callback functions in your patch
need to be whitelisted by modpost, as those are __init functions
who can legally reference __exit (when HOTPLUG_CPU=n).
__init_refok can't be used here, so some special whitelisting might
be required, I presume.

Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/