Re: [PATCH] misc/cs5535: Fix section mismatch derived from cs5535_mfgpt_drv variable

Previous thread: Should we be using unlikely() around tests of GFP_ZERO? by Theodore Ts'o on Sunday, January 2, 2011 - 4:48 pm. (9 messages)

Next thread: [PATCH] scripts/kernel-doc: reorg code for readability by Randy Dunlap on Sunday, January 2, 2011 - 8:55 pm. (1 message)
From: Sedat Dilek
Date: Sunday, January 2, 2011 - 7:51 pm

From my build.log:

WARNING: drivers/misc/cs5535-mfgpt.o(.data+0x0): Section mismatch in reference from the variable cs5535_mfgpt_drv to the function .devinit.text:cs5535_mfgpt_probe()
The variable cs5535_mfgpt_drv references
the function __devinit cs5535_mfgpt_probe()
If the reference is valid then annotate the
variable with __init* or __refdata (see linux/init.h) or name the variable:
*driver, *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console,

This patch fixes the warning.

Tested with linux-next (next-20101231)

Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
---
 drivers/misc/cs5535-mfgpt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/misc/cs5535-mfgpt.c b/drivers/misc/cs5535-mfgpt.c
index d02d302..d80bd14 100644
--- a/drivers/misc/cs5535-mfgpt.c
+++ b/drivers/misc/cs5535-mfgpt.c
@@ -329,7 +329,7 @@ done:
 	return err;
 }
 
-static struct platform_driver cs5535_mfgpt_drv = {
+static struct platform_driver cs5535_mfgpt_drv __refdata = {
 	.driver = {
 		.name = DRV_NAME,
 		.owner = THIS_MODULE,
-- 
1.7.4.rc0

--

From: Andres Salomon
Date: Monday, January 3, 2011 - 2:52 am

On Mon,  3 Jan 2011 03:51:28 +0100

Hm, I'm confused.  There are plenty of other drivers in mfd/ and gpio/
that have their probe/remove functions marked as __dev{init,exit}, with
their associated platform_driver definitions not marked with any kind
of init marking.  Are they all generating this warning and getting
the same __refdata treatment?


--

From: Sedat Dilek
Date: Monday, January 3, 2011 - 3:04 am

Yes, there is also a section mismatch in "drivers/misc/ioc4.c" (see P.S.).
I hadn't time to look at it as it was very late 3 or 4 a.m. (and first
time sending patches via git-send-email :-)).

There are still some more section mismatches in other sub-trees, for
example "drivers/mtd/devices/sst25l.c" which I haven't catched.
If someone can look at it?

- Sedat -

P.S.: From my build_linux-next_next20101231.dileks.4.log:

WARNING: drivers/misc/ioc4.o(.data+0xc): Section mismatch in reference
from the variable ioc4_load_modules_work to the function
.devinit.text:ioc4_load_modules()
The variable ioc4_load_modules_work references
the function __devinit ioc4_load_modules()
If the reference is valid then annotate the
variable with __init* or __refdata (see linux/init.h) or name the variable:
*driver, *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console,

- EOT -
From: Andrew Morton
Date: Monday, January 3, 2011 - 12:43 pm

On Mon,  3 Jan 2011 03:51:28 +0100

Confused.  cs5535_mfgpt_probe() _does_ have a name of the form
"*_probe", so why did it warn?


--

From: Sam Ravnborg
Date: Monday, January 3, 2011 - 12:51 pm

The varibale need to follow the naming scheme.
In this case the function was named *_probe - which has no significance
    ^^^^^^^^                                                          ^^^^^^^^^ 

	Sam
--

From: Andres Salomon
Date: Monday, January 3, 2011 - 1:34 pm

On Mon, 3 Jan 2011 20:51:31 +0100

I see.  So it should really be renamed cs5535_mfgpt_driver, or _drv
added to the whitelist.  Or the whitelist dropped, and all drivers
changed to explicitly mark the structs with __devinitdata.

It's kind of bizarre that we have variable names signifying whether or
not warnings get issued..
--

From: Sam Ravnborg
Date: Monday, January 3, 2011 - 1:50 pm

It really helped to remove a lot of nosie when we introduced this check.

	Sam
--

From: Andres Salomon
Date: Monday, January 3, 2011 - 2:14 pm

On Mon, 3 Jan 2011 21:50:19 +0100

Should they not be devinitdata, since they're structures referencing
devinit (ie, hotplug init) hooks?

The two places where I traditionally went for documentation on such
things was linux/init.h and Documentation/PCI/pci.txt.  The latter
states:

        __devinit       Device initialization code.
                        Identical to __init if the kernel is not
        compiled with CONFIG_HOTPLUG, normal function otherwise.

        o Do not mark the struct pci_driver.

linux/init.h says:

 * modpost not to issue a warning.  Intended semantics is that a code or
 * data tagged __ref* can reference code or data from init section
   without
 * producing a warning (of course, no warning does not mean code is
 * correct, so optimally document why the __ref is needed and why it's
   OK).


It would be nice to know why refdata is correct here, and devinitdata
is not.
--

From: Sam Ravnborg
Date: Monday, January 3, 2011 - 10:59 pm

The annotation really depends on the lifetime of the variable.

A variable that is used when the kernel is fully operational shall be
annotated with __refdata.
This will relocate the data to a special section that modpost
recognize and thus do not warn about references to functions
/data that are discarded during the startup phase.

__init is used for data that are uncnditionally discarded after
the init phase.
__devinitdata is used for data that is discarded after the initphase
is hotplug is not enabled - if hotplug is enabled then the
data are not discared.

So __init and __initdata have impact on the lifetime of the
data  + how modpost check references - where __refdata is
only used to shut up warnings from modpost.

__refdata was introduced late in the process where we
fixed a lot of section mismatch warnings.
So most of the fixes that hit the kernel was renaming
variables so references to init data/functions did not
cause warnings.

I looked into a more precise way to do the checkss
back then so we could do the check down on member
level in for example a "struct driver" - but I never
came up with anything good.
So therefore we are stuck with the less optimal check
algorithm today.

	Sam
--

Previous thread: Should we be using unlikely() around tests of GFP_ZERO? by Theodore Ts'o on Sunday, January 2, 2011 - 4:48 pm. (9 messages)

Next thread: [PATCH] scripts/kernel-doc: reorg code for readability by Randy Dunlap on Sunday, January 2, 2011 - 8:55 pm. (1 message)