Re: [PATCH 2.6.24] x86: add sysfs interface for cpuid module

[Sam Ravnborg]

On Wed, Jan 30, 2008 at 06:22:43AM +0800, Yi Yang wrote:
> On Tue, 2008-01-29 at 09:44 +0100, Sam Ravnborg wrote:
> > > +
> > > +static struct notifier_block __cpuinitdata cpuid_sysfs_cpu_notifier = {
> > > +	.notifier_call = cpuid_sysfs_cpu_callback,
> > > +};
> > Data is annotated _cpuintidata
> > 
> > but
> > 
> > > +
> > Data is annotated _cpuintidata
> > 
> > > @@ -217,11 +445,14 @@ static void __exit cpuid_exit(void)
> > >  {
> > >  	int cpu = 0;
> > >  
> > > -	for_each_online_cpu(cpu)
> > > +	for_each_online_cpu(cpu) {
> > >  		cpuid_device_destroy(cpu);
> > > +		remove_cpuid_sysfs(cpu);
> > > +	}
> > >  	class_destroy(cpuid_class);
> > >  	unregister_chrdev(CPUID_MAJOR, "cpu/cpuid");
> > >  	unregister_hotcpu_notifier(&cpuid_class_cpu_notifier);
> > > +	unregister_hotcpu_notifier(&cpuid_sysfs_cpu_notifier);
> > 
> > used in an __exit function.
> > 
> > You should have seen a Section mismatch warning for this.
> > The right fix is to annotate the cpuid_sysfs_cpu_notifier
> > with __initdata_refok (soon to be named __refdata)
> > Or even better to declare it const and use _refconst.
> I think __cpuinitdata is different from __initdata, i have tested it
> by insmod, rmmod, echo 0/1 > /sys/devices/system/cpu/cpu1/online
> repeatly, it hasn't any issue.

__cpuinit & _cpuinitdata have over time been used for 
different purposes:
a) To annotate code/data used in the init path and that in the
non HOTPLUG_CPU case can be discarded after init.
b) To annotate code/data used in the 'core' HOTPLUG_CPU
functionality that isonly in use if HOTPLUG_CPU='y'


The b) usage is questionable and the annotation
of cpuid_sysfs_cpu_notifier beongs in the b) category.

The correct solution would be to factor out the 'core'
HOTPLUG_CPU=y code to a set of separate files and used to
usual mechanishm in the Makefile to select when to include
this code in the kernel.

The improved section mismatch checks by modpost has just
brought this issue to the attention and now you add code
that does the wrong thing it is being discussed.

	Sam
--
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/